Bug 106675

Summary:

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

Product:

WebKit

Reporter:

Ojan Vafai <ojan>

Component:

New Bugs

Assignee:

Ojan Vafai <ojan>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

buildbot, dglazkov, eae, eric, esprehn, jchaffraix, leviw, mifenton, ojan.autocc, rniwa, tkent, tony, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

106617

Bug Blocks:

106143

Attachments:

Description	Flags
Patch	none
Patch	eae: review+, webkit.review.bot: commit-queue-

Ojan Vafai

Reported 2013-01-11 10:13:18 PST

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

Attachments
Patch (11.67 KB, patch) 2013-01-11 10:14 PST, Ojan Vafai	no flags	Details Formatted Diff Diff
Patch (11.67 KB, patch) 2013-01-11 10:21 PST, Ojan Vafai	eae: review+ webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Ojan Vafai

Comment 1 2013-01-11 10:14:33 PST

Created attachment 182361 [details] Patch

Ojan Vafai

Comment 2 2013-01-11 10:21:33 PST

Created attachment 182363 [details] Patch

WebKit Review Bot

Comment 3 2013-01-11 11:06:43 PST

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

Comment 4 2013-01-11 11:08:02 PST

(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 5 2013-01-11 11:58:53 PST

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 6 2013-01-11 12:27:47 PST

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

Comment 7 2013-01-11 12:50:26 PST

(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 8 2013-01-11 12:55:42 PST

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

Comment 9 2013-01-11 12:58:21 PST

(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

Comment 10 2013-01-11 13:03:28 PST

(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

Comment 11 2013-01-11 18:21:20 PST

(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

Comment 12 2013-01-11 18:30:53 PST

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

Note You need to log in before you can comment on or make changes to this bug.