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

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[Mauro Sanabria]

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.

Comment 1

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

Attachment: Bug 1391549 - persist positionend value to ensure sidebar side is correct on startup when sidebar is closed,

Review commit: https://reviewboard.mozilla.org/r/171644/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/171644/

Comment 2

[Brian Grinstead [:bgrins]]

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.

Comment 3

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

Comment on attachment 8900254 [details]
Bug 1391549 - persist positionend value to ensure sidebar side is correct on startup when sidebar is closed,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171644/diff/1-2/

Comment 4

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

(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 5

[Brian Grinstead [:bgrins]]

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

Comment 6

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

Comment on attachment 8900254 [details]
Bug 1391549 - persist positionend value to ensure sidebar side is correct on startup when sidebar is closed,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171644/diff/2-3/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
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

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/59014a021378

Comment 9

[Emil Ghitta, QA [:emilghitta]]

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.