Bug 217248 – Introduce Selection specific mixin of GlobalEventHandlers

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Rob Buis]

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

[Rob Buis]

Created attachment 410364 [details]
Patch

[Rob Buis]

Created attachment 410370 [details]
Patch

[Rob Buis]

Created attachment 410379 [details]
Patch

[Rob Buis]

Created attachment 410423 [details]
Patch

[Rob Buis]

Created attachment 410480 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/70151600>

[Rob Buis]

Created attachment 411019 [details]
Patch

[Rob Buis]

Created attachment 411594 [details]
Patch

[Rob Buis]

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

[Sam Weinig]

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

[Sam Weinig]

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

[Rob Buis]

Created attachment 411756 [details]
Patch

[Rob Buis]

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

[Darin Adler]

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.

[Darin Adler]

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

[Sam Weinig]

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

[Rob Buis]

Created attachment 411782 [details]
Patch

[Rob Buis]

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

[Rob Buis]

(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 :)

[Sam Weinig]

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

[EWS]

Committed r268745: <https://trac.webkit.org/changeset/268745>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411782 [details].

[Alexey Proskuryakov]

What are the answers to Darin's questions? Were they discussed outside Bugzilla somewhere?

[Rob Buis]

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

[Darin Adler]

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?

[Alexey Shvayka]

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

[Alexey Shvayka]

(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>")`

[Darin Adler]

So annoying that the test detected the regression and we just landed it anyway and broke it for a year!

[Alexey Shvayka]

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