Bug 49341 - REGRESSION: <option> outside of <select> hides children
Summary: REGRESSION: <option> outside of <select> hides children
Status: RESOLVED DUPLICATE of bug 8351
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Nobody
URL:
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2010-11-10 13:44 PST by Robert Hogan
Modified: 2023-05-15 00:46 PDT (History)
14 users (show)

See Also:


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 Details | Formatted Diff | Diff
Patch (6.02 KB, patch)
2010-11-13 06:07 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (5.96 KB, patch)
2010-11-13 06:52 PST, Robert Hogan
abarth: review-
Details | Formatted Diff | Diff
All browsers differ from each other (494.37 KB, image/png)
2022-08-13 11:21 PDT, Ahmad Saleem
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Hogan 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.
Comment 1 Robert Hogan 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.
Comment 2 Robert Hogan 2010-11-10 15:06:48 PST
Created attachment 73545 [details]
Patch
Comment 3 Eric Seidel (no email) 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.
Comment 4 Alexey Proskuryakov 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.
Comment 5 Robert Hogan 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?
Comment 6 Robert Hogan 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."
Comment 7 Alexey Proskuryakov 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.
Comment 8 Eric Seidel (no email) 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
Comment 9 Robert Hogan 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
Comment 10 Robert Hogan 2010-11-13 06:07:57 PST
Created attachment 73820 [details]
Patch
Comment 11 WebKit Review Bot 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.
Comment 12 Robert Hogan 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?
Comment 13 Robert Hogan 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?
Comment 14 Robert Hogan 2010-11-13 06:52:23 PST
Created attachment 73822 [details]
Patch
Comment 15 WebKit Review Bot 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.
Comment 16 Alexey Proskuryakov 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.
Comment 17 Eric Seidel (no email) 2010-11-13 12:00:20 PST
Um... definitely looks like a check-webkit-style bug.
Comment 18 Adam Barth 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.
Comment 19 Alexey Proskuryakov 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?
Comment 20 Robert Hogan 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
Comment 21 David Levin 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
Comment 22 David Levin 2010-11-13 19:46:24 PST
Removing dbates, hamaji, and levin due to new bug being filed for check-webkit-style issue.
Comment 23 Robert Hogan 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?
Comment 24 Dave Hyatt 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>.
Comment 25 Alexey Proskuryakov 2011-01-20 16:35:39 PST
<rdar://problem/8895805>
Comment 26 Maciej Stachowiak 2011-01-20 18:38:20 PST
Is this known to affect any real-world sites?
Comment 27 Robert Hogan 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
Comment 28 Ahmad Saleem 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!
Comment 29 Alexey Proskuryakov 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?
Comment 30 Anne van Kesteren 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.
Comment 31 Karl Dubost 2023-05-15 00:46:37 PDT

*** This bug has been marked as a duplicate of bug 8351 ***