Bug 234047 - [WebIDL] Remove the now-unused [DocumentEventHandler] extended attribute
Summary: [WebIDL] Remove the now-unused [DocumentEventHandler] extended attribute
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Enhancement
Assignee: Alexey Shvayka
URL:
Keywords: InRadar
Depends on: 234349
Blocks:
  Show dependency treegraph
 
Reported: 2021-12-08 17:09 PST by Alexey Shvayka
Modified: 2021-12-15 12:15 PST (History)
9 users (show)

See Also:


Attachments
Patch (20.54 KB, patch)
2021-12-08 17:16 PST, Alexey Shvayka
darin: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Shvayka 2021-12-08 17:09:06 PST
Remove [DocumentEventHandler] extended attribute
Comment 1 Alexey Shvayka 2021-12-08 17:16:45 PST
Created attachment 446465 [details]
Patch
Comment 2 Darin Adler 2021-12-08 17:30:11 PST
Comment on attachment 446465 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=446465&action=review

No progression on WPT tests?

> Source/WebCore/ChangeLog:3
> +        Remove [DocumentEventHandler] extended attribute

In future, consider making the patch that removes the last use separate from the patch removing the implementation.

Makes it a little easier to concentrate on those two separate concerns, and I don’t think there are a lot of downsides.
Comment 3 Darin Adler 2021-12-08 17:35:04 PST
I think the behavior change is much more interesting and needs attention and focus (which I did when reviewing, I think) than the "remove the IDL extended attribute", even though the latter is more satisfying and helps the project out long term.
Comment 4 Alexey Shvayka 2021-12-08 17:42:55 PST
(In reply to Darin Adler from comment #2)

Thank you for review!

> No progression on WPT tests?

As far as I tested, no, and I'm a bit hesitant to contribute a test that asserts the lack of Document-reflecting for "selectionchange" event, considering the spec has no such concept and no other browser has the same issue.

> > Source/WebCore/ChangeLog:3
> > +        Remove [DocumentEventHandler] extended attribute
> 
> In future, consider making the patch that removes the last use separate from
> the patch removing the implementation.
> 
> Makes it a little easier to concentrate on those two separate concerns, and
> I don’t think there are a lot of downsides.

Noted, thanks!
Comment 5 Alexey Shvayka 2021-12-08 17:48:04 PST
(In reply to Darin Adler from comment #3)
> I think the behavior change is much more interesting and needs attention and
> focus (which I did when reviewing, I think) than the "remove the IDL
> extended attribute", even though the latter is more satisfying and helps the
> project out long term.

If you don't mind, I will land the reviewed diff in 2 bugs:

1) `el.setAttribute("onselectionchange", "<code">)` now properly sets event listener;

2) `document.body.onselectionchange = function() {}` now doesn't forward event listener to Document.
Comment 6 Alexey Shvayka 2021-12-08 17:50:39 PST
(In reply to Alexey Shvayka from comment #4)
> (In reply to Darin Adler from comment #2)
> 
> > No progression on WPT tests?
> 
> As far as I tested, no, and I'm a bit hesitant to contribute a test that
> asserts the lack of Document-reflecting for "selectionchange" event,
> considering the spec has no such concept and no other browser has the same
> issue.

But I can surely add a WPT for `el.setAttribute("onselectionchange", "<code">)` part.
Comment 7 Darin Adler 2021-12-08 18:02:51 PST
(In reply to Alexey Shvayka from comment #4)
> As far as I tested, no, and I'm a bit hesitant to contribute a test that
> asserts the lack of Document-reflecting for "selectionchange" event,
> considering the spec has no such concept and no other browser has the same
> issue.

I wouldn’t necessarily rush to do this, why go out of our way to quickly create a test that highlights something generally harmless but wrong in the current versions of Safari, but there is some precedent for these kinds of tests, ones that check for possible mistakes that have been done in the past.
Comment 8 Darin Adler 2021-12-08 18:03:34 PST
(In reply to Alexey Shvayka from comment #5)
> If you don't mind, I will land the reviewed diff in 2 bugs:
> 
> 1) `el.setAttribute("onselectionchange", "<code">)` now properly sets event
> listener;
> 
> 2) `document.body.onselectionchange = function() {}` now doesn't forward
> event listener to Document.

Why not 3 steps?

3) Remove the now-unused DocumentEventHandler extended attribute.
Comment 9 Alexey Shvayka 2021-12-08 18:14:41 PST
(In reply to Darin Adler from comment #7)
> (In reply to Alexey Shvayka from comment #4)
> > As far as I tested, no, and I'm a bit hesitant to contribute a test that
> > asserts the lack of Document-reflecting for "selectionchange" event,
> > considering the spec has no such concept and no other browser has the same
> > issue.
> 
> I wouldn’t necessarily rush to do this, why go out of our way to quickly
> create a test that highlights something generally harmless but wrong in the
> current versions of Safari, but there is some precedent for these kinds of
> tests, ones that check for possible mistakes that have been done in the past.

Please note that L318-322 of LayoutTests/fast/dom/event-handler-attributes.html do test that wrong behaviour, observable in current version of Safari, is now fixed.

(In reply to Darin Adler from comment #8)
> Why not 3 steps?
> 
> 3) Remove the now-unused DocumentEventHandler extended attribute.

👍🏻
Comment 10 Darin Adler 2021-12-08 18:17:11 PST
(In reply to Alexey Shvayka from comment #9)
> Please note that L318-322 of
> LayoutTests/fast/dom/event-handler-attributes.html do test that wrong
> behaviour, observable in current version of Safari, is now fixed.

I did!

Had some feelings of pride about the fact that the test has output that is so easy to understand. Still have dim memories of when I originally wrote it.
Comment 11 Alexey Shvayka 2021-12-10 12:56:31 PST
Comment on attachment 446465 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=446465&action=review

> Source/WebCore/html/HTMLBodyElement.cpp:-167
> -    if (name == onselectionchangeAttr) {

We can't remove this until we implement emitting "selectionchange" on elements (selection targets). Otherwise we risk breaking web-compat as caught by the failing test.
But we can still remove [DocumentEventHandler] as no other browser reflects IDL attribute to Document.
Comment 12 Alexey Shvayka 2021-12-15 12:14:15 PST
Committed r287095 (245287@trunk): <https://commits.webkit.org/245287@trunk>
Comment 13 Radar WebKit Bug Importer 2021-12-15 12:15:20 PST
<rdar://problem/86536563>