Closed Bug 1507704 Opened 6 years ago Closed 6 years ago

Migrate the columnpicker binding into a custom element

Categories

(Toolkit :: XUL Widgets, task, P3)

Product:
Toolkit
Component:
XUL Widgets
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla66
Tracking Flags:
Tracking Status
firefox66 --- fixed

People

(Reporter: vporof, Assigned: vporof)

References

Details

Attachments

(1 file, 4 obsolete files)

remove-columnpicker-2019-01-17-0930.diff
(deleted), patch
vporof
: review+
Details | Diff | Splinter Review
No description provided.
Assignee: nobody → vporof
Attached patch remove-columnpicker-2018-11-16-1037.diff (obsolete) (deleted) — — Splinter Review
For this one, the command event listener isn't firing. Any hints?
Attachment #9025580 - Flags: feedback?(bgrinstead)
Comment on attachment 9025580 [details] [diff] [review] remove-columnpicker-2018-11-16-1037.diff Review of attachment 9025580 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/content/widgets/tree.js @@ +170,5 @@ > + super(); > + > + this.addEventListener("command", (event) => { > + if (event.originalTarget == this) { > + var popup = document.getAnonymousElementByAttribute(this, "anonid", "popup"); document.getAnonymousElementByAttribute doesn't work on CE as far as I know.
AFAICT, the command event handler isn't even entered.
Emilio, we have a situation here where the "columnpicker" XBL binding (which maps to the <xul:treecolpicker> tag) sets an event listener for "command" [0]. When switching to a Custom Element, that event doesn't fire anymore when clicking on the button (for example: * Open about:preferences#privacy * Click View Certificates * Click the "column picker" button on the right of the tree header (next to Security Device column). Could this be related to display="xul:button" on the XBL binding [1]? If so, will the typical "change layout frame based on tag name" pattern we've used for other bugs blocking Bug 1450652 actually solve this? FWIW, switching the event listener to "click" does fire as expected, so I'm wondering if we could change that instead. [0]: https://searchfox.org/mozilla-central/rev/b03a62c3c82316e733a3b09622c1cb7e59f64cc3/toolkit/content/widgets/tree.xml#1029 [1]: https://searchfox.org/mozilla-central/rev/b03a62c3c82316e733a3b09622c1cb7e59f64cc3/toolkit/content/widgets/tree.xml#975
Flags: needinfo?(emilio)
Comment on attachment 9025580 [details] [diff] [review] remove-columnpicker-2018-11-16-1037.diff Review of attachment 9025580 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/content/widgets/tree.js @@ +168,5 @@ > + class MozTreecolPicker extends MozElements.BaseControl { > + constructor() { > + super(); > + > + this.addEventListener("command", (event) => { This may be related to the [display] attr on the XBL binding, let's see. But in the meantime, if you switch this to "click" then it will start firing and you can work through the issues in this handler (like switching getAnonymousElementByAttribute to querySelector or a prop lookup, for example)
Attachment #9025580 - Flags: feedback?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #4) > Could this be related to display="xul:button" on the XBL binding [1]? If so, > will the typical "change layout frame based on tag name" pattern we've used > for other bugs blocking Bug 1450652 actually solve this? Yes, it is related to that: https://searchfox.org/mozilla-central/rev/b03a62c3c82316e733a3b09622c1cb7e59f64cc3/layout/xul/nsButtonBoxFrame.cpp#215 And yes, it will solve it. > FWIW, switching the event listener to "click" does fire as expected, so I'm > wondering if we could change that instead. Looking at that, the button's MouseClick function (and thus the 'command' event) is also called on keypressing the space key, or the return key on Mac: https://searchfox.org/mozilla-central/rev/b03a62c3c82316e733a3b09622c1cb7e59f64cc3/layout/xul/nsButtonBoxFrame.cpp#129 Click event listeners also seem to fire for that case for html buttons: data:text/html,<button onclick="alert('foo')">Focus me and press Space / Enter on Mac OS</button> https://searchfox.org/mozilla-central/rev/b03a62c3c82316e733a3b09622c1cb7e59f64cc3/dom/html/HTMLButtonElement.cpp#271 So maybe a custom element extending <html:button> + using the click event is better than special-casing this to use XUL frames in nsCSSFrameConstructor? It'd be a slightly riskier change, but probably better in the long term.
Flags: needinfo?(emilio)
Attached patch remove-columnpicker-2018-11-20-1120.diff (obsolete) (deleted) — — Splinter Review
This seems to work fine. Not sure about only having the 'click' handler though.
Attachment #9025580 - Attachment is obsolete: true
Attachment #9026333 - Flags: review?(bgrinstead)
Comment on attachment 9026333 [details] [diff] [review] remove-columnpicker-2018-11-20-1120.diff Review of attachment 9026333 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, just a couple notes. ::: toolkit/content/widgets/tree.js @@ +168,5 @@ > + class MozTreecolPicker extends MozElements.BaseControl { > + constructor() { > + super(); > + > + this.addEventListener("click", (event) => { As per Comment 6, can you switch this back to "command" and then add a special case for this tag name to use NS_NewButtonBoxFrame, similar to: https://searchfox.org/mozilla-central/rev/55895c49f55073d82d977cb74ec1d3a71ae4b25f/layout/base/nsCSSFrameConstructor.cpp#4220 This should wire up the command handler for click and also for keyboard accessibility. I think this'll be the easiest change for now. ::: toolkit/themes/shared/tree.inc.css @@ +142,4 @@ > /* ::::: column picker ::::: */ > > .tree-columnpicker-icon { > + pointer-events: none; Why is this change here? Will handling command as per the previous comment remove the need for this?
Attachment #9026333 - Flags: review?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #8) > Comment on attachment 9026333 [details] [diff] [review] > remove-columnpicker-2018-11-20-1120.diff > > Review of attachment 9026333 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good, just a couple notes. > > ::: toolkit/themes/shared/tree.inc.css > @@ +142,4 @@ > > /* ::::: column picker ::::: */ > > > > .tree-columnpicker-icon { > > + pointer-events: none; > > Why is this change here? Will handling command as per the previous comment > remove the need for this? It's there because the the internal icon prevent clicks from registering on the parent custom element. Handling command events instead might take care of this, I'll check.
Priority: -- → P3
Attached patch remove-columnpicker-2018-11-24-0900.diff (obsolete) (deleted) — — Splinter Review
Looks like it worked.
Attachment #9026333 - Attachment is obsolete: true
Attachment #9027298 - Flags: review?(bgrinstead)
Depends on: 1503824
Blocks: 1503826
Attachment #9027298 - Flags: review?(bgrinstead) → review+
Also, was surprised to see an unexpected pass on a crashtest (https://searchfox.org/mozilla-central/source/layout/generic/crashtests/382745-1.xhtml): https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2639d154dd88227d50c788a958394a75d4019a3&selectedJob=213898149. It's fxpecting to fail as of whitespace handling changes in Bug 1487568. - https://bugzilla.mozilla.org/show_bug.cgi?id=1487568#c16 - https://bugzilla.mozilla.org/show_bug.cgi?id=758695#c16 I guess we could just remove the expected failure at https://searchfox.org/mozilla-central/rev/d35199ef15ffa57d190886e20d5df6471faf0870/layout/generic/crashtests/crashtests.list#118, although I assume that means the test won't be testing what it meant to anymore (once treecols is a Custom Element). The alternative would be to rebuild the test using tags that still are using XBL, but given that it's already failing and that we are actively removing XBL it seems like removing the expected failure would be OK. What do you think Emilio?
Flags: needinfo?(emilio)
Yeah, I would remove the annotation.
Flags: needinfo?(emilio)
(In reply to Emilio Cobos Álvarez (:emilio) from comment #13) > Yeah, I would remove the annotation. OK. Victor - I think that the `asserts(1)` from https://searchfox.org/mozilla-central/rev/d35199ef15ffa57d190886e20d5df6471faf0870/layout/generic/crashtests/crashtests.list#118 can be removed in the patch in Bug 1503824.
Oops thought I removed it already, uploading a new patch now.
Attached patch remove-columnpicker-2018-11-27-1840.diff (obsolete) (deleted) — — Splinter Review
Rebased.
Attachment #9027298 - Attachment is obsolete: true
Attachment #9027956 - Flags: review+
Argh, another a11y test failing, looking into it today.
What is the status of this bug?
Flags: needinfo?(vporof)
On it!
Flags: needinfo?(vporof)

Jamie, we are seeing a regression on accessible/tests/mochitest/treeupdate/test_menubutton.xul with this patch applied: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9526999a2bc95ca6f39022f01fda6b2e68b1a67a&selectedJob=214177265.

Wrong value of property 'role' for ['menuitem node', address: [object XULElement], role: menuitem, name: 'Restore Column Order', address: 0xd5e39e80].got 'menuitem', expected 'check menu item'

Before going further trying to make this test path, I wanted to run this by you. I think this menu item should actually have the menuitem role and not the 'check menu item' role. The reason is that this is an item that you click once and it performs an action (restoring tree column order, I guess) - it never actually gets checked.

Do you think it would make sense to update this test to expect the 'menuitem' role for this button? Presumably XBL is causing some magic (likely due to tag name remapping from 'treecolpicker' to 'button'), but if it's actually leading to a better outcome without it I'd rather save time trying to restore the old behavior.

Flags: needinfo?(jteh)

This test failure isn't quite what it seems. The test is checking that the first 2 children (2 is hard-coded) in the menu have the desired role:
https://searchfox.org/mozilla-central/source/accessible/tests/mochitest/treeupdate/test_menubutton.xul#52
In this case, it's checking for ROLE_CHECK_MENU_ITEM:
https://searchfox.org/mozilla-central/source/accessible/tests/mochitest/treeupdate/test_menubutton.xul#82

Because this tree has two columns:
https://searchfox.org/mozilla-central/source/accessible/tests/mochitest/treeupdate/test_menubutton.xul#133
I would indeed expect the first two children to have ROLE_CHECK_MENU_ITEM (one per column). The "Restore Column Order" should be the last child, not the first or second. Furthermore, I just checked on Nightly and "Restore Column Order" has ROLE_MENUITEM already.

So, either the column items aren't being exposed at all to accessibility now (very bad) or they've been moved after "Restore Column Order" instead of before (UX change). If the latter is the case and it's intentional, the test would need to be tweaked to look at the last two children in this case.

Flags: needinfo?(jteh)

(In reply to James Teh [:Jamie] from comment #22)

This test failure isn't quite what it seems. The test is checking that the first 2 children (2 is hard-coded) in the menu have the desired role:
https://searchfox.org/mozilla-central/source/accessible/tests/mochitest/treeupdate/test_menubutton.xul#52
In this case, it's checking for ROLE_CHECK_MENU_ITEM:
https://searchfox.org/mozilla-central/source/accessible/tests/mochitest/treeupdate/test_menubutton.xul#82

Because this tree has two columns:
https://searchfox.org/mozilla-central/source/accessible/tests/mochitest/treeupdate/test_menubutton.xul#133
I would indeed expect the first two children to have ROLE_CHECK_MENU_ITEM (one per column). The "Restore Column Order" should be the last child, not the first or second. Furthermore, I just checked on Nightly and "Restore Column Order" has ROLE_MENUITEM already.

So, either the column items aren't being exposed at all to accessibility now (very bad) or they've been moved after "Restore Column Order" instead of before (UX change). If the latter is the case and it's intentional, the test would need to be tweaked to look at the last two children in this case.

Interesting, thank you. When I was debugging this locally, I seem to remember only seeing two items total (one option plus the "restore" button), so maybe this is surfacing an actual bug with the conversion where it's not rendering all the columns in the popup.

Victor, can you reproduce this locally? I was commenting out the line gQueue.invoke() at https://searchfox.org/mozilla-central/source/accessible/tests/mochitest/treeupdate/test_menubutton.xul#83 and then manually clicking on the treecolpicker.

Comment on attachment 9027956 [details] [diff] [review] remove-columnpicker-2018-11-27-1840.diff Review of attachment 9027956 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/content/widgets/tree.js @@ +242,5 @@ > + > + var hidden = !tree.enableColumnDrag; > + const anonids = ["menuseparator", "menuitem"]; > + for (var i = 0; i < anonids.length; i++) { > + var element = this.querySelector(anonids[i]); So I think I found the bug. This should be: `var element = this.querySelector(`[anonid=${anonids[i]}]`);`. It is currently query selectoring the first menuitem (which is the one generated for the first column), and removing it.

Fascinating! Let me try this out.

I had to surround anonids[i] in quotes, and also extend this to a few other call sites. Fingers crossed.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b21cfebb14493e78c55b47794e10d04d2e56d7f

Attachment #9027956 - Attachment is obsolete: true
Attachment #9037153 - Flags: review+

Green try!

Keywords: checkin-needed

Pushed by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dae7a246b32d
Migrate the columnpicker binding into a custom element, r=bgrins

Keywords: checkin-needed

https://hg.mozilla.org/mozilla-central/rev/dae7a246b32d

Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox66: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Type: defect → task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: