Bug 217248 - Introduce Selection specific mixin of GlobalEventHandlers
Summary: Introduce Selection specific mixin of GlobalEventHandlers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-10-02 13:44 PDT by Rob Buis
Modified: 2021-12-10 12:53 PST (History)
16 users (show)

See Also:


Attachments
Patch (27.51 KB, patch)
2020-10-02 13:48 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (27.42 KB, patch)
2020-10-02 14:16 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (26.03 KB, patch)
2020-10-02 15:25 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (19.75 KB, patch)
2020-10-03 08:10 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (19.61 KB, patch)
2020-10-04 12:57 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (19.70 KB, patch)
2020-10-10 13:39 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (19.74 KB, patch)
2020-10-16 10:20 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (32.63 KB, patch)
2020-10-19 09:52 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (20.50 KB, patch)
2020-10-19 13:08 PDT, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Buis 2020-10-02 13:44:47 PDT
Introduce Selection specific mixin of GlobalEventHandlers [1] by
splitting out the interface from Document+Selection.idl. This
also means the HTMLBodyElement specific selectionchange is
superflous and can be removed.

[1] https://w3c.github.io/selection-api/#dom-globaleventhandlers
Comment 1 Rob Buis 2020-10-02 13:48:31 PDT
Created attachment 410364 [details]
Patch
Comment 2 Rob Buis 2020-10-02 14:16:44 PDT
Created attachment 410370 [details]
Patch
Comment 3 Rob Buis 2020-10-02 15:25:51 PDT
Created attachment 410379 [details]
Patch
Comment 4 Rob Buis 2020-10-03 08:10:16 PDT
Created attachment 410423 [details]
Patch
Comment 5 Rob Buis 2020-10-04 12:57:08 PDT
Created attachment 410480 [details]
Patch
Comment 6 Radar WebKit Bug Importer 2020-10-09 13:45:24 PDT
<rdar://problem/70151600>
Comment 7 Rob Buis 2020-10-10 13:39:13 PDT
Created attachment 411019 [details]
Patch
Comment 8 Rob Buis 2020-10-16 10:20:46 PDT
Created attachment 411594 [details]
Patch
Comment 9 Rob Buis 2020-10-16 13:59:12 PDT
Sam, can you think of a reason why windows would behave differently on fast/dom/event-attribute-availability.html? I don't run windows and looking at the code I see no special code paths just for windows that could be relevant...
Comment 10 Sam Weinig 2020-10-18 19:11:32 PDT
(In reply to Rob Buis from comment #9)
> Sam, can you think of a reason why windows would behave differently on
> fast/dom/event-attribute-availability.html? I don't run windows and looking
> at the code I see no special code paths just for windows that could be
> relevant...

No, seems totally bizarre. I'll take a closer look though.
Comment 11 Sam Weinig 2020-10-18 19:15:32 PDT
(In reply to Sam Weinig from comment #10)
> (In reply to Rob Buis from comment #9)
> > Sam, can you think of a reason why windows would behave differently on
> > fast/dom/event-attribute-availability.html? I don't run windows and looking
> > at the code I see no special code paths just for windows that could be
> > relevant...
> 
> No, seems totally bizarre. I'll take a closer look though.

I think that is the only CMake based bot that runs the tests, so my initial guess is something isn't being rebuilt completely.
Comment 12 Rob Buis 2020-10-19 09:52:06 PDT
Created attachment 411756 [details]
Patch
Comment 13 Rob Buis 2020-10-19 12:37:49 PDT
(In reply to Sam Weinig from comment #11)
> (In reply to Sam Weinig from comment #10)
> > (In reply to Rob Buis from comment #9)
> > > Sam, can you think of a reason why windows would behave differently on
> > > fast/dom/event-attribute-availability.html? I don't run windows and looking
> > > at the code I see no special code paths just for windows that could be
> > > relevant...
> > 
> > No, seems totally bizarre. I'll take a closer look though.

Thanks for checking, good that it is not me that found it strange.

> I think that is the only CMake based bot that runs the tests, so my initial
> guess is something isn't being rebuilt completely.

How about adding a special expected.txt for windows for now like in my latest patch?
Comment 14 Darin Adler 2020-10-19 12:42:07 PDT
Comment on attachment 411756 [details]
Patch

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

> LayoutTests/fast/dom/event-handler-attributes-expected.txt:255
> +FAIL testElementAttribute(element, "selectionchange") should be target. Was script: target; content: none.

If this is correct, then we should change what the test expects, rather than just expecting a failure.

If this is incorrect, then this is a regression, and we should explain why it’s OK to regress.

> LayoutTests/platform/win/fast/dom/event-attribute-availability-expected.txt:1
> +This tests what event handler attributes are available on what objects.

Is there a way we can change the test to avoid having to have platform-specific results? I’d be willing to slightly dumb down the output, having some lines just say "PASS" without saying what’s tested, if it meant we didn’t have to keep an extra file around.
Comment 15 Darin Adler 2020-10-19 12:42:40 PDT
(In reply to Rob Buis from comment #13)
> How about adding a special expected.txt for windows for now like in my
> latest patch?

Not a fan of that, no.
Comment 16 Sam Weinig 2020-10-19 12:57:22 PDT
(In reply to Darin Adler from comment #15)
> (In reply to Rob Buis from comment #13)
> > How about adding a special expected.txt for windows for now like in my
> > latest patch?
> 
> Not a fan of that, no.

Me either. Here are two tests you could try.

1) Upload the patch with an innocuous change to page/DOMWindow.idl (update whitespace or something) to see if that triggers regeneration of the binding.
2) Upload the patch with an innocuous change to CodeGeneratorJS.pm (update whitespace or something) to see if that triggers regeneration of the binding.

My guess is the CMake later doesn't handle recursively supplemental dependencies which  are at work here.
Comment 17 Rob Buis 2020-10-19 13:08:05 PDT
Created attachment 411782 [details]
Patch
Comment 18 Rob Buis 2020-10-19 13:09:11 PDT
(In reply to Sam Weinig from comment #16)
> (In reply to Darin Adler from comment #15)
> > (In reply to Rob Buis from comment #13)
> > > How about adding a special expected.txt for windows for now like in my
> > > latest patch?
> > 
> > Not a fan of that, no.
> 
> Me either. Here are two tests you could try.
> 
> 1) Upload the patch with an innocuous change to page/DOMWindow.idl (update
> whitespace or something) to see if that triggers regeneration of the binding.
> 2) Upload the patch with an innocuous change to CodeGeneratorJS.pm (update
> whitespace or something) to see if that triggers regeneration of the binding.
> 
> My guess is the CMake later doesn't handle recursively supplemental
> dependencies which  are at work here.

Sounds like a plan, trying 1 first.
Comment 19 Rob Buis 2020-10-20 05:46:28 PDT
(In reply to Rob Buis from comment #18)
> (In reply to Sam Weinig from comment #16)
> > My guess is the CMake later doesn't handle recursively supplemental
> > dependencies which  are at work here.
> 
> Sounds like a plan, trying 1 first.

Option 1 seems to work :)
Comment 20 Sam Weinig 2020-10-20 09:26:08 PDT
(In reply to Rob Buis from comment #19)
> (In reply to Rob Buis from comment #18)
> > (In reply to Sam Weinig from comment #16)
> > > My guess is the CMake later doesn't handle recursively supplemental
> > > dependencies which  are at work here.
> > 
> > Sounds like a plan, trying 1 first.
> 
> Option 1 seems to work :)

Cool, but also blerg. It would be great to have someone fix the CMake stack to handle these dependencies correctly.
Comment 21 EWS 2020-10-20 10:24:42 PDT
Committed r268745: <https://trac.webkit.org/changeset/268745>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411782 [details].
Comment 22 Alexey Proskuryakov 2020-10-21 19:11:56 PDT
What are the answers to Darin's questions? Were they discussed outside Bugzilla somewhere?
Comment 23 Rob Buis 2020-10-22 00:48:09 PDT
(In reply to Alexey Proskuryakov from comment #22)
> What are the answers to Darin's questions? Were they discussed outside
> Bugzilla somewhere?

I think Darin only had one question and that it was invalidated by the fact the patch did not add a platform-specific test result for event-attribute-availabilit.html in the end.
Comment 24 Darin Adler 2020-10-22 08:42:43 PDT
Comment on attachment 411756 [details]
Patch

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

>> LayoutTests/fast/dom/event-handler-attributes-expected.txt:255
>> +FAIL testElementAttribute(element, "selectionchange") should be target. Was script: target; content: none.
> 
> If this is correct, then we should change what the test expects, rather than just expecting a failure.
> 
> If this is incorrect, then this is a regression, and we should explain why it’s OK to regress.

What was the resolution of this?
Comment 25 Alexey Shvayka 2021-12-08 17:16:55 PST
(In reply to Darin Adler from comment #24)
> Comment on attachment 411756 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411756&action=review
> 
> >> LayoutTests/fast/dom/event-handler-attributes-expected.txt:255
> >> +FAIL testElementAttribute(element, "selectionchange") should be target. Was script: target; content: none.
> > 
> > If this is correct, then we should change what the test expects, rather than just expecting a failure.
> > 
> > If this is incorrect, then this is a regression, and we should explain why it’s OK to regress.
> 
> What was the resolution of this?

This is a confirmed regression: tests basically say that `el.setAttribute("selectionchange", "<code>")` doesn't add an event listener.

It's being fixed in (r?) https://bugs.webkit.org/show_bug.cgi?id=234047 by adding "selectionchange" to HTMLElement::createEventHandlerNameMap().
Comment 26 Alexey Shvayka 2021-12-08 17:22:05 PST
(In reply to Alexey Shvayka from comment #25)
> This is a confirmed regression: tests basically say that `el.setAttribute("selectionchange", "<code>")` doesn't add an event listener.

Typo: `el.setAttribute("onselectionchange", "<code>")`
Comment 27 Darin Adler 2021-12-08 18:04:35 PST
So annoying that the test detected the regression and we just landed it anyway and broke it for a year!
Comment 28 Alexey Shvayka 2021-12-10 12:53:36 PST
(In reply to Darin Adler from comment #27)
> So annoying that the test detected the regression and we just landed it anyway and broke it for a year!

If this helps:

1) we don't emit "selectionchange" on elements (selection targets), only Gecko does: it's a spec draft / experimental feature;

2) this regression proves that it's easy to miss adding a global event handler to HTMLElement::createEventHandlerNameMap(); we should consider code-generating that map to ensure it's in sync with on* attributes of HTMLAttributeNames.in.