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.
Note that WebKit only displays one drop-down box (for the month), but Firefox, Opera and IE display the year drop-down box also.
Created attachment 73545 [details] Patch
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.
> 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.
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?
(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."
> "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.
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
(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
Created attachment 73820 [details] Patch
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.
(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?
(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?
Created attachment 73822 [details] Patch
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.
> 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.
Um... definitely looks like a check-webkit-style bug.
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.
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?
(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
(In reply to comment #17) > Um... definitely looks like a check-webkit-style bug. Oops https://bugs.webkit.org/show_bug.cgi?id=49504
Removing dbates, hamaji, and levin due to new bug being filed for check-webkit-style issue.
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?
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>.
<rdar://problem/8895805>
Is this known to affect any real-world sites?
(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
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!
Seems like this needs to be analyzed from scratch at this point. Is this even still a parsing issue?
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.
*** This bug has been marked as a duplicate of bug 8351 ***