Make <selectedcontent> work with <select multiple> [chromium/src : main]
Mason Freed (Gerrit)
Mason Freed added 1 comment![]( "Open in Gerrit")
Una is curious about this functionality. Any ETA For landing it?
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 10 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
10 comments
Looks good I think! Just some smaller things.
void UpdateSelectedcontent(HTMLSelectedContentElement&);I think something like `UpdateContainedSelectedcontentElement()` might be a bit better? It's confusing to have both `UpdateAllSelectedcontents*` and `UpdateSelectedcontent`, and one updates all for the current `<select>` and the other updates just a single `<selectedcontent>` that is assumed to be within this select.
// elemnts into the provided selectedcontent element. This is called when thePlease fix this WARNING reported by Spellchecker: "elemnts" is a possible misspelling of "elements".
To bypass Spellchecker, add ...
"elemnts" is a possible misspelling of "elements".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
// can use the cached currently selected option element as an optimization.uber-nit: `currently-selected`
}Shouldn't this branch have an `UpdateAllSelectedcontentsMultiple()` somewhere?
for (HTMLSelectedContentElement* selectedcontent :this would probably be easier to read as `auto*`.
if (IsMultiple()) {Can you `DCHECK()` in here that the provided `<selectedcontent>` is contained within this `<select>`?
void CloneContentsFromOptionElement(const HTMLOptionElement*);Would be good to have a comment here also, about the difference between the two.
for (Node& child : NodeTraversal::ChildrenOf(*option)) {
container->appendChild(child.cloneNode(/*deep=*/true));
}How come you can't use `ReplaceChildren()` here too?
test(() => {
const optionOne = document.querySelector('option.one');
const optionTwo = document.querySelector('option.two');
const optionThree = document.querySelector('option.three');
const selectedcontent = document.querySelector('selectedcontent');
assert_equals(selectedcontent.innerHTML, expectedHtml([optionOne, optionTwo]),
'The innerHTML of <selectedcontent> should initially match the innerHTML of the initially selected options.');
optionTwo.selected = false;
assert_equals(selectedcontent.innerHTML, expectedHtml([optionOne]),
'The innerHTML of <selectedcontent> should update after de-selecting option two.');
optionThree.selected = true;
assert_equals(selectedcontent.innerHTML, expectedHtml([optionOne, optionThree]),
'The innerHTML of <selectedcontent> should update after selecting option three.');
}, '<selectedcontent> should support <select multiple>.');
</script>It would be good to check the corner cases, like adding and removing selected options, and adding and removing `<selectedcontents>`.
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 9 comments![]( "Open in Gerrit")
void UpdateSelectedcontent(HTMLSelectedContentElement&);I think something like `UpdateContainedSelectedcontentElement()` might be a bit better? It's confusing to have both `UpdateAllSelectedcontents*` and `UpdateSelectedcontent`, and one updates all for the current `<select>` and the other updates just a single `<selectedcontent>` that is assumed to be within this select.
I renamed it to UpdateIndividualSelectedContent. How does that look?
// elemnts into the provided selectedcontent element. This is called when thePlease fix this WARNING reported by Spellchecker: "elemnts" is a possible misspelling of "elements".
To bypass Spellchecker, add ...
"elemnts" is a possible misspelling of "elements".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
Done
// can use the cached currently selected option element as an optimization.Joey Arharuber-nit: `currently-selected`
Done
Shouldn't this branch have an `UpdateAllSelectedcontentsMultiple()` somewhere?
done and added a test for it
for (HTMLSelectedContentElement* selectedcontent :this would probably be easier to read as `auto*`.
David keeps telling me to not use auto* in cases like this, I suspect that the style guide says not to use auto here.
if (IsMultiple()) {Can you `DCHECK()` in here that the provided `<selectedcontent>` is contained within this `<select>`?
Done
void CloneContentsFromOptionElement(const HTMLOptionElement*);Would be good to have a comment here also, about the difference between the two.
Done
for (Node& child : NodeTraversal::ChildrenOf(*option)) {
container->appendChild(child.cloneNode(/*deep=*/true));
}How come you can't use `ReplaceChildren()` here too?
This NodeTraversal::ChildrenOf(*option) loop is called in the same way as it is in the single-select method. If I wanted to use ReplaceChildren to replace the children of each generated <div> with the cloned children the option elements, then I'd have to create a vector to hold all the generated nodes with so I can pass them all to ReplaceChildren(), which would be more code than it looks like here - unless you have a more specific suggestion that I'm not seeing?
test(() => {
const optionOne = document.querySelector('option.one');
const optionTwo = document.querySelector('option.two');
const optionThree = document.querySelector('option.three');
const selectedcontent = document.querySelector('selectedcontent');
assert_equals(selectedcontent.innerHTML, expectedHtml([optionOne, optionTwo]),
'The innerHTML of <selectedcontent> should initially match the innerHTML of the initially selected options.');
optionTwo.selected = false;
assert_equals(selectedcontent.innerHTML, expectedHtml([optionOne]),
'The innerHTML of <selectedcontent> should update after de-selecting option two.');
optionThree.selected = true;
assert_equals(selectedcontent.innerHTML, expectedHtml([optionOne, optionThree]),
'The innerHTML of <selectedcontent> should update after selecting option three.');
}, '<selectedcontent> should support <select multiple>.');
</script>It would be good to check the corner cases, like adding and removing selected options, and adding and removing `<selectedcontents>`.
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 4 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
4 comments
void UpdateSelectedcontent(HTMLSelectedContentElement&);Joey ArharI think something like `UpdateContainedSelectedcontentElement()` might be a bit better? It's confusing to have both `UpdateAllSelectedcontents*` and `UpdateSelectedcontent`, and one updates all for the current `<select>` and the other updates just a single `<selectedcontent>` that is assumed to be within this select.
I renamed it to UpdateIndividualSelectedContent. How does that look?
Sure, that works, thanks.,
for (HTMLSelectedContentElement* selectedcontent :Joey Arharthis would probably be easier to read as `auto*`.
David keeps telling me to not use auto* in cases like this, I suspect that the style guide says not to use auto here.
The style guide basically says "whatever's more readable". And people differ. The approach I use is that if you can read that one line and tell roughly what kind of object `auto` comes out to, then use `auto`. In this case, it's on the fence, but it's a pretty good guess that you'll get a `<selectedcontent>` element, based on all of the `selectedcontent` words in that line.
I leave it to you! Either way is ok.
for (Node& child : NodeTraversal::ChildrenOf(*option)) {
container->appendChild(child.cloneNode(/*deep=*/true));
}Joey ArharHow come you can't use `ReplaceChildren()` here too?
This NodeTraversal::ChildrenOf(*option) loop is called in the same way as it is in the single-select method. If I wanted to use ReplaceChildren to replace the children of each generated <div> with the cloned children the option elements, then I'd have to create a vector to hold all the generated nodes with so I can pass them all to ReplaceChildren(), which would be more code than it looks like here - unless you have a more specific suggestion that I'm not seeing?
No I guess you're right. Maybe we should add a helper version of ReplaceChildren that takes in a `NodeList*` or something? I've seen this problem before.
Ok to do that later, and this is ok as-is.
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar voted and added 2 comments![]( "Open in Gerrit")
Votes added by Joey Arhar
| Code-Review | +1 |
| Commit-Queue | +2 |
2 comments
for (HTMLSelectedContentElement* selectedcontent :Joey Arharthis would probably be easier to read as `auto*`.
Mason FreedDavid keeps telling me to not use auto* in cases like this, I suspect that the style guide says not to use auto here.
The style guide basically says "whatever's more readable". And people differ. The approach I use is that if you can read that one line and tell roughly what kind of object `auto` comes out to, then use `auto`. In this case, it's on the fence, but it's a pretty good guess that you'll get a `<selectedcontent>` element, based on all of the `selectedcontent` words in that line.
I leave it to you! Either way is ok.
Acknowledged
for (Node& child : NodeTraversal::ChildrenOf(*option)) {
container->appendChild(child.cloneNode(/*deep=*/true));
}Joey ArharHow come you can't use `ReplaceChildren()` here too?
Mason FreedThis NodeTraversal::ChildrenOf(*option) loop is called in the same way as it is in the single-select method. If I wanted to use ReplaceChildren to replace the children of each generated <div> with the cloned children the option elements, then I'd have to create a vector to hold all the generated nodes with so I can pass them all to ReplaceChildren(), which would be more code than it looks like here - unless you have a more specific suggestion that I'm not seeing?
No I guess you're right. Maybe we should add a helper version of ReplaceChildren that takes in a `NodeList*` or something? I've seen this problem before.
Ok to do that later, and this is ok as-is.
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/59376.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |