Implement <select> parser “relaxation” — for “customizable” <select>#113
Implement <select> parser “relaxation” — for “customizable” <select>#113sideshowbarker wants to merge 6 commits intomasterfrom
Conversation
cdf2934 to
a1d8a3d
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
hsivonen
left a comment
There was a problem hiding this comment.
Sorry about the delay.
As discussed on Matrix, I think the way this patch makes the cloning work on what the parser saw is incorrect and the spec instead requires the cloning to work on what's in the live DOM, which may be different if the option element opens, the network stalls, setTimeout modifies the DOM, and network resumes.
Further remarks in an inline comment.
020921d to
f7b4c0f
Compare
This change, in order to support the “customizable” <select> element: - “relaxes” parsing rules in <select> (to allow more elements as children) - adds the <selectedcontent> element for cloning selected <option> content ElementName.java ---------------- - Add <selectedcontent> element with TreeBuilder.SELECTEDCONTENT group TreeBuilder.java ---------------- - Add SELECTEDCONTENT group constant and IN_SELECT_IN_CONTENT mode - Track selectedContentPointer and activeOptionStackPos for cloning - Allow <div>, <span>, and other elements in <select> in “relaxed” mode - Deep clone <option> content to <selectedcontent> when <option> closes - Handle <selectedcontent> special-casing (store pointer, no stack push) SAXTreeBuilder.java ------------------- - Implement clearSelectedContentChildren() for clearing before clone - Implement deepCloneChildren() and deepCloneNode() for <option> cloning ParentNode.java --------------- - Add clearChildren() method, to support clearing <selectedcontent> html5lib-tests submodule ------------------------ - Updated to pull in corresponding tests for “relaxed” <select> parsing
The initial implementation did inline cloning during parsing (mirroring elements/characters into <selectedcontent> as tokens were processed) AND deep-cloned from the DOM when option was popped. The inline cloning was wrong per spec — cloning should operate on the DOM, not on what the parser saw (the adoption agency algorithm can restructure the tree) — and redundant, since pop() cleared <selectedcontent> and deep-cloned from the DOM anyway. This replaces all inline cloning machinery with a single overridable hook method, cloneOptionContentToSelectedContent(), called when <option> is popped. The tree builder tracks which <option> is active and where <selectedcontent> is, but delegates the actual DOM cloning to subclasses.
…no longer needed
Add necessary corresponding implementations to the other tree builders that construct persistent trees: - DOMTreeBuilder: uses the DOM cloneNode method for deep cloning - XOMTreeBuilder: uses XOM’s Node.copy() for deep cloning - BrowserTreeBuilder: uses the existing cloneNodeDeep method
Add a getBuffer() accessor to CharBufferNode so that deepCloneNode() can copy directly from char[] to char[] via the Characters constructor, instead of going through toString() and toCharArray().
f7b4c0f to
87fe37a
Compare
0466829 to
b3bbf98
Compare
hsivonen
left a comment
There was a problem hiding this comment.
I'm sorry that I've been in a very bad time management loop.
This looks very good, but there are some minor issues.
Thank you!
b3bbf98 to
66f24d6
Compare
This comment was marked as outdated.
This comment was marked as outdated.
…comments Move selectedcontent cloning from TreeBuilder to DOM-side subclasses: the optionElementPopped() hook now walks the live DOM to find the ancestor <select> and its <selectedcontent>, check selectedness, and clone option children — rather than relying on parser-tracked state. Implemented in all four subclasses (DOMTreeBuilder, SAXTreeBuilder, XOMTreeBuilder, BrowserTreeBuilder). Remove all dead IN_SELECT / IN_SELECT_IN_TABLE mode code (handlers, constants, end-tag special cases) — with <select> “relaxation”, those modes are never entered. Fix resetTheInsertionMode to skip past <select> elements on the stack instead of incorrectly returning IN_BODY — an enclosing td/th now correctly yields IN_CELL, an enclosing table yields IN_TABLE, etc. Align start-tag handling with the spec: - HR: use generateImpliedEndTags() instead of manual isCurrent/pop - OPTION/OPTGROUP: remove redundant pop loops (spec says parse error only), add missing reconstructTheActiveFormattingElements for optgroup, fix optgroup to check both option and optgroup in scope - SELECT: check fragment case first, remove generateImpliedEndTags from nested case, add framesetOk = false - Table elements (caption, tbody, tr, td, th): treat as stray start tags, removing the old IN_SELECT_IN_TABLE dual-scope check Fold </select> end tag into the standard block-element group (with div, fieldset, button, etc.) — the dedicated resetTheInsertionMode call is no longer needed. Add spec URLs and verbatim spec-text comments throughout all <select>-related code sections — for traceability and review-ability.
66f24d6 to
8ad0683
Compare
|
@hsivonen OK, all comments now resolved, I think. Ready for you to review again. |
| JavaScriptObject ancestor = getParentNode(option); | ||
| JavaScriptObject select = null; | ||
| while (ancestor != null && getNodeType(ancestor) == 1) { | ||
| if ("select".equals(getLocalName(ancestor))) { |
There was a problem hiding this comment.
Unlike the other subclasses, this one does not appear to check the HTMLness of either select or selectedcontent. It's likely possible to fool at least the selectedcontent search with SVG. Can be a follow-up to fix.
There was a problem hiding this comment.
I’ll take a shot at this before we merge
hsivonen
left a comment
There was a problem hiding this comment.
Let's merge this. Thank you and sorry about delay.
This change, in order to support the "customizable"
<select>element:<select>(to allow more elements as children)<selectedcontent>element for cloning selected<option>contentElementName.java<selectedcontent>element withTreeBuilder.SELECTEDCONTENTgroupTreeBuilder.javaStructural changes:
IN_SELECT/IN_SELECT_IN_TABLEconstants and all associated mode handlers and end-tag special cases — with select "relaxation", those modes are never enteredresetTheInsertionModeto skip past<select>elements on the stack instead of incorrectly returningIN_BODY— an enclosing<td>/<th>now correctly yieldsIN_CELL, an enclosing<table>yieldsIN_TABLE, etc.Start-tag alignment with spec:
<hr>: usegenerateImpliedEndTags()instead of manualisCurrent/pop<option>: remove redundant pop loop (spec says parse error only aftergenerateImpliedEndTagsExceptFor("optgroup"))<optgroup>: remove redundant pop loop, add missingreconstructTheActiveFormattingElements(), fix parse error to check both<option>and<optgroup>in scope<select>: check fragment case first, removegenerateImpliedEndTagsfrom nested case, addframesetOk = false<input>: pop select and reprocess when select is in scope<caption>,<tbody>,<tr>,<td>,<th>): treat as stray start tags, removing the oldIN_SELECT_IN_TABLEdual-scope checkEnd-tag changes:
</select>into the standard block-element end-tag group (with</div>,</fieldset>,</button>, etc.)</option>and</optgroup>end-tag cases — "any other end tag" handles themCloning hook:
optionElementPopped()hook called from allpop()variantsSpec comments:
DOMTreeBuilder.javaoptionElementPopped(): walk ancestors to find<select>, find<selectedcontent>descendant, check selectedness, deep-clone option children viacloneNode(true)SAXTreeBuilder.javaoptionElementPopped()as no-op (streaming/SAX mode has no live DOM to clone into)XOMTreeBuilder.javaoptionElementPopped()using XOM'sNode.copy()for deep cloningBrowserTreeBuilder.javaoptionElementPopped()usingcloneNodeDeep()for deep cloningCharBufferNode.javagetBuffer()accessor for directchar[]access during cloningParentNode.javaclearChildren()method, to support clearing<selectedcontent>