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

Editorial: major cleanup around firing events #1886

Merged
merged 20 commits into from
Oct 18, 2016

Conversation

annevk
Copy link
Member

@annevk annevk commented Oct 11, 2016

There’s no need to state defaults again as that only leads to confusion
when they are not explicitly stated.

There’s no need to state defaults again as that only leads to confusion
when they are not explicitly stated.
@domenic
Copy link
Member

domenic commented Oct 11, 2016

This is part of #1713 FYI

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional cleanups to do:

  • #downloading-or-updating-an-application-cache:concept-event-fire
  • #downloading-or-updating-an-application-cache:concept-event-fire-2
  • #runtime-script-errors-2:concept-event-fire
  • #the-storage-event:concept-event-fire
  • #the-end:concept-event-fire

Following the dfn popup for dispatch also has many of these cases, where it uses the terminology "does not bubble and is not cancelable" or similar when setting up the event and then dispatching it in a subsequent step. (Many of these cases could probably become "fire" instead of "dispatch" by collapsing some steps... I don't understand why they're separate in most cases. Probably cargo-culting. If you collapse the steps and use "fire", you also can get rid of the "trusted" concept, since trusted is the default for firing.)

  • #fire-a-focus-event
  • #the-dragevent-interface:concept-event-dispatch
  • #unloading-documents:concept-event-dispatch
  • #runtime-script-errors:concept-event-dispatch
  • #unhandled-promise-rejections:concept-event-dispatch
  • #the-hostpromiserejectiontracker-implementation:concept-event-dispatch
  • #event-firing:concept-event-dispatch
  • #event-firing:concept-event-dispatch-2
  • #event-stream-interpretation:concept-event-dispatch
  • #feedback-from-the-protocol:concept-event-dispatch
  • #feedback-from-the-protocol:concept-event-dispatch-2
  • #posting-messages:concept-event-dispatch
  • #message-ports:concept-event-dispatch
  • #broadcasting-to-other-browsing-contexts:concept-event-dispatch
  • #worker-processing-model:concept-event-dispatch
  • #shared-workers-and-the-sharedworker-interface:concept-event-dispatch

In general I think you'll want to Ctrl+F for "does not bubble" and "is not cancelable".

<code>MouseEvent</code> interface, at the element for which the menu was requested. The context
information of the event must be initialized to the same values as the last
<dd><p>The user agent must <span data-x="concept-event-fire">fire</span> an event with the name
<code data-x="event-contextmenu">contextmenu</code>, that bubbles and is cancelable, and that
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use the new style for bubbles and cancelable, I think

<code data-x="dom-PopStateEvent-state">state</code> attribute initialized to the value of <var>state</var>. This event must bubble but not be cancelable and has no default
action.</p></li>
<code data-x="dom-Event-bubbles">bubbles</code> attribute initialized to true and the <code
data-x="dom-PopStateEvent-state">state</code> attribute initialized to the value of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"initialized to state" would be a bit nicer while you're here

@@ -83197,8 +83187,7 @@ State: &lt;OUTPUT NAME=I>1&lt;/OUTPUT> &lt;INPUT VALUE="Increment" TYPE=BUTTON O

</li>

<li><p><span data-x="concept-event-fire">Fire</span> a <span
data-x="concept-events-trusted">trusted</span> event with the name <code
<li><p><span data-x="concept-event-fire">Fire</span> an event with the name <code
data-x="event-pageshow">pageshow</code> at the <code>Window</code> object of that
<code>Document</code>, with <i
data-x="concept-event-target-override">target override</i> set to the <code>Document</code>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one has "This event must not bubble, must not be cancelable, and has no default action." which should be removed.

@domenic domenic added the clarification Standard could be clearer label Oct 12, 2016
@annevk
Copy link
Member Author

annevk commented Oct 13, 2016

So I mainly removed trusted and then did some cleanup alongside. I guess we could turn this into something bigger, but then I should probably first redesign "fire an event" on the DOM side and make all that work too.

@domenic
Copy link
Member

domenic commented Oct 13, 2016

Hmm OK. I think it'd be nice to land more at once but if it'd be helpful to work incrementally then I think just addressing the line comments (without doing the additional cleanups on the IDs I listed) would be fine. Maybe with a commit message that notes this is just an initial pass and there are many other call sites that need updating.

@annevk
Copy link
Member Author

annevk commented Oct 13, 2016

I'm happy to make this bigger. I already created a PR for DOM to redesign "fire an event": whatwg/dom#344. Would be best to review and land that first, then work on this.

@domenic domenic added the do not merge yet Pull request must not be merged per rationale in comment label Oct 13, 2016
@annevk
Copy link
Member Author

annevk commented Oct 14, 2016

Document object's Window object or Document object's relevant global object? Kept the former for now, should maybe be a separate change for all instances (not that many). (I also found one instance of the latter not being correctly linked. Apparently wattsi does start complaining, but only after you have two instances of data-x-less <span>s. Having said that, perhaps they should all be data-x-less and we just use an ID on the <dfn>, as the term is rather unique anyway.)

@annevk
Copy link
Member Author

annevk commented Oct 14, 2016

