888817 - Back-button mis-styled after customizing with multiple windows because other windows' currentset attributes aren't updated

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[fx4waldi]

Attachment: bug_cbcf.png

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:25.0) Gecko/20130630 Firefox/25.0 (Nightly/Aurora)
Build ID: 20130630040203

Steps to reproduce:

1. Open window
2. Open second window
3. In the first window open customization mode
4. Move the back/forward buttons to another place
5. Close customization mode


Actual results:

In the first window buttons is normal.
In other windows buttons are styles such as if they were next to Location Bar

Comment 1

[Dão Gottwald [:dao]]

Sounds like the customization code fails to update the currentset attribute in all windows.

Comment 2

[Justin Dolske [:Dolske]]

Does bug 755598 avoid this? Or is there just a more fundamental issue?

Comment 3

[:Gijs (out for now; he/him)]

I can reproduce this, so confirming. Dao's comment #1 looks to be spot-on AFAICT.

(In reply to Justin Dolske [:Dolske] from comment #2)
> Does bug 755598 avoid this? Or is there just a more fundamental issue?

Yes, it will avoid the symptom, but we should probably update the attribute correctly everywhere to avoid issues with our legacy/add-on support. Marking P3 because of theoretical compat impact, assuming 755598 gets fixed and avoids the more easily visible issue here.

Comment 4

[:Gijs (out for now; he/him)]

Attachment: Patch v1

This WFM in a quick test, and also makes more sense to me than our current implementation. What do you think?

Comment 5

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

Comment on attachment 776376 [details] [diff] [review]
Patch v1

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

This looks like it should be relatively painless to write a regression test for. Would you mind giving that a shot when you have a minute? I can review that separately.

Comment 6

[:Gijs (out for now; he/him)]

Attachment: test that currentset should be updated when items are moved,

Good point! Here's a test.

Comment 7

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

Comment on attachment 776425 [details] [diff] [review]
test that currentset should be updated when items are moved,

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

::: browser/components/customizableui/test/browser_888817_currentset_updating.js
@@ +15,5 @@
> +      let navbar = document.getElementById(CustomizableUI.AREA_NAVBAR);
> +      let personalbar = document.getElementById(CustomizableUI.AREA_BOOKMARKS);
> +      let navbarCurrentset = navbar.getAttribute("currentset") || navbar.currentSet;
> +      let personalbarCurrentset = personalbar.getAttribute("currentset") || personalbar.currentSet;
> +

It might be worth opening up another window and ensuring that the currentset is updated there too.

Comment 8

[:Gijs (out for now; he/him)]

Attachment: test that currentset should be updated when items are moved,

Alright, let's also open another window.

Comment 9

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

Comment on attachment 776435 [details] [diff] [review]
test that currentset should be updated when items are moved,

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

LGTM!

Comment 10

[:Gijs (out for now; he/him)]

https://hg.mozilla.org/projects/ux/rev/984279fe88e5

Comment 11

[:Gijs (out for now; he/him)]

https://hg.mozilla.org/mozilla-central/rev/984279fe88e5