Closed Bug 1393574 Opened 7 years ago Closed 7 years ago

Flexible Space cannot be removed on some profiles

Categories

(Firefox :: Toolbars and Customization, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 57
Iteration:
57.3 - Sep 19
Tracking Status
firefox57 --- verified

People

(Reporter: tcampbell, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [reserve-photon-structure])

Attachments

(1 file)

I've run into a bug where I sometimes cannot remove flexible spaces while in customization view. It seems to be triggered by dragging them around and play with the restore defaults button but I am not able to reproduce in a consistent way.

Here is an example of browser.uiCustomization.state when my browser gets stuck:
> {"placements":{"widget-overflow-fixed-list":[],"PersonalToolbar":["personal-bookmarks"],"nav-bar":["back-button","forward-button","stop-reload-button","home-button","customizableui-special-spring3","urlbar-container","search-container","library-button","sidebar-button","_testpilot-containers-browser-action","geckoprofiler_mozilla_com-browser-action","screenshots_mozilla_org-browser-action","_testpilot-addon-browser-action"],"TabsToolbar":["tabbrowser-tabs","new-tab-button","alltabs-button"],"toolbar-menubar":["menubar-items"]},"seen":["_testpilot-containers-browser-action","geckoprofiler_mozilla_com-browser-action","developer-button","screenshots_mozilla_org-browser-action","_testpilot-addon-browser-action"],"dirtyAreaCache":["PersonalToolbar","nav-bar","TabsToolbar","toolbar-menubar"],"currentVersion":11,"newElementCount":2}

I see that there is a *-spring3 element, but the newElementCount is only 2. Perhaps you have an idea of what could lead to this.

I also notice that when I restore to defaults there are no add-ons buttons, but they get added during restart.

If I restore defaults and restart browser a few times, my toolbars can be customized normally again.
STR:
1) In Customizations, add a new flex space to toolbar
2) Click Reset to Default, Done
3) Confirm about:config / browser.uiCustomization.state is empty string
4) Restart browser

Expected:
 - Layout is reset with two default flex spaces in toolbar

Actual:
 - Less than two flex spaces in toolbar