FWIW, anything in the AppCache section I rather not touch until we decide what to do there. It seems better for that to either be removed, or be cleaned up altogether and integrated with Fetch if we decide to keep it.

@domenic
Copy link
Member

domenic commented Oct 14, 2016

I'd rather clean up the appcache ones as well. It's a mechanical transformation with no normative impact. We don't gain anything by making it inconsistent with the rest of the spec.

@annevk annevk changed the title Editorial: minor cleanup around firing events Editorial: major cleanup around firing events Oct 14, 2016
@annevk
Copy link
Member Author

annevk commented Oct 17, 2016

This is ready for review now. I tried filing follow up issues whenever I saw something bogus.

@domenic
Copy link
Member

domenic commented Oct 17, 2016

Missed:

  • #the-dragevent-interface:concept-events-trusted
  • #feedback-from-the-protocol:concept-events-trusted
  • "When a key event is to be routed in a top-level browsing context, the user agent must run the following steps:" should link to fire and use the return value. Probably a preexisting issue.

Typos:

  • "intiialized" (many times)
  • "Let report be the result of firing an event named invalid at this element, with the attribute initialized to true." at #the-constraint-validation-api:concept-event-fire-2

Other:

  • Add data-x-href to firing and dispatching so they link to DOM instead of to HTML. Maybe move them to the dependencies section.
  • Add a note to "Firing a simple event named e" saying this is going away and should not be used, and link to Remove "Firing a simple event named e" #1887?
  • Sometimes you say "using StorageEvent", sometimes "that uses the TrackEvent interface". This doesn't necessarily seem worth fixing but I wanted to make sure you were aware.
  • Lots of places use "If the event is canceled" or similar phrasing. Maybe file a follow-up to use the return values instead, or add it to the OP of Event dispatch/firing should reuse DOM terms better #1713.
  • "Fire a DND event" should probably return the value of dispatching, and could probably consolidate its create/dispatch steps into a single fire step.
  • #unloading-documents:concept-event-dispatch could consolidate creation and dispatching steps

Commit message should mention the follow-ups if possible.

@annevk
Copy link
Member Author

annevk commented Oct 18, 2016

Commit message prep:

Editorial: major cleanup around firing and dispatching events

This aligns firing and dispatching of events with updated terminology in the DOM Standard. Among the changes:

* A lot less usage of "default action" which isn't really a thing. Instead we make use of the return value of the fire and dispatch algorithms.
* Instead of saying things bubble or are cancelable we initialize the attributes as such.
* We now use the legacy target override flag rather than supplying a named argument.

This fixes #1713, but plenty of follow up issues remain:

* #805 for the remainder of "default action" usage
* #1394 for updating synthetic click events
* #1887 for removing "fire a simple event" usage
* #1893 for updating when checkboxes get checked
* #1900 for figuring out if event dispatch requires more hooks
* #1912 for revisiting isTrusted usage
* #1913 for updating synthetic mouse events
* #1922 for making more events composed

@annevk
Copy link
Member Author

annevk commented Oct 18, 2016

When a key event is to be routed in a top-level browsing context

This should use dispatch actually, since the event is already created, we're just rerouting it. This is another thing that is monkey patching the non-existing UI events specification.

@annevk
Copy link
Member Author

annevk commented Oct 18, 2016

Add a note to "Firing a simple event named e" saying this is going away and should not be used, and link to #1887?

I plan on not doing this given that we have a volunteer. If that doesn't pan out I'll make it happen.

@annevk
Copy link
Member Author

annevk commented Oct 18, 2016

#unloading-documents:concept-event-dispatch could consolidate creation and dispatching steps

This is #1900.

@annevk
Copy link
Member Author

annevk commented Oct 18, 2016

Lots of places use "If the event is canceled" or similar phrasing.

"is canceled" and "not canceled" don't give many results, some are in notes. I will clean it up, but not those I found inside the "Activation" section as that will largely disappear with #1394.

@annevk
Copy link
Member Author

annevk commented Oct 18, 2016

"Fire a DND event" should probably return the value of dispatching, and could probably consolidate its create/dispatch steps into a single fire step.

This actually looks complicated, since it does steps after dispatch. I wonder if the text as written is even correct. I'm not planning on fixing this here.

@annevk
Copy link
Member Author

annevk commented Oct 18, 2016

So we might want to open an issue for the DND processing model (or the larger lack of user interaction specification), but other than that I once again think I'm done.

@@ -89705,6 +89705,9 @@ interface <dfn>DocumentAndElementEventHandlers</dfn> {
<li><p>Initialize <var>event</var>'s <code data-x="dom-Event-bubbles">bubbles</code> and <code
data-x="dom-Event-cancelable">cancelable</code> attributes to true.

<li><p>Initialize <var>event</var>'s <code data-x="dom-Event-isTrusted">isTrusted</code>
attribute to true (unless otherwise stated).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing closing tags

@domenic domenic removed the do not merge yet Pull request must not be merged per rationale in comment label Oct 18, 2016
@domenic domenic merged commit 8ffbd14 into master Oct 18, 2016
@domenic domenic deleted the annevk/event-firing-cleanup branch October 18, 2016 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification Standard could be clearer topic: events
Development

Successfully merging this pull request may close these issues.

2 participants