RESOLVED FIXED 93862
Text Autosizing: Only take into account block width <= document layout width.
https://bugs.webkit.org/show_bug.cgi?id=93862
Summary Text Autosizing: Only take into account block width <= document layout width.
John Mellor
Reported 2012-08-13 10:16:10 PDT
Text Autosizing: Only take into account block width <= document layout width.
Attachments
Patch (9.82 KB, patch)
2012-08-13 11:11 PDT, John Mellor
no flags
Patch (23.73 KB, patch)
2012-08-14 09:43 PDT, John Mellor
no flags
Patch (23.91 KB, patch)
2012-08-14 10:02 PDT, John Mellor
no flags
Patch (23.92 KB, patch)
2012-08-20 07:49 PDT, John Mellor
no flags
Patch (24.06 KB, patch)
2012-08-20 08:08 PDT, John Mellor
no flags
Archive of layout-test-results from gce-cr-linux-03 (652.45 KB, application/zip)
2012-08-20 09:09 PDT, WebKit Review Bot
no flags
Patch (24.09 KB, patch)
2012-08-20 09:57 PDT, John Mellor
no flags
Kenneth Rohde Christiansen
Comment 1 2012-08-13 10:23:42 PDT
What about flattened iframes modifying the width of their parent?
John Mellor
Comment 2 2012-08-13 11:11:12 PDT
Kenneth Rohde Christiansen
Comment 3 2012-08-13 11:22:59 PDT
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)
John Mellor
Comment 4 2012-08-13 11:31:52 PDT
(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.
Kenneth Rohde Christiansen
Comment 5 2012-08-13 11:42:33 PDT
(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.
John Mellor
Comment 6 2012-08-14 09:43:29 PDT
Created attachment 158349 [details] Patch Updated patch based on ideas in comment #4 (including new tests). Please note the caveat in LayoutTests/ChangeLog.
Kenneth Rohde Christiansen
Comment 7 2012-08-14 09:56:18 PDT
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?
John Mellor
Comment 8 2012-08-14 10:01:35 PDT
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.
John Mellor
Comment 9 2012-08-14 10:02:02 PDT
Created attachment 158353 [details] Patch Addressed kenneth's review comments.
Kenneth Rohde Christiansen
Comment 10 2012-08-14 10:04:03 PDT
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.
John Mellor
Comment 11 2012-08-20 07:45:40 PDT
(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.
John Mellor
Comment 12 2012-08-20 07:49:21 PDT
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.
Kenneth Rohde Christiansen
Comment 13 2012-08-20 08:02:36 PDT
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>
John Mellor
Comment 14 2012-08-20 08:08:42 PDT
Created attachment 159432 [details] Patch Added DOCTYPES.
John Mellor
Comment 15 2012-08-20 08:10:29 PDT
(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.
WebKit Review Bot
Comment 16 2012-08-20 09:09:02 PDT
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
WebKit Review Bot
Comment 17 2012-08-20 09:09:07 PDT
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
John Mellor
Comment 18 2012-08-20 09:57:50 PDT
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.
WebKit Review Bot
Comment 19 2012-08-20 13:23:47 PDT
Comment on attachment 159460 [details] Patch Clearing flags on attachment: 159460 Committed r126058: <http://trac.webkit.org/changeset/126058>
WebKit Review Bot
Comment 20 2012-08-20 13:23:53 PDT
All reviewed patches have been landed. Closing bug.
Kenneth Russell
Comment 21 2012-08-20 14:20:23 PDT
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.
Kenneth Russell
Comment 22 2012-08-20 14:27:11 PDT
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.
Adam Barth
Comment 23 2012-08-20 14:35:10 PDT
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.
Note You need to log in before you can comment on or make changes to this bug.