Bug 49341

Summary: REGRESSION: <option> outside of <select> hides children
Product: WebKit Reporter: Robert Hogan <robert>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: abarth, ahmad.saleem792, annevk, ap, bfulgham, cdumez, eric, hyatt, karlcow, mjs, rniwa, simon.fraser, webkit.review.bot, zalan
Priority: P1 Keywords: InRadar, Regression
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=169039
Attachments:
Description Flags
Test Case
none
Patch
none
Patch
none
Patch
abarth: review-
All browsers differ from each other none

Robert Hogan
Reported 2010-11-10 13:44:18 PST
Created attachment 73534 [details] Test Case The html is mis-formatted but the attached test case displays correctly in Opera and Firefox, and probably Internet Explorer as it's taken from a live web site.
Attachments
Test Case (2.38 KB, text/html)
2010-11-10 13:44 PST, Robert Hogan 		no flags
Details
Patch (3.94 KB, patch)
2010-11-10 15:06 PST, Robert Hogan 		no flags
DetailsFormatted DiffDiff
Patch (6.02 KB, patch)
2010-11-13 06:07 PST, Robert Hogan 		no flags
DetailsFormatted DiffDiff
Patch (5.96 KB, patch)
2010-11-13 06:52 PST, Robert Hogan 		abarth: review-
DetailsFormatted DiffDiff
All browsers differ from each other (494.37 KB, image/png)
2022-08-13 11:21 PDT, Ahmad Saleem 		no flags
Details
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Robert Hogan
Comment 1 2010-11-10 13:45:16 PST
Note that WebKit only displays one drop-down box (for the month), but Firefox, Opera and IE display the year drop-down box also.
Robert Hogan
Comment 2 2010-11-10 15:06:48 PST
Created attachment 73545 [details] Patch
Eric Seidel (no email)
Comment 3 2010-11-10 15:28:12 PST
Comment on attachment 73545 [details] Patch This doesn't need to be a rendering test. Please make it a dumpAsText or dumpAsMarkup test. Alternatively it could be an html5lib test. See LayoutTests/html5lib/resources for examples.
Alexey Proskuryakov
Comment 4 2010-11-10 17:19:40 PST
> The html is mis-formatted but the attached test case displays correctly in Opera and Firefox, and probably Internet Explorer as it's taken from a live web site. It also displays fine in Safari 5.0.2. + Per http://www.w3.org/MarkUp/1995-archive/Elements/OPTION.html This is not an adequate source. We should be looking at HTML5, and if HTML5 is wrong, it should be fixed together with WebKit.
Robert Hogan
Comment 5 2010-11-11 11:07:33 PST
Eric, thanks for pointing me to those. The patch fixes two current failures: resources/tests7.dat: fixes 30 resources/tests19.dat: fixes 63 It creates the following new failures. I've added comments to each. Test 30 of 115 in resources/tests1.dat failed. Input: <select><b><option><select><option></b></select>X Got: | <html> | <head> | <body> | <select> | <option> | "X" Expected: | <html> | <head> | <body> | <select> | <option> | <option> | "X" Given the errors expected from this test: Line: 1 Col: 8 Unexpected start tag (select). Expected DOCTYPE. Line: 1 Col: 11 Unexpected start tag token (b) in the select phase. Ignored. Line: 1 Col: 27 Unexpected select start tag in the select phase treated as select end tag. Line: 1 Col: 39 End tag (b) violates step 1, paragraph 1 of the adoption agency algorithm. Line: 1 Col: 48 Unexpected end tag (select). Ignored. Line: 1 Col: 49 Expected closing tag. Unexpected end of file. the expected markup seems wrong to me, and the new markup seems right. Test 35 of 115 in resources/tests1.dat failed. Input: <!DOCTYPE html>A<option>B<optgroup>C<select>D</option>E Got: | <!DOCTYPE html> | <html> | <head> | <body> | "ABC" | <select> | "DE" Expected: | <!DOCTYPE html> | <html> | <head> | <body> | "A" | <option> | "B" | <optgroup> | "C" | <select> | "DE" Again, were the expectations correct here? Test 103 of 115 in resources/tests1.dat failed. Input: <select><b><option><select><option></b></select> Got: | <html> | <head> | <body> | <select> | <option> Expected: | <html> | <head> | <body> | <select> | <option> | <option> The illegal option element gets dropped. Simiar to 30 above. Test 36 of 61 in resources/tests2.dat failed. Input: <!DOCTYPE html><select><optgroup><option></optgroup><option><select><option> Got: | <!DOCTYPE html> | <html> | <head> | <body> | <select> | <optgroup> | <option> | <option> Expected: | <!DOCTYPE html> | <html> | <head> | <body> | <select> | <optgroup> | <option> | <option> | <option> This is the same as test 30 above. Test 38 of 61 in resources/tests2.dat failed. Input: <!DOCTYPE html><datalist><option>foo</datalist>bar Got: | <!DOCTYPE html> | <html> | <head> | <body> | <datalist> | "foo" | "bar" Expected: | <!DOCTYPE html> | <html> | <head> | <body> | <datalist> | <option> | "foo" | "bar" I'm updating the patch to allow datalist and optgroup contexts for the option element. That should fix this error. resources/tests7.dat: fixes 30 resources/tests19.dat: fixes 63 resources/webkit01.dat: 29 Test 29 of 47 in resources/webkit01.dat failed. Input: <select><option>A<select><option>B<select><option>C<select><option>D<select><option>E<select><option>F<select><option>G<select> Got: | <html> | <head> | <body> | <select> | <option> | "A" | "B" | <select> | <option> | "C" | "D" | <select> | <option> | "E" | "F" | <select> | <option> | "G" Expected: | <html> | <head> | <body> | <select> | <option> | "A" | <option> | "B" | <select> | <option> | "C" | <option> | "D" | <select> | <option> | "E" | <option> | "F" | <select> | <option> | "G" Again <option> outside select is getting dropped. Before I submit, do these new results look right to you?
Robert Hogan
Comment 6 2010-11-11 11:10:50 PST
(In reply to comment #4) > > The html is mis-formatted but the attached test case displays correctly in Opera and Firefox, and probably Internet Explorer as it's taken from a live web site. > > It also displays fine in Safari 5.0.2. > > + Per http://www.w3.org/MarkUp/1995-archive/Elements/OPTION.html > > This is not an adequate source. That's a bit of an understatement. :-) >We should be looking at HTML5, and if HTML5 is wrong, it should be fixed together with WebKit. http://www.w3.org/TR/html5/the-button-element.html#the-option-element says: "Contexts in which this element can be used: As a child of a select element. As a child of a datalist element. As a child of an optgroup element."
Alexey Proskuryakov
Comment 7 2010-11-11 11:15:53 PST
> "Contexts in which this element can be used: > As a child of a select element. > As a child of a datalist element. > As a child of an optgroup element." These look like author requirements, which don't apply to us. It's parsing algorithm that is we should match.
Eric Seidel (no email)
Comment 8 2010-11-11 11:18:03 PST
If you're breaking libhtml5 tests, then you're likely violating the html5 spec. Please file a bug with the html5 spec if you want to change those results. Looking at "in body" mode, I don't see <option> being ignored: http://www.whatwg.org/specs/web-apps/current-work/multipage/tokenization.html#parsing-main-inbody
Robert Hogan
Comment 9 2010-11-11 12:47:59 PST
(In reply to comment #8) > If you're breaking libhtml5 tests, then you're likely violating the html5 spec. Please file a bug with the html5 spec if you want to change those results. > > Looking at "in body" mode, I don't see <option> being ignored: > http://www.whatwg.org/specs/web-apps/current-work/multipage/tokenization.html#parsing-main-inbody Right, the spec there says: "A start tag whose tag name is "select" - Reconstruct the active formatting elements, if any. - Insert an HTML element for the token. etc." Which is what webkit does. However looking at the web inspect in Opera, it looks like it does: "A start tag whose tag name is "select" - If the current node is an option element, then act as if an end tag with the tag name "option" had been seen. - Reconstruct the active formatting elements, if any. - Insert an HTML element for the token. etc." And doing the same in Webkit also fixes it. It also fixes two failing tests in the html5 test. However it still has one failure in webkit01.dat. I take it those are webkit-generated tests rather than spec-generated? Interestingly if I change the stray <option> to a <datalist> in opera it exhibits the same bug we see here with <option>. So it looks like it has implemented the workaround for <option> only. I filed a comment on this to the html5 spec and referenced the bug here. http://www.w3.org/Bugs/Public/show_bug.cgi?id=11302
Robert Hogan
Comment 10 2010-11-13 06:07:57 PST
Created attachment 73820 [details] Patch
WebKit Review Bot
Comment 11 2010-11-13 06:11:42 PST
Attachment 73820 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/html5lib/resources/webkit01.dat', u'LayoutTests/html5lib/runner-expected.txt', u'WebCore/ChangeLog', u'WebCore/html/parser/HTMLTreeBuilder.cpp']" exit_code: 1 Traceback (most recent call last): File "WebKitTools/Scripts/check-webkit-style", line 134, in <module> main() File "WebKitTools/Scripts/check-webkit-style", line 121, in main patch_checker.check(patch) File "/mnt/git/webkit-style-queue/WebKitTools/Scripts/webkitpy/style/patchreader.py", line 66, in check self._text_file_reader.process_file(file_path=path, line_numbers=line_numbers) File "/mnt/git/webkit-style-queue/WebKitTools/Scripts/webkitpy/style/filereader.py", line 128, in process_file self._processor.process(lines, file_path, **kwargs) File "/mnt/git/webkit-style-queue/WebKitTools/Scripts/webkitpy/style/checker.py", line 739, in process checker.check(lines) File "/mnt/git/webkit-style-queue/WebKitTools/Scripts/webkitpy/style/checkers/cpp.py", line 3054, in check self.handle_style_error, self.min_confidence) File "/mnt/git/webkit-style-queue/WebKitTools/Scripts/webkitpy/style/checkers/cpp.py", line 2934, in _process_lines include_state, function_state, class_state, file_state, error) File "/mnt/git/webkit-style-queue/WebKitTools/Scripts/webkitpy/style/checkers/cpp.py", line 2895, in process_line check_for_function_lengths(clean_lines, line, function_state, error) File "/mnt/git/webkit-style-queue/WebKitTools/Scripts/webkitpy/style/checkers/cpp.py", line 1195, in check_for_function_lengths function = line[match_function.start(1):match_function.end(1)] AttributeError: 'NoneType' object has no attribute 'start' If any of these errors are false positives, please file a bug against check-webkit-style.
Robert Hogan
Comment 12 2010-11-13 06:40:47 PST
(In reply to comment #10) > Created an attachment (id=73820) [details] > Patch This is my suggested workaround. It fixes a two html5 tests that WebKit currently and requires one change to a webkit specific test that maybe Adam can comment on. If HTML5 genuinely expects a select element to nest inside an option element then the fix is wrong. After a bit more investigation: - When parsing the original reduced test case: - Firefox removes the stray option element from the dom entirely - Opera closes the stray option element - WebKit (Chromium, QtTestBrowser) nests everything that follows the stray option element. - Opera exhibits the same bug if you replace the stray option element with a datalist element. WebKit will continue to exhibit the bug even with this proposed fix. It does look like the 'in-body' parsing algorithm in html5 considers option and optgroup elements outside select scope as valid. It doesn't specify behaviour for open option and optgroup elements outside select scope when a select start tag is encountered. Any thoughts on the appropriate next step?
Robert Hogan
Comment 13 2010-11-13 06:45:50 PST
(In reply to comment #4) > > The html is mis-formatted but the attached test case displays correctly in Opera and Firefox, and probably Internet Explorer as it's taken from a live web site. > > It also displays fine in Safari 5.0.2. > Presumably Safari is not using the HTML5 parser then? If it is, why does it work on Safari but not QtTestBrowser or Chromium?
Robert Hogan
Comment 14 2010-11-13 06:52:23 PST
Created attachment 73822 [details] Patch
WebKit Review Bot
Comment 15 2010-11-13 06:57:18 PST
Attachment 73822 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/html5lib/resources/webkit01.dat', u'LayoutTests/html5lib/runner-expected.txt', u'WebCore/ChangeLog', u'WebCore/html/parser/HTMLTreeBuilder.cpp']" exit_code: 1 Traceback (most recent call last): File "WebKitTools/Scripts/check-webkit-style", line 134, in <module> main() File "WebKitTools/Scripts/check-webkit-style", line 121, in main patch_checker.check(patch) File "/mnt/git/webkit-style-queue/WebKitTools/Scripts/webkitpy/style/patchreader.py", line 66, in check self._text_file_reader.process_file(file_path=path, line_numbers=line_numbers) File "/mnt/git/webkit-style-queue/WebKitTools/Scripts/webkitpy/style/filereader.py", line 128, in process_file self._processor.process(lines, file_path, **kwargs) File "/mnt/git/webkit-style-queue/WebKitTools/Scripts/webkitpy/style/checker.py", line 739, in process checker.check(lines) File "/mnt/git/webkit-style-queue/WebKitTools/Scripts/webkitpy/style/checkers/cpp.py", line 3054, in check self.handle_style_error, self.min_confidence) File "/mnt/git/webkit-style-queue/WebKitTools/Scripts/webkitpy/style/checkers/cpp.py", line 2934, in _process_lines include_state, function_state, class_state, file_state, error) File "/mnt/git/webkit-style-queue/WebKitTools/Scripts/webkitpy/style/checkers/cpp.py", line 2895, in process_line check_for_function_lengths(clean_lines, line, function_state, error) File "/mnt/git/webkit-style-queue/WebKitTools/Scripts/webkitpy/style/checkers/cpp.py", line 1195, in check_for_function_lengths function = line[match_function.start(1):match_function.end(1)] AttributeError: 'NoneType' object has no attribute 'start' If any of these errors are false positives, please file a bug against check-webkit-style.
Alexey Proskuryakov
Comment 16 2010-11-13 10:44:59 PST
> Presumably Safari is not using the HTML5 parser then? Yes, shipping Safari/WebKit do not use the HTML5 parser. I'm getting the same results in nightlies as you do. It appears critical to verify that our new behavior matches HTML5.
Eric Seidel (no email)
Comment 17 2010-11-13 12:00:20 PST
Um... definitely looks like a check-webkit-style bug.
Adam Barth
Comment 18 2010-11-13 12:38:31 PST
Comment on attachment 73822 [details] Patch This patch does not fix the <keygen> tests. Those tests likely pass on Qt without this patch. Also, this patch changes webkit01.dat, which shouldn't be changed. It's content reflect the state of the HTML5 spec. If you want to change that file, you'll first need to change the spec and then change the dat file to match the spec. As far as I can tell, this patch causes us to violate the HTML5 spec. If there's a good reason to change the parser in this way, we should present our case to the HTML working group and get the spec changed. That ensures that everyone implementing the HTML5 parsing algorithm gets the same result.
Alexey Proskuryakov
Comment 19 2010-11-13 12:43:23 PST
Marking this as an HTML5 parser compatibility regression. > it's taken from a live web site. Is this a public site? Could you provide its URL?
Robert Hogan
Comment 20 2010-11-13 12:57:09 PST
(In reply to comment #19) > Marking this as an HTML5 parser compatibility regression. Firefox 4 creates the same DOM as WebKit but renders both of the select elements in the test case rather than one. As Adam pointed out on IRC this means it is probably not a parsing algorithm bug but a rendering bug in WebKit. > > > it's taken from a live web site. > > Is this a public site? Could you provide its URL? You can access the problem page by entering dummy details here and clicking 'next': https://epayments.lgcsb.ie/fillForm2.taf?_function=1&completedFormID=1480003&nc=4950085141793906137
David Levin
Comment 21 2010-11-13 19:44:55 PST
(In reply to comment #17) > Um... definitely looks like a check-webkit-style bug. Oops https://bugs.webkit.org/show_bug.cgi?id=49504
David Levin
Comment 22 2010-11-13 19:46:24 PST
Removing dbates, hamaji, and levin due to new bug being filed for check-webkit-style issue.
Robert Hogan
Comment 23 2010-11-14 04:14:28 PST
diff --git a/WebCore/html/HTMLOptionElement.h b/WebCore/html/HTMLOptionElement.h index c1791d7..1572acd 100644 --- a/WebCore/html/HTMLOptionElement.h +++ b/WebCore/html/HTMLOptionElement.h @@ -68,7 +68,7 @@ private: virtual bool supportsFocus() const; virtual bool isFocusable() const; - virtual bool rendererIsNeeded(RenderStyle*) { return false; } + virtual bool rendererIsNeeded(RenderStyle*) { return true; } virtual void attach(); virtual void detach(); virtual void setRenderStyle(PassRefPtr<RenderStyle>); fixes the bug. It only breaks 1 layout test on Qt - adding an extra line of whitespace to fast/table/incomplete-table-in-fragment-hang.html - which seems benign. Is it the correct solution though? Rendering elements inside an option element isn't something the rendere has had to do before so it seems like the kind of change that could break sites on WebKit. What do you think?
Dave Hyatt
Comment 24 2010-11-16 14:09:18 PST
That seems suboptimal to me. Creating renderers for option elements would significantly slow down large list boxes, and in general we don't want to make renderers in the render tree when they aren't needed. From a rendering perspective, selects are replaced elements, and so there's no obligation to render foreign content that is placed inside them. If the HTML5 spec is indicating that weird content should end up inside an <option>, then we can follow that spec and not render those contents, and we should not be violating the spec. If the expectation is that misplaced content inside an <option> should render, then I think the HTML5 parsing algorithm should be amended to ensure that the misplaced content doesn't stay inside the <select>.
Alexey Proskuryakov
Comment 25 2011-01-20 16:35:39 PST
<rdar://problem/8895805>
Maciej Stachowiak
Comment 26 2011-01-20 18:38:20 PST
Is this known to affect any real-world sites?
Robert Hogan
Comment 27 2011-01-22 03:14:54 PST
(In reply to comment #26) > Is this known to affect any real-world sites? This is the only one I know of: https://bugs.webkit.org/show_bug.cgi?id=49341#c20
Ahmad Saleem
Comment 28 2022-08-13 11:21:41 PDT
Created attachment 461596 [details] All browsers differ from each other I am able to reproduce this bug in Safari 15.6 and Safari Technical Preview 151 and all browsers differ from each other (as can be seen from attached screenshot), I think Firefox is most appropriate to show both drop-down and we should match with it but just wanted to share updated testing result. Thanks!
Alexey Proskuryakov
Comment 29 2022-08-20 16:42:38 PDT
Seems like this needs to be analyzed from scratch at this point. Is this even still a parsing issue?
Anne van Kesteren
Comment 30 2023-04-05 08:51:15 PDT
The problem is that the <option> element is not treated as if it were the equivalent of a <span> when it doesn't have a <select> parent. More minimal testcase: https://software.hixie.ch/utilities/js/live-dom-viewer/?%3Cp%3EYou%20should%20see%20two%20dropdowns%3A%0A%3Cp%3E%3Cselect%3E%3Coption%3E1%3C%2Fselect%3E%0A%3Cp%3E%3Coption%3E%3Cselect%3E%3Coption%3E2%3C%2Fselect%3E Chromium does this somewhat wrong too, but it doesn't hide the children completely as the screenshot from Ahmad shows.
Karl Dubost
Comment 31 2023-05-15 00:46:37 PDT
*** This bug has been marked as a duplicate of bug 8351 ***
Note You need to log in before you can comment on or make changes to this bug.