Bug 8398 - REGRESSION: LABEL in OPTION element is clobbering display #TEXT
Summary: REGRESSION: LABEL in OPTION element is clobbering display #TEXT
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P1 Normal
Assignee: David Carson
Keywords: EasyFix, HasReduction, InRadar, Regression
: 8617 (view as bug list)
Depends on:
Reported: 2006-04-14 14:29 PDT by Bakafish
Modified: 2006-06-01 22:50 PDT (History)
5 users (show)

See Also:

test case (150 bytes, text/html)
2006-04-15 02:44 PDT, Alexey Proskuryakov
no flags Details
patch, no change log or test case yet (1.40 KB, patch)
2006-04-16 23:53 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (13.44 KB, patch)
2006-05-22 16:44 PDT, David Carson
adele: review+
Details | Formatted Diff | Diff
updated with new test cases (17.68 KB, patch)
2006-05-22 20:40 PDT, David Carson
mjs: review+
Details | Formatted Diff | Diff
only change to the patch was to remove tabs and replace with spaces (17.69 KB, patch)
2006-05-31 09:58 PDT, David Carson
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bakafish 2006-04-14 14:29:15 PDT
This may be the desired behavior, it is not how Mozilla or current Safari implementations display it though. I would think that if there is a valid #TEXT child that that should take priority over the LABEL value.

   Should say bar (I think.)
      <option label="foo">bar</option>
Comment 1 Alexey Proskuryakov 2006-04-15 02:44:15 PDT
Confirmed; WinIE 6 also has the same behavior as Firefox and shipping Safari.

BTW, attaching test cases to the bugs (rather than pasting inline) makes them somewhat easier to work on.
Comment 2 Alexey Proskuryakov 2006-04-15 02:44:55 PDT
Created attachment 7725 [details]
test case

Same test case, as an attachment.
Comment 3 Darin Adler 2006-04-16 23:52:56 PDT
The regression was caused when we fixed bug 7255. That bug complained that the label was ignored altogether. We need to change RenderSelect::updateFromElement to use the label only when the text is empty.
Comment 4 Darin Adler 2006-04-16 23:53:22 PDT
Created attachment 7756 [details]
patch, no change log or test case yet
Comment 5 mitz 2006-04-18 10:11:31 PDT
I think the fix for this was accidently committed in r13955 together with the fix for bug 8437. Leaving the bug open for now since at least the change log still needs to be updated.
Comment 6 Darin Adler 2006-04-18 11:31:11 PDT
Comment 7 Darin Adler 2006-04-18 11:37:42 PDT
I'll need to fix the change log and write a layout test.
Comment 8 Darin Adler 2006-04-18 17:09:22 PDT
I'm going to roll back my fix.

The tests fast/forms/HTMLOptionElement_label0[1-5] make it clear that we researched this when we implemented this. Alexey says that IE 6 has the same behavior as Firefox, but these tests seem to indicate otherwise. More research is required.
Comment 9 Darin Adler 2006-04-18 17:40:13 PDT
I just tested with WinIE and my results match Alexey's. Those HTMLOptionElement tests are wrong. I have to fix them.
Comment 10 Darin Adler 2006-04-20 23:54:17 PDT
The good news is, we don't have to make new test cases -- just fix those old ones. But anyone can do this based on my patch, so I'm throwing this back to "unassigned".

Be sure to test what happens when the element does have content, but it's all whitespace, or if the content has something other than plain text in it.
Comment 11 Alexey Proskuryakov 2006-04-26 21:52:12 PDT
*** Bug 8617 has been marked as a duplicate of this bug. ***
Comment 12 David Carson 2006-05-19 16:09:03 PDT
I'm confused on what is the correct behaviour. I tested the test cases fast/forms/HTMLOptionElement_label0[1-5] in Safari and in FF, and they both render them the same. When I tested them in IE7 beta2, the handling is different. But according to the comment #8 IE and FF behave the same. This is not what I am seeing. I am testing with TOT Safari and FF
Comment 13 Alexey Proskuryakov 2006-05-20 02:17:40 PDT
I have just re-tested with:
 Safari (Mac OS X 10.4.6)
 Mac Firefox
 WinIE 6.0.2900.2180.xpsp_sp2_rtm.040803-2158
 ToT WebKit r14501

Stock Safari, Firefox and WinIE gave the same results; ToT was different in each test. I don't have IE7.
Comment 14 David Carson 2006-05-21 14:33:58 PDT
Retested. Got the same results as Alexey. However, Win IE7beta7 does the same as TOT Safari. I wonder if IE are changing their behaviour.
Which result is correct, IE6, Firefox & stock Safari or WinIE7 & TOT Safari?
Comment 15 Alice Liu 2006-05-22 00:25:39 PDT
Comment 16 David Carson 2006-05-22 16:44:03 PDT
Created attachment 8468 [details]

Applied patch to new code base - original patch file has moved.
Included ChangeLogs, updated test cases and 2 new test cases.
Comment 17 Adele Peterson 2006-05-22 16:47:57 PDT
Comment on attachment 8468 [details]

Comment 18 David Carson 2006-05-22 20:40:38 PDT
Created attachment 8470 [details]
updated with new test cases

Only change from previous patch was adding the new test cases.
Comment 19 Maciej Stachowiak 2006-05-22 20:45:00 PDT
Comment on attachment 8470 [details]
updated with new test cases

Comment 20 David Carson 2006-05-31 09:58:05 PDT
Created attachment 8626 [details]
only change to the patch was to remove tabs and replace with spaces

Remove tabs and repace with spaces. Fixed alignment of braces, ie {, in test case. No other changes.
Comment 21 Darin Adler 2006-05-31 11:02:05 PDT
Comment on attachment 8626 [details]
only change to the patch was to remove tabs and replace with spaces

r=me again
Comment 22 Bakafish 2006-05-31 15:50:44 PDT
Okay, after looking at the HTML 4.01 spec it seems that is actually behaving correctly. LABLE has priority over the #TEXT content. However, in quirks mode this has not been the behavior as far as I could tell. In quirks mode Safari 2.0.3 uses the #TEXT contents. Firefox uses the #TEXT in either case.
Comment 23 Darin Adler 2006-06-01 22:50:33 PDT
Committed revision 14679.