Details

Reviewers

Commits

rMOZILLACENTRAL9305bb731ce8: Bug 1857887 - Add invoketarget & invoketarget action attributes r=smaug

Bugzilla Bug ID

Summary

This adds support for the experimental invoketarget and invokeaction
attributes, as specified in the open-ui "Invokers" explainer.

(https://open-ui.org/components/invokers.explainer/)

The invoketarget attribute maps to the IDL invokeTargetElement,
similar to popoverTargetElement, and the invokeaction is a freeform
string.

The Button behaviour checks for invokeTargetElement in its activation
behaviour, and dispatches an InvokeEvent if there is one.

This also adds some basic scaffolding for handleInvokeInternal which
will allow elements to provide their own invocation action algorithms.

Diff Detail

Repository

rMOZILLACENTRAL mozilla-central

Build Status

Buildable 595312
Build 693444: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

smaug added inline comments.Oct 12 2023, 11:13 AM

dom/events/InvokeEvent.h
2	There shouldn't be need for InvokeEvent.h/.cpp
dom/html/HTMLInputElement.cpp
3993–3994	(hmm, not about this patch, but is there a bug in popover implementation or spec if it lets type=reset to have also other side effect through triggering other functionality)
dom/html/nsGenericHTMLElement.cpp
2892	No need for HasAttr check. GetParsedAttr should return null if there is no attribute. So just take its return value and null check, that way we avoid extra attribute search.
2901	This should return null if StaticPrefs::dom_element_invokers_enabled() is false.
dom/webidl/InvokeEvent.webidl
22	This is a simple event, and our code generator can create its whole C++ implementation. Just have this .webidl file and add it to https://searchfox.org/mozilla-central/rev/e9b338c2d597067f99e96d5f20769f41f312fa8f/dom/webidl/moz.build#1161

This revision now requires changes to proceed.Oct 12 2023, 11:13 AM

keithamus added inline comments.Oct 12 2023, 10:56 PM

dom/html/HTMLInputElement.cpp
3993–3994	FWIW I think it's desirable as you might want a Dialog's close button to reset the form.
dom/webidl/InvokeEvent.webidl
22	Thanks for the advice. One issue I'm having is establishing how best to ensure `relatedTarget` is correctly re-targeting when the event bubbles out of a shadowroot, and that behaviour doesn't seem to come for free hence the (attempt) of using `EnsureWebAccessibleRelatedTarget` in the cpp file. Is there a way to get re-targeting of `relaedTarget` while relying on the idl?

Oh I see, it has that related target.
And the idea is that idref (or rather, the explicitly set element) can be also to outside the current tree?

ok, then we do need the explicit .cpp implementation, since it is rather unusual event.

But the related target makes dispatch a bit odd
https://dom.spec.whatwg.org/#dispatching-events 5.9.7

In some cases the event could propagate from the shadow dom, in some cases it wouldn't. Is that expected for invoke, which is like an alternative event for click?

smaug added inline comments.Oct 13 2023, 10:24 AM

widget/EventClassList.h
52 ↗	(On Diff #774172)	I don't think this is needed. These event classes are about internal low level events.

keithamus requested review of this revision.Oct 18 2023, 11:42 AM

keithamus updated this revision to Diff 776954.

keithamus marked 2 inline comments as done.

Harbormaster completed remote builds in B588496: Diff 776954.Oct 18 2023, 11:42 AM

keithamus added inline comments.Oct 18 2023, 11:44 AM

dom/events/InvokeEvent.h
2	This is now needed because `GetInvoker` needs to `Retarget` the invoker to the `currentTarget`, which AFAIK the IDL doesn't have provisions for.

Code analysis found 3 defects in diff 776954:

2 build errors and 1 defect found by clang-tidy

1 defect unresolved compared to the previous diff 774172.

WARNING: Found 1 defect (warning level) that can be dismissed.

IMPORTANT: Found 2 defects (error level) that must be fixed before landing.

You can run this analysis locally with:

./mach static-analysis check --outgoing (C/C++)

If you see a problem in this automated review, please report it here.

You can view these defects in the Diff Detail section of Phabricator diff 776954.

keithamus updated this revision to Diff 777389.Oct 18 2023, 10:38 PM

Harbormaster completed remote builds in B588868: Diff 777389.Oct 18 2023, 10:38 PM

Code analysis found 1 defect in diff 777389:

1 defect found by clang-tidy

1 defect unresolved and 2 defects closed compared to the previous diff 776954.

WARNING: Found 1 defect (warning level) that can be dismissed.

You can run this analysis locally with:

./mach static-analysis check --outgoing (C/C++)

If you see a problem in this automated review, please report it here.

You can view these defects in the Diff Detail section of Phabricator diff 777389.

smaug requested changes to this revision.Oct 23 2023, 10:51 PM

smaug added inline comments.

dom/events/InvokeEvent.cpp
29	This is unexpected. If empty string is pass, action should stay empty. If we want "auto" to be the default value, then "auto" should be used in webidl. But even then if one explicitly passes empty string, then the value should stay empty, that is how event interfaces work.
dom/events/InvokeEvent.h
42	This needs to be a strong reference. RefPtr<Element> mInvoker; and then that needs to be added to cycle collection. See for example https://searchfox.org/mozilla-central/rev/e4afef5d3ff67781dc1377912344694f3cf3a226/dom/events/DeviceMotionEvent.h#77,79 and https://searchfox.org/mozilla-central/rev/e4afef5d3ff67781dc1377912344694f3cf3a226/dom/events/DeviceMotionEvent.cpp#16-23 how to add stuff to cycle collection. The idea is to just let CC to know about the strong member variable.
dom/html/nsGenericHTMLElement.cpp
2890	Why RefPtr and not just nsAtom* ?
testing/web-platform/tests/html/semantics/invokers/invokeevent-interface.tentative.html
46	There should be a test where action "" is passed to the constructor and then event.action should be ""

This revision now requires changes to proceed.Oct 23 2023, 10:51 PM

keithamus requested review of this revision.Oct 24 2023, 6:49 PM

keithamus updated this revision to Diff 779208.

keithamus marked an inline comment as done.

Harbormaster completed remote builds in B590416: Diff 779208.Oct 24 2023, 6:50 PM

Thanks for your feedback, I've addressed the comments, hopefully satisfactorily.

keithamus marked 2 inline comments as done.Oct 24 2023, 8:40 PM

Starting to look really good

testing/web-platform/tests/html/semantics/invokers/invokeelement-interface.tentative.html
68	Perhaps this test could have a check for getAttribute("invokeaction") value
testing/web-platform/tests/html/semantics/invokers/invokeevent-dispatch-shadow.tentative.html
13 ↗	(On Diff #779208)	Do you use these elements anywhere?
testing/web-platform/tests/html/semantics/invokers/invokeevent-interface.tentative.html
87	Not sure what this is trying to test. Would use of an object with toString be more interesting?
testing/web-platform/tests/html/semantics/invokers/invoketarget-button-event-dispatch.tentative.html
23	This is surprising. Why isn't the event composed? Same also elsewhere
27	This test should check the value of cancelable too. Same also elsewhere.
92	I would expect invoke event be cancelable and there should be tests for that. (When I'm writing this I haven't yet gone through all the tests, so maybe there are tests)
testing/web-platform/tests/interfaces/invokers.tentative.idl
14	"auto"

This revision now requires changes to proceed.Oct 24 2023, 11:12 PM

keithamus requested review of this revision.Oct 25 2023, 8:54 AM

keithamus updated this revision to Diff 779466.

keithamus marked 5 inline comments as done.

Harbormaster completed remote builds in B590651: Diff 779466.Oct 25 2023, 8:54 AM

1 defect closed compared to the previous diff 779208.

If you see a problem in this automated review, please report it here.

keithamus added inline comments.Oct 25 2023, 9:42 AM

testing/web-platform/tests/html/semantics/invokers/invoketarget-button-event-dispatch.tentative.html
23	It should be composed? Do we want to expose invoke events through a shadow boundary?
92	There are tests for this in the subsequent default behaviours, but I'm not sure what I'd test here. Would the test be that preventDefault() sets the cancelled flag and defaultPrevented() returns it? Is that not already a base truth of those methods?

@smaug I don't know how much it matters but the Chrome was merged (including the WPT files) which means https://bugzilla.mozilla.org/show_bug.cgi?id=1858194 has been filed. Should we wait for that to land and then rebase this?

smaug added inline comments.Oct 25 2023, 11:12 AM

testing/web-platform/tests/html/semantics/invokers/invoketarget-button-event-dispatch.tentative.html
23	I thought we did want that. Click is composed.
92	ah, ok, if there are tests elsewhere, then fine.

keithamus added inline comments.Oct 25 2023, 11:26 AM

testing/web-platform/tests/html/semantics/invokers/invoketarget-button-event-dispatch.tentative.html
23	I have no strong preference, but I had set it to non-composed because I feel as though invokes within a single shadow root would leak information about the structure of the shadow root (that it has a button, and that it is invoking another part of its tree). But perhaps this is okay as it's not leaking any node references, just information?

smaug requested changes to this revision.Oct 28 2023, 10:37 AM

smaug added inline comments.

testing/web-platform/tests/html/semantics/invokers/invoketarget-button-event-dispatch.tentative.html
23	If it is not composed, then embedder/user of some component is forced to use click always, to for example call preventDefault(). And if it is not composed, what would be the reason to retarget invoker?

This revision now requires changes to proceed.Oct 28 2023, 10:37 AM

keithamus requested review of this revision.Nov 3 2023, 12:04 AM

keithamus updated this revision to Diff 782809.

Harbormaster completed remote builds in B593440: Diff 782809.Nov 3 2023, 12:04 AM

Okay @smaug I think this is ready for another review!

keithamus updated this revision to Diff 782810.Nov 3 2023, 12:11 AM

Harbormaster completed remote builds in B593441: Diff 782810.Nov 3 2023, 12:12 AM

keithamus updated this revision to Diff 782939.Nov 3 2023, 11:31 AM

Harbormaster completed remote builds in B593538: Diff 782939.Nov 3 2023, 11:31 AM

Minor nits still.

And you got some (valid) feedback to the intent-to-prototype, but I guess that is more for the spec discussion. And we can land initial prototype and tweak it later when the spec evolves.

dom/html/HTMLButtonElement.cpp
282	Are there tests that invoketarget has higher priority than popovertarget?
dom/html/nsGenericHTMLElement.cpp
2845	Nit, would be good to combine these two 'if (aNamespaceID == kNameSpaceID_None &&' ifs to one, I mean there is now one for popover and one for invoke
2914	Hmm, why does the target need to be HTML element? The event seems to be dispatched to any kind of HTML element, even those which won't then do anything with HandleInvokeInternal(). So why is HTML special here? couldn't we let all the elements be possible targets for the event? That would make use of the event a bit more flexible, I think. Are there tests for the case when target is non-HTML element?

This revision now requires changes to proceed.Nov 3 2023, 4:33 PM

keithamus requested review of this revision.Nov 3 2023, 10:49 PM

keithamus updated this revision to Diff 783248.

Harbormaster completed remote builds in B593819: Diff 783248.Nov 3 2023, 10:49 PM

keithamus marked an inline comment as done.Nov 3 2023, 10:50 PM

keithamus added inline comments.

dom/html/HTMLButtonElement.cpp
282	It's definitely the intent but there aren't yet. Is it okay for me to follow up with tests to that confirm this, when I land the popover invoker behaviour?
dom/html/nsGenericHTMLElement.cpp
2914	Yes of course, I'm not sure why I did this. Element it is! I've added a test for this with an SVG element also.

smaug accepted this revision.Nov 4 2023, 11:47 AM

smaug added a project: testing-approved.

This revision is now accepted and ready to land.Nov 4 2023, 11:47 AM

keithamus marked 3 inline comments as done.Nov 8 2023, 1:41 PM

Closed by commit rMOZILLACENTRAL9305bb731ce8: Bug 1857887 - Add invoketarget & invoketarget action attributes r=smaug (authored by keithamus). · Explain WhyNov 8 2023, 1:49 PM

This revision was automatically updated to reflect the committed changes.

keithamus added a commit: rMOZILLACENTRAL9305bb731ce8: Bug 1857887 - Add invoketarget & invoketarget action attributes r=smaug.

Bug 1857887 - Add invoketarget & invoketarget action attributes r=smaug
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 784749

dom/base/Element.h

dom/events/InvokeEvent.h

dom/events/InvokeEvent.cpp

dom/events/moz.build

dom/html/HTMLButtonElement.cpp

dom/html/HTMLInputElement.cpp

dom/html/nsGenericHTMLElement.h

dom/html/nsGenericHTMLElement.cpp

dom/webidl/HTMLButtonElement.webidl

dom/webidl/HTMLInputElement.webidl

dom/webidl/InvokeEvent.webidl

dom/webidl/InvokerElement.webidl

dom/webidl/moz.build

modules/libpref/init/StaticPrefList.yaml

testing/web-platform/meta/html/semantics/invokers/dir.ini

testing/web-platform/meta/html/semantics/invokers/idlharness.tentative.html.ini

testing/web-platform/meta/html/semantics/invokers/invokeelement-interface.tentative.html.ini

testing/web-platform/meta/html/semantics/invokers/invokeevent-dispatch-shadow.tentative.html.ini

testing/web-platform/meta/html/semantics/invokers/invokeevent-interface.tentative.html.ini

testing/web-platform/meta/html/semantics/invokers/invoketarget-button-event-dispatch.tentative.html.ini

testing/web-platform/tests/html/semantics/invokers/invokeelement-interface.tentative.html

testing/web-platform/tests/html/semantics/invokers/invokeevent-interface.tentative.html

testing/web-platform/tests/html/semantics/invokers/invoketarget-button-event-dispatch.tentative.html

testing/web-platform/tests/interfaces/invokers.tentative.idl

xpcom/ds/StaticAtoms.py

Bug 1857887 - Add invoketarget & invoketarget action attributes r=smaugClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 784749

dom/base/Element.h

dom/events/InvokeEvent.h

dom/events/InvokeEvent.cpp

dom/events/moz.build

dom/html/HTMLButtonElement.cpp

dom/html/HTMLInputElement.cpp

dom/html/nsGenericHTMLElement.h

dom/html/nsGenericHTMLElement.cpp

dom/webidl/HTMLButtonElement.webidl

dom/webidl/HTMLInputElement.webidl

dom/webidl/InvokeEvent.webidl

dom/webidl/InvokerElement.webidl

dom/webidl/moz.build

modules/libpref/init/StaticPrefList.yaml

testing/web-platform/meta/html/semantics/invokers/__dir__.ini

testing/web-platform/meta/html/semantics/invokers/idlharness.tentative.html.ini

testing/web-platform/meta/html/semantics/invokers/invokeelement-interface.tentative.html.ini

testing/web-platform/meta/html/semantics/invokers/invokeevent-dispatch-shadow.tentative.html.ini

testing/web-platform/meta/html/semantics/invokers/invokeevent-interface.tentative.html.ini

testing/web-platform/meta/html/semantics/invokers/invoketarget-button-event-dispatch.tentative.html.ini

testing/web-platform/tests/html/semantics/invokers/invokeelement-interface.tentative.html

testing/web-platform/tests/html/semantics/invokers/invokeevent-interface.tentative.html

testing/web-platform/tests/html/semantics/invokers/invoketarget-button-event-dispatch.tentative.html

testing/web-platform/tests/interfaces/invokers.tentative.idl

xpcom/ds/StaticAtoms.py

Bug 1857887 - Add invoketarget & invoketarget action attributes r=smaug
ClosedPublic
Actions

Revision Contents
Changeset List

testing/web-platform/meta/html/semantics/invokers/dir.ini