There seems to be something strange with resetting state and the newElementCount variable. The old newElementCount is used to generate the new names for the flex spaces when starting, but then newElementCount gets reset to 2. This cascades into weird behaviour.
(In reply to Ted Campbell [:tcampbell] from comment #0)
> I also notice that when I restore to defaults there are no add-ons buttons,
> but they get added during restart.

Please can you file a separate bug about this, with presumably separate STR?

I'll try to find time to look at the spacer issue tomorrow, but I'm heading for PTO next week (back in a week) so it might have to wait until afterwards if nobody beats me to it.
Flags: qe-verify+
Priority: -- → P3
Whiteboard: [reserve-photon-structure]
(In reply to :Gijs from comment #2)
> (In reply to Ted Campbell [:tcampbell] from comment #0)
> > I also notice that when I restore to defaults there are no add-ons buttons,
> > but they get added during restart.
> 
> Please can you file a separate bug about this, with presumably separate STR?

ni for this. :-)
Flags: needinfo?(tcampbell)
Ni me to look into the flexible space thing when it's not 1am.
Flags: needinfo?(gijskruitbosch+bugs)
QA Contact: gwimberly
Created Bug 1393661 - Resetting customization doesn't show add-ons until restart
Flags: needinfo?(tcampbell)
In episode #4263 of how legacy add-on compat destroys lives, this bug is caused by our trying to keep a `currentset` property on the toolbar XBL binding, which we persist using legacy XUL storage, which is (a) wrong (ie doesn't contain the right content) in this case, and (b) being picked up when we restart because we don't have placements information in the pref.

While forcing a save after restoring to defaults (which I still expect fixes the add-on bug, but I haven't checked yet... one thing at a time) would probably wallpaper around the issue here, we actually read `currentSet` in a few places, notably to determine whether the toolbar is even in the default state, and so we should probably actually make sure that it continues to return the right thing -- or just kill the entire thing, which feels risky / involved. I'll see how hard either of those is. Keeping ni for now...
I have a WIP patch, but it needs some more work and a test.
Assignee: nobody → gijskruitbosch+bugs
Blocks: 1387512, 1352693
Status: NEW → ASSIGNED
Iteration: --- → 57.3 - Sep 19
Flags: needinfo?(gijskruitbosch+bugs)
Priority: P3 → P1
(In reply to Ted Campbell [:tcampbell] from comment #0)
> I've run into a bug where I sometimes cannot remove flexible spaces while in
> customization view.

So, to be clear, the steps to reproduce in comment #1 for me result in the wrong number of spaces in the toolbar after restarting (which is what comment #1 says, which is different from this point in comment #0). However, I can't reproduce not being able to remove items. 

The correctness fixes in the patch I've attached fix the counting of spacers as well as the gNewElementCount being incremented unnecessarily and/or not being reset correctly. But not being able to reproduce the "remove spacers" issue means it's difficult to say with confidence that they also fix that issue.

If you have reliable STR that make one of the spacers not removable for you, I would still be very interested because it's possible there is more to fix here. In these STR, please make sure to include how you're trying to remove the spacers - dragging or the context menu, and which of them works (or doesn't). It's possible you're basically running into bug 1396423, but some of the dupes of this bug also show the context menu being broken.
Flags: needinfo?(tk)
Flags: needinfo?(tcampbell)
Comment on attachment 8904959 [details]
Bug 1393574 - fix flexible spacers not being removable in some circumstances,

I figured out at least one way of reproducing the issue:

1. clean profile, open customize mode
2. add a new spacer between the two other spacers somewhere
3. restore defaults
4. try to drag the right-most spacer anywhere.

The reason is that the ids of the spacers stop corresponding to the placement lists that CUI keeps, and so stuff breaks.
Attachment #8904959 - Flags: review?(jaws)
(keeping needinfo because the screencast from bug 1396406 shows this also happening when the toolbar isn't in default state, and I still don't have a good idea why that would happen)
Sorry, I can't come up with repeatable steps. Generally resetting customization, adding/removing/moving spacers, and restarting browser leads me to these states like I describe in Comment #0. I find it requires up to three reset customization + restart browser cycles to reset customization. In some cases just restarting the browser without touching customization causes further changes to placements pref. I'm not sure where data is persisted in the profile and how to distinguish between start-up modifications and shutdown modifications. When I can't delete (right-click or dragging) I am in a state where one of the spring elements is labelled greater than the newElementCount.

I don't think it is Bug 1396423, since I tried the hitbox region that has worked when deleting works. As well, right-click delete is affected. I think you are on the right track with XBL persistence. Perhaps look at the data while browser is off. I can't distinguish between startup code changes and shutdown code changes and am unable to tell if my profile is in a default state.
Flags: needinfo?(tcampbell)
Comment on attachment 8904959 [details]
Bug 1393574 - fix flexible spacers not being removable in some circumstances,

https://reviewboard.mozilla.org/r/176784/#review181826

::: browser/base/content/browser.xul:786
(Diff revision 2)
>                         ondragexit="homeButtonObserver.onDragExit(event)"
>                         key="goHome"
>                         onclick="BrowserGoHome(event);"
>                         cui-areatype="toolbar"
>                         aboutHomeOverrideTooltip="&abouthome.pageTitle;"/>
> +        <toolbarspring removable="true" cui-areatype="toolbar" class="chromeclass-toolbar-additional"/>

Adding the default flexible space items here is something of a bonus, in that this will reduce the work we'll do on startup for the default layout. However, I should probably remove the 'removable=true' attribute - that shouldn't be necessary.
(In reply to Ted Campbell [:tcampbell] from comment #15)
> Sorry, I can't come up with repeatable steps. Generally resetting
> customization, adding/removing/moving spacers, and restarting browser leads
> me to these states like I describe in Comment #0. I find it requires up to
> three reset customization + restart browser cycles to reset customization.
> In some cases just restarting the browser without touching customization
> causes further changes to placements pref. I'm not sure where data is
> persisted in the profile and how to distinguish between start-up
> modifications and shutdown modifications. When I can't delete (right-click
> or dragging) I am in a state where one of the spring elements is labelled
> greater than the newElementCount.
> 
> I don't think it is Bug 1396423, since I tried the hitbox region that has
> worked when deleting works. As well, right-click delete is affected. I think
> you are on the right track with XBL persistence. Perhaps look at the data
> while browser is off. I can't distinguish between startup code changes and
> shutdown code changes and am unable to tell if my profile is in a default
> state.

Thanks for trying. I believe the updated patch should fix the issue. Trypush that should have builds that you could try to use to test: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcbc5919baba . If you have time to verify that you can't get into a broken state on those builds, that would be helpful, to be sure I'm not missing something.
Sorry for the spam but I'm a bit new to this tracker. I'm marked "need info" on this - does that mean I need to try one of the new builds and report back?
(In reply to Mike P from comment #18)
> Sorry for the spam but I'm a bit new to this tracker.

No worries, definitely not spam to ask for clarification. Bugzilla's learning curve is not optimal.

> I'm marked "need info"
> on this - does that mean I need to try one of the new builds and report back?

I'm trying to figure out at least one of these two things:

1) what steps to follow, from a clean/empty Nightly profile, to get the situation you described in bug 1396406 and had a screencast of. I found some alternative steps that caused a similar situation, but only after 'restore defaults', and your screencast doesn't show a default setup. Maybe you restored to defaults at some point, and then just reordered other things around the spacers, or something? But maybe you're running into a separate problem. I'd like to be sure I'm fixing things "properly". :-)

2) whether the new builds fix the problem for you (direct link to 64-bit windows build: https://queue.taskcluster.net/v1/task/P4y1ksHCSta332J7EhgQwQ/runs/0/artifacts/public/build/target.zip ), either on a Nightly profile where you already have this problem, or when following steps from thing (1).

If you could post answers to either/both of these that would be helpful. Thank you!
Gijs, I tried that build and I notice that after reseting customization + restarting browser a few times, I end up with a newElementCount of 0. Then trying to add a new spacer to the UI will delete an existing one (since they are both called spring1).
(In reply to Ted Campbell [:tcampbell] from comment #20)
> Gijs, I tried that build and I notice that after reseting customization +
> restarting browser a few times, I end up with a newElementCount of 0. Then
> trying to add a new spacer to the UI will delete an existing one (since they
> are both called spring1).

Ugh. Thanks for spotting that.

I went and just fixed bug 1393661 here and also forced a save after the reset() completes, which persists a non-0 newElementCount and thereby fixes this issue. With the current default set of items, this means we always force a save to happen after a reset(), because there's always "new" flexible spacers that increase the element count. I can't just change the newElementCount default because that will break upgrading users in some circumstances. This code is hairy. :-(

Updated trypush: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5bb3b00f4e07
Blocks: 1393661
This build looks a whole lot better for both the add-on buttons and the deleting spacers issue.

I did notice that sometimes the newElementCount quickly increases when I'm dragging (a move causes the count to go up be two or three). Then after restart it sometimes went back down below what I saw before closing browser. I wasn't able to get into a bad state, but it is possible we may still be able to hit delete issues just at a much lower rate.
(In reply to :Gijs from comment #19)

Great, thanks.
 
> 1) what steps to follow, from a clean/empty Nightly profile, to get the
> situation you described in bug 1396406 and had a screencast of. I found some
> alternative steps that caused a similar situation, but only after 'restore
> defaults', and your screencast doesn't show a default setup. Maybe you
> restored to defaults at some point, and then just reordered other things
> around the spacers, or something? But maybe you're running into a separate
> problem. I'd like to be sure I'm fixing things "properly". :-)

This was immediately after a fresh install on a clean profile. (First time ever installing Nightly on that machine, so there's no chance of any pollution)

My setup process was:

1. Set the theme to dark
2. Install 1Password
3. Install uBlock origin

This was noticed after a restart or two and some random daily driving (though no configuration changes), so that may or may not mater. At no time was restore defaults ever used.

> 
> 2) whether the new builds fix the problem for you (direct link to 64-bit
> windows build:
> https://queue.taskcluster.net/v1/task/P4y1ksHCSta332J7EhgQwQ/runs/0/
> artifacts/public/build/target.zip ), either on a Nightly profile where you
> already have this problem, or when following steps from thing (1).

The new build fixed the problem on the existing profile, as well as after a restore defaults :D
Flags: needinfo?(tk)
(In reply to Mike P from comment #25)
> The new build fixed the problem on the existing profile, as well as after a
> restore defaults :D

Great!

(In reply to Ted Campbell [:tcampbell] from comment #24)
> This build looks a whole lot better for both the add-on buttons and the
> deleting spacers issue.
> 
> I did notice that sometimes the newElementCount quickly increases when I'm
> dragging (a move causes the count to go up be two or three).

STR for this would be helpful... I couldn't really reproduce this. Though note that the spacer that you can drag from within the toolbox / grid of icons / palette area in customize mode, to the toolbar, counts. So I *could* see this:

1. start after a restore defaults: count (from the JSM's internals) is 2
2. open customize mode: count is 3
3. drag from toolbox/palette to toolbar: count is 4
4. restore defaults: count is 3, but saved count is 2 (because the one in the palette/toolbox doesn't end up counting)
5. restart: back to saved count (2).

> Then after
> restart it sometimes went back down below what I saw before closing browser.

So I wonder if the above explains what you saw or not. How are you checking the new element count? :-)
Flags: needinfo?(tcampbell)
I'm checking about:config browser.uiCustomization.state to find newElementCount. Your theory sounds quite plausible. I'm having trouble making it happen now. I wasn't able to make it manifest as any real problems, so it may all be working and I just was seeing intermediate values in the config I shouldn't be paying attention to.
Flags: needinfo?(tcampbell)
Comment on attachment 8904959 [details]
Bug 1393574 - fix flexible spacers not being removable in some circumstances,

https://reviewboard.mozilla.org/r/176784/#review181966

::: browser/components/customizableui/CustomizableUI.jsm:2728
(Diff revision 3)
> -      widgetId = aWidget.id;
> +      // Use "spring" / "spacer" / "separator" for special widgets without ids
> +      widgetId = aWidget.id || aWidget.nodeName.substring(7 /* "toolbar".length */);

Can we do some kind of assertion here if the nodeName doesn't start with "toolbar"?
Attachment #8904959 - Flags: review?(jaws) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/48ed09611986
fix flexible spacers not being removable in some circumstances, r=jaws
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #28)
> Comment on attachment 8904959 [details]
> Bug 1393574 - fix flexible spacers not being removable in some circumstances,
> 
> https://reviewboard.mozilla.org/r/176784/#review181966
> 
> ::: browser/components/customizableui/CustomizableUI.jsm:2728
> (Diff revision 3)
> > -      widgetId = aWidget.id;
> > +      // Use "spring" / "spacer" / "separator" for special widgets without ids
> > +      widgetId = aWidget.id || aWidget.nodeName.substring(7 /* "toolbar".length */);
> 
> Can we do some kind of assertion here if the nodeName doesn't start with
> "toolbar"?

Done. I went with throwing. It's the closest to an actual assert we have. Feels a bit scary, but there really ought not to be non-special nodes without IDs that CUI tries to inquire about. Besides the special nodes, I'm pretty sure everything in the toolbars that doesn't have skipintoolbarset has an id, so we should be good, but we'll see whether there's any edgecases left... certainly tests locally didn't turn anything up.
https://hg.mozilla.org/mozilla-central/rev/48ed09611986
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Reproduced this issue using STR from comment 12 on an affected Nightly build 57.0a1 (20170824100243).

Verified fixed on latest Beta 57.0b9 under Mac OS X 10.11, Win 10 x64 and Ubuntu 16.04 x64
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.