Closed Bug 975719 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect)

30 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 30
Tracking Status
firefox29 + verified
firefox30 + verified

People

(Reporter: walker.pilot.b, Assigned: Gijs)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [Australis:P2])

Attachments

(2 files, 2 obsolete files)

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.
Confirming.  You can uncheck the custom toolbars in the toolbars menu and they disappear, but they reappear on restart.
Status: UNCONFIRMED → NEW
Component: Untriaged → Toolbars and Customization
Ever confirmed: true
OS: Windows 8.1 → All
Hardware: x86_64 → All
Summary: Toolbars appear in Nightly but can not add icons to it → Custom toolbars added pre-Australis can't be modified or permanently removed after upgrading to Australis
Yeah, that sounds pretty broken. Marking P2 unless there's any objection.
Whiteboard: [Australis:P2]
Investigating.
Assignee: nobody → gijskruitbosch+bugs
I ran into this. Simple fix + test.
Attachment #8380900 - Flags: review?(jaws)
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 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.
Attachment #8380900 - Flags: review?(jaws) → review+
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?
Attachment #8380910 - Flags: ui-review?(madhava)
Attachment #8380910 - Attachment description: unbreak custom toolbars in Australis, ui- → unbreak custom toolbars in Australis
Attachment #8380910 - Flags: review?(jaws)
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
Attachment #8380900 - Flags: checkin+
Keywords: leave-open
Confirmed in Firefox 29.0 Aurora
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.
Attachment #8380910 - Flags: review?(jaws)
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? :-)
Attachment #8380910 - Flags: ui-review?(madhava) → review?(jaws)
(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 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?
Attachment #8380910 - Flags: review?(jaws) → review+
(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.
Flags: needinfo?(gijskruitbosch+bugs)
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).
Attachment #8383823 - Flags: review?(jaws)
Attachment #8380910 - Attachment is obsolete: true
Flags: needinfo?(gijskruitbosch+bugs)
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.
Attachment #8383823 - Flags: review?(jaws) → review-
Noticed another test left a button behind, so fixed that on the way. Otherwise, this should cover all the things you pointed out.
Attachment #8384816 - Flags: review?(jaws)
Attachment #8383823 - Attachment is obsolete: true
Comment on attachment 8384816 [details] [diff] [review]
unbreak custom toolbars in Australis,

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

Thanks!
Attachment #8384816 - Flags: review?(jaws) → review+
remote:   https://hg.mozilla.org/integration/fx-team/rev/49f90c71153f
Status: NEW → ASSIGNED
Keywords: leave-open
Whiteboard: [Australis:P2] → [Australis:P2][fixed-in-fx-team]
Depends on: 978975
Depends on: 978979
Filed two followup bugs for 'undo reset' and for the collapsed state of these toolbars.
https://hg.mozilla.org/mozilla-central/rev/49f90c71153f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P2][fixed-in-fx-team] → [Australis:P2]
Target Milestone: --- → Firefox 30
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
Attachment #8380900 - Flags: approval-mozilla-aurora?
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.
Attachment #8384816 - Flags: approval-mozilla-aurora?
Updated CustomizableUI.jsm docs on MDN.
Attachment #8384816 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8380900 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
QA Contact: cornel.ionce
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.