Bug 106675 – Fixed width overrides intrinsic min-width/max-width for text inputs and listboxes

• Component: New Bugs
• Status: RESOLVED FIXED

[Ojan Vafai]

Fixed width overrides intrinsic min-width/max-width for text inputs and listboxes

[Ojan Vafai]

Created attachment 182361 [details]
Patch

[Ojan Vafai]

Created attachment 182363 [details]
Patch

[WebKit Review Bot]

Comment on attachment 182363 [details]
Patch

Attachment 182363 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15804443

New failing tests:
fast/forms/select/listbox-intrinsic-min-width-applies-with-fixed-width.html

[Ojan Vafai]

(In reply to comment #3)
> (From update of attachment 182363 [details])
> Attachment 182363 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/15804443
> 
> New failing tests:
> fast/forms/select/listbox-intrinsic-min-width-applies-with-fixed-width.html

This will pass once 106617 lands.

[Build Bot]

Comment on attachment 182363 [details]
Patch

Attachment 182363 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15803529

New failing tests:
fast/forms/select/listbox-intrinsic-min-width-applies-with-fixed-width.html

[Emil A Eklund]

Comment on attachment 182363 [details]
Patch

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

> Source/WebCore/rendering/RenderListBox.cpp:208
> +    maxLogicalWidth = m_optionsWidth + 2 * optionsSpacingHorizontal;

I know you are just moving this code but why are we doubling the horizontal spacing here?

[Ojan Vafai]

(In reply to comment #6)
> (From update of attachment 182363 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182363&action=review
> 
> > Source/WebCore/rendering/RenderListBox.cpp:208
> > +    maxLogicalWidth = m_optionsWidth + 2 * optionsSpacingHorizontal;
> 
> I know you are just moving this code but why are we doubling the horizontal spacing here?

Not sure. We use optionsSpacingHorizontal elsewhere in the file without doubling it though. As you said, I'm just moving the code.

[Emil A Eklund]

Comment on attachment 182363 [details]
Patch

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

>>> Source/WebCore/rendering/RenderListBox.cpp:208
>>> +    maxLogicalWidth = m_optionsWidth + 2 * optionsSpacingHorizontal;
>> 
>> I know you are just moving this code but why are we doubling the horizontal spacing here?
> 
> Not sure. We use optionsSpacingHorizontal elsewhere in the file without doubling it though. As you said, I'm just moving the code.

Would you mind adding a FIXME?

[Ojan Vafai]

(In reply to comment #8)
> (From update of attachment 182363 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182363&action=review
> 
> >>> Source/WebCore/rendering/RenderListBox.cpp:208
> >>> +    maxLogicalWidth = m_optionsWidth + 2 * optionsSpacingHorizontal;
> >> 
> >> I know you are just moving this code but why are we doubling the horizontal spacing here?
> > 
> > Not sure. We use optionsSpacingHorizontal elsewhere in the file without doubling it though. As you said, I'm just moving the code.
> 
> Would you mind adding a FIXME?

It's not clear to me that this is wrong without digging through the history.

[Emil A Eklund]

(In reply to comment #9)
> It's not clear to me that this is wrong without digging through the history.

I was thinking something along the lines of "FIXME: Why are we doubling the horizontal spacing here but not elsewhere" like the fixmes we have in other places where we have unusual logic without an explanation.

Anyway if you don't think it is helpful then feel free to ignore my suggestion.

[Ojan Vafai]

(In reply to comment #10)
> (In reply to comment #9)
> > It's not clear to me that this is wrong without digging through the history.
> 
> I was thinking something along the lines of "FIXME: Why are we doubling the horizontal spacing here but not elsewhere" like the fixmes we have in other places where we have unusual logic without an explanation.
> 
> Anyway if you don't think it is helpful then feel free to ignore my suggestion.

I can see from inspecting the code...this is the spacing used on either side of the text in an option. So, 2x is the correct value here.

[Ojan Vafai]

Committed r139536: <http://trac.webkit.org/changeset/139536>