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

Redesign fire an event and target override #344

Merged
merged 3 commits into from
Oct 14, 2016
Merged

Redesign fire an event and target override #344

merged 3 commits into from
Oct 14, 2016

Conversation

annevk
Copy link
Member

@annevk annevk commented Oct 13, 2016

Fixes #187. Follow up work in HTML is tracked in
whatwg/html#1713.

Fixes #187. Follow up work in HTML is tracked in
whatwg/html#1713.
@annevk
Copy link
Member Author

annevk commented Oct 13, 2016

@domenic can you review this one too?

@cvrebert thanks for raising this back in March!

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.

All nits and suggestions. Overall a definite improvement.


<ol>
<li><p>Set <var>event</var>'s <a>dispatch flag</a>.

<li>
<p>If <var>targetOverride</var> is not given, let <var>targetOverride</var> be <var>target</var>.
<p>Let <var>targetOverride</var> be <var>target</var>, if <var>legacy target override flag</var>
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I think "the legacy target override flag" is more consistent with what we've done so far.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not in my prose... (Aside: I'm really starting to dislike flags. I wish we'd just have optional arguments with default values and use booleans here.)

are to be initialized, and a <var>legacy target override flag</var>, run these steps:

<ol>
<li><p>If <var>eventClass</var> is not given ,let <var>eventClass</var> be {{Event}}.
Copy link
Member

Choose a reason for hiding this comment

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

misplaced space before comma

<var>e</var>, and its {{Event/isTrusted}}
attribute initialized to <code>true</code>, is to be
<a>dispatched</a> to the given object.
<p>To <dfn export id=concept-event-fire lt="fire an event">fire an event</dfn> named <var>e</var> at
Copy link
Member

Choose a reason for hiding this comment

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

This lt="" is now unnecessary

attribute initialized to <code>true</code>, is to be
<a>dispatched</a> to the given object.
<p>To <dfn export id=concept-event-fire lt="fire an event">fire an event</dfn> named <var>e</var> at
<var>target</var>, optionally given an <var>eventClass</var>, a description of how IDL attributes
Copy link
Member

Choose a reason for hiding this comment

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

Maybe eventInterface is more accurate? I dunno, I want to use JS terms, but this is kind of Web IDL land. Could go either way.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess we can go with interface for now. IDL should really rename things though...


<li><p>Return the result of <a>dispatching</a> <var>event</var> at <var>target</var>, with
<var>legacy target override flag</var> set if set.
</ol>

<p class="note no-backref">Fire in the context of DOM is short for creating, initializing,
Copy link
Member

Choose a reason for hiding this comment

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

It might be worth adding a more complex example that uses eventClass and initializes some extra attributes, and possibly uses the return value.

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.

Merge at will, just found a couple more potential nits.


<li><p>Initialize <var>event</var>'s {{Event/type}} attribute to <var>e</var>.
<li><p>Let <var>event</var> be the result of <a for=Event lt=constructor>invoking</a> the initial
value of <var>eventConstructor</var> with <var>e</var> as argument.
Copy link
Member

Choose a reason for hiding this comment

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

"with the argument e"

{{Event/cancelable}} attribute initialized to true".

<p>Or, when a custom constructor is required, "<a>fire an event</a> named <code>click</code> at
<var>target</var> using <code>MouseEvent</code> with its {{Event/isTrusted}} attribute initialized
Copy link
Member

Choose a reason for hiding this comment

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

Use {{MouseEvent}} instead of <code>MouseEvent</code> to get some free auto-linking.

@annevk
Copy link
Member Author

annevk commented Oct 14, 2016

Will fix those and then land.

@annevk annevk merged commit e19d7ee into master Oct 14, 2016
@annevk
Copy link
Member Author

annevk commented Oct 14, 2016

Thanks for reviewing!

@annevk annevk deleted the target-override branch October 14, 2016 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants