WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
103840
Text Autosizing: don't autosize horizontal menu items implemented via floating list items
https://bugs.webkit.org/show_bug.cgi?id=103840
Summary
Text Autosizing: don't autosize horizontal menu items implemented via floatin...
Anton Vayvod
Reported
2012-12-02 13:11:34 PST
Text Autosizing: don't autosize horizontal menu items implemented via floating list items
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Anton Vayvod
Comment 1
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
John Mellor
Comment 2
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?
Anton Vayvod
Comment 3
2012-12-03 07:13:11 PST
Created
attachment 177249
[details]
Patch
Anton Vayvod
Comment 4
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.
John Mellor
Comment 5
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).
Anton Vayvod
Comment 6
2012-12-03 09:03:11 PST
Created
attachment 177264
[details]
Patch
Kenneth Rohde Christiansen
Comment 7
2012-12-03 09:15:34 PST
Comment on
attachment 177264
[details]
Patch This looks fine to me
WebKit Review Bot
Comment 8
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
>
WebKit Review Bot
Comment 9
2012-12-03 10:27:12 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug