Bug 234047 – [WebIDL] Remove the now-unused [DocumentEventHandler] extended attribute

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Alexey Shvayka]

Remove [DocumentEventHandler] extended attribute

[Alexey Shvayka]

Created attachment 446465 [details]
Patch

[Darin Adler]

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.

[Darin Adler]

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.

[Alexey Shvayka]

(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!

[Alexey Shvayka]

(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.

[Alexey Shvayka]

(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.

[Darin Adler]

(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.

[Darin Adler]

(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.

[Alexey Shvayka]

(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.

👍🏻

[Darin Adler]

(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.

[Alexey Shvayka]

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.

[Alexey Shvayka]

Committed r287095 (245287@trunk): <https://commits.webkit.org/245287@trunk>

[Radar WebKit Bug Importer]

<rdar://problem/86536563>