975719 - Custom toolbars added pre-Australis can't be modified or permanently removed after upgrading to Australis

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Rika Pi]

I have Firefox 30.0a1 on a windows 8.1 64 bit computer

I had a previous profile for Firefox then I opened Nightly
I had added a few toolbars in the previous profile and they appeared
in Nightly (even with australis) but they are un-modifiable, so I can't add addon icons
to them for example.

Steps to reproduce:

Have a previous profile of Firefox that does not have australis (27 or 28) 
and add a toolbar to it 

open the same profile in a Firefox Nightly

go to the customize page

take an icon and try to place it in the old toolbar

Expected result:
the toolbar is present and modifiable, if not then a means to removing the toolbar

Actual result:
toolbar is present but icons can not be added to it.
You also can't remove the toolbar.

Comment 1

[Drew Willcoxon :adw]

Confirming.  You can uncheck the custom toolbars in the toolbars menu and they disappear, but they reappear on restart.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Yeah, that sounds pretty broken. Marking P2 unless there's any objection.

Comment 3

[:Gijs (he/him)]

Investigating.

Comment 4

[:Gijs (he/him)]

Attachment: Australis - right clicking an empty bit of toolbar shouldn't cause errors,

I ran into this. Simple fix + test.

Comment 5

[:Gijs (he/him)]

Attachment: unbreak custom toolbars in Australis

So this unbreaks the toolbar in the sense that you can interact with it and it 'works'. Collapsing state doesn't persist on Holly either, and so I presume that's been broken for a while now. However, now it's slightly worse because you also can't remove the extra bar, nor can you add it back if you do remove it by accident. I suspect 'restore defaults' would also not remove it. The latter could be fixed, but that makes the former worse.

Adding UI for adding the toolbars is something we've explored in the past and didn't want to do, and would likely involve strings (although we could use the toolkit ones, maybe?).

The other option is just removing the extra custom toolbars in a UI migration step, so that when people open Firefox-with-Australis the first time, we remove any custom toolbars they may have configured. I guess it's up to UX what we would prefer to do here.

Comment 6

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

Comment on attachment 8380900 [details] [diff] [review]
Australis - right clicking an empty bit of toolbar shouldn't cause errors,

Review of attachment 8380900 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +4242,5 @@
>    let toolbarItem = popup.triggerNode;
>  
>    if (toolbarItem && toolbarItem.localName == "toolbarpaletteitem") {
>      toolbarItem = toolbarItem.firstChild;
> +  } else if (toolbarItem.localName != "toolbar") {

You need to check if toolbarItem is not null before dereferencing it.

Comment 7

[:Gijs (he/him)]

Comment on attachment 8380910 [details] [diff] [review]
unbreak custom toolbars in Australis

(In reply to :Gijs Kruitbosch from comment #5)
> Created attachment 8380910 [details] [diff] [review]
> 
> So this unbreaks the toolbar in the sense that you can interact with it and
> it 'works'. Collapsing state (that is, right clicking the toolbars
> and toggling the toolbar) doesn't persist on Holly either, and so I
> presume that's been broken for a while now. However, now it's slightly worse
> because you also can't remove the extra bar, nor can you add it back if you
> do remove it by accident. I suspect 'restore defaults' would also not remove
> it. The latter could be fixed, but that makes the former worse.
> 
> Adding UI for adding the toolbars is something we've explored in the past
> and didn't want to do, and would likely involve strings (although we could
> use the toolkit ones, maybe?).
> 
> The other option is just removing the extra custom toolbars in a UI
> migration step, so that when people open Firefox-with-Australis the first
> time, we remove any custom toolbars they may have configured. That would cause
> dataloss in a certain sense, so it's also not great. I guess it's
> up to UX what we would prefer to do here.


Madhava, can you clarify what we should do here?

Comment 8

[:Gijs (he/him)]

Comment on attachment 8380900 [details] [diff] [review]
Australis - right clicking an empty bit of toolbar shouldn't cause errors,


remote:   https://hg.mozilla.org/integration/fx-team/rev/6a8b0834c5fc

Comment 9

[Rika Pi]

Confirmed in Firefox 29.0 Aurora

Comment 10

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

Comment on attachment 8380910 [details] [diff] [review]
unbreak custom toolbars in Australis

(Clearing the review request that I added.) Please request review after ui-r has been granted.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6a8b0834c5fc

Comment 12

[:Gijs (he/him)]

Comment on attachment 8380910 [details] [diff] [review]
unbreak custom toolbars in Australis

Per discussion on IRC, we want to pursue trying to make this not break as badly as it does right now. These two hunks are pretty straightforward. I need to check if, once you remove all the items from the toolbar in customize mode, the existing toolbox code takes care of removing the extra toolbars, or if we'd need to write more code for that.

We should file a separate bug for the collapsed state not working; that's a problem on holly too so presumably has existed for a while, without anyone noticiing because really, who'd add a custom toolbar themselves and then collapse it and want it to still be collapsed after a restart? :-)

Comment 13

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

(In reply to :Gijs Kruitbosch from comment #12)
> Comment on attachment 8380910 [details] [diff] [review]
> unbreak custom toolbars in Australis
> 
> Per discussion on IRC, we want to pursue trying to make this not break as
> badly as it does right now. These two hunks are pretty straightforward. I
> need to check if, once you remove all the items from the toolbar in
> customize mode, the existing toolbox code takes care of removing the extra
> toolbars, or if we'd need to write more code for that.

I don't remember it doing that pre-Australis.
 
> We should file a separate bug for the collapsed state not working; that's a
> problem on holly too so presumably has existed for a while, without anyone
> noticiing because really, who'd add a custom toolbar themselves and then
> collapse it and want it to still be collapsed after a restart? :-)

Yeah, a follow-up bug is fine.

