Text Autosizing: Only take into account block width <= document layout width.

• Component: New Bugs
• Status: RESOLVED FIXED

[John Mellor]

Text Autosizing: Only take into account block width <= document layout width.

[Kenneth Rohde Christiansen]

What about flattened iframes modifying the width of their parent?

[John Mellor]

Created attachment 158047 [details]
Patch

[Kenneth Rohde Christiansen]

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]

(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]

(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]

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 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 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]

Created attachment 158353 [details]
Patch

Addressed kenneth's review comments.

[Kenneth Rohde Christiansen]

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]

(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]

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 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]

Created attachment 159432 [details]
Patch

Added DOCTYPES.

[John Mellor]

(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 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]

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]

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 on attachment 159460 [details]
Patch

Clearing flags on attachment: 159460

Committed r126058: <http://trac.webkit.org/changeset/126058>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kenneth Russell]

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]

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]

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.