Closed Bug 989338 Opened 7 years ago Closed 6 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect)

29 Branch
x86_64
Windows 7
defect
Not set
normal
Points:
2

Tracking

()

VERIFIED FIXED
Firefox 35
Iteration:
35.1

People

(Reporter: quicksaver, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [Australis:P4])

Attachments

(2 files, 2 obsolete files)

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.
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?
(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.
Gijs is going on vacation... Mike?
Flags: needinfo?(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
Flags: needinfo?(mconley) → needinfo?(quicksaver)
Whiteboard: [Australis:P-]
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.
Flags: needinfo?(quicksaver)
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)
Flags: needinfo?(mconley)
Flags: needinfo?(gijskruitbosch+bugs)
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.
Flags: needinfo?(mconley)
Flags: needinfo?(gijskruitbosch+bugs)
(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.
s/became required good/became required for good
(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. :-)
(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.
I need to look at the area persistence though, because presumably that's going to be a problem in other cases as well.
Flags: needinfo?(gijskruitbosch+bugs)
(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. :) )
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?
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(gavin.sharp)
Flags: needinfo?(bmcbride)
Flags: firefox-backlog+
Keywords: regression
Summary: Removing status-bar from addon-bar makes it lose customization after safe mode restart → Custom area placements are lost if the user customizes while the area is not registered
Whiteboard: [Australis:P-] → [Australis:P4]
(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.
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.
Flags: needinfo?(bmcbride)
Added this to our Australis list that we'll tackle in an upcoming iteration.
Flags: needinfo?(gavin.sharp)
(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?
(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.
Flags: needinfo?(gavin.sharp)
Points: --- → 2
Flags: qe-verify+
I added it to our scratchpad.
Flags: needinfo?(gavin.sharp)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 35.1
With test.
Attachment #8483651 - Flags: review?(bmcbride)
(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.
Yeah, LOG() should be used. It only prints out to the console when the browser.uiCustomization.debug pref is enabled.
I actually didn't intend to leave that in at all, it was a debug leftover. :-)
Attachment #8483715 - Flags: review?(bmcbride)
Attachment #8483651 - Attachment is obsolete: true
Attachment #8483651 - Flags: review?(bmcbride)
(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 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.
Attachment #8483715 - Flags: review?(bmcbride) → review-
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? :-)
Attachment #8484061 - Flags: review?(bmcbride)
Attachment #8483715 - Attachment is obsolete: true
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     /~~^^^^^^^
    ~^^^^ ~~~~^^~~~~^^~~^^~~~~~
      ~~^^      ~^^~     ~^~ ~^ ~^
           ~^~~        ~~~^^~
Attachment #8484061 - Flags: review?(bmcbride) → review+
remote:   https://hg.mozilla.org/integration/fx-team/rev/637fb0079bb4
Whiteboard: [Australis:P4] → [Australis:P4][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/637fb0079bb4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [Australis:P4][fixed-in-fx-team] → [Australis:P4]
Target Milestone: --- → Firefox 35
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0

Verified fixed on latest Nightly, build ID: 20140914030209.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.