Comment 14

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

Comment on attachment 8380910 [details] [diff] [review]
unbreak custom toolbars in Australis

Review of attachment 8380910 [details] [diff] [review]:
-----------------------------------------------------------------

Should we add a test for this?

Comment 15

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #14)
> Comment on attachment 8380910 [details] [diff] [review]
> unbreak custom toolbars in Australis
> 
> Review of attachment 8380910 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Should we add a test for this?

Per discussion on IRC, I'll look into these issues some more and attempt to write a test for most of what this bug covers, seeing as there don't seem to be any toolkit tests, either, adding coverage here seems wise if it's not too herculean an effort.

Comment 16

[:Gijs (he/him)]

Attachment: unbreak custom toolbars in Australis,

So this takes care of everything except undoReset... the currentset fix was necessary because if toolbar.currentSet == '', then the split still generates a 1-item array, and the initial creation code deals with not having a defaultset (although I've added an or there for safety).

Comment 17

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

Comment on attachment 8383823 [details] [diff] [review]
unbreak custom toolbars in Australis,

Review of attachment 8383823 [details] [diff] [review]:
-----------------------------------------------------------------

r- just due to the issues in the test.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +2199,5 @@
>      });
>    },
>  
> +  removeExtraToolbar: function(aToolbar) {
> +    this._resetExtraToolbars(aToolbar);

s/aToolbar/aToolbarId/

@@ +2976,5 @@
> +   * Remove a custom toolbar added in a previous version of Firefox or using
> +   * an add-on. NB: only works on the customizable toolbars generated by
> +   * the toolbox itself. Intended for use from CustomizeMode, not by
> +   * other consumers.
> +   * @param aToolbarId the toolbar to remove

the ID of the toolbar to remove

::: browser/components/customizableui/test/browser_975719_customtoolbars_behaviour.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +add_task(function customizeToolbarAndAutoRemoveIt() {

Should you add a test that shows that exiting and entering customize mode *doesn't* remove the custom toolbar if it is non-empty?

@@ +7,5 @@
> +add_task(function customizeToolbarAndAutoRemoveIt() {
> +  ok(gNavToolbox.toolbarset, "There should be a toolbarset");
> +  let toolbarID = "testAustralisCustomToolbar";
> +  gNavToolbox.appendCustomToolbar(toolbarID, "");
> +  let toolbarDOMID = "__customToolbar_" + toolbarID.replace(" ", "_");

toolbarID ("testAustralisCustomToolbar") doesn't have spaces, why do you need to replace them with underscores here? Here and below as well.

@@ +19,5 @@
> +     "Toolbar should have been inserted in toolbox, before toolbarset element");
> +  let cuiAreaType = CustomizableUI.getAreaType(toolbarDOMID);
> +  is(cuiAreaType, CustomizableUI.TYPE_TOOLBAR,
> +     "CustomizableUI should know the area and think it's a toolbar");
> +  if (cuiAreaType != CustomizableUI.getAreaType(toolbarDOMID)) {

How is this ever going to be false? I think you meant to compare cuiAreaType to CustomizableUI.TYPE_TOOLBAR. Here and below as well.

@@ +42,5 @@
> +  ok(!CustomizableUI.inDefaultState, "Shouldn't be in default state because there's a non-collapsed toolbar again.");
> +  yield endCustomizing();
> +  ok(CustomizableUI.inDefaultState, "Should be in default state because the toolbar should have been removed.");
> +  ok(!toolbarElement.parentNode, "Toolbar should no longer be in the DOM.");
> +  let cuiAreaType = CustomizableUI.getAreaType(toolbarDOMID);

This is a redeclaration of cuiAreaType, which was declared at line 20 of this file.

Comment 18

[:Gijs (he/him)]

Attachment: unbreak custom toolbars in Australis,

Noticed another test left a button behind, so fixed that on the way. Otherwise, this should cover all the things you pointed out.

Comment 19

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

Comment on attachment 8384816 [details] [diff] [review]
unbreak custom toolbars in Australis,

Review of attachment 8384816 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 20

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/49f90c71153f

Comment 21

[:Gijs (he/him)]

Filed two followup bugs for 'undo reset' and for the collapsed state of these toolbars.

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/49f90c71153f

Comment 23

[:Gijs (he/him)]

Comment on attachment 8380900 [details] [diff] [review]
Australis - right clicking an empty bit of toolbar shouldn't cause errors,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: exceptions thrown when right-clicking an empty bit of toolbar
Testing completed (on m-c, etc.):  on m-c for quite a while, locally, has automated test
Risk to taking this patch (and alternatives if risky): very low
String or IDL/UUID changes made by this patch: none

Comment 24

[:Gijs (he/him)]

Comment on attachment 8384816 [details] [diff] [review]
unbreak custom toolbars in Australis,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: additional user-created custom toolbars from previous versions can't be removed or customized
Testing completed (on m-c, etc.): locally, on m-c, has automated test
Risk to taking this patch (and alternatives if risky): medium. This isn't super-safe, but the automated tests should lower any risk substantially.
String or IDL/UUID changes made by this patch: none, although there's a JSM change that should be documented.

Comment 25

[:Gijs (he/him)]

Updated CustomizableUI.jsm docs on MDN.

Comment 26

[:Gijs (he/him)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c9fe13225c56
https://hg.mozilla.org/releases/mozilla-aurora/rev/06a9bd39402a

Comment 27

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

Verified on Windows 7 64bit, Ubuntu 12.04,Mac OS X 10.9 and also on Microsoft Surface Pro 2 running Windows 8.1 64 bit using Firefox 29 beta 1 (build ID: 20140318013849) and latest Firefox Aurora (build ID:).

Marking this verified as bug 978975 and bug 978979 were logged for the remaining issues.