Remove the "toolbarbutton-image" binding and migrate consumers of it to the "toolbarbutton" binding

RESOLVED FIXED in Firefox 59

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: bgrins, Assigned: Kwan)

Tracking

(Blocks 2 bugs)

unspecified
Firefox 59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

(Whiteboard: [xbl-available])

Attachments

(2 attachments)

Reporter

Description

2 years ago
The "toolbarbutton-image" binding provides custom content to generate simpler markup inside a toolbarbutton. There's only one consumer of "toolbarbutton-image" in m-c ("tabbrowser-close-tab-button").

I believe we can just update the consumer to use "toolbarbutton", as the .toobarbutton-text element will be empty and shouldn't take up any space. This can be confirmed via mozscreenshots or manual testing.

- https://dxr.mozilla.org/mozilla-central/search?q=toolbarbutton-image&redirect=true
- https://dxr.mozilla.org/mozilla-central/rev/709f355a7a8c4ae426d1824841a71ffdb5ce0137/toolkit/content/widgets/toolbarbutton.xml#78
- https://dxr.mozilla.org/mozilla-central/rev/709f355a7a8c4ae426d1824841a71ffdb5ce0137/browser/base/content/tabbrowser.xml#7785
- https://dxr.mozilla.org/mozilla-central/search?q=path%3Atoolbarbutton&redirect=false

Updated

2 years ago
Depends on: 1422969

Updated

2 years ago
Depends on: 1422970
Priority: -- → P3
Comment hidden (mozreview-request)
Reporter

Comment 3

2 years ago
mozreview-review
Comment on attachment 8936599 [details]
Bug 1422934 - Remove unused toolbarbutton-image binding.

https://reviewboard.mozilla.org/r/207352/#review213180

Redirecting review to Gijs for an extra sanity check since I proposed this approach
Reporter

Updated

2 years ago
Attachment #8936598 - Flags: review?(bgrinstead) → review?(gijskruitbosch+bugs)
Attachment #8936599 - Flags: review?(bgrinstead) → review?(gijskruitbosch+bugs)
Assignee

Comment 4

2 years ago
As suspected the .toolbarbutton-text (and .toolbarbutton-multiline-text) don't take up any space.

One could keep the old behaviour by moving the <content/> from toolbarbutton-image to tabbrowser-close-tab-button, but given that changing between icons, text, and icons + text modes for toolbars seems like it probably went away with australis, I'm not sure there is any point.
Assignee

Updated

2 years ago
Assignee: nobody → moz-ian
Status: NEW → ASSIGNED

Comment 5

2 years ago
mozreview-review
Comment on attachment 8936598 [details]
Bug 1422934 - Make the tab close button an image, and remove the extends from its binding.

https://reviewboard.mozilla.org/r/207350/#review213226

Tentative r+, see below.

::: browser/base/content/tabbrowser.xml:7739
(Diff revision 1)
>         This binding relies on the structure of the tabbrowser binding.
>         Therefore it should only be used as a child of the tab or the tabs
>         element (in both cases, when they are anonymous nodes of <tabbrowser>).
>    -->
>    <binding id="tabbrowser-close-tab-button"
> -           extends="chrome://global/content/bindings/toolbarbutton.xml#toolbarbutton-image">
> +           extends="chrome://global/content/bindings/toolbarbutton.xml#toolbarbutton">

I mean, this does mean that given a window with 100 tabs we now create 200 extra nodes, right? That seems kind of suboptimal... but I also don't really have a much better idea. Plus if there are a lot of tabs then the tab button is hidden except on the selected tab, so maaaaybe we (can?) avoid constructing all the nodes immediately? I dunno if we do or don't... but perhaps it's worth just doing some performance checking here?
Attachment #8936598 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 6

2 years ago
mozreview-review
Comment on attachment 8936599 [details]
Bug 1422934 - Remove unused toolbarbutton-image binding.

https://reviewboard.mozilla.org/r/207352/#review213228
Attachment #8936599 - Flags: review?(gijskruitbosch+bugs) → review+
Assignee

Comment 7

2 years ago
mozreview-review
Comment on attachment 8936598 [details]
Bug 1422934 - Make the tab close button an image, and remove the extends from its binding.

https://reviewboard.mozilla.org/r/207350/#review213240

::: browser/base/content/tabbrowser.xml:7739
(Diff revision 1)
>         This binding relies on the structure of the tabbrowser binding.
>         Therefore it should only be used as a child of the tab or the tabs
>         element (in both cases, when they are anonymous nodes of <tabbrowser>).
>    -->
>    <binding id="tabbrowser-close-tab-button"
> -           extends="chrome://global/content/bindings/toolbarbutton.xml#toolbarbutton-image">
> +           extends="chrome://global/content/bindings/toolbarbutton.xml#toolbarbutton">

Ah, right, that'd be the other reason to consider moving <content/> to tabbrowser-close-tab-button.  I did wonder about the performance implications of the extra nodes but forgot to mention it.

Comment 8

2 years ago
I mean, if we don't already, we could try if it's OK to mark the node as -moz-binding: none while the tab is not selected and close buttons on non-selected tabs are hidden. That should avoid the node creation (including the image!) assuming that styling kicks in "in time". cf. bug 1374691.
Assignee

Comment 9

2 years ago
(In reply to :Gijs from comment #8)
> I mean, if we don't already, we could try if it's OK to mark the node as
> -moz-binding: none while the tab is not selected and close buttons on
> non-selected tabs are hidden. That should avoid the node creation (including
> the image!) assuming that styling kicks in "in time". cf. bug 1374691.

Unfortunately it doesn't seem to, the browser toolbox still shows those nodes as there.

I'm assuming talos chrome is the suite that'd be useful for this, in which case restoring the content optimisation doesn't seem to make much difference:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=a5c4ff436ae0&newProject=try&newRevision=637632d28b2a6189cada377fa6a3838e73d0d926&framework=1

(assuming there's actually something in there that'd test this meaningfully)
Reporter

Comment 10

2 years ago
(In reply to Ian Moody [:Kwan] (UTC+0) from comment #9)
> (In reply to :Gijs from comment #8)
> > I mean, if we don't already, we could try if it's OK to mark the node as
> > -moz-binding: none while the tab is not selected and close buttons on
> > non-selected tabs are hidden. That should avoid the node creation (including
> > the image!) assuming that styling kicks in "in time". cf. bug 1374691.
> 
> Unfortunately it doesn't seem to, the browser toolbox still shows those
> nodes as there.

The Browser Toolbox doesn't always get the right mutations inside of XBL anonymous content, so if you've already expanded one of the nodes before the -moz-binding: none was set they may not be removed. Can you double check this is the case by waiting to inspect the node until after the binding is set to none, or by running `document.getAnonymousNodes($0)` after selecting the element in the inspector?
Reporter

Comment 11

2 years ago
(In reply to Ian Moody [:Kwan] (UTC+0) from comment #9)
> I'm assuming talos chrome is the suite that'd be useful for this, in which
> case restoring the content optimisation doesn't seem to make much difference:
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=a5c4ff436ae0&newProject=try&newR
> evision=637632d28b2a6189cada377fa6a3838e73d0d926&framework=1
> 
> (assuming there's actually something in there that'd test this meaningfully)

I would run all of the talos suites (and not just chrome) to be sure - but yes I would say if Talos doesn't catch a regression then it should generally be OK to land.
Assignee

Comment 12

2 years ago
(In reply to Brian Grinstead [:bgrins] from comment #10)
> (In reply to Ian Moody [:Kwan] (UTC+0) from comment #9)
> > (In reply to :Gijs from comment #8)
> > > I mean, if we don't already, we could try if it's OK to mark the node as
> > > -moz-binding: none while the tab is not selected and close buttons on
> > > non-selected tabs are hidden. That should avoid the node creation (including
> > > the image!) assuming that styling kicks in "in time". cf. bug 1374691.
> > 
> > Unfortunately it doesn't seem to, the browser toolbox still shows those
> > nodes as there.
> 
> The Browser Toolbox doesn't always get the right mutations inside of XBL
> anonymous content, so if you've already expanded one of the nodes before the
> -moz-binding: none was set they may not be removed. Can you double check
> this is the case by waiting to inspect the node until after the binding is
> set to none, or by running `document.getAnonymousNodes($0)` after selecting
> the element in the inspector?

I did wonder about the toolbox not handling XUL/XBL 100%, so my test was to get the tab in such a state, then open the toolbox and inspect.  I just tried getAnonymousNodes() to double check as well and it does indeed return all three nodes.

(In reply to Brian Grinstead [:bgrins] from comment #11)
> (In reply to Ian Moody [:Kwan] (UTC+0) from comment #9)
> > I'm assuming talos chrome is the suite that'd be useful for this, in which
> > case restoring the content optimisation doesn't seem to make much difference:
> > https://treeherder.mozilla.org/perf.html#/
> > compare?originalProject=try&originalRevision=a5c4ff436ae0&newProject=try&newR
> > evision=637632d28b2a6189cada377fa6a3838e73d0d926&framework=1
> > 
> > (assuming there's actually something in there that'd test this meaningfully)
> 
> I would run all of the talos suites (and not just chrome) to be sure - but
> yes I would say if Talos doesn't catch a regression then it should generally
> be OK to land.

Okay, I'll try a -t all run.

(I take it not including the <content/> in toolbar-close-tab-button is preferable if there's no performance regression since it reduces the XBL LoC?)
Assignee

Comment 13

2 years ago
While I don't have any experience interpreting talos results, nothing in this seems obviously concerning:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=93b69e696f01b79f6a7bb1252bef87381f655c26&framework=1&filter=linux%20opt&selectedTimeRange=172800

(The few large changes seem within the error margins anyway)

Second opinion?
Flags: needinfo?(bgrinstead)
I'm not sure that we actually need the close button to be a <toolbarbutton>. For instance, the soundplaying / mute thingy is just an <image>.
Assignee

Comment 15

a year ago
(In reply to Dão Gottwald [::dao] from comment #14)
> I'm not sure that we actually need the close button to be a <toolbarbutton>.
> For instance, the soundplaying / mute thingy is just an <image>.

This does seem to work by the way (junking the extends entirely and switching to <xul:image> at https://dxr.mozilla.org/mozilla-central/rev/5d203926da51a7e949a20818664b19d5b115572d/browser/base/content/tabbrowser.xml#7816 ), at least from cursory testing, haven't tried running tests.
Reporter

Comment 16

a year ago
(In reply to Ian Moody [:Kwan] (UTC+0) from comment #15)
> (In reply to Dão Gottwald [::dao] from comment #14)
> > I'm not sure that we actually need the close button to be a <toolbarbutton>.
> > For instance, the soundplaying / mute thingy is just an <image>.
> 
> This does seem to work by the way (junking the extends entirely and
> switching to <xul:image> at
> https://dxr.mozilla.org/mozilla-central/rev/
> 5d203926da51a7e949a20818664b19d5b115572d/browser/base/content/tabbrowser.
> xml#7816 ), at least from cursory testing, haven't tried running tests.

Either way would work for me. I don't see any Talos issues with your current patches.
Flags: needinfo?(bgrinstead)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 19

a year ago
Comment on attachment 8936598 [details]
Bug 1422934 - Make the tab close button an image, and remove the extends from its binding.

Try seems fine with the image change: https://treeherder.mozilla.org/#/jobs?repo=try&revision=51f8cb8187054201578d5c92b0a13e18a75aa4bc&selectedJob=152744768
(though bug 1425879 is annoying)

And though bug 1381817, bug 1414741, and bug 1381430, make mozscreenshots oh-so-fun right now, I think the only real difference is about a pixel more of tab title is shown before the fadeout:
https://screenshots.mattn.ca/comparisons/try/fe552e288c5fbca5df96b7d6eda9714fbf41d977/try/cdad026ab6d833ac224c6725709bfff2e14fef15/osx-10-10/primaryUI_205_tabsInTitlebar_twoPinnedWithOverflow_maximized_allToolbars_compactLight_compactDensity.png
Attachment #8936598 - Flags: review+ → review?(gijskruitbosch+bugs)

Comment 20

a year ago
mozreview-review
Comment on attachment 8936598 [details]
Bug 1422934 - Make the tab close button an image, and remove the extends from its binding.

https://reviewboard.mozilla.org/r/207350/#review215016

Yeah, this wfm. Thanks for the thorough work here!
Attachment #8936598 - Flags: review?(gijskruitbosch+bugs) → review+
Assignee

Comment 21

a year ago
Thanks Gijs!

Sheriff, try run is in comment #19, though that's pre-histedit, so change three is now change one and old-one is gone (and 4 was another bug).
Keywords: checkin-needed

Comment 22

a year ago
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/123d69b3834a
Make the tab close button an image, and remove the extends from its binding. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/b97013a21476
Remove unused toolbarbutton-image binding. r=Gijs
Keywords: checkin-needed
(In reply to Ian Moody [:Kwan] (UTC+0) from comment #19)
> And though bug 1381817, bug 1414741, and bug 1381430, make mozscreenshots
> oh-so-fun right now, I think the only real difference is about a pixel more
> of tab title is shown before the fadeout:
> https://screenshots.mattn.ca/comparisons/try/
> fe552e288c5fbca5df96b7d6eda9714fbf41d977/try/
> cdad026ab6d833ac224c6725709bfff2e14fef15/osx-10-10/
> primaryUI_205_tabsInTitlebar_twoPinnedWithOverflow_maximized_allToolbars_comp
> actLight_compactDensity.png

Is this Mac-only? It might fix bug 1415671 then...

If it wasn't Mac-only, I'd like to better understand what's causing the difference, as this is one of the primary UI pieces where we care about such minor stuff.
Flags: needinfo?(moz-ian)
Depends on: 1426826
Assignee

Comment 25

a year ago
(In reply to Dão Gottwald [::dao] from comment #23)
> (In reply to Ian Moody [:Kwan] (UTC+0) from comment #19)
> > And though bug 1381817, bug 1414741, and bug 1381430, make mozscreenshots
> > oh-so-fun right now, I think the only real difference is about a pixel more
> > of tab title is shown before the fadeout:
> > https://screenshots.mattn.ca/comparisons/try/
> > fe552e288c5fbca5df96b7d6eda9714fbf41d977/try/
> > cdad026ab6d833ac224c6725709bfff2e14fef15/osx-10-10/
> > primaryUI_205_tabsInTitlebar_twoPinnedWithOverflow_maximized_allToolbars_comp
> > actLight_compactDensity.png
> 
> Is this Mac-only? It might fix bug 1415671 then...
> 
> If it wasn't Mac-only, I'd like to better understand what's causing the
> difference, as this is one of the primary UI pieces where we care about such
> minor stuff.

Yes mac only, as far as I can tell amongst all the noise.
https://screenshots.mattn.ca/compare/?oldProject=try&oldRev=a5060cf8a8705577d58ba16067b3e5e842be395a&newProject=try&newRev=cdad026ab6d833ac224c6725709bfff2e14fef15

(I'll buy a drink for anyone who can find the 1px difference on the Windows comparisons, I can't)

(In reply to Andreea Pavel [:apavel] from comment #24)
> Backed out for failing e.g.
> testing/firefox-ui/tests/puppeteer/test_tabbar.py r=backout on a CLOSED TREE
> 
> Push with failures:
> https://treeherder.mozilla.org/#/
> jobs?repo=autoland&revision=b97013a2147601ed4464172e3fa493c33f543d50
> 
> Failure logs:
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=153111029&repo=autoland&lineNumber=22759
> 
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=153111640&repo=autoland&lineNumber=7621
> 
> Backout:
> https://hg.mozilla.org/integration/autoland/rev/
> 39f703c43a6b353d6525f6db4c1ad6324487b22c

So the test_tabbar.py failure isn't particularly interesting, just an assertion that needs updating (and I need to remember there're now other testsuites than mochitest-bc for chrome changes):
https://searchfox.org/mozilla-central/rev/22c55eb7b7e6494a8615a7af3b613ff899d2cdba/testing/firefox-ui/tests/puppeteer/test_tabbar.py#131

However the accessibility failures are much more valuable:
https://treeherder.mozilla.org/logviewer.html#?job_id=153111640&repo=autoland&lineNumber=7621
https://searchfox.org/mozilla-central/rev/22c55eb7b7e6494a8615a7af3b613ff899d2cdba/accessible/tests/mochitest/tree/test_tabbrowser.xul#136
I'm guessing this would have regressed the experience for screen reader etc users, since the close button would no longer have been annotated as a button.  role="button" is enough to fix it, so I'll update the patch with that and the assertion change shortly, but I won't request re-review until after the holidays.
(though I now wonder why the mentioned mute tab button is role="presentation" instead of button)

Merry Christmas everyone!
Flags: needinfo?(moz-ian)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(In reply to Ian Moody [:Kwan] (UTC+0) from comment #25)
> However the accessibility failures are much more valuable:
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=153111640&repo=autoland&lineNumber=7621
> https://searchfox.org/mozilla-central/rev/
> 22c55eb7b7e6494a8615a7af3b613ff899d2cdba/accessible/tests/mochitest/tree/
> test_tabbrowser.xul#136
> I'm guessing this would have regressed the experience for screen reader etc
> users, since the close button would no longer have been annotated as a
> button.  role="button" is enough to fix it, so I'll update the patch with
> that and the assertion change shortly, but I won't request re-review until
> after the holidays.
> (though I now wonder why the mentioned mute tab button is
> role="presentation" instead of button)

Because there's a keyboard shortcut for muting the selected tab. Of course there's one for closing the selected tab too. Neither shortcuts work for background tabs though. The idea that screenreader users would walk through the a11y tree to find a particular background tab's close or mute button seems a bit weird to me, but a11y folks will probably disagree.

Comment 29

a year ago
(In reply to Dão Gottwald [::dao] from comment #28)
> Because there's a keyboard shortcut for muting the selected tab. Of course
> there's one for closing the selected tab too. Neither shortcuts work for
> background tabs though. The idea that screenreader users would walk through
> the a11y tree to find a particular background tab's close or mute button
> seems a bit weird to me, but a11y folks will probably disagree.

I'm not sure they would... it's quite possible this is just legacy stuff that's never been updated. Marco, is there a good reason for having the tab close button be represented as a button in the a11y tree, given there's a keyboard shortcut as well as menu entries for it, and presumably no real need for people who use the a11y tree to access it that way - similar to the tab mute button?

Note that if we accept this not being a button, it should probably get role=presentation.
Flags: needinfo?(mzehe)

Comment 30

a year ago
(fwiw, rs=me on adding role=button/presentation based on what Marco says, no need to re-request review - Merry Christmas! :-) )
Blocks: 1415671
(In reply to :Gijs from comment #29)
> I'm not sure they would... it's quite possible this is just legacy stuff
> that's never been updated. Marco, is there a good reason for having the tab
> close button be represented as a button in the a11y tree, given there's a
> keyboard shortcut as well as menu entries for it, and presumably no real
> need for people who use the a11y tree to access it that way - similar to the
> tab mute button?

I've not used it myself, because it is only accessible via techniques like the NVDA object navigator. And yes, a keyboard shortcut is a much faster way to do the same thing.

> Note that if we accept this not being a button, it should probably get
> role=presentation.

Yes, it should.
Flags: needinfo?(mzehe)
Ian, care to update the patch? You might also want to address bug 1426826.
Flags: needinfo?(moz-ian)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 35

a year ago
mozreview-review
Comment on attachment 8936598 [details]
Bug 1422934 - Make the tab close button an image, and remove the extends from its binding.

https://reviewboard.mozilla.org/r/207350/#review217848
Attachment #8936598 - Flags: review+
Duplicate of this bug: 1426826
Assignee

Comment 37

a year ago
Thanks for the review of the CSS Dão!

Since it's so close to merge day I'm going to wait until 60 to land this so it gets a little longer to bake on nightly, just in-case we do get some negative accessible feedback.  While there is Ctrl+W for the current tab, the buttons also allow closing other tabs (at least when there aren't too many), which won't be possible now.  No idea how often that'd actually be used though.
(though feel free to disagree and request landing it.  I don't know if we even have much accessible use on nightly so it might be pointless)
(In reply to Ian Moody [:Kwan] (UTC+0) from comment #37)
> While there is Ctrl+W for the current tab,
> the buttons also allow closing other tabs (at least when there aren't too
> many), which won't be possible now.  No idea how often that'd actually be
> used though.

It's not like this ever worked well even if people may have tried it. The button doesn't currently have an accessible label.

I think this is good to land now.

Comment 39

a year ago
(In reply to Dão Gottwald [::dao] from comment #38)
> (In reply to Ian Moody [:Kwan] (UTC+0) from comment #37)
> > While there is Ctrl+W for the current tab,
> > the buttons also allow closing other tabs (at least when there aren't too
> > many), which won't be possible now.  No idea how often that'd actually be
> > used though.
> 
> It's not like this ever worked well even if people may have tried it. The
> button doesn't currently have an accessible label.
> 
> I think this is good to land now.

FWIW, +1 on this. I think it's perfectly OK to land now.
Assignee

Comment 40

a year ago
Okay, I've kicked off a try (with the previous backout-causing ally and functional-ui jobs) and if it comes back green I'll request checkin (after I've checked if it's auto-rebasable):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e1fb676bedc845b47d1132fdf6416e65447c1d6

(hmm, meant to clear the needinfo with comment 37)
Flags: needinfo?(moz-ian)
Assignee

Comment 41

a year ago
Try run in comment 40 in green except for bug 1427510 due to old tree, and a couple intermittents.
Keywords: checkin-needed

Comment 42

a year ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a6346aa459b3
Make the tab close button an image, and remove the extends from its binding. r=dao,Gijs
https://hg.mozilla.org/integration/autoland/rev/8339907be1cd
Remove unused toolbarbutton-image binding. r=Gijs
Keywords: checkin-needed
Assignee

Updated

a year ago
Blocks: 1429929

Comment 43

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a6346aa459b3
https://hg.mozilla.org/mozilla-central/rev/8339907be1cd
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Assignee

Updated

a year ago
Depends on: 1432630
You need to log in before you can comment on or make changes to this bug.