Bug 103840 - Text Autosizing: don't autosize horizontal menu items implemented via floating list items
Summary: Text Autosizing: don't autosize horizontal menu items implemented via floatin...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Anton Vayvod
URL:
Keywords:
Depends on:
Blocks: FontBoosting
  Show dependency treegraph
 
Reported: 2012-12-02 13:11 PST by Anton Vayvod
Modified: 2012-12-03 10:27 PST (History)
6 users (show)

See Also:


Attachments
Patch (11.71 KB, patch)
2012-12-02 13:24 PST, Anton Vayvod
no flags Details | Formatted Diff | Diff
Patch (12.06 KB, patch)
2012-12-03 07:13 PST, Anton Vayvod
no flags Details | Formatted Diff | Diff
Patch (12.03 KB, patch)
2012-12-03 09:03 PST, Anton Vayvod
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anton Vayvod 2012-12-02 13:11:34 PST
Text Autosizing: don't autosize horizontal menu items implemented via floating list items
Comment 1 Anton Vayvod 2012-12-02 13:24:17 PST
Created attachment 177149 [details]
Patch

Turn floating list items into clusters, in particular to improve rendering of horizontal menus
Comment 2 John Mellor 2012-12-03 06:25:47 PST
Comment on attachment 177149 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177149&action=review

Looks great, thanks! Just a few very minor nits.

> Source/WebCore/ChangeLog:9
> +        disregards such elements as autosizing containers. List items are then autosized as part of a bigger

s/are then/are often then/

> Source/WebCore/ChangeLog:12
> +        resulting in no font size multiplication for menu items.

s/for menu items/for menu items implemented in this manner/

> Source/WebCore/rendering/TextAutosizer.cpp:208
> +    // * except for floating list items or positioned out of the list's flow.

Minor nit - I'd prefer:

// - Must not be normal list items, as items in the same list should look
//   consistent, unless they are floating or position:absolute/fixed.

> LayoutTests/ChangeLog:12
> +        resulting in no font size multiplication for menu items.

s/for menu items/for menu items implemented in this manner/

> LayoutTests/fast/text-autosizing/cluster-list-item-expected.html:19
> +	<li style="position: absolute; width: 800px; font-size: 2.5rem">

From looking at these styles, I suspect these list items are going to overlap significantly, making it hard to see what's going on in the test. Could you make sure that doesn't happen, by adding margins or clear:left etc if necessary?
Comment 3 Anton Vayvod 2012-12-03 07:13:11 PST
Created attachment 177249 [details]
Patch
Comment 4 Anton Vayvod 2012-12-03 07:14:39 PST
Comment on attachment 177149 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177149&action=review

>> Source/WebCore/ChangeLog:9
>> +        disregards such elements as autosizing containers. List items are then autosized as part of a bigger
> 
> s/are then/are often then/

Done.

>> Source/WebCore/ChangeLog:12
>> +        resulting in no font size multiplication for menu items.
> 
> s/for menu items/for menu items implemented in this manner/

Done.

>> Source/WebCore/rendering/TextAutosizer.cpp:208
>> +    // * except for floating list items or positioned out of the list's flow.
> 
> Minor nit - I'd prefer:
> 
> // - Must not be normal list items, as items in the same list should look
> //   consistent, unless they are floating or position:absolute/fixed.

Done

>> LayoutTests/ChangeLog:12
>> +        resulting in no font size multiplication for menu items.
> 
> s/for menu items/for menu items implemented in this manner/

Done.

>> LayoutTests/fast/text-autosizing/cluster-list-item-expected.html:19
>> +	<li style="position: absolute; width: 800px; font-size: 2.5rem">
> 
> From looking at these styles, I suspect these list items are going to overlap significantly, making it hard to see what's going on in the test. Could you make sure that doesn't happen, by adding margins or clear:left etc if necessary?

Done.
Comment 5 John Mellor 2012-12-03 08:09:47 PST
Comment on attachment 177249 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177249&action=review

Looks good to me. Kenneth/Julien, what do you think?

> LayoutTests/fast/text-autosizing/cluster-list-item-expected.html:19
> +	<li style="clear:both; font-size: 1.25rem">

Nit: the clear:both is redundant here (I suspect this is a left-over from before you reordered these).
Comment 6 Anton Vayvod 2012-12-03 09:03:11 PST
Created attachment 177264 [details]
Patch
Comment 7 Kenneth Rohde Christiansen 2012-12-03 09:15:34 PST
Comment on attachment 177264 [details]
Patch

This looks fine to me
Comment 8 WebKit Review Bot 2012-12-03 10:27:08 PST
Comment on attachment 177264 [details]
Patch

Clearing flags on attachment: 177264

Committed r136411: <http://trac.webkit.org/changeset/136411>
Comment 9 WebKit Review Bot 2012-12-03 10:27:12 PST
All reviewed patches have been landed.  Closing bug.