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
Created attachment 410364 [details] Patch
Created attachment 410370 [details] Patch
Created attachment 410379 [details] Patch
Created attachment 410423 [details] Patch
Created attachment 410480 [details] Patch
<rdar://problem/70151600>
Created attachment 411019 [details] Patch
Created attachment 411594 [details] Patch
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...
(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.
(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.
Created attachment 411756 [details] Patch
(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 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.
(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.
(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.
Created attachment 411782 [details] Patch
(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.
(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 :)
(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.
Committed r268745: <https://trac.webkit.org/changeset/268745> All reviewed patches have been landed. Closing bug and clearing flags on attachment 411782 [details].
What are the answers to Darin's questions? Were they discussed outside Bugzilla somewhere?
(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 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?
(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().
(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>")`
So annoying that the test detected the regression and we just landed it anyway and broke it for a year!
(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.