972822 - Tab groups button presence not persisted correctly when added in Australis

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Mike Hommey [:glandium]]

STR:
- Open a fresh Firefox
- Observe that there is no way to get Tab groups: no menu on the rightmost side of the tab bar, no item in the menu that i could find.
- Open enough tabs so that the tab bar shows scrollers.
- Now there is a menu on the rightmost side the tab bar, and the first item in that menu is "Tab groups". Close tabs and it disappears.
Now, this is where users can easily become lost:
- Click on Tab groups in that menu. This opens the panorama UI.
- Create a new tab group and click on it.
- The UI switches the new tab group, with no tabs in it.
- There is now no way to get back to the other tab group... besides adding the tab group button to the toolbar or creating plenty of tabs.

Before Australis, whenever you start using tab groups, the tab group button would be kept in the UI in the tabbar. But that's gone in Australis, so there is no obvious way for users to recover their tabs.

I think that menu item should just be removed, removing any confusion. People who do want tab groups can add the button to their toolbar, at which point it will be available in all situations, not only when there are many tabs.

Comment 1

[:Gijs (he/him)]

The 'all tabs' menu has always functioned like this. I don't know if the tab groups button no longer being auto-added is intentional or not. Tim/Matt?

Comment 2

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #1)
> I don't know if the tab groups button no longer being auto-added is intentional or not.

The code is still there: http://hg.mozilla.org/mozilla-central/annotate/eac89fb04bb9/browser/base/content/browser-tabview.js#l127

Comment 3

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #2)
> (In reply to :Gijs Kruitbosch from comment #1)
> > I don't know if the tab groups button no longer being auto-added is intentional or not.
> 
> The code is still there:
> http://hg.mozilla.org/mozilla-central/annotate/eac89fb04bb9/browser/base/
> content/browser-tabview.js#l127

This is odd, though, because the getElementById() call will always have failed if the button was in the palette. It's in the palette on holly as well. I wonder how it's working there. :-\

Comment 4

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #3)
> (In reply to Dão Gottwald [:dao] from comment #2)
> > (In reply to :Gijs Kruitbosch from comment #1)
> > > I don't know if the tab groups button no longer being auto-added is intentional or not.
> > 
> > The code is still there:
> > http://hg.mozilla.org/mozilla-central/annotate/eac89fb04bb9/browser/base/
> > content/browser-tabview.js#l127
> 
> This is odd, though, because the getElementById() call will always have
> failed if the button was in the palette. It's in the palette on holly as
> well. I wonder how it's working there. :-\

Except I can't read, there isn't a negation there.

Comment 5

[:Gijs (he/him)]

Actually, attempting to reproduce this on a clean profile, I do see the button being added, also on current nightly. Mike, does this not happen for you on a clean profile?

Note that if you have a profile where you once had the button added and it is then removed (either directly by you or by using 'restore defaults'), it won't be re-added again (this is controlled by a panorama pref). This is the same behaviour as on non-Australis.

Comment 6

[Mike Hommey [:glandium]]

So interestingly, it works like pre-Australis with a fresh profile. The weird thing is that the profile I was using is rather new and without much customization besides switching between versions.

Comment 7

[:Gijs (he/him)]

(In reply to Mike Hommey [:glandium] from comment #6)
> So interestingly, it works like pre-Australis with a fresh profile. The
> weird thing is that the profile I was using is rather new and without much
> customization besides switching between versions.

If the pref browser.panorama.experienced_first_run is set to true, it will have been added at some point (and that'd be why it's not being readded at this point).

I'm going to mark this WFM, but if you can reproduce the button disappearing with something like:

1. Use beta, get the button added.
2. Use aurora on that profile, button disappears

or something similar, then please reopen with STR. :-)

Comment 8

[Mike Hommey [:glandium]]

So here is one way to reproduce, but I don't know if there are others:
- Open nightly with fresh profile
- Open tabs until you get the list more tabs menu
- Open tab groups from there
- Create new tab group, switch to it
- Close nightly
- Start release.

I don't know how much that relates to how i actually got that in the first place, but one thing I know for sure is that another very old profile that i have does have browser.panorama.experienced_first_run set to true but the button is not there, and that profile has only upgraded to australis once, and never went back.

Comment 9

[ithinc]

(In reply to :Gijs Kruitbosch from comment #3)
> This is odd, though, because the getElementById() call will always have
> failed if the button was in the palette. It's in the palette on holly as
> well. I wonder how it's working there. :-\

True, but getElementById() won't fail when a button is in the Panel. That's the cause.

Comment 10

[:Gijs (he/him)]

(In reply to ithinc from comment #9)
> (In reply to :Gijs Kruitbosch from comment #3)
> > This is odd, though, because the getElementById() call will always have
> > failed if the button was in the palette. It's in the palette on holly as
> > well. I wonder how it's working there. :-\
> 
> True, but getElementById() won't fail when a button is in the Panel. That's
> the cause.

This doesn't make sense, because the button isn't in the panel.

(In reply to Mike Hommey [:glandium] from comment #8)
> So here is one way to reproduce, but I don't know if there are others:
> - Open nightly with fresh profile
> - Open tabs until you get the list more tabs menu
> - Open tab groups from there
> - Create new tab group, switch to it
> - Close nightly
> - Start release.

If you then reuse nightly, is the button back in the tabs bar? That's what I'd expect. Also, alternatively, if you open customize mode before closing, change something else, and exit customize mode again, and then close nightly and open release, I'd expect the button to be there on release. Are my suspicions correct?

Comment 11

[Mike Hommey [:glandium]]

(In reply to :Gijs Kruitbosch from comment #10)
> (In reply to ithinc from comment #9)
> > (In reply to :Gijs Kruitbosch from comment #3)
> > > This is odd, though, because the getElementById() call will always have
> > > failed if the button was in the palette. It's in the palette on holly as
> > > well. I wonder how it's working there. :-\
> > 
> > True, but getElementById() won't fail when a button is in the Panel. That's
> > the cause.
> 
> This doesn't make sense, because the button isn't in the panel.
> 
> (In reply to Mike Hommey [:glandium] from comment #8)
> > So here is one way to reproduce, but I don't know if there are others:
> > - Open nightly with fresh profile
> > - Open tabs until you get the list more tabs menu
> > - Open tab groups from there
> > - Create new tab group, switch to it
> > - Close nightly
> > - Start release.
> 
> If you then reuse nightly, is the button back in the tabs bar?

No

> That's what I'd expect. Also, alternatively, if you open customize mode before closing,
> change something else, and exit customize mode again, and then close nightly
> and open release, I'd expect the button to be there on release.

Yes, it's there.

> Are my suspicions correct?

Only half.

Comment 12

[:Gijs (he/him)]

So at a minimum we need to sync currentsets. I think we don't do that except when changing something in customize mode. I'm not sure if that was a conscious decision, seems to me now that it's just wrong, and CustomizableUI should do it whenever stuff changes on a toolbar in any way.

For this specific case, we can/(should?) also fix the tabgroups button to use CUI instead of what it does now.

I don't understand why I'm only half-correct, unless the release version used already had the "fix Australis changes" UI migration. Which it seems like it does:

http://hg.mozilla.org/releases/mozilla-release/file/4ab889190444/browser/components/nsBrowserGlue.js#l1292

in which case, I would expect:

> - Open nightly with fresh profile
> - Open tabs until you get the list more tabs menu
> - Open tab groups from there
> - Create new tab group, switch to it
> - Close nightly


> - reopen nightly

(without opening release inbetween)

to work. I'll try to investigate more the coming week.

Comment 13

[:Gijs (he/him)]

Attachment: Australis - persist the currentset attribute correctly when appending tabgroups button,

Adjusted the doc.getElementById check, got rid of the useless verification that the alltabsbutton is where it is (it's non-removable, so it'll always be in the tabstoolbar) and converted this to just use CUI throughout. The bit that should fix the issue from comment #11 is the document.persist() call after inserting the node. I changed my mind wrt calling that all the time whenever something changes because existing code already had to do this - I just erroneously removed the call in this bit of code back in bug 874371.

Comment 14

[Dão Gottwald [:dao]]

Why is it necessary to call document.persist when using the CustomizableUI API? Shouldn't this "just work"? In fact, I'd have expected this from toolbar.insertItem as well.

Comment 15

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #14)
> Why is it necessary to call document.persist when using the CustomizableUI
> API? Shouldn't this "just work"? In fact, I'd have expected this from
> toolbar.insertItem as well.

As I noted in comment #14, this has never "just worked" as far as currentset is concerned, and I don't think we should change that now, mostly because code relying on toolbar.insertItem or toolbar.currentSet = x, has always had to do this itself, and calling document.persist all the time isn't free. Here's the toolkit implementation (which hasn't changed) and doesn't deal with currentset at all:

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/toolbar.xml#353

Most add-ons using this would then manually run code to do a currentset update.

The original code here (before bug 874371 changed it) used the currentSet setter, then assigned that value to the currentset attribute, then document.persisted it. See the original patch where I removed the code: https://bug874371.bugzilla.mozilla.org/attachment.cgi?id=752168


It does actually "just work" when looking purely at the new CustomizableUI stuff, which stores its placements in a pref. That's updated immediately (though we don't flush the prefs, IIRC). Likewise, the CustomizableUI bits take care of updating the currentset attribute immediately. Persisting the attributes is only done from customize mode.

However, the steps in comment #8 include opening release, which still relies on currentset, and if that isn't persisted, it gets lost. The Australis UI migration code then  updates currentset to deal with the unified URL bar, and then clears the pref to ensure it doesn't do that more than once. Because the currentset didn't get persisted, the button is gone in release, and because the pref got cleared, it's then gone on aurora/nightly as well.

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 8379094 [details] [diff] [review]
Australis - persist the currentset attribute correctly when appending tabgroups button,

>-    if (document.getElementById(buttonId))
>+    if (CustomizableUI.getPlacementOfWidget(buttonId))

Why is this better?

>+    document.persist("TabsToolbar", "currentset");

Can you file a bug on removing this in a future version and put the bug number in a code comment?

Comment 17

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #16)
> Comment on attachment 8379094 [details] [diff] [review]
> Australis - persist the currentset attribute correctly when appending
> tabgroups button,
> 
> >-    if (document.getElementById(buttonId))
> >+    if (CustomizableUI.getPlacementOfWidget(buttonId))
> 
> Why is this better?

Because document.getElementById() would also return null if the button is in the panel, but the panel hasn't been opened yet in that window, meaning we wouldn't return early and the button would be moved from the panel to the toolbar, which isn't what we want.

> >+    document.persist("TabsToolbar", "currentset");
> 
> Can you file a bug on removing this in a future version and put the bug
> number in a code comment?

Filed bug 976041, and mentioned in:

https://hg.mozilla.org/integration/fx-team/rev/cf82ac9f8141

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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

Comment 19

[:Gijs (he/him)]

Comment on attachment 8379094 [details] [diff] [review]
Australis - persist the currentset attribute correctly when appending tabgroups button,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis / bug 874371
User impact if declined: tab groups button might go missing if switching between australis/non-australis builds. Tab groups button might also automatically be moved from panel to toolbar when it shouldn't be.
Testing completed (on m-c, etc.): on m-c, has small existing automated test that'd have caught outright breakage
Risk to taking this patch (and alternatives if risky): very low
String or IDL/UUID changes made by this patch: none

Comment 20

[Mihaela Velimiroviciu (:mihaelav)]

Setting in-testsuite:- because this would be rather hard to cover by automation given that it needs switching between different FF versions (Australis/non-Autralis). If anyone thinks otherwise, please change it.

Comment 21

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/ee7708c2e0e5

Comment 22

[Cornel Ionce [:noni] [Hubs QA]]

Verified as fixed on latest Firefox builds.
Aurora - build ID: 20140314004001
Nightly - build ID: 20140314030202