Closed Bug 1422934 Opened 7 years ago Closed 6 years ago

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

Categories

(Firefox :: Tabbed Browser, task, P3)

Product:
Firefox
Component:
Tabbed Browser
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: bgrins, Assigned: Kwan)

References

(Blocks 1 open bug)

Details

(Whiteboard: [xbl-available])

Attachments

(2 files)

Bug 1422934 - Make the tab close button an image, and remove the extends from its binding.
59 bytes, text/x-review-board-request
Gijs
: review+
dao
: review+
Details
Bug 1422934 - Remove unused toolbarbutton-image binding.
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
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
Depends on: 1422969
Depends on: 1422970
Priority: -- → P3
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
Attachment #8936598 - Flags: review?(bgrinstead) → review?(gijskruitbosch+bugs)
Attachment #8936599 - Flags: review?(bgrinstead) → review?(gijskruitbosch+bugs)
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: nobody → moz-ian
Status: NEW → ASSIGNED
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 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+
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.
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.
(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)
(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?
(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.
(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?)
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>.
(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.
(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 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 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+
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
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
(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)
(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.
(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)
(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 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+
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.
(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.
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)
Try run in comment 40 in green except for bug 1427510 due to old tree, and a couple intermittents.
Keywords: checkin-needed
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
Blocks: 1429929
https://hg.mozilla.org/mozilla-central/rev/a6346aa459b3
https://hg.mozilla.org/mozilla-central/rev/8339907be1cd
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Depends on: 1432630
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: