-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Re add inert #4288
Re add inert #4288
Conversation
Replying to #1474 (comment)
That actually seems right to me. @robdodson had the same reaction when I checked with him. (Edited to add) Specifically, I think it would be more confusing for authors if some events worked, sometimes, than if events simply didn't work. However, I just realised there's still an issue with event delegation as written, so some edits to UI events are still needed...
Agreed.
More like (2) (which was poorly worded) - the event fires, but nothing happens at the targeting phase. Capture/bubble work as normal. |
Actually, I think we can avoid that. Will make some edits and ping this PR when ready. |
Hmm. I don't think this is OK. We can't have a high-level, UI/UX HTML-level content attribute interfere with foundational EventTarget-level technologies like dispatching custom events. We should only be changing UI interaction behavior, not general event functionality. |
Ok, I'm convinced. Will look at UI Events. |
Hm - so I had a chat to Gary K and apparently there's no easy place for it to hook in right now. Some options:
|
Hit testing is indeed not defined, but can be observed through various means (e.g., Focusing is defined in HTML, so could be patched there. Selection is in https://w3c.github.io/selection-api/. |
I don't think there's a great solution here in terms of creating a perfect spec, at least until w3c/uievents#200 is done. So we just need to focus practically on writing a good enough spec to get interop. For that route, I would:
As for defining in terms of pointer-events, it appears that's not specced anywhere precisely because properly speccing it would require defining hit testing: https://wiki.csswg.org/spec/css4-ui#pointer-events :) |
I'm not sure I understand why not. It's commonly understood that most UI Events always goes through hit testing (keyboard stuff relies on focus). |
@alice, can you comment on how Chrome's implementation of |
When checking whether a layout object/paint fragment is visible to hit testing, the associated Node is queried for both pointer events style and inertness. pointer-events explicitly allows the "transparent to click events" behaviour, which I find frankly alarming if not paired with at least partial transparency. |
I spoke to Tab about this, and he explained what So, I think that's kind of back to the drawing board a little bit. I definitely want the "absorb events" behaviour rather than the "transparent to hit testing" behaviour. Will keep thinking about how that might be written. |
Sounds like the difference between the two will make for some good web platform tests! But yeah, writing up bespoke text that's clear enough to get interoperability for now, and later maybe grounding in a new CSS primitive, makes sense to me. |
I added a blacklist of events not to be fired, and a comment about I decided the hit testing piece is probably unnecessary - for example, |
On further thought, decided I did need to say something about hit testing - these elements will still show up for hit testing in general, but events which would be targeted by hit testing should be targeted at ancestors instead. The wording is a little convoluted - my intention is to avoid the "click through to an unrelated element" scenario which |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this!
source
Outdated
|
||
<ul> | ||
<li> | ||
<p>the user agent must not fire any of the following events on the node:</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- When would this safelist apply? Does this only apply when the user agent is responsible for dispatching these events? Or would this also happen for synthetic events?
- It seems highly likely this safelist will go out-of-date when new events get added. What about pointer events for instance? I'd be much more comfortable with this kind of setup if there were some intrinsic properties on the event object that indicate whether to filter it as such.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- In either case.
- There's not really any good option here; I agree the blacklist will need to be kept up to date until we have some kind of way of referring to user input events as a class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So then it should probably be defined as a modification of https://dom.spec.whatwg.org/#concept-event-dispatch?
Are pointer events in or out? That wasn't clear from your answer.
Does it apply regardless of what class the event implements? E.g., click
with CustomEvent
gets filtered?
A lot of that seems rather different from existing setups, so I'm not entirely convinced it's the design we want to use.
(FWIW, we try to avoid using "whitelist" and "blacklist", preferring "safelist" and "blocklist": https://whatwg.org/style-guide#tone.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood re language (and thanks for the pointer) - I meant more that it's a blocklist rather than a safelist, since these events will not fire.
I do think this is the design we want in terms of behaviour - in lay terms, we want any event which is or could be fired via a user interaction (which, yes, includes pointer events) not to be fired on an element which is inert. Perhaps I can find a better way to say that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't think this should change event dispatch or prevent people from doing dispatchEvent
... I think instead inert should be about blocking user interaction from translating into events.
Stated another way, I think it's better if the base EventTarget class does not give special semantics to certain event names, or to whether the EventTarget is actually a HTML Element with a certain content attribute. That seems like a layering violation; EventTarget should just be concerned with a generic eventing system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I'm not sure we'd want people to be able to dispatch click
events programmatically on inert
elements, would we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should stop them... programmatic event dispatch is basically just an indirect way of doing function calls, by coordinating everything through various strings. Whether those strings are named "click"
or "myevent"
shouldn't, IMO, effect whether this indirect-function-call mechanism works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmmm. I guess we could try this out, then:
- Remove the list of forbidden events
- Include the "topmost event target" re-targeting language
- Include the existing language in https://html.spec.whatwg.org/#focus-processing-model about not focusing inert elements and note in the inert section that inert elements generally aren't focusable
- Include the points about find in page and selection potentially being disallowed by the user agent.
That would make inert
elements effectively unable to be interacted with via a user interaction, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Did that ☝️)
source
Outdated
|
||
<li><p>the user agent may prevent the user from selecting text in that node, and may prevent | ||
code calling <code data-x="dom-textarea/input-setSelectionRange">setSelectionRange()</code> | ||
on the node.</p></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be better defined as a modification of the setSelectionRange()
algorithm, as currently it's unclear what this means.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
source
Outdated
<span data-x="inert">inertness</span>.</p> | ||
named "inert". This section merely defines an abstract <em>concept</em> of <span | ||
data-x="inert">inertness</span>. See <code data-x="attr-inert">inert</code> for an explanation of | ||
the attribute of the same name.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd make this something like "See also the inert attribute." if we think we need to keep this note at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
104a982
to
b9b3fb2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great to me. I made some minor editorial tweaks, but I am quite happy with the event dispatching changes. I know @annevk and @smaug---- are a bit more demanding there, so it'll be good to get their take. And of course web platform tests will help.
But from my perspective, this is ready to merge.
source
Outdated
is omitted, it will be reset to be the platform default (none or forward).</p> | ||
<p>If <var>element</var> is not <span>inert</span>, changes the selection to cover the given | ||
substring in the given direction. If the direction is omitted, it will be reset to be the | ||
platform default (none or forward).</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the non-normative description of the method. The normative description should also be changed, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added an early return into the normative description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, reading this more closely, shouldn't this affect the other selection methods as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved it into the "set the selection range" algorithm, what do you think?
source
Outdated
<li> | ||
<p>If an event would be fired as a result of user input, such that the | ||
<span>inert</span> element would be the <span data-x="topmost-event-target">topmost event | ||
target</span>, an alternative <span data-x="event-target">event target</span> must be found as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is topmost event target always an element? The earlier text suggests it could be a text node too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://w3c.github.io/uievents/#topmost-event-target says "[t]he topmost event target MUST be the element highest in the rendering order which is capable of being an event target", so it seems like we can assume it is always an element.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe? Not sure UI Events can be trusted. But that means the earlier text is wrong, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessarily. The following two points specifically concern text nodes:
The user interaction events, agent may ignore the node for the purposes of text search user interfaces (commonly known as "find in page"), page"); and
The user agent may prevent the user from selecting text in that node.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is unclear is if the current text applies when it's not an element that is inert or if it should be ignored in that case. (Perhaps there's no way to get UI events to fire on non-elements though, but then it comes back to the synthetic cases and I've somewhat forgotten what we decided on there.) It's rather hard to revisit this every couple of weeks and remember all the various details. 😟
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On reflection, I'm happy to change it to Node, seems like that would be the most inclusive and not inaccurate.
source
Outdated
<li><p>Add <var>candidate</var> to <var>ineligible</var>.</p></li> | ||
<li><p>Let <var>candidate</var> be the element which would be the <span | ||
data-x="topmost-event-target">topmost event target</span> if all the elements in | ||
<var>ineligible</var> were excluded from consideration.</p></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you'd have to perform hit testing again? In particular, if a position a div
under an inert
element, that div
element would become candidate?
Do we have tests for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working on tests, yeah.
The intent is that if you position a div
which is not an ancestor under the inert
element, that div
would not be targeted. The old wording, and indeed the old implementation in blink, would have targeted the event at the non-ancestor div
, as does pointer-events: none
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarification, sorry - the div
would become candidate, but would be added to ineligible as it is not an ancestor.
I've looked at implementing this in Blink, and it seems to be possible to do without performing redundant hit testing at least in our codebase - rather, you continue the hit testing process until a suitable candidate is found.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right, thanks. Ideally this shares infrastructure with https://drafts.csswg.org/cssom-view/#dom-document-elementsfrompoint I think, which should use the same primitive.
This also needs to be rephrased a bit using "Set candidate to" as you already initialized it earlier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally this shares infrastructure with https://drafts.csswg.org/cssom-view/#dom-document-elementsfrompoint I think, which should use the same primitive.
Do you mean the implementation, or the spec? The implementation does indeed share infastructure.
This also needs to be rephrased a bit using "Set candidate to" as you already initialized it earlier.
Done, thanks.
(Edited because I originally pasted the wrong thing into my quote)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a lot of spec technical debt to be cleared up around hit testing.
UIEvents says:
The topmost event target MUST be the element highest in the rendering order which is capable of being an event target. In graphical user interfaces this is the element under the user’s pointing device. A user interface’s hit testing facility is used to determine the target. For specific details regarding hit testing and stacking order, refer to the host language.
As far as I know, "hit testing" is not defined for HTML.
The elementsFromPoint
definition says:
For each layout box in the viewport, in paint order, starting with the topmost box, that would be a target for hit testing at coordinates x,y even if nothing would be overlapping it, when applying the transforms that apply to the descendants of the viewport, append the associated element to sequence.
without even attempting to define "hit testing" or "paint order".
source
Outdated
<li><dfn data-x="event-compositionupdate" data-x-href="https://w3c.github.io/uievents/#event-type-compositionupdate"><code>compositionupdate</code></dfn> event</li> | ||
<li><dfn data-x="event-compositionend" data-x-href="https://w3c.github.io/uievents/#event-type-compositionend"><code>compositionend</code></dfn> event</li> | ||
|
||
<li>The <dfn data-x="event-target" data-x-href="https://w3c.github.io/uievents/#event-target">event target</dfn> concept</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should add this reference as it's rather confusing and duplicative of the DOM Standard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, the term event target
is not actually defined in the DOM standard, and the way I'm using it refers specifically to the sense used in UIEvents.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, but I think we need to find some other way as UI Events really shouldn't define this term and we shouldn't take a new dependency on that "flaw".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem like a reasonable objection, to be honest. I don't think there is another way (if you think of one, please let me know), and I don't think it affects implementability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to discuss this further, but I don't think adding a dependency on a bug in UI Events is a good strategy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bug being that UIEvents defines this term?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per my understanding of the setup here all of this happens before the dispatch algorithm. The dispatch algorithm is handed a target and an event, but it might fiddle with these as it sees fit (e.g., to account for shadow trees).
The manipulation you define is therefore about the eventual input for the dispatch algorithm at which point you can't really speak about an event target. And UI Events also shouldn't define the term the way it does because that gives the impression you are able to manipulate it in this manner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. Leaving aside "topmost event target", the only references to "event target" are these two, in the retargeting rules:
... an alternative event target must be found as follows:
If candidate is not null, let candidate be the new event target.
I agree that the manipulation is about the eventual input for the dispatch algorithm. The input for these steps is what the UIEvents spec refers to as the "topmost event target" (really more a potential event target), and the output is the input to the dispatch algorithm.
I think this is correct; we only want to do this manipulation when there is hit testing involved, not as part of the dispatch algorithm in general. Effectively, we want to change the output of the hit testing.
source
Outdated
|
||
<ul> | ||
<li> | ||
<p>If an event would be fired as a result of user input, such that the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear what this would count. E.g., form submission can be the result of user input, can that be prevented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would the form be the topmost event target in that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's still not entirely clear to me what topmost event target means.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A better question, to help us figure this out: what user input would cause a form to be submitted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might be a click somewhere or some such (though I suppose even click is not technically user input).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can read that in two ways:
- The user clicks at the location of the submit button within the form.
- In that case, the submit button is the topmost event target, and since it is within the form and the form is inert, the click event is retargeted, possibly at the form's parent element (depending on the circumstances which led to the form being inert).
- The user clicks at the location of a non-inert button outside the form, which would have the effect of submitting the form due to javascript running in its click event handler.
- In that case, the button is the topmost event target, and the click event is fired as a result of the user interaction since the button is not inert.
I'm guessing you're thinking of the second case? I guess you could argue that in that case, the form submission event is "as a result of user input", making "topmost event target" ambiguous, but I don't know that it would be a likely interpretation.
Just a quick comment to say thanks for the thorough review. I'll be out for the next week and a half so it might take me a bit to get back to all of it, but just wanted to say it's well appreciated, and in particular that I'm planning to work on the WPT tests for the new behaviour (including retargeting) soon. |
Sorry for all the noise, I am so bad at github. Anyway, I've tried to address all your comments (WPT tests are still WIP) - thanks again! |
(Not sure why the build failed; it passed for me locally. Would it be possible to re-run?) |
Sorry for the delay; build re-started, and filed whatwg/html-build#194 to track making the script more resilient to network failures. |
Firefox 81 makes a second implementation, see https://bugzilla.mozilla.org/show_bug.cgi?id=1655722 |
Excellent! Do either Firefox or Chrome have plans to ship? |
@asurkov Just to confirm, will inert be behind a flag in Firefox 81? @alice Is inert still behind a feature flag in Chrome? [Edit, April 9, 2021: Yes, still behind a flag in both Firefox and Chrome. The only mainstream browser that doesn't have an implementation yet is webkit/Safari]. |
correct, Firefox 81 will have html5.inert.enabled preference to turn the feature on
I think there are at least two blocking issues before it can be shipped in Firefox, which are
|
This reverts the edits made in whatwg@5ddfc78 (and whatwg@b0ec716) which removed the inert attribute. The inert attribute was originally added in whatwg@2fb24fc as part of the <dialog> feature. It is currently being re-considered as part of the https://github.com/WICG/inert proposal.
<li><p>Let <var>ineligible</var> be an empty list.</p></li> | ||
<li> | ||
<p>While <var>candidate</var> is <span>inert</span>, or <var>candidate</var> is not an an | ||
ancestor of <var>originalElement</var> in the <span>flat tree</span>:</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the use of flat tree here makes much sense. This would mean that an element becomes inert if it's assigned to a slot which is inert. I don't think that's right given inert is a content attribute. We typically don't let content attribute on an element affecting slotted nodes or their descendants.
I think what we want here instead is the concept of shadow-including inclusive ancestor.
Based on work in #4288 by Alice Boxhall. Tests: https://github.com/web-platform-tests/wpt/tree/master/inert. Closes #5650. Co-authored-by: Alice Boxhall <aboxhall@chromium.org>
Based on work in whatwg#4288 by Alice Boxhall. Tests: https://github.com/web-platform-tests/wpt/tree/master/inert. Closes whatwg#5650. Co-authored-by: Alice Boxhall <aboxhall@chromium.org>
Re-adds the
inert
attribute, and clarifies mouse pointer and event behaviour for the inert concept.See https://github.com/WICG/inert/blob/master/explainer.md for justification.
Taken over from #1474.
💥 Error: Wattsi server error 💥
PR Preview failed to build. (Last tried on Mar 31, 2021, 2:47 AM UTC).
More
PR Preview relies on a number of web services to run. There seems to be an issue with the following one:
🚨 Wattsi Server - Wattsi Server is the web service used to build the WHATWG HTML spec.
🔗 Related URL
If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.