Migrate the columnpicker binding into a custom element

RESOLVED FIXED in Firefox 66

Status

()

defect
P3
normal
RESOLVED FIXED
6 months ago
4 months ago

People

(Reporter: vporof, Assigned: vporof)

Tracking

(Blocks 1 bug)

unspecified
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Assignee

Description

6 months ago
No description provided.
Assignee

Updated

6 months ago
Assignee: nobody → vporof
Assignee

Comment 1

6 months ago
For this one, the command event listener isn't firing. Any hints?
Attachment #9025580 - Flags: feedback?(bgrinstead)

Comment 2

6 months ago
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.
Assignee

Comment 3

6 months ago
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)
Blocks: 1450652
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)
Assignee

Comment 7

6 months ago
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)
Assignee

Comment 9

6 months ago
(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.

Updated

6 months ago
Priority: -- → P3
Assignee

Comment 10

6 months ago
Looks like it worked.
Attachment #9026333 - Attachment is obsolete: true
Attachment #9027298 - Flags: review?(bgrinstead)
Assignee

Updated

6 months ago
Depends on: 1503824
Assignee

Updated

6 months ago
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.
Assignee

Comment 15

6 months ago
Oops thought I removed it already, uploading a new patch now.
Assignee

Comment 16

6 months ago
Rebased.
Attachment #9027298 - Attachment is obsolete: true
Attachment #9027956 - Flags: review+
Assignee

Comment 18

6 months ago
Argh, another a11y test failing, looking into it today.

Comment 19

5 months ago
What is the status of this bug?
Flags: needinfo?(vporof)
Assignee

Comment 20

5 months ago
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.
Assignee

Comment 25

4 months ago

Fascinating! Let me try this out.

Assignee

Comment 26

4 months ago

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+
Assignee

Comment 27

4 months ago

Green try!

Keywords: checkin-needed

Comment 28

4 months ago

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

Comment 29

4 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.