1370791 - New tab button (+) disappears when disabling a tool that is positioned in front of it

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect, P1)

Description

[Bogdan Maris, Desktop QA [Holiday+PTO 1-6 May]]

Attachment: Screencast showing the issue

[Affected versions]:
- Firefox 53 RC
- Firefox 54 RC
- Latest Nightly 55.0a1
- old Nightly 37.0a1

[Affected platforms]:
- macOS 10.12.5
- Windows 10 64bit
- Ubuntu 16.04 32bit

[Steps to reproduce]:
1. Start Firefox
2. Enter customize and drag any tool that can be turned off by pref in front of 'Open a new tab' (+)
(I tested with Report Site Issue tool, disabling by pref 'extensions.webcompat-reporter.enabled' switched to 'false'
3. Exit customize and visit about:config
4. Disable the tool dragged at step 2 by flipping its pref to false

[Expected result]:
- Open a new tab button is still visible and accessible.

[Actual result]:
- Open a new tab button is gone along with the tool that was disabled at step 4. Flipping the pref back to 'true' will make 'Open a new tab' button reappear.

[Regression range]:
- This is not a recent regression, I reproduced this as far back as Nightly 37.0a1 where I used 'Forget' button to reproduce.

[Additional notes]:
- Note that this does not affect the possibility of opening a new tab using cmd/ctrl + t or File/New Tab.
- I reproduced this with three tools (of which I knew their pref) but I am sure it will reproduce with all that can be disabled by pref:
 - Forget - privacy.panicButton.useLocaleList
 - Firefox Screenshots - extensions.screenshots.system-disabled
 - Web Compatibility - extensions.webcompat-reporter.enabled

Comment 1

[chyiz]

I can confirm this is still affecting latest Firedoge Nightly.
To make it worse, the icon can also be from an extension. After disabling or removing the extension providing the icon, the new tab icon disappears together with the extension icon. It will keep hidden even after restarts. If the user can not remember which particular extension icon was put before the new tab icon, they may be forced to reset Firefox to get out of this bug.

Comment 2

[:Gijs (he/him)]

This is caused by this CSS:

https://dxr.mozilla.org/mozilla-central/rev/3ecda4678c49ca255c38b1697142b9118cdd27e7/browser/base/content/browser.css#112-115

#tabbrowser-tabs:not([overflow="true"]) ~ #alltabs-button,
#tabbrowser-tabs:not([overflow="true"]) + #new-tab-button,
#tabbrowser-tabs[overflow="true"] > .tabbrowser-arrowscrollbox > .tabs-newtab-button,
#TabsToolbar[currentset]:not([currentset*="tabbrowser-tabs,new-tab-button"]) > #tabbrowser-tabs > .tabbrowser-arrowscrollbox > .tabs-newtab-button,
#TabsToolbar[customizing="true"] > #tabbrowser-tabs > .tabbrowser-arrowscrollbox > .tabs-newtab-button {
  visibility: collapse;
}


There are 2 new tab buttons. 1 is the customizable widget which you can put wherever. The other is the one that's part of the tabstrip and which will show up directly next to the last tab when there's only a few tabs (rather than all the way at the end when the tabstrip is overflowing).

The problem is that this check:

#TabsToolbar[currentset]:not([currentset*="tabbrowser-tabs,new-tab-button"]) > #tabbrowser-tabs > .tabbrowser-arrowscrollbox > .tabs-newtab-button,

hides the non-overflow version of the new tab button when the new tab button isn't directly after the tabstrip (which is the case because of the disabled item), and this check:

#tabbrowser-tabs:not([overflow="true"]) + #new-tab-button,

hides the overflow / fallback version of the new tab button when it is directly after the tabstrip (which is the case because the disabled or non-existant item doesn't actually have a corresponding DOM node).

I see two relatively straightforward solutions:
1) stop using the currentset attribute, and use a small JS object that listens to CUI events to add an attribute that show/hides the non-overflow new tab button for this case. IMO this is preferable because the currentset attribute should eventually go away.
2) use something like this instead:

#TabsToolbar[currentset]:not([currentset*="new-tab-button"]) > #tabbrowser-tabs > .tabbrowser-arrowscrollbox > .tabs-newtab-button,

which would only hide the non-overflow new tab button whenever the permanent/customizable/overflow new tab button is outside the tabstrip -- this would have the downside of causing the button to show up twice if the user moved the button elsewhere in the tabstrip...

Dão, thoughts/ideas/preferences on how to proceed here?

Comment 3

[B.J. Herbison]

