[Platform-provided behaviors] Small refactor and added missing tests [chromium/src : main]
Ana Sollano Kim (Gerrit)
Ana Sollano Kim added 1 comment![]( "Open in Gerrit")
PTAL (I don't think the order of which CL is reviewed first matters).
Related details
- Dan Clark
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dan Clark (Gerrit)
Dan Clark added 1 comment![]( "Open in Gerrit")
HTMLFormElement* HTMLSubmitButtonBehavior::form(I'm sorry for missing this before, but it is probably better that these getters not throw, as a general principle:
https://www.w3.org/TR/design-principles/#attributes-vs-methods
Can these return an empty string / empty list instead, if there is no `internals`?
For the `labels` case we'll need to be a bit careful because of this other principle further down that list:
```
If the underlying object has not changed, getters should return the same
object each time it is called. This means obj.property === obj.property must
always hold. Returning a new value from an getter each time is not allowed. If
this does not hold, the getter should be a method.
```
This means we can't just create a new empty NodeList every time -- we'll need to cache one the first time this is called without `internals`, and then use that for future no-`internals` calls.
Related details
- Ana Sollano Kim
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. |
Ana Sollano Kim (Gerrit)
Ana Sollano Kim added 1 comment![]( "Open in Gerrit")
HTMLFormElement* HTMLSubmitButtonBehavior::form(I'm sorry for missing this before, but it is probably better that these getters not throw, as a general principle:
https://www.w3.org/TR/design-principles/#attributes-vs-methodsCan these return an empty string / empty list instead, if there is no `internals`?
For the `labels` case we'll need to be a bit careful because of this other principle further down that list:
```
If the underlying object has not changed, getters should return the same
object each time it is called. This means obj.property === obj.property must
always hold. Returning a new value from an getter each time is not allowed. If
this does not hold, the getter should be a method.
```This means we can't just create a new empty NodeList every time -- we'll need to cache one the first time this is called without `internals`, and then use that for future no-`internals` calls.
Looks like both `form()`[1] and `labels()`[2] already throw as getters. I was thinking we would need to follow that and don't prevent these exceptions from happening as we're delegating to ElementInternals for both of these cases. Do we want to not throw these exceptions if they happen?
[1] https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#dom-elementinternals-form
[2] https://html.spec.whatwg.org/multipage/forms.html#dom-elementinternals-labels
Related details
- Dan Clark
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. |
Dan Clark (Gerrit)
Dan Clark voted and added 2 comments![]( "Open in Gerrit")
Votes added by Dan Clark
| Code-Review | +1 |
2 comments
LGTM
HTMLFormElement* HTMLSubmitButtonBehavior::form(Ana Sollano KimI'm sorry for missing this before, but it is probably better that these getters not throw, as a general principle:
https://www.w3.org/TR/design-principles/#attributes-vs-methodsCan these return an empty string / empty list instead, if there is no `internals`?
For the `labels` case we'll need to be a bit careful because of this other principle further down that list:
```
If the underlying object has not changed, getters should return the same
object each time it is called. This means obj.property === obj.property must
always hold. Returning a new value from an getter each time is not allowed. If
this does not hold, the getter should be a method.
```This means we can't just create a new empty NodeList every time -- we'll need to cache one the first time this is called without `internals`, and then use that for future no-`internals` calls.
Looks like both `form()`[1] and `labels()`[2] already throw as getters. I was thinking we would need to follow that and don't prevent these exceptions from happening as we're delegating to ElementInternals for both of these cases. Do we want to not throw these exceptions if they happen?
[1] https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#dom-elementinternals-form
[2] https://html.spec.whatwg.org/multipage/forms.html#dom-elementinternals-labels
Ah, I missed that. I think it was probably a design mistake for those to throw, but being [consistent](https://www.w3.org/TR/design-principles/#consistency) with that behavior for these new getters makes sense.
Related details
- Ana Sollano Kim
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/59411.
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
- Ana Sollano Kim
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ana Sollano Kim (Gerrit)
Ana Sollano Kim voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[Platform-provided behaviors] Small refactor and added missing tests
Introduce a private helper to retrieve ElementInternals and throw
InvalidStateError exceptions when the behavior is not attached to an
element in HTMLSubmitButtonBehavior.
Two more tests are added to
ElementInternals-submit-behavior.tentative.html for form and formTarget
properties.
Explainer:
https://github.com/MicrosoftEdge/MSEdgeExplainers/blob/main/PlatformProvidedBehaviors/explainer.md
Design doc:
https://docs.google.com/document/d/1LA1hhzxmi4OmZoGtIdnwvL3g7y48YjXTOoUvFtxFugE/edit?usp=sharing
- M third_party/blink/renderer/core/html/forms/html_submit_button_behavior.cc
- M third_party/blink/renderer/core/html/forms/html_submit_button_behavior.h
- M third_party/blink/web_tests/external/wpt/custom-elements/element-internals-behaviors.tentative.html
- M third_party/blink/web_tests/external/wpt/custom-elements/form-associated/ElementInternals-submit-behavior.tentative.html
Code-Review: +1 by Dan Clark
| 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")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/59411
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |