989338 - Custom area placements are lost if the user customizes while the area is not registered

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Luís Miguel [:quicksaver] (not reading bugmail)]

These are very specific steps to reproduce (which I'm sure will give it a low priority :) ), so please bare with me.

1. Open clean profile in latest nightly
2. Enter in the browser console:
> var testBar = document.createElement('toolbar');
> testBar.id = 'testBar';
> testBar.setAttribute('toolbarname', 'Test Bar');
> testBar.hidden = false;
> testBar.collapsed = false;
> testBar.setAttribute('class', 'toolbar-primary chromeclass-toolbar');
> testBar.setAttribute('context', 'toolbar-context-menu');
> testBar.setAttribute('toolboxid', 'navigator-toolbox');
> testBar.setAttribute('customizable', 'true');
> CustomizableUI.registerArea('testBar', 
>   { type: CustomizableUI.TYPE_TOOLBAR, legacy: true }
> );
> gNavToolbox.appendChild(testBar);
This will create a test toolbar at the top of the browser which can be customized.

3. Enter this bit in the browser console:
> document.getElementById('status-bar').setAttribute('removable', 'true');
> CustomizableUI.removeWidgetFromArea('status-bar');
Obviously, by itself this accomplishes next to nothing, but I use this bit in my code to remove the statusbar element from the add-on bar, so I can append it somewhere else without the CustomizableUI system going nuts every time I open and close new browser windows and try to customize them (it works sometimes without this bit of code but not always, so I figured it was because the node wasn't where CUI expected it to be, and I thought it was safer to just remove it in CUI as well; I will still try to play around with not using this bit of code for my specific case but that's not the point of this bug).

4. Enter customize mode, add a couple of widgets to the new toolbar, and go ahead and add a couple more to nav-bar to compare later. Close customize mode.
5. Restart firefox with add-ons disabled (safe mode). It should (or seems to) restart successfully.
6. Close firefox and open it again (normal mode).
7. Re-enter the first bit of code to re-add the toolbar.

Result: the customization state was lost for this custom toolbar! The widgets added before are now back in the palette if you open customize mode. The customization state for the nav-bar is not affected, as the widgets added to it remain there.

Something somewhere doesn't like that the status-bar was removed from the add-on bar; it's not a matter of having it physically present in the document, as it always is in my add-on; nor a matter of having it physically as a child of the add-on bar either, as I've also tried appendChild()'ing it back in there before restarting in safe mode.

I have to add back the status-bar before I restart in safe mode, physically with appendChild'ing the node to the add-on bar, then through CUI.addWidgetToArea, to keep the customization state of the toolbar; this is also an easy fix to do on the add-on's side, provided CUI calls still work after quit-application has been dispatched (I haven't tested this yet), but shouldn't be necessary IMO and could probably be a bigger of something else that might be failing after safe mode. The same steps above without the second bit of code work fine, as do normal browser restarts.

Comment 1

[Justin Dolske [:Dolske]]

Why are you moving the status-bar? AFAIK this is a compatibility shim left over from Firefox 4 that we've been meaning to remove.

I don't know the CUI bits well enough to say if this indicates a deeper problem in that code, though... Gijs?

Comment 2

[Luís Miguel [:quicksaver] (not reading bugmail)]

(In reply to Justin Dolske [:Dolske] from comment #1)
> Why are you moving the status-bar? AFAIK this is a compatibility shim left
> over from Firefox 4 that we've been meaning to remove.
My add-on (The Puzzle Piece at https://addons.mozilla.org/en-US/firefox/addon/the-puzzle-piece/) restores the add-on bar, and it has an option to restore the status bar as well, until it is completely removed at least (filed bug 956731 to track this).

A lot (a lot!) of add-ons still use the status-bar, so basically I just want to facilitate the transition for those users that still rely on it for some reason. Granted they'll be less and less over time, but while the element remains physically in the DOM and fully functional, there's no harm in restoring it if the user chooses to.

> I don't know the CUI bits well enough to say if this indicates a deeper
> problem in that code, though... Gijs?
This was why I filed this bug mostly, it doesn't seem to me like this is expected behavior at all, regardless of the shim's ultimate fate.

Comment 3

[Justin Dolske [:Dolske]]

Gijs is going on vacation... Mike?

Comment 4

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

Hey Quicksaver,

I seem to be unable to reproduce this with the steps you've given me using the latest Nightly.

Would it be possible for you to provide to me the add-on (or the shell of an add-on) that adds the toolbar as you describes? That'd make testing and tracking this down a bit easier.

Thanks,

-Mike

Comment 5

[Luís Miguel [:quicksaver] (not reading bugmail)]

That's weird. I just quickly tried again the steps above to make sure and it happened again.

Exact STR (I'm in Windows 7 running today's Nightly btw):
1. Create new profile in latest Nightly. Open browser tools and enable chrome, add-on and remote debugging (otherwise can't use the console of course)
2. Open browser console by Ctrl+Shift+J. Input the first bit to create the toolbar
3. Input the second bit to remove the status-bar
4. Enter customize mode.
4.1 Drag "Open file" and "Email link" to the testBar
4.2 Drag "Character Encoding" to the nav-bar
5. Exit customize mode.
6. Open menu-panel, click in the help button at the bottom, click in "Restart with add-ons closed"
6.1 The browser seems to restart successfully.
7. Click the close button
8. Re-open the browser again
9. Input the first bit again to re-add the testBar
9.1 No widgets are moved to the testBar, but the Character Encoding widget is still in the nav-bar

I'll build an add-on package to reproduce it first thing in the morning.

Comment 6

[Luís Miguel [:quicksaver] (not reading bugmail)]

Attachment: Test CUI placements after safe-mode reboot

Just install the package on a clean profile and follow the instructions in the dialogs.

Ie did notice something. When I installed this package on a new profile, after the safe mode restart the customization state of the nav-bar was also lost, unlike from what happens when I follow the steps in my previous commnt.

From what I can tell, if I actually drag the widget to the nav-bar while in customize, it will keep the customization state for the nav-bar, but if I add it automatically like I'm doing in the package, then that will also be lost after restarting in safe mode.

But after manually dragging the widget once, the customization state will always stay, regardless of how many times I run this test xpi. I have to clear the browser.uiCustomization.state pref for the nav-bar to also lose its state with this test. (the testBar placements are always lost in every test run though)

Comment 7

[:Gijs (he/him)]

First of all, sorry for the long delay here. Here's what I'm seeing:

This has to do with the fact that the testBar is not available in safe mode (because the add-on is disabled), and your add-on can't mark the statusbar removable. Because of these two things, it stays in the add-on bar. That means the saved state doesn't match the real state, so we save the real state to prefs. At that point the testBar placements disappear because the area isn't registered. So when you restart the items are equally not removed from where they were, and the testBar is empty.

I have bad gut feelings about somehow maintaining area data for areas that aren't there explicitly. If you uninstall or disable an add-on that adds an area, I don't think we should have to do extra work to keep those placements for the next time you install the add-on (which might be "we keep this around for eternity / until you reset to defaults / until you move the widgets elsewhere from the palette where they will now be").

However... we do do this for widgets, and we used to do it for other areas because of currentset persistence. I expect that that is why the manual drag and drop stuff makes a difference - customize mode will persist to currentset, which will still be around after you've gone to safe mode and back, and so we'll restore from there. But we're planning on removing the currentset persistence eventually, so we should think about what it means to do this in CUI directly.

So presumably, we need to:
1) make the status-bar removable by default
2) make area state be persisted even if the area is not registered at that time.

Comment 8

[Luís Miguel [:quicksaver] (not reading bugmail)]

(In reply to :Gijs Kruitbosch from comment #7)
> So presumably, we need to:
> 1) make the status-bar removable by default

Maybe this is not worth pursuing, since the status-bar is supposed to be removed eventually. If you start tweaking it or adding extra compatibility code, the same thing will happen like the last time the status bar was also supposed to be removed, where what was initially just a compatibility shim became required good browser behavior with add-ons and still widely used.

Comment 9

[Luís Miguel [:quicksaver] (not reading bugmail)]

s/became required good/became required for good

Comment 10

[:Gijs (he/him)]

(In reply to Luís Miguel [:Quicksaver] from comment #8)
> (In reply to :Gijs Kruitbosch from comment #7)
> > So presumably, we need to:
> > 1) make the status-bar removable by default
> 
> Maybe this is not worth pursuing, since the status-bar is supposed to be
> removed eventually. If you start tweaking it or adding extra compatibility
> code, the same thing will happen like the last time the status bar was also
> supposed to be removed, where what was initially just a compatibility shim
> became required good browser behavior with add-ons and still widely used.

Yeah, this is a fair point. I'm not sure if making it removable is something that will dramatically affect expectations though. Most users and new add-on devs should be unaware it's even there. :-)

Comment 11

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #10)
> (In reply to Luís Miguel [:Quicksaver] from comment #8)
> > (In reply to :Gijs Kruitbosch from comment #7)
> > > So presumably, we need to:
> > > 1) make the status-bar removable by default
> > 
> > Maybe this is not worth pursuing, since the status-bar is supposed to be
> > removed eventually. If you start tweaking it or adding extra compatibility
> > code, the same thing will happen like the last time the status bar was also
> > supposed to be removed, where what was initially just a compatibility shim
> > became required good browser behavior with add-ons and still widely used.
> 
> Yeah, this is a fair point. I'm not sure if making it removable is something
> that will dramatically affect expectations though. Most users and new add-on
> devs should be unaware it's even there. :-)

As in, I would still totally wontfix any bugs filed about the status bar's styling anywhere else, or its behaviour in those cases. It just seems unfortunate to be breaking add-ons that try hard to keep it functional. Adding an attribute is not terribly much work.

Comment 12

[:Gijs (he/him)]

I need to look at the area persistence though, because presumably that's going to be a problem in other cases as well.

Comment 13

[Luís Miguel [:quicksaver] (not reading bugmail)]

(In reply to :Gijs Kruitbosch from comment #10)
> Yeah, this is a fair point. I'm not sure if making it removable is something
> that will dramatically affect expectations though. Most users and new add-on
> devs should be unaware it's even there. :-)

I understand what you mean, and of course, it's just changing a false to a true pretty much. But IMO, doing this is essentially telling add-on developers "you can play with this", where as the current false means more "stop playing with this". (I'm one of those developers that keep it alive, only because I'm too nice of a guy to say no to simple things like these, and most times I think it ends up causing more trouble than it's worth.)

Of course, if this ended up being required to fix all the considered cases where this situation can occur, then I wouldn't oppose it. But like you said, the persistence inner workings would still need to be looked at, and if this could be fixed without the need to change the removable on the status bar, I think it would be preferable. (Maybe I went overboard with the status bar discussion on this bug, sorry about that. :) )

Comment 14

[:Gijs (he/him)]

So this is also a problem if you don't borrow the status bar from somewhere else, if the user customizes something in safe mode, the saved area placements for areas that aren't registered when you save go away (checked with a modified version of the test add-on - thanks, Luís!).

We should fix this, at least. It shouldn't be hard, we just need to re-save the saved placements from last time, for areas that aren't registered, AIUI. Blair, does that make sense to you, too, or am I missing something?

Gavin, this will cause customization data loss for users if they temporarily disable add-ons which create toolbars (either in safe mode or for some other reason) and then re-enable them after having customized anything else at any point. It's also a regression compared to the currentset customization we used to do, because XUL persistence never lost anything. I think it's edge-casey enough that I can live with shipping this in 30, but we should ideally try to fix for 31, at least. Can we add this to the prio backlog for the next iteration?

Comment 15

[:Gijs (he/him)]

(In reply to Luís Miguel [:Quicksaver] from comment #13)
> (In reply to :Gijs Kruitbosch from comment #10)
> > Yeah, this is a fair point. I'm not sure if making it removable is something
> > that will dramatically affect expectations though. Most users and new add-on
> > devs should be unaware it's even there. :-)
> 
> I understand what you mean, and of course, it's just changing a false to a
> true pretty much. But IMO, doing this is essentially telling add-on
> developers "you can play with this", where as the current false means more
> "stop playing with this". (I'm one of those developers that keep it alive,
> only because I'm too nice of a guy to say no to simple things like these,
> and most times I think it ends up causing more trouble than it's worth.)

I think you've successfully convinced me on this front. Let's not make the statusbar removable by default. However, we should still fix the area persistence.

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Ah, yes, I see what we're doing wrong now :( Wow, that's been a big glaring bug since I first wrote the state save/restore code...

Should be able to merge gSavedState.placements and gPlacements into a temporary object inside saveState() when it saves things.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Added this to our Australis list that we'll tackle in an upcoming iteration.

Comment 18

[Luís Miguel [:quicksaver] (not reading bugmail)]

(In reply to :Gijs Kruitbosch from comment #14)
> I think it's
> edge-casey enough that I can live with shipping this in 30, but we should
> ideally try to fix for 31, at least. Can we add this to the prio backlog for
> the next iteration?

Just asking for a status update on this, since 31 has come and almost gone now. Is this still in the list of to-tackle bugs?

Comment 19

[:Gijs (he/him)]

(In reply to Luís Miguel [:Quicksaver] from comment #18)
> (In reply to :Gijs Kruitbosch from comment #14)
> > I think it's
> > edge-casey enough that I can live with shipping this in 30, but we should
> > ideally try to fix for 31, at least. Can we add this to the prio backlog for
> > the next iteration?
> 
> Just asking for a status update on this, since 31 has come and almost gone
> now. Is this still in the list of to-tackle bugs?

Ugh. In fact, it's too late for 32 now. :-|

Gavin, can we get this included for the next iteration so we can uplift for 33, at least? This is really not a hard fix (see comment 14 / comment 16) - it just needs to be written and prioritized so that we can fix the dataloss issue here.

Comment 20

[:Gavin Sharp [email: [e-mail]]]

I added it to our scratchpad.

Comment 21

[:Gijs (he/him)]

Attachment: save non-placement stuff from previously saved placements,

With test.

Comment 22

[Luís Miguel [:quicksaver] (not reading bugmail)]

(In reply to :Gijs Kruitbosch from comment #21)
> Created attachment 8483651 [details] [diff] [review]
> save non-placement stuff from previously saved placements,

Shouldn't LOG() be used instead of Services.console.logStringMessage()? I honestly am not really sure what LOG() does exactly even, it's just something I noticed, Services.console is never used within CUI, so I thought I would ask.

Comment 23

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

Yeah, LOG() should be used. It only prints out to the console when the browser.uiCustomization.debug pref is enabled.

Comment 24

[:Gijs (he/him)]

Attachment: save non-placement stuff from previously saved placements,

I actually didn't intend to leave that in at all, it was a debug leftover. :-)

Comment 25

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #23)
> Yeah, LOG() should be used. It only prints out to the console when the
> browser.uiCustomization.debug pref is enabled.

Well, no, Log.jsm should be used... but that's a different bug ;) (bug 1062623)

Comment 26

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8483715 [details] [diff] [review]
save non-placement stuff from previously saved placements,

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

::: browser/components/customizableui/CustomizableUI.jsm
@@ +1855,5 @@
> +    // Merge in previously saved areas if not present in gPlacements.
> +    // This way, state is still persisted for e.g. temporarily disabled
> +    // add-ons - see bug 989338.
> +    if (gSavedState && gSavedState.placements) {
> +      for (let [area, placements] of Iterator(gSavedState.placements)) {

We shouldn't be using Iterator - it's a non-standard (SpiderMonkey specific) feature that will be removed. Iterate over Object.keys(...) instead.

@@ +1856,5 @@
> +    // This way, state is still persisted for e.g. temporarily disabled
> +    // add-ons - see bug 989338.
> +    if (gSavedState && gSavedState.placements) {
> +      for (let [area, placements] of Iterator(gSavedState.placements)) {
> +        if (!(area in state.placements)) {

state.placements is a Map - the `in` operator won't work on the data here, you have to use .has().

Which presumably means the tests aren't working correctly either.

Comment 27

[:Gijs (he/him)]

Attachment: save non-placement stuff from previously saved placements,

D'oh. And yeah, there wasn't a test to check the current placements override previously saved ones (and they didn't...). I've added one now, verified it broke with the previous patch, and fixed up the code so that it passes now... better like this? :-)

Comment 28

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8484061 [details] [diff] [review]
save non-placement stuff from previously saved placements,

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

SHIP IT!

                  _==|
             _==|   )__)  |
               )_)  )___) ))
              )___) )____))_)
         _    )____)_____))__)\
          \---__|____/|___|___-\\---
  ^^^^^^^^^\   oo oo oo oo     /~~^^^^^^^
    ~^^^^ ~~~~^^~~~~^^~~^^~~~~~
      ~~^^      ~^^~     ~^~ ~^ ~^
           ~^~~        ~~~^^~

Comment 29

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/637fb0079bb4

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/637fb0079bb4

Comment 31

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

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0

Verified fixed on latest Nightly, build ID: 20140914030209.