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.

Sign up for GitHub

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

Jump to bottom

Define a "has listeners for" algorithm on EventTarget #660

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Define a "has listeners for" algorithm on EventTarget #660

wants to merge 2 commits into from

Conversation

gterzian
Copy link
Member

@gterzian gterzian commented Jun 28, 2018 •
edited by pr-preview bot
Loading

Fix #453

💥 Error: 500 Internal Server Error 💥

PR Preview failed to build. (Last tried on Jan 15, 2021, 7:33 AM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 CSS Spec Preprocessor - CSS Spec Preprocessor is the web service used to build Bikeshed specs.

🔗 Related URL

<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML 2.0//EN">
<html><head>
<title>500 Internal Server Error</title>
</head><body>
<h1>Internal Server Error</h1>
<p>The server encountered an internal error or
misconfiguration and was unable to complete
your request.</p>
<p>Please contact the server administrator at 
 [no address given] to inform them of the time this error occurred,
 and the actions you performed just before this error.</p>
<p>More information about this error may be available
in the server error log.</p>
<hr>
<address>Apache/2.4.10 (Debian) Server at api.csswg.org Port 443</address>
</body></html>

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.

@gterzian gterzian force-pushed the master branch 4 times, most recently from 7ae41a1 to 8e5d463 Compare June 28, 2018 10:54
@gterzian
Copy link
Member Author 

curl: (22) The requested URL returned error: 400 Bad Request
make: *** [deploy] Error 22

Is this the result of a failed check, or is there a problem with the build?

@gterzian
Copy link
Member Author

@annevk ready for review(I think).

@gterzian
Copy link
Member Author

@annevk Ping on this one. From your Github timeline it looks like you were away for a couple of weeks, hope you had a nice holiday...

annevk
annevk reviewed Jul 31, 2018
View reviewed changes
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Thanks, this looks pretty good, but I would phrase it a little differently:

"An EventTarget object target is said to have a listener for type if target's event listener list contains an event listener whose type is type."

I think something along those lines is probably sufficient here. (To figure out what linking text to allow we might want to look at the phrasing for future callers in advance, too.)

@gterzian
Copy link
Member Author

gterzian commented Aug 6, 2018

Ok thanks, I think an example of callers would be Step 9 of unload a document, which I guess could read like:
If document's Window object has any event listeners that were triggered by the earlier unload event step(...)

@annevk
Copy link
Member

annevk commented Aug 8, 2018

Sorry for the continued delay in replies, it'll hopefully get better soon.

I'd expect we'd update that caller to be more specific and no longer reference the previous step. (This might also mean it has to be moved, in case the listeners removing themselves at that step still counts.)

annevk
annevk reviewed Aug 8, 2018
View reviewed changes
dom.bs Outdated
@@ -995,6 +995,9 @@ and a <dfn export for=EventTarget>legacy-canceled-activation behavior</dfn> algo
<p class="note no-backref">These algorithms only exist for checkbox and radio <{input}> elements and
are not to be used for anything else. [[!HTML]]

<p> An {{EventTarget}} object <var>target</var> is said to <dfn export>have a listener for</dfn> <var>type</var> if <var>target</var>'s <a>event listener list</a> <a for=list>contains</a> an
Copy link
Member

Choose a reason for hiding this comment

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

No space after <p>. Also needs to be rewrapped for 100 columns.

@gterzian
Copy link
Member Author

gterzian commented Aug 8, 2018

@annevk thanks, comment addressed. Yes you are right about the unload steps, we could reword it and put the link to here in a step before the actual firing of the event. I'd be happy to follow up on that too...

@annevk
Copy link
Member

annevk commented Aug 9, 2018

Thanks! @TimothyGu since you've been looking at events and similar algorithms, would you please double check?

@TimothyGu
Copy link
Member

It should be fine. I don't know the event dispatching process too well, but could it be possible that when "firing an event at the Window object with legacy target override flag set", event listeners registered on another object were also invoked? I'm guessing no, but it happened to be true then merely checking if the Window object has an unload event listener wouldn't been enough… (body.onload doesn't count, as the event listener is registered on Window still.)

@gterzian
Copy link
Member Author

Reminds me of a discussion we had previously at #453 (comment), because I assumed one would have to check the entire "event-path"...

annevk
annevk approved these changes Aug 10, 2018
View reviewed changes
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

I'm going to approve this, but let's wait with landing it until we have some dependency PRs lined up to make sure it really works.

@annevk annevk added do not merge yet Pull request must not be merged per rationale in comment and removed do not merge yet Pull request must not be merged per rationale in comment labels Aug 10, 2018
@annevk
Copy link
Member

annevk commented Aug 10, 2018

Actually, we should probably also add a note here to discourage new usage as checking for listeners is bad practice (as we point out in https://dom.spec.whatwg.org/#observing-event-listeners).

@annevk annevk added the do not merge yet Pull request must not be merged per rationale in comment label Aug 10, 2018
@annevk annevk mentioned this pull request Aug 10, 2018
Open
@gterzian gterzian force-pushed the master branch 3 times, most recently from 117cd25 to 399b58b Compare August 10, 2018 14:39
@gterzian
Copy link
Member Author

Ok, I've added a note, let me know what you think.

@annevk
Copy link
Member

annevk commented Aug 13, 2018

Thanks, I tweaked that a bit in a follow-up commit (will squash before landing). Let me know what you think.

As per above, I'm going to wait with landing until Service Workers / HTML are closer to adopting this.

@gterzian
Copy link
Member Author

Looks good to me, I previously got a warning when using should not, since it's apparently a non-normative section, and your 'can not' is a nice way around that :)

@TimothyGu
Copy link
Member

@gterzian As a note there are many alternatives, like "could not" or "would not." "Can not" isn't really a word.

@gterzian
Copy link
Member Author

@TimothyGu thanks! By the way the change is from my use of 'can not'(which was an attempt at avoiding the use of 'should'), to a 'is not', so definitely progress if 'can not' is not a word :)

@annevk
Copy link
Member

annevk commented Aug 14, 2018

The reason I wanted to avoid "can not" is that an API designer certainly can rely on it, but they are not supposed to.

@domenic domenic mentioned this pull request Oct 12, 2020
Open
Base automatically changed from master to main January 15, 2021 07:32
@annevk annevk mentioned this pull request Mar 1, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@annevk annevk annevk approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

Assignees
No one assigned
Labels
do not merge yet Pull request must not be merged per rationale in comment
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Define "has event listener" operation
3 participants
@gterzian @annevk @TimothyGu