Remove [DocumentEventHandler] extended attribute
Created attachment 446465 [details] Patch
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.
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.
(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!
(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.
(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.
(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.
(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.
(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. 👍🏻
(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 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.
Committed r287095 (245287@trunk): <https://commits.webkit.org/245287@trunk>
<rdar://problem/86536563>