While testing to report bug 1402400 I customized several times, moving New Tab various places and putting other widgets on each side of it.

At some point Nightly gave up and won't show the new tab button when it is the standard position (the first widget to the right of the tabs). It will show up (on the far right) if there is a button to the left of it, and will show up if placed to the left of the tabs.

No disabled tool needed.

Comment 4

[:Gijs (he/him)]

(In reply to B.J. Herbison from comment #3)
> While testing to report bug 1402400 I customized several times, moving New
> Tab various places and putting other widgets on each side of it.
> 
> At some point Nightly gave up and won't show the new tab button when it is
> the standard position (the first widget to the right of the tabs). It will
> show up (on the far right) if there is a button to the left of it, and will
> show up if placed to the left of the tabs.
> 
> No disabled tool needed.

If you run:

CustomizableUI.getWidgetIdsInArea("TabsToolbar")

in the browser console ( https://developer.mozilla.org/en-US/docs/Tools/Browser_Console ), what does that return, when the tabstrip is in this state?

Comment 6

[B.J. Herbison]

The value for TabsToolbar is:
Array [ "tabbrowser-tabs", "containers-panelmenu", "new-tab-button", "alltabs-button" ]

I hadn't noticed I no longer have the all tabs drop-down.

Testing notes:
* In the configuration shown above, neither "new tab" or "all tabs" show up.
* If I drag "all tabs" left of "new tab" then "new tab" shows up (on the right side) but "all tabs" is hidden.
* If I drag "all tabs" left of the tabs then the "all tabs" drop-down shows up.
* With "new tab" to the left of tabs and "all tabs" on the right the "all tabs" drop-down doesn't show up.

Comment 7

[:Gijs (he/him)]

(In reply to B.J. Herbison from comment #6)
> The value for TabsToolbar is:
> Array [ "tabbrowser-tabs", "containers-panelmenu", "new-tab-button",
> "alltabs-button" ]

This shows that you had/have a "containers" add-on and/or feature enabled and now it is either not installed or disabled, which is exactly the issue this bug is about.

The all-tabs hiding/showing behaviour has to do with whether it's in a custom position and, if it isn't, with whether the tabstrip overflows. It's the same CSS rules mentioned in comment #2. TBH, I'm not aware of us ever having shown the 'all tabs' menu when it's next to the tabstrip while the tabstrip isn't overflown (ie scrollable), so I don't think that that is a bug.

Comment 8

[B.J. Herbison]

Thank you, you are correct about the all tabs item.

State:
* TabsToolbar is Array [ "tabbrowser-tabs", "containers-panelmenu", "new-tab-button", "alltabs-button" ]
* Tab bar not overflowed.
* No buttons appear to the right of tabs.
Action:
* Add tabs until the tab bar overflows.
Effect:
* Both "new tab" button and "all tabs" drop-down appear.

As far as "containers-panelmenu", I suspect that it the Firefox container drawer widget removed in bug 1400812. (So, something internal removed, but not an add-on or a feature I disabled.)

It's a shame Customize doesn't toss out entries with no implementation. Should that be a new bug report?

Comment 9

[:Gijs (he/him)]

(In reply to B.J. Herbison from comment #8)
> It's a shame Customize doesn't toss out entries with no implementation.
> Should that be a new bug report?

No, there's no way for Customize to do that because it doesn't know if the add-on/item is just temporarily disabled, or permanently gone, etc. We don't want to lose the position of items whenever people e.g. use safe mode.

In this particular case, bug 1400812 should have cleaned up and removed the item. I'll file a bug for that.

In general, we should still function correctly even when there's "leftover junk" in this list. This bug is one of the few cases where we don't.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Gijs from comment #2)
> 1) stop using the currentset attribute, and use a small JS object that
> listens to CUI events to add an attribute that show/hides the non-overflow
> new tab button for this case. IMO this is preferable because the currentset
> attribute should eventually go away.

I feel most comfortable with #1. CSS selectors don't offer enough functionality to work with the issue presented in this bug. JS will allow us the freedom to make this work and it should be relatively straightforward.

Comment 12

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
Requesting tracking for 57 because tabmixplus being disabled (no more legacy add-ons) is triggering this for a non-trivial number of (power) users, and it's non-obvious how to recover / fix. (x-ref bug 1405200)

I chatted about this with Jared, and Jared or I will probably pick this up in the next few days depending on how many other things require our attention.

Comment 13

[Karl Tomlinson (:karlt)]

(In reply to :Gijs from comment #12)
> [Tracking Requested - why for this release]:
> Requesting tracking for 57 because tabmixplus being disabled (no more legacy
> add-ons) is triggering this for a non-trivial number of (power) users,

It turns out that disabling tabmixplus triggers this bug only if the last run
of 56 did not shut down cleanly.  That means fewer users will trigger this by
this mechanism.  There is still risk of other triggers, however, I assume.

Comment 14

[Julien Cristau [:jcristau]]

tracking per comment 12.

Comment 15

[:Gijs (he/him)]

Attachment: Bug 1370791 - use CUI listener and an attribute to toggle hiding/showing the new tab button,

Review commit: https://reviewboard.mozilla.org/r/186470/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/186470/

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8915241 [details]
Bug 1370791 - use CUI listener and an attribute to toggle hiding/showing the new tab button,

https://reviewboard.mozilla.org/r/186470/#review191564

::: browser/base/content/tabbrowser.xml:5836
(Diff revision 1)
> +        <body><![CDATA[
> +          let sib = this.tabContainer.nextElementSibling;
> +          while (sib && sib.hidden) {
> +            sib = sib.nextElementSibling;
> +          }
> +          let newTabAdjacent = sib.id == "new-tab-button";

Need to bail out of here before dereferencing `sib` if it is null.

::: browser/base/content/tabbrowser.xml:5838
(Diff revision 1)
> +          while (sib && sib.hidden) {
> +            sib = sib.nextElementSibling;
> +          }
> +          let newTabAdjacent = sib.id == "new-tab-button";
> +          const kAttr = "hasadjacentnewtabbutton";
> +          let attributePresent = this.tabContainer.hasAttribute(kAttr);

Is there really a performance benefit from only calling removeAttribute if the attribute is already present? Otherwise I think we can remove this and make the conditionals below easier to read.

::: browser/base/content/tabbrowser.xml:5870
(Diff revision 1)
> +          }
> +        ]]></body>
> +      </method>
> +      <method name="onAreaReset">
> +        <body><![CDATA[
> +          this.onAreaNodeRegistered(...arguments);

Personal style here, but I think it would be better to be literal about the two parameters we expect here and then pass them explicitly. This would protect against a future change to the API and someone not updating this line because they just did a search of the codebase for specific variable names.

Feel free to disregard.

Comment 17

[:Gijs (he/him)]

Comment on attachment 8915241 [details]
Bug 1370791 - use CUI listener and an attribute to toggle hiding/showing the new tab button,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186470/diff/1-2/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3894d341fe16
use CUI listener and an attribute to toggle hiding/showing the new tab button, r=jaws

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3894d341fe16

Comment 20

[B.J. Herbison]

This change made my New Tab + reappear, and I was unable to make it vanish again.

I tested on Firefox Nightly 57 in the context of comment 3 through comment 8. I didn't try to disable an extension and, in particular, I didn't try the non-clean shutdown described in comment 13.

Comment 21

[:Gijs (he/him)]

Marking this as verified per comment #20 . Going to request beta uplift in a bit.

Comment 22

[:Gijs (he/him)]

Comment on attachment 8915241 [details]
Bug 1370791 - use CUI listener and an attribute to toggle hiding/showing the new tab button,

Approval Request Comment
[Feature/Bug causing the regression]: old issue, but more serious for 57 because we're disabling legacy add-ons
[User impact if declined]: new tab button goes AWOL and it's not easy to figure out how to get it back
[Is this code covered by automated tests?]: sadly no
[Has the fix been verified in Nightly?]: yep, by one of the affected users, see comment 20.
[Needs manual test from QE? If yes, steps to reproduce]: probably a good idea. The steps in comment #0 should be sufficient, or they can:
1. install tabmixplus in a clean profile in 56
2. close the browser
3. run the same profile in 57/58 with/without patch
and verify that the new tab button shows up.
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: not very
[Why is the change risky/not risky?]: well, we're changing some long-standing CSS selectors which is never entirely free of risk, but the patch is relatively small and we have sufficient beta runway left that I'm not too worried about it. The actual implementation here is also relatively straightforward and easy to understand and reason about, so the number of avenues for side effects or other negative consequences is very limited.
[String changes made/needed]: nope

Comment 24

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8915241 [details]
Bug 1370791 - use CUI listener and an attribute to toggle hiding/showing the new tab button,

Seems like a must fix, Beta57+

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0bb44cd291e1

Comment 26

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

I have reproduced this issue using Firefox  55.0a1 (2017.06.06) on Win 8.1 x64.
I can confirm this issue is fixed, I verified using Firefox 57.0b6 on Win 8.1 x64, Mac OS X 10.11 and Ubuntu 14.04 x64.