Bug 93862

What about flattened iframes modifying the width of their parent?

Created attachment 158047 [details] Patch

Comment on attachment 158047 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158047&action=review LGTM > Source/WebCore/ChangeLog:26 > + by wrapping the text in such excesively wide blocks to the layoutWidth. excesively is probably with two s'es (I am not native speaker)

(In reply to comment #1) > What about flattened iframes modifying the width of their parent? Hmm. I'm currently using the main frame's layoutWidth for all frames, so I think that particular case should be fine. Though now I think about it it's probably not ideal to use the main frame's layout width. Really I want to limit it to the maximum block width that can be seen onscreen at once; since afaik all ports only allow the main frame to the scaled by pinch zoom etc (i.e. all child frames are scaled in strict proportion), the maximum block width that can be onscreen at once should be the min of the layoutWidth of all enclosing frames. Since flattened iframes don't impose a strict width limit, perhaps they could simply be excluded from the set of enclosing frames that are considered (hence if all iframes are flattened this would fall back to using the main frame's layoutWidth). Does that sound reasonable? If so I'll spin up an updated patch tomorrow... (In reply to comment #3) > excesively is probably with two s'es (I am not native speaker) Oops, I'll fix that.

(In reply to comment #4) > (In reply to comment #1) > > What about flattened iframes modifying the width of their parent? > > Hmm. I'm currently using the main frame's layoutWidth for all frames, so I think that particular case should be fine. Though now I think about it it's probably not ideal to use the main frame's layout width. Really I want to limit it to the maximum block width that can be seen onscreen at once; since afaik all ports only allow the main frame to the scaled by pinch zoom etc (i.e. all child frames are scaled in strict proportion), the maximum block width that can be onscreen at once should be the min of the layoutWidth of all enclosing frames. > > Since flattened iframes don't impose a strict width limit, perhaps they could simply be excluded from the set of enclosing frames that are considered (hence if all iframes are flattened this would fall back to using the main frame's layoutWidth). Does that sound reasonable? If so I'll spin up an updated patch tomorrow... I think so, would be good to create some tests though. > > (In reply to comment #3) > > excesively is probably with two s'es (I am not native speaker) > > Oops, I'll fix that.

Created attachment 158349 [details] Patch Updated patch based on ideas in comment #4 (including new tests). Please note the caveat in LayoutTests/ChangeLog.

Comment on attachment 158349 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158349&action=review > Source/WebCore/ChangeLog:18 > + To determine the maximum onscreen block width, we take the minimum of > + the block width and the layoutWidth of the narrowest non-flattened > + ancestor frame. Note that on mobile the layoutWidth of the main frame > + is the fixed layout width aka viewport width. Maybe you should describe why flattened ancestor frames are ignored > Source/WebCore/rendering/TextAutosizer.cpp:68 > + for (Frame* frame = m_document->frame(); frame; frame = frame->tree()->parent()) { > + if (!frame->view()->isInChildFrameWithFrameFlattening()) > + minLayoutSize = minLayoutSize.shrunkTo(frame->view()->layoutSize()); > + } Could any of these be offscreen?

Comment on attachment 158349 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158349&action=review >> Source/WebCore/ChangeLog:18 >> + is the fixed layout width aka viewport width. > > Maybe you should describe why flattened ancestor frames are ignored Done. >> Source/WebCore/rendering/TextAutosizer.cpp:68 >> + } > > Could any of these be offscreen? It shouldn't matter if they're offscreen, as they could be scrolled onscreen, and I care about the maximum displayable size when optimally scrolled. Note that since they're Sizes not Rects, shrunkTo won't do anything funny if they're misaligned.

Created attachment 158353 [details] Patch Addressed kenneth's review comments.

they could be offscreen like to the top left (offscreen iframe). Some pages use this for some reason (preloading css resources?). I don't know if that is an issue, but we had issues with this and frame flattening before.

(In reply to comment #10) > they could be offscreen like to the top left (offscreen iframe). Some pages use this for some reason (preloading css resources?). I don't know if that is an issue, but we had issues with this and frame flattening before. It doesn't seem like that would be a problem, since I simply ignore flattened frames, and for non-flattened frames I'm only looking at their sizes, so it shouldn't matter if they're offscreen.

Created attachment 159427 [details] Patch Rebased following the changes in bug 91660. The EWS bots should now be able to run this, and assuming they go green it should be ready for commit.

Comment on attachment 159427 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159427&action=review > Source/WebCore/ChangeLog:13 > + Instead of calculating the textAutosizingMultiplier purely based on the > + width of each block, we now work out the maximum width of the block > + that could be displayed onscreen at any one time, and use that value. > + This avoids excessive text size multiplication (there's no point making > + text bigger than this, since you wouldn't be able to zoom out far > + enough to read it!). Should we consider the viewport meta maximum-scale ? > LayoutTests/fast/text-autosizing/narrow-iframe-flattened.html:1 > +<html> I think we are supposed to add the html doctype <!DOCTYPE html>

Created attachment 159432 [details] Patch Added DOCTYPES.

(In reply to comment #13) > Should we consider the viewport meta maximum-scale ? Yes, I suppose it would make sense to consider the minimum scale (the maximum doesn't affect this). I'll update the patch... > > LayoutTests/fast/text-autosizing/narrow-iframe-flattened.html:1 > > +<html> > > I think we are supposed to add the html doctype <!DOCTYPE html> Done.

Comment on attachment 159432 [details] Patch Attachment 159432 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13537753 New failing tests: fast/text-autosizing/wide-block.html

Created attachment 159448 [details] Archive of layout-test-results from gce-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Created attachment 159460 [details] Patch Fixed tests (needed div not p due to doctype). On second thoughts I think I'll punt considering the minimum scale to a new bug (I opened bug 94491 for this), since the minimum scale isn't yet available in WebCore, and this patch is already useful without it.

Comment on attachment 159460 [details] Patch Clearing flags on attachment: 159460 Committed r126058: <http://trac.webkit.org/changeset/126058>

All reviewed patches have been landed. Closing bug.

This patch seems to need a rebaseline on Mac OS X. See http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=fast%2Ftext-autosizing%2Fnested-em-line-height.html . I am going to do the rebaseline, but John, please double-check it and make sure it looks correct.

I spoke too soon; this test is a reftest, so the reference version of the test needs to be updated. Filing this issue separately and pointing it at this one.

This might be failing for the same reason as Bug 90741. We might want to mark all these tests as failing on Mac OS X until we get that issue worked out.