Allow <hr> to be used inside <select> as a separator #9124
Conversation
annevk
added
addition/proposal
annevk
force-pushed
the
annevk/hr-in-select
branch
from
26624a5
to
28eed06
Compare
This is a long-standing WebKit feature that regressed as part of the standardized HTML parser effort. This suggests bringing it back with optional semantics, but a mandatory HTML parser change. The HTML parser change is not expected to be significant for existing content or XSS. When the feature is correctly used it will also not hurt HTML parsers that have not yet incorporated the change. I.e., it should be fully backwards compatible. Fixes #3410.
annevk
force-pushed
the
annevk/hr-in-select
branch
from
28eed06
to
b80708f
Compare
|
cc @whatwg/html-parser |
|
Thanks for the feedback, on closer inspection I was indeed making it too difficult. Much easier to support it as an arbitrary child of |
There was a problem hiding this comment.
Editorially LGTM. I'm not 100% sure on the parser section, but I assume @zcorpan has that covered; if a second set of eyes is appreciated, let me know and I can read more parser spec and try to develop better reviewer-eyes for that part.
domenic
added
the
needs implementer interest
|
I'm pretty confident that is correct, but more review is always welcome. Could you find someone from Chromium who can speak to implementer interest? @zcorpan can you speak for Gecko? (I haven't updated the tests by the way, but I will do once there are some more signals.) |
|
This feels like an area for @mfreed7 to take a look at. |
There was a problem hiding this comment.
Looks reasonable to me, and this does seem like a useful feature for developers. I can't commit to adding it to Chromium soon, but I'm supportive of the spec change. Please file a Chromium bug. And I see the WPT addition for the parser behavior, but is it possible to do any kind of testing of the behavior in the listbox? Likely not, just worth asking.
| @@ -125793,6 +125828,9 @@ progress { appearance: auto; }</code></pre> | |||
| data-x="concept-option-label">label</span>, indented under its <code>optgroup</code> element if it | |||
| has one.</p> | |||
|
|
|||
| <p>Each sequence of one or more child <code>hr</code> element siblings may be rendered as a single | |||
| separator.</p> | |||
There was a problem hiding this comment.
Why this optional behavior? Why not render two separators?
There was a problem hiding this comment.
Thanks for the review! I'll update the tests so we can land this.
As widgets are sometimes rendered by the OS I didn't want to require features that might not exist. And as it's a fairly minor semantic it doesn't really matter if it's not supported. It's just nicer when it is.
And a single separator is rendered because that is what Chromium and WebKit implement today and breaking with that seemed unnecessary:
data:text/xml,<select%20xmlns="http://www.w3.org/1999/xhtml"><option>1</option><hr/><hr/><option>2</option></select>.
annevk
removed
the
needs implementer interest
|
To address the one remaining question: I didn't see a way to test the |
Yeah I figured that was the answer. I don’t see a great way either. It seems that WPT is missing a great way to do reference tests on popup widgets like this. |
|
What is the use case for hr in select? |
https://bugs.webkit.org/show_bug.cgi?id=80686 rdar://107656886 Reviewed by Ryosuke Niwa. Revive support in the HTML parser for <hr>-in-<select>, thereby making an existing UI feature much more accessible to web developers. This reflects a recent change in the HTML Standard: whatwg/html#9124. * LayoutTests/html5lib/resources/webkit02.dat: These are being upstreamed via html5lib/html5lib-tests#167 and will then be incorporated into web-platform-tests. * Source/WebCore/html/parser/HTMLTreeBuilder.cpp: (WebCore::HTMLTreeBuilder::processStartTag): Canonical link: https://commits.webkit.org/263624@main
As per whatwg/html#9124. Tests from WebKit/WebKit#12407.
|
Parser change and HTML syntax change LGTM. |
Upstream changes: https://github.com/sparklemotion/nokogiri/releases/tag/v1.15.3 https://github.com/sparklemotion/nokogiri/releases/tag/v1.15.2 https://github.com/sparklemotion/nokogiri/releases/tag/v1.15.1 https://github.com/sparklemotion/nokogiri/releases/tag/v1.15.0 1.15.3 / 2023-07-05 Fixed * Passing an object that is not a kind of XML::Node as the first parameter to CDATA.new now raises a TypeError. Previously this would result in either a segfault (CRuby) or a Java exception (JRuby). [#2920] * Passing an object that is not a kind of XML::Node as the first parameter to Schema.from_document now raises a TypeError. Previously this would result in either a segfault (CRuby) or a Java exception (JRuby). [#2920] * [CRuby] Passing an object that is not a kind of XML::Node as the second parameter to Text.new now raises a TypeError. Previously this would result in a segfault. [#2920] * [CRuby] Replacing a node's children via methods like Node#inner_html=, # children=, and #replace no longer defensively dups the node's next sibling if it is a Text node. This behavior was originally adopted to work around libxml2's memory management (see #283 and #595) but should not have included operations involving xmlAddChild(). [#2916] * [JRuby] Fixed NPE when serializing an unparented HTML node. [#2559, #2895] (Thanks, @cbasguti!) 1.15.2 / 2023-05-24 Dependencies * [JRuby] Vendored org.nokogiri:nekodtd is updated to v0.1.11.noko2. This is functionally equivalent to v0.1.11.noko1 but restores support for Java 8. Fixed * [JRuby] Java 8 support is restored, fixing a regression present in v1.14.0..v1.14.4 and v1.15.0..v1.15.1. [#2887] 1.15.1 / 2023-05-19 Dependencies * [CRuby] Vendored libxml2 is updated to v2.11.4 from v2.11.3. For details please see https://gitlab.gnome.org/GNOME/libxml2/-/releases/v2.11.4 Fixed * [CRuby] The libxml2 update fixes an encoding regression when push-parsing UTF-8 sequences. [#2882, upstream issue and commit] 1.15.0 / 2023-05-15 Notes Ability to opt into system malloc and free Since 2009, Nokogiri has configured libxml2 to use ruby_xmalloc et al for memory management. This has provided benefits for memory management, but comes with a performance penalty. Users can now opt into using system malloc for libxml2 memory management by setting an environment variable: # "default" here means "libxml2's default" which is system malloc NOKOGIRI_LIBXML_MEMORY_MANAGEMENT=default Benchmarks show that this setting will significantly improve performance, but be aware that the tradeoff may involve poorer memory management including bloated heap sizes and/or OOM conditions. You can read more about this in the decision record at adr/ 2023-04-libxml-memory-management.md. Dependencies * [CRuby] Vendored libxml2 is updated to v2.11.3 from v2.10.4. For details please see: + https://gitlab.gnome.org/GNOME/libxml2/-/releases/v2.11.0 + https://gitlab.gnome.org/GNOME/libxml2/-/releases/v2.11.1 + https://gitlab.gnome.org/GNOME/libxml2/-/releases/v2.11.2 + https://gitlab.gnome.org/GNOME/libxml2/-/releases/v2.11.3 * [CRuby] Vendored libxslt is updated to v1.1.38 from v1.1.37. For details please see: + https://gitlab.gnome.org/GNOME/libxslt/-/releases/v1.1.38 Added * Encoding objects may now be passed to serialization methods like #to_xml, # to_html, #serialize, and #write_to to specify the output encoding. Previously only encoding names (strings) were accepted. [#2774, #2798] (Thanks, @ellaklara!) * [CRuby] Users may opt into using system malloc for libxml2 memory management. For more detail, see note above or adr/ 2023-04-libxml-memory-management.md. Changed * [CRuby] Schema.from_document now makes a defensive copy of the document if it has blank text nodes with Ruby objects instantiated for them. This prevents unsafe behavior in libxml2 from causing a segfault. There is a small performance cost, but we think this has the virtue of being "what the user meant" since modifying the original is surprising behavior for most users. Previously this was addressed in v1.10.9 by raising an exception. Fixed * [CRuby] XSLT.transform now makes a defensive copy of the document if it has blank text nodes with Ruby objects instantiated for them and the template uses xsl:strip-spaces. This prevents unsafe behavior in libxslt from causing a segfault. There is a small performance cost, but we think this has the virtue of being "what the user meant" since modifying the original is surprising behavior for most users. Previously this would allow unsafe memory access and potentially segfault. [#2800] Improved * Nokogiri::XML::Node::SaveOptions#inspect now shows the names of the options set in the bitmask, similar to ParseOptions. [#2767] * #inspect and pretty-printing are improved for AttributeDecl, ElementContent, ElementDecl, and EntityDecl. * [CRuby] The C extension now uses Ruby's TypedData API for managing all the libxml2 structs. Write barriers may improve GC performance in some extreme cases. [#2808] (Thanks, @etiennebarrie and @byroot!) * [CRuby] ObjectSpace.memsize_of reports a pretty good guess of memory usage when called on Nokogiri::XML::Document objects. [#2807] (Thanks, @etiennebarrie and @byroot!) * [CRuby] Users installing the "ruby" platform gem and compiling libxml2 and libxslt from source will now be using a modern config.guess and config.sub that supports new architectures like loongarch64. [#2831] (Thanks, @zhangwenlong8911!) * [CRuby] HTML5 parser: + adjusts the specified attributes, adding xlink:arcrole and removing xml:base [#2841, #2842] + allows <hr> in <select> [whatwg/html#3410, whatwg/html#9124] * [JRuby] Node#first_element_child now returns nil if there are only non-element children. Previously a null pointer exception was raised. [# 2808, #2844] * Documentation for Nokogiri::XSLT now has usage examples including custom function handlers. Deprecated * Passing a Nokogiri::XML::Node as the first parameter to CDATA.new is deprecated and will generate a warning. This parameter should be a kind of Nokogiri::XML::Document. This will become an error in a future version of Nokogiri. * Passing a Nokogiri::XML::Node as the first parameter to Schema.from_document is deprecated and will generate a warning. This parameter should be a kind of Nokogiri::XML::Document. This will become an error in a future version of Nokogiri. * Passing a Nokogiri::XML::Node as the second parameter to Text.new is deprecated and will generate a warning. This parameter should be a kind of Nokogiri::XML::Document. This will become an error in a future version of Nokogiri. * [CRuby] Calling a custom XPath function without the nokogiri namespace is deprecated and will generate a warning. Support for non-namespaced functions will be removed in a future version of Nokogiri. (Note that JRuby has never supported non-namespaced custom XPath functions.)
|
I fail to see why this is a thing at all; what's the use case/justification for this What is a separator if not a grouping element. By separating things you are grouping them; it's one and the same thing. All I see happening here is that a generation of people already not aware of the <select>
<option disabled>Section 1</option>
<option value=1>One</option>
<option value=2>Two</option>
<hr>
<option disabled>Section 2</option>
<option value=2-1>One</option>
<option value=2-2>Two</option>
</select>This seems like a solution to a styling problem that isn't actually a problem, and is going to make more problems for more people, IMO. You want just a simple separator? Style optgroup. optgroup { margin-inline-start: 0; padding-inline-start: 0; }
optgroup + optgroup { border-block-start: 1px solid currentColor; } |
This is a long-standing WebKit feature that regressed as part of the standardized HTML parser effort. This suggests bringing it back with optional semantics, but a mandatory HTML parser change.
The HTML parser change is not expected to be significant for existing content or XSS. When the feature is correctly used it will also not hurt HTML parsers that have not yet incorporated the change. I.e., it should be fully backwards compatible.
Fixes #3410.
(See WHATWG Working Mode: Changes for more details.)
/acknowledgements.html ( diff )
/form-elements.html ( diff )
/grouping-content.html ( diff )
/parsing.html ( diff )
/rendering.html ( diff )
/syntax.html ( diff )