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

RESOLVED FIXED in Firefox 28

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: fx4waldi, Assigned: Gijs)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 28
x86
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:M8][Australis:P3])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 769584 [details]
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
(Reporter)

Updated

5 years ago
Blocks: 872617
Component: Untriaged → Theme
Whiteboard: [Australis:M?]
Version: 25 Branch → Trunk
Sounds like the customization code fails to update the currentset attribute in all windows.
Component: Theme → Toolbars and Customization
Summary: Circular the back button after customizing at multiple window → Circular the back button after customizing at multiple window (currentset attribute not updated reliably?)
Summary: Circular the back button after customizing at multiple window (currentset attribute not updated reliably?) → Circular back button out-of-place after customizing in multiple windows (currentset attribute not updated reliably?)
Does bug 755598 avoid this? Or is there just a more fundamental issue?
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.
Assignee: nobody → gijskruitbosch+bugs
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Summary: Circular back button out-of-place after customizing in multiple windows (currentset attribute not updated reliably?) → Back-button mis-styled after customizing with multiple windows because other windows' currentset attributes aren't updated
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P3]
Created attachment 776376 [details] [diff] [review]
Patch v1

This WFM in a quick test, and also makes more sense to me than our current implementation. What do you think?
Attachment #776376 - Flags: review?(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.
Attachment #776376 - Flags: review?(mconley) → review+
Created attachment 776425 [details] [diff] [review]
test that currentset should be updated when items are moved,

Good point! Here's a test.
Attachment #776425 - Flags: review?(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.
Created attachment 776435 [details] [diff] [review]
test that currentset should be updated when items are moved,

Alright, let's also open another window.
Attachment #776435 - Flags: review?(mconley)
(Assignee)

Updated

5 years ago
Attachment #776425 - Attachment is obsolete: true
Attachment #776425 - Flags: review?(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!
Attachment #776435 - Flags: review?(mconley) → review+
https://hg.mozilla.org/projects/ux/rev/984279fe88e5
Whiteboard: [Australis:M?][Australis:P3] → [Australis:M8][Australis:P3][fixed-in-ux]
Depends on: 901058
https://hg.mozilla.org/mozilla-central/rev/984279fe88e5
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M8][Australis:P3][fixed-in-ux] → [Australis:M8][Australis:P3]
Target Milestone: --- → Firefox 28
(Assignee)

Updated

5 years ago
Depends on: 945753
You need to log in before you can comment on or make changes to this bug.