Closed Bug 1391549 Opened 7 years ago Closed 7 years ago

Initial state of sidebar icon should reflect the sidebar side (left/right) pref even if sidebar was closed before restarting Firefox

Categories

(Firefox :: Theme, defect, P1)

57 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 57
Iteration:
57.2 - Aug 29
Tracking Status
firefox57 --- verified

People

(Reporter: maurosan, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [reserve-photon-structure])

Attachments

(1 file)

Bug 1366851 made the sidebar icon to display the current side of the sidebar by accordingly flipping the icon. However, the initial state of the sidebar icon always shows a left sidebar even if the sidebar has been moved to the right.

STR:
A) Create a new profile and start Firefox with it.
B) Initial state of the icon sidebar is on the left and clicking on it shows the sidebar on the left [1].
C) Click the sidebar header button and select "Move Sidebar to Right"; both sidebar and sidebar icon change accordingly.
D) Close the sidebar (important).
E.1) Restart Firefox, or...
E.2) ... open a new browser window.

ER:
Closed sidebar on the right, with the sidebar icon showing a right sidebar.

AR:
Closed sidebar on the right, but sidebar icon shows a left sidebar; opening the sidebar fixes the sidebar icon.

After step E.2 this happens on the new browser window (in the original browser window the sidebar icon correctly shows a right sidebar). Omitting step D (i.e. leaving the sidebar open) workarounds the issue and the sidebar button correctly shows a right sidebar.

This is Firefox 57.0a1 x64 on Windows 10 Enterprise x64; ATM I cannot test other platforms. As for a regression range, this happens ever since Bug 1366851 landed.

[1] I'm on a LTR locale; I guess the opposite happens on RTL locales.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: qe-verify+
Priority: -- → P4
QA Contact: gwimberly
Whiteboard: [reserve-photon-structure]
Summary: Initial state of sidebar icon should reflect saved state → Initial state of sidebar icon should reflect the sidebar side (left/right) pref even if sidebar was closed before restarting Firefox
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 57.2 - Aug 29
Priority: P4 → P1
Comment on attachment 8900254 [details]
Bug 1391549 - persist positionend value to ensure sidebar side is correct on startup when sidebar is closed,

https://reviewboard.mozilla.org/r/171644/#review176822

document.persist doesn't work well for attributes that get removed like this one (once the attr gets set it will always remain persisted even if it's removed). So we'd either need to change to [positionend=false] instead of removing it, or call setPosition in SidebarUI.init() instead of using document.persist.  I'd prefer the latter since it'll mean we have more consistent state after startup.
Attachment #8900254 - Flags: review?(bgrinstead) → review-
(In reply to Brian Grinstead [:bgrins] from comment #2)
> Comment on attachment 8900254 [details]
> Bug 1391549 - persist positionend value to ensure sidebar side is correct on
> startup when sidebar is closed,
> 
> https://reviewboard.mozilla.org/r/171644/#review176822
> 
> document.persist doesn't work well for attributes that get removed like this
> one (once the attr gets set it will always remain persisted even if it's
> removed). So we'd either need to change to [positionend=false] instead of
> removing it, or call setPosition in SidebarUI.init() instead of using
> document.persist.  I'd prefer the latter since it'll mean we have more
> consistent state after startup.

Hm, I'm worried about doing more work than necessary on startup. It is actually possible to remove stuff from the persist store, so I went with doing that instead as it seemed a more minimal patch. I also tried adding code to read the pref (and then set the attribute) in the button's onCreated handler, but that seemed to get overridden by the observer (so it removed the attribute again). I could still go with the setPosition option if you think that's better than this solution - let me know! :-)
Comment on attachment 8900254 [details]
Bug 1391549 - persist positionend value to ensure sidebar side is correct on startup when sidebar is closed,

https://reviewboard.mozilla.org/r/171644/#review177680

::: browser/base/content/browser-sidebar.js:76
(Diff revision 2)
>        document.persist("sidebar-box", "sidebaropen");
> +      if (this._box.hasAttribute("positionend")) {
> +        document.persist("sidebar-box", "positionend");
> +      } else {
> +        let xulStore = Cc["@mozilla.org/xul/xulstore;1"].getService(Ci.nsIXULStore);
> +        xulStore.removeValue(document.documentURI, "sidebar-box", "positionend");

TIL
Attachment #8900254 - Flags: review?(bgrinstead) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/59014a021378
persist positionend value to ensure sidebar side is correct on startup when sidebar is closed, r=bgrins
https://hg.mozilla.org/mozilla-central/rev/59014a021378
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
I have reproduced the issue mentioned in comment 0 using an affected Firefox 57.0a1 build (BuildId:20170818100226).

I have verified that the issue is not reproducible using Firefox 57.0b7 (Build Id:20171009192146) on Windows 10 64bit, macOS 10.11.6 and Ubuntu 16.04 64bit.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: