[Platform-provided behaviors] Introduce HTMLSubmitButtonBehavior [chromium/src : main]
Ana Sollano Kim (Gerrit)
Ana Sollano Kim added 1 comment![]( "Open in Gerrit")
Related details
- Dan Clark
- Mason Freed
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 7 comments![]( "Open in Gerrit")
Looks good overall!
void SetBehaviors(HeapVector<Member<ElementBehavior>> behaviors,Given the recent discussion about only allowing behaviors to be set once, should this be removed from the IDL and made a private method that only gets called during `attachInternals`?
DCHECK(!internals_) << "Behavior is already attached to an element.";// HTMLSubmitButtonBehavior provides submit button behavior toSince we don't have a spec yet, could this link to the explainer?
attribute boolean disabled;Can we move this stuff into an `interface mixin` that's included by both this and by `HTMLButtonElement`? For an example see [`PopoverTargetAttributes`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/popover_target_attributes.idl).
const behavior = new HTMLSubmitButtonBehavior();Can you also check default values for `form` and `labels` here?
}, 'HTMLSubmitButtonBehavior properties are writable');Somewhere let's also check that `form` and `labels` are not writable
Related details
- Ana Sollano Kim
- 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 3 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
3 comments
This looks like a great start. Modulo my small comments and Dan's, this LGTM.
void SetBehaviors(HeapVector<Member<ElementBehavior>> behaviors,Given the recent discussion about only allowing behaviors to be set once, should this be removed from the IDL and made a private method that only gets called during `attachInternals`?
+1, if that's the direction you think it'll go.
"This behavior instance is already attached to another element.");nit: "This" is a bit tough to follow, if you pass in multiple. If it's easy (and only if its easy) I wonder if you could add the name of the behavior or something.
While I'm thinking about this, if you gave each ElementBehavior a statically defined `name` (e.g. `"HTMLSubmitButtonBehavior"` in this case), then you could use it here for display, and also use that to de-dupe the types, rather than `WrapperTypeInfo` comparisons.
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 7 comments![]( "Open in Gerrit")
void SetBehaviors(HeapVector<Member<ElementBehavior>> behaviors,Mason FreedGiven the recent discussion about only allowing behaviors to be set once, should this be removed from the IDL and made a private method that only gets called during `attachInternals`?
+1, if that's the direction you think it'll go.
Done. `SetBehaviors` was already not exposed in the IDL (`readonly attribute FrozenArray<ElementBehavior> behaviors`), but I made it private here.
"This behavior instance is already attached to another element.");nit: "This" is a bit tough to follow, if you pass in multiple. If it's easy (and only if its easy) I wonder if you could add the name of the behavior or something.
While I'm thinking about this, if you gave each ElementBehavior a statically defined `name` (e.g. `"HTMLSubmitButtonBehavior"` in this case), then you could use it here for display, and also use that to de-dupe the types, rather than `WrapperTypeInfo` comparisons.
Done. Added a `BehaviorName()` method to `ElementBehavior`. `SetBehaviors` now uses it for both deduplication and error messages:
`HTMLSubmitButtonBehavior instance is already in the behaviors list.`
`Only one instance of HTMLSubmitButtonBehavior is allowed per element.`
`HTMLSubmitButtonBehavior instance is already attached to another element.`
DCHECK(!internals_) << "Behavior is already attached to an element.";Done
// HTMLSubmitButtonBehavior provides submit button behavior toSince we don't have a spec yet, could this link to the explainer?
Done
attribute boolean disabled;Can we move this stuff into an `interface mixin` that's included by both this and by `HTMLButtonElement`? For an example see [`PopoverTargetAttributes`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/popover_target_attributes.idl).
I tried this but the properties on `HTMLButtonElement` and `HTMLSubmitButtonBehavior` seem to have incompatible IDL annotations:
- `HTMLButtonElement` uses `[CEReactions, Reflect]` or `[CEReactions]` on these properties.
- `HTMLSubmitButtonBehavior` doesn't reflect attributes like `<button>` does and `CEReactions` doesn't seem to apply since behaviors aren't elements.
I think we would need identical annotations in both contexts in order to use an `interface mixin`. Feel free to correct me if I'm missing something.
Can you also check default values for `form` and `labels` here?
Done
Somewhere let's also check that `form` and `labels` are not writable
- Dan Clark
- 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. |
Dan Clark (Gerrit)
Dan Clark voted and added 4 comments![]( "Open in Gerrit")
Votes added by Dan Clark
| Code-Review | +1 |
4 comments
if (seen_behaviors.Contains(behavior)) {I guess it's not strictly necessary to check this if we're also checking `seen_names`, since a behavior will always have the same name as itself. So this could be removed. But maybe this error message is more precise for this case, so I'm OK to leave it as-is if that's your preference.
attribute boolean disabled;Ana Sollano KimCan we move this stuff into an `interface mixin` that's included by both this and by `HTMLButtonElement`? For an example see [`PopoverTargetAttributes`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/popover_target_attributes.idl).
I tried this but the properties on `HTMLButtonElement` and `HTMLSubmitButtonBehavior` seem to have incompatible IDL annotations:
- `HTMLButtonElement` uses `[CEReactions, Reflect]` or `[CEReactions]` on these properties.
- `HTMLSubmitButtonBehavior` doesn't reflect attributes like `<button>` does and `CEReactions` doesn't seem to apply since behaviors aren't elements.
I think we would need identical annotations in both contexts in order to use an `interface mixin`. Feel free to correct me if I'm missing something.
That makes sense, thanks for trying it.
internals.SetBehaviors(std::move(behaviors), exception_state);Not a new issue to this change, but `internals.SetBehaviors` should be moved into the `if (options && options->hasBehaviors())` block. Otherwise we'll call `SetBehaviors` on every `attachInternals` call even if no behaviors were given, which will end up creating an empty behaviors `FrozenArray`.
Related details
- Ana Sollano Kim
- 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 3 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
3 comments
LGTM, thanks for the changes.
if (seen_behaviors.Contains(behavior)) {I guess it's not strictly necessary to check this if we're also checking `seen_names`, since a behavior will always have the same name as itself. So this could be removed. But maybe this error message is more precise for this case, so I'm OK to leave it as-is if that's your preference.
+1, and I don't actually think the error message is any different between the cases. So I'd be in favor of removing one of these.
(Ideally, there'd be a DCHECK or test somewhere that validates that all defined behavior classes indeed have unique names.)
"This behavior instance is already attached to another element.");Ana Sollano Kimnit: "This" is a bit tough to follow, if you pass in multiple. If it's easy (and only if its easy) I wonder if you could add the name of the behavior or something.
While I'm thinking about this, if you gave each ElementBehavior a statically defined `name` (e.g. `"HTMLSubmitButtonBehavior"` in this case), then you could use it here for display, and also use that to de-dupe the types, rather than `WrapperTypeInfo` comparisons.
Done. Added a `BehaviorName()` method to `ElementBehavior`. `SetBehaviors` now uses it for both deduplication and error messages:
`HTMLSubmitButtonBehavior instance is already in the behaviors list.`
`Only one instance of HTMLSubmitButtonBehavior is allowed per element.`
`HTMLSubmitButtonBehavior instance is already attached to another element.`
Nice!
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 voted and added 3 comments![]( "Open in Gerrit")
Votes added by Ana Sollano Kim
| Commit-Queue | +2 |
Thanks for the feedback!
Mason FreedI guess it's not strictly necessary to check this if we're also checking `seen_names`, since a behavior will always have the same name as itself. So this could be removed. But maybe this error message is more precise for this case, so I'm OK to leave it as-is if that's your preference.
+1, and I don't actually think the error message is any different between the cases. So I'd be in favor of removing one of these.
(Ideally, there'd be a DCHECK or test somewhere that validates that all defined behavior classes indeed have unique names.)
Removed the `seen_behaviors` instance check.
Regarding validating unique names across behavior classes: I'll add a DCHECK/test when a second behavior class is introduced.
internals.SetBehaviors(std::move(behaviors), exception_state);Not a new issue to this change, but `internals.SetBehaviors` should be moved into the `if (options && options->hasBehaviors())` block. Otherwise we'll call `SetBehaviors` on every `attachInternals` call even if no behaviors were given, which will end up creating an empty behaviors `FrozenArray`.
This approach makes sense to me. I changed it because this comment on CL 1 https://chromium-review.googlesource.com/c/chromium/src/+/7666964/comment/1cbf45b0_ae22550f/. Happy to revisit again if needed.
Related details
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/58892.
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
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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dan Clark
| Code-Review | +1 |
1 comment
internals.SetBehaviors(std::move(behaviors), exception_state);Ana Sollano KimNot a new issue to this change, but `internals.SetBehaviors` should be moved into the `if (options && options->hasBehaviors())` block. Otherwise we'll call `SetBehaviors` on every `attachInternals` call even if no behaviors were given, which will end up creating an empty behaviors `FrozenArray`.
This approach makes sense to me. I changed it because this comment on CL 1 https://chromium-review.googlesource.com/c/chromium/src/+/7666964/comment/1cbf45b0_ae22550f/. Happy to revisit again if needed.
Ah, sorry, I forgot we could still need to hand out the empty array. There might actually be a problem with the `DEFINE_STATIC_LOCAL` approach here because if two `internals` with an empty `behaviors` array compare their `behaviors` array, the comparison would actually return true because they're reusing the same empty value. But they should be different.
Probably Mason's suggestion in the earlier CL was right and it's simplest to just always call `SetBehaviors`. OK to fix now or in a follow-up (with a test for comparing empty `behaviors` array identity from two different `ElementInternals`, which would be a good one to add) in order to not delay this CL further. Apologies for the bad suggestion.
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. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[Platform-provided behaviors] Introduce HTMLSubmitButtonBehavior
This change introduces the HTMLSubmitButtonBehavior interface, which
will allow custom elements to adopt submit button functionality and
attribute handling by attaching this behavior via ElementInternals.
In this CL, ElementInternals is updated to validate behavior attachment,
preventing duplicate behavior types or the reuse of behavior instances
across multiple elements. Property delegation for form association and
labeling and support for standard attributes is added to
HTMLSubmitButtonBehavior as well. Upcoming CLs will implement form
submission logic.
Explainer:
https://github.com/MicrosoftEdge/MSEdgeExplainers/blob/main/PlatformProvidedBehaviors/explainer.md
Design doc:
https://docs.google.com/document/d/1LA1hhzxmi4OmZoGtIdnwvL3g7y48YjXTOoUvFtxFugE/edit?usp=sharing
- M android_webview/test/data/web_tests/webexposed/global-interface-listing-expected.txt
- M third_party/blink/renderer/bindings/generated_in_core.gni
- M third_party/blink/renderer/bindings/idl_in_core.gni
- M third_party/blink/renderer/core/html/build.gni
- M third_party/blink/renderer/core/html/custom/element_internals.cc
- M third_party/blink/renderer/core/html/custom/element_internals.h
- M third_party/blink/renderer/core/html/forms/element_behavior.cc
- M third_party/blink/renderer/core/html/forms/element_behavior.h
- M third_party/blink/renderer/core/html/forms/element_behavior.idl
- A third_party/blink/renderer/core/html/forms/html_submit_button_behavior.cc
- A third_party/blink/renderer/core/html/forms/html_submit_button_behavior.h
- A third_party/blink/renderer/core/html/forms/html_submit_button_behavior.idl
- M third_party/blink/renderer/core/html/html_element.cc
- M third_party/blink/web_tests/external/wpt/custom-elements/element-internals-behaviors.tentative.html
- A third_party/blink/web_tests/external/wpt/custom-elements/form-associated/ElementInternals-submit-behavior.tentative.html
- M third_party/blink/web_tests/webexposed/global-interface-listing-expected.txt
Code-Review: +1 by Dan Clark, +1 by Mason Freed
| 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/58892
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |