Skip to content
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

Add EventListenerOptions and passive event listeners #82

Closed
wants to merge 1 commit into from

Conversation

@RByers
Copy link
Contributor

@RByers RByers commented Sep 22, 2015

My first crack at incorporating the ideas and discussion from my EventListenerOptions proposal into the spec. The result can be previewed here.

One thing I'm still not super happy with is the somewhat confusing defaults for mayCancel. On the one hand we agreed that we wanted the performance behavior (mayCancel:false) the be the default when using the new API form. But we of course can't change the existing behavior. Let's discuss that further here if desired.

@annevk
Copy link
Member

@annevk annevk commented Sep 22, 2015

How does this address e.g., WICG/EventListenerOptions#20? Also, it's very confusing to still have outstanding discussion elsewhere. Not really sure what to pay attention to now.

@annevk
Copy link
Member

@annevk annevk commented Sep 22, 2015

Basically, the way this is written I don't see much room for optimizations, unless you're doing something that's not allowed.

@RByers
Copy link
Contributor Author

@RByers RByers commented Sep 22, 2015

It's addressed by what @smaug--- said. I.e. whether or not a particular event is cancelable is not the purview of DOM, it's up to the individual specs. So a handler being added 'too late' is the same as other scenarios where a non-cancelable event is received by a handler, so already covered by the DOM spec. If we can get consensus here, then I'll add something to touch-events (w3c/touch-events/#6) that indicates that a TouchEvent may be uncancelable if, at the time it was first received, there are no mayCancel=true handlers for it. WDYT? Is there something more I should add here (perhaps expand on the note?).

Regarding discussion, I thought there were enough separate non-trivial issues (with non-trivial discussion history) that it was valuable to continue to use a separate issue tracker for them. But if you prefer I can try to close them all and merge all discussion into this PR. I don't have a preference on whether minor editorial discussion occurs here or in separate issues. But I defer to you on all of this - what's easiest for you at this stage?

@annevk
Copy link
Member

@annevk annevk commented Sep 23, 2015

Well, what I said in WICG/EventListenerOptions#20 (comment) is that what touch events do today doesn't require any changes in DOM. What you are suggesting here is that they observe listeners, which is something we want to avoid.

I don't really care where we settle on the design, but I'd like to settle on the design first. (I asked for the processing model before because the design wasn't clear. Now the processing model is clear, but the design seems wrong.)

@RByers
Copy link
Contributor Author

@RByers RByers commented Sep 23, 2015

Ok, I suggest we continue this conversation in WICG/EventListenerOptions#20 since it's (IMHO) a continuation of the same debate.

@RByers RByers force-pushed the RByers:event-listener-options branch from f440630 to 29925e7 Sep 25, 2015
@annevk
Copy link
Member

@annevk annevk commented Nov 10, 2015

@RByers what is the plan for this feature?

@RByers
Copy link
Contributor Author

@RByers RByers commented Nov 10, 2015

@annevk Sorry for the delay, we're still working on it (I've just bee caught up in some other things). @dtapuska is actively working on the blink implementation. I expect to be able to make the spec work here my top priority in a couple weeks.

@RByers RByers force-pushed the RByers:event-listener-options branch from cddfc43 to 7e05d12 Nov 30, 2015
@RByers
Copy link
Contributor Author

@RByers RByers commented Nov 30, 2015

Ok @annevk, I've finally reworked this pull request according to your suggestions in WICG/EventListenerOptions#20. WDYT?

@RByers RByers force-pushed the RByers:event-listener-options branch from 7e05d12 to 49ba2a3 Nov 30, 2015
@RByers RByers changed the title Add EventListenerOptions Add EventListenerOptions and passive event listeners feature Dec 2, 2015
@RByers RByers changed the title Add EventListenerOptions and passive event listeners feature Add EventListenerOptions and passive event listeners Dec 2, 2015
dom.bs Outdated
just a boolean, in which case it determines the value of the <b>capture</b> option.

When set to true,
the <var>capture</var> option prevents <b>callback</b> from being invoked when

This comment has been minimized.

@annevk

annevk Dec 15, 2015
Member

<var>capture</var> is no longer a variable so this seems wrong.

This comment has been minimized.

@RByers

RByers Dec 15, 2015
Author Contributor

How about the <b>capture</b> member of <var>options</var>? Or do you prefer simply the capture option?

This comment has been minimized.

@annevk

annevk Dec 16, 2015
Member

I think "<var>options</var>' <code>capture</code> member" is what is typically used.

dom.bs Outdated
the <a>event</a>'s {{Event/eventPhase}} attribute value is {{Event/BUBBLING_PHASE}}.
When false, <b>callback</b> will not be invoked when <a>event</a>'s {{Event/eventPhase}}
attribute value is {{Event/CAPTURING_PHASE}}. Either way, <b>callback</b> will be
invoked if <a>event</a>'s {{Event/eventPhase}} attribute value is {{Event/AT_TARGET}}.

When set to true, the <var>passive</var> option indicates that the <b>callback</b>

This comment has been minimized.

@annevk

annevk Dec 15, 2015
Member

Same problem here.

This comment has been minimized.

@RByers

RByers Dec 15, 2015
Author Contributor

Done

dom.bs Outdated
<li>If <var>options</var> is of type boolean, let <var>capture</var> be
<var>options</var> and let <var>passive</var> be false. Otherwise let
<var>capture</var> and <var>passive</var> be the corresponding values in the
<var>options</var> {{EventListenerOptions}} dictionary.

This comment has been minimized.

@annevk

annevk Dec 15, 2015
Member

  1. Given we need this, why have default values in IDL?
  2. I would prefer initializing variables separately and then setting them based on the argument. I.e., use "let" once per variable name.

This comment has been minimized.

@RByers

RByers Dec 15, 2015
Author Contributor

Given we need this, why have default values in IDL?

Because we want to support cases like addEventListener(t, h, {passive:true}) and addEventListener(t, h, {capture:true}).

I would prefer initializing variables separately and then setting them based on the argument. I.e., use "let" once per variable name.

Ok.

This comment has been minimized.

@annevk

annevk Dec 16, 2015
Member

That is not justification for having default values in the IDL though.

This comment has been minimized.

@RByers

RByers Dec 16, 2015
Author Contributor

Ah, I see what you're saying. We could leave the default values out and update the spec text to say something like `If options has a capture member with value true, set capture to true'. The way it's written now ("let capture and passive be the corresponding values in ..." seems to imply that those members will ALWAYS be present).

Personally I prefer the default values because it makes it more explicit that eg. {capture:false} and {} have identical behavior. But if you think it's better to have the defaults be defined only by the algorithm, that's fine with me too. I'm curious though, what's the advantage of that?

This comment has been minimized.

@annevk

annevk Dec 17, 2015
Member

Well, they have to be defined in the algorithm anyway. Since the dictionary is not guaranteed. So the IDL has no meaningful effect. I'd be okay with comments in the IDL saying what it effectively comes down to.

This comment has been minimized.

@RByers

RByers Dec 17, 2015
Author Contributor

Ok, fair enough. How's this?

Also my developer confusion argument is probably academic because any reasonable developer should assume that boolean options default to false anyway :-)

dom.bs Outdated
<li>If <var>options</var> is of type boolean, let <var>capture</var> be
<var>options</var> and let <var>passive</var> be false. Otherwise let
<var>capture</var> and <var>passive</var> be the corresponding values in the
<var>options</var> {{EventListenerOptions}} dictionary.

This comment has been minimized.

@annevk

annevk Dec 15, 2015
Member

Same applies here.

This comment has been minimized.

@RByers

RByers Dec 15, 2015
Author Contributor

Done

dom.bs Outdated
@@ -1057,6 +1096,25 @@ invoked, must run these steps:
<li><p><a>Dispatch</a> the <var>event</var> and return the value that returns.
</ol>

<h3 id=observing-event-listeners>Observing event listeners</h3>
In general, developers do not expect the presence of an <a>event listener</a> to be

This comment has been minimized.

@annevk

annevk Dec 15, 2015
Member

Use a newline after <h3> and an additional newline before the <h3>.

This comment has been minimized.

@RByers

RByers Dec 15, 2015
Author Contributor

Done

dom.bs Outdated

Unfortunately, some event APIs have been designed such that implementing them
efficiently requires observing <a>event listeners</a>. For example, sensor APIs which
enable an underlying device sensor, and touch APIs which can be used to block

This comment has been minimized.

@annevk

annevk Dec 15, 2015
Member

Implementing them efficiently is not really sufficient motivation. Not executing code that cannot be observed is always fine. What is problematic is when it can be observed that adding and removing listeners has an effect. That is already the case with touch events as I understand it and therefore we are doing this, so we should use that as motivation, no?

This comment has been minimized.

@RByers

RByers Dec 15, 2015
Author Contributor

Yes, agreed that the observability needs more mention here. Part of the problem is that "observability" is a bit of a fuzzy concept when it comes to performance. Is the scroll performance of a page "observable"? Well if scrolling is blocked for 1s, or there's noticeable UI mis-synchronization (as can be seen here) then I'd say that counts as "observable". I'm tweaking this text to better reflect that.

This comment has been minimized.

@annevk

annevk Dec 16, 2015
Member

I thought we already changed the way we dispatch certain events (in a script-observable way, without timers) based on listeners registered. That's what I would count.

This comment has been minimized.

@RByers

RByers Dec 16, 2015
Author Contributor

Ok. I thought about this some more, and there is at least one subtle script-observable behavior change (at least in Chrome and Safari where I tested): if there are no touch listeners present and a listener is added mid-gesture, no events are sent for the already touching finger. I built a demo of this here. You could argue however that this is a bug we could fix (or at least reduce to a small race window), though not sure we'd ever get Safari to change.

Do you think that's worth mentioning here?

This comment has been minimized.

@annevk

annevk Dec 17, 2015
Member

Yeah maybe. I must be missing something since I thought we had a very clear observable case to go from and therefore observing even more for this passive stuff was okay...

This comment has been minimized.

@RByers

RByers Dec 17, 2015
Author Contributor

How about we move this discussion back to the github issue so we have a permanent record of the debate? I'm sure it won't be the last time I have it .

dom.bs Outdated
non-{{EventListenerOptions/passive}} listener. For example, non-{{EventListenerOptions/passive}}
{{TouchEvent}} listeners must block scrolling, but if all listeners are {{EventListenerOptions/passive}} then
scrolling can be allowed to start <a>in parallel</a> by making the {{TouchEvent}}
uncancelable (so that calls to {{Event/preventDefault()}} are ignored).

This comment has been minimized.

@annevk

annevk Dec 15, 2015
Member

We should make a note here that this "passive" hint is being observed by the code that dispatches the event since any listeners added during dispatch that are not "passive" will not be able to affect the event. In effect, what happens is that having solely passive observers at the time of action influences the event being dispatched. (The dispatcher code will set its passive flag.)

This comment has been minimized.

@RByers

RByers Dec 15, 2015
Author Contributor

Ok, I'm adding some more text along these lines. Rather than say the "passive" hint is being observed, I think it's valuable to be specific that it's only the "absence of non-passive listeners" that's being observed. Eg. the dispatching code does NOT make any distinction between the cases of mixed passive/non-passive listeners and non-passive only listeners.

See also the discussion below about the passive flag.

dom.bs Outdated
@@ -1138,9 +1196,15 @@ invoked, must run these steps:
<var>listener</var>'s <b>capture</b> is true, terminate these substeps (and run them for the next
<a>event listener</a>).

<li>If <var>listener</var>'s <b>passive</b> is true, set <var>event</var>'s <a>passive flag</a>.

This comment has been minimized.

@annevk

annevk Dec 15, 2015
Member

This seems wrong. Since before we said this would only happen if all listeners are passive. So this is in fact something that is done by those invoking dispatch.

This comment has been minimized.

@RByers

RByers Dec 15, 2015
Author Contributor

I thought this is what we agreed on here and summarized here?

I.e. calls to preventDefault from a passive listener never has any impact (since passive is a promise that the listener doesn't need to cancel). But Event.cancelable is immutable and so can be changed only by code invoking dispatch based on the observation of ALL listeners.

This comment has been minimized.

@annevk

annevk Dec 16, 2015
Member

Ah okay. I think we should rename the flag on event then to "in passive listener flag" to make it clear what the scope is.

This comment has been minimized.

@RByers

RByers Dec 16, 2015
Author Contributor

Sounds good to me, thanks.

@RByers
Copy link
Contributor Author

@RByers RByers commented Dec 15, 2015

Thanks for the review! I've updated the pull request based on your feedback. I assume I should avoid squashing all the commits until the review is done and we're ready to merge.

@annevk
Copy link
Member

@annevk annevk commented Dec 16, 2015

Feel free to rebase and squash once you're confident it's in a good shape and I'll just do another pass. Might be easier.

@RByers
Copy link
Contributor Author

@RByers RByers commented Dec 16, 2015

Ok I think the only thing left is what exactly we want to cover in the "observing event listeners" section. Let's keep iterating on that, then once you're happy I'll rebase/squash (which I think will cause all the line-by-line discussions to disappear from GitHub :-( ).

@RByers RByers force-pushed the RByers:event-listener-options branch from d4322fe to c01304d Dec 17, 2015
dom.bs Outdated
{{Event/preventDefault()}} was invoked
while the {{Event/cancelable}} attribute
value is true, and false otherwise.
{{Event/preventDefault()}} was used successfully to indicate cancellation.

This comment has been minimized.

@annevk

annevk Jan 1, 2016
Member

(Adding more nitpicky comments. Feel free to ignore for now.) s/used/invoked/ We don't want to omit mentioning when it returns false. The original text did that correctly.

This comment has been minimized.

@RByers

RByers Jan 4, 2016
Author Contributor

Whoops, must have lost this by accident somewhere along the line. Fixed.

dom.bs Outdated
<p class="note no-backref">
User agents are encouraged to generate a console warning or other debugging
aid to help authors identify places where calls to {{preventDefault()}}
have no effect.

This comment has been minimized.

@annevk

annevk Jan 1, 2016
Member

@domenic started using "User agents are encouraged to log the precise cause in a developer console, to aid debugging." in HTML. I suggest we start from there.

This comment has been minimized.

@RByers

RByers Jan 4, 2016
Author Contributor

Ok, how's this?

present in <var>options</var> with value true, then set <var>capture</var> to true.

<li>If <var>options</var> is a dictionary and <code>{{EventListenerOptions/passive}}</code> is
present in <var>options</var> with value true, then set <var>passive</var> to true.

This comment has been minimized.

@annevk

annevk Jan 1, 2016
Member

Since these four steps are repeated below I feel like we should introduce an abstract operation that returns two values for capture and passive given a dictionary or boolean.

This comment has been minimized.

@RByers

RByers Jan 4, 2016
Author Contributor

I attempted that here and I agree it looks cleaner, thanks! I couldn't find another example of returning multiple values, is what I've done precise enough? Any suggestion for a term better than "normalize" (which could perhaps be confused with Node.normalize)?

This comment has been minimized.

@annevk

annevk Jan 4, 2016
Member

"flatten" perhaps? What you did seems great. Returning multiple values in English feels quite natural and I've done it before without confusing anybody.

This comment has been minimized.

@RByers

RByers Jan 4, 2016
Author Contributor

Thanks. Sure "flatten" sounds better to me - done.

dom.bs Outdated
<h3 id=observing-event-listeners>Observing event listeners</h3>

<p>In general, developers do not expect the presence of an <a>event listener</a> to be
observable. The impact of an <a>event listener</a> is determined by its <b>callback</b>.

This comment has been minimized.

@annevk

annevk Jan 1, 2016
Member

Throughout, there should only be a single space after a period.

This comment has been minimized.

@RByers

RByers Jan 4, 2016
Author Contributor

Done.

dom.bs Outdated
listeners, and use that to clear the {{Event/cancelable}} property of the event
being dispatched.

<p>Ideally, any new event types are defined such that they don't need this

This comment has been minimized.

@annevk

annevk Jan 1, 2016
Member

s/new event types/new event APIs/
s/don't/do not/

This comment has been minimized.

@RByers

RByers Jan 4, 2016
Author Contributor

Done.

@RByers RByers force-pushed the RByers:event-listener-options branch from b2fa94e to 530c38b Jan 4, 2016
dom.bs Outdated

<li>Clear <var>event</var>'s <a>in passive listener flag</a>.

<li>If the call to {{EventListener/handleEvent()}} threw any exception, <a>report the exception</a>.

This comment has been minimized.

@annevk

annevk Jan 5, 2016
Member

Are you sure this is correct? Reporting the exception is synchronous. Seems like you could circumvent the passive restriction by storing the event somewhere and then throwing.

This comment has been minimized.

@RByers

RByers Jan 5, 2016
Author Contributor

If reporting the exception is synchronous, then is it OK to just clear the passive listener flag afterwards (updated)? That would definitely be preferable, I was just thinking that wouldn't be possible.

This comment has been minimized.

@annevk

annevk Jan 5, 2016
Member

As far as I can tell that's fine. Where do you think it would break?

This comment has been minimized.

@RByers

RByers Jan 5, 2016
Author Contributor

I just wasn't thinking about it properly (thinking in terms of an algorithm with a 'throw' statement). Thanks for catching this, I agree it's better and I don't anticipate any implementation issue.

@annevk
Copy link
Member

@annevk annevk commented Jan 5, 2016

(Sorry, I was about to merge, but then I spotted that. Not sure why I didn't catch it earlier.)

@RByers RByers force-pushed the RByers:event-listener-options branch from 530c38b to d17e7f8 Jan 5, 2016
@annevk
Copy link
Member

@annevk annevk commented Jan 5, 2016

The only other thing that looks a bit odd is the wrapping. I can clean that up I suppose.

@RByers
Copy link
Contributor Author

@RByers RByers commented Jan 5, 2016

The only other thing that looks a bit odd is the wrapping. I can clean that up I suppose.

Looks like the pattern is to use ~100 except in <pre> and <code> blocks, right? I'm fixing that now.

This introduces an EventListenerOptions dictionary which can
be used to explicitly specify options to addEventListener and
removeEventListener.

This also introduces a "passive" option, which disables the
ability for a listener to cancel the event.

See https://github.com/RByers/EventListenerOptions/blob/gh-pages/explainer.md
for a high-level overview, and
https://github.com/RByers/EventListenerOptions/issues?q=is%3Aissue
for most of the debate that went into the design.
@RByers RByers force-pushed the RByers:event-listener-options branch from d17e7f8 to 0ad4e56 Jan 5, 2016
@RByers
Copy link
Contributor Author

@RByers RByers commented Jan 5, 2016

Ok, re-wrapped all lines I touched to 100 columns (except <pre> and <code> blocks).

annevk added a commit that referenced this pull request Jan 6, 2016
This introduces an EventListenerOptions dictionary which can
be used to explicitly specify options for addEventListener()
and removeEventListener().

This also introduces a "passive" option, which disables the
ability for a listener to cancel the event.

See https://github.com/RByers/EventListenerOptions/blob/gh-pages/explainer.md
for a high-level overview, and
https://github.com/RByers/EventListenerOptions/issues?q=is%3Aissue
for most of the debate that went into the design.

PR: #82
@annevk
Copy link
Member

@annevk annevk commented Jan 6, 2016

Landed as 253a21b.

@annevk annevk closed this Jan 6, 2016
@annevk
Copy link
Member

@annevk annevk commented Jan 6, 2016

Thank you!

@RByers RByers deleted the RByers:event-listener-options branch Sep 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.