1372954 - Sidebar button looks pressed/checked/hovered after restarting Firefox if sidebar was opened previously

Status: VERIFIED FIXED

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

Description

[randomreporter]

Attachment: Right icon vs Wrong icon

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170614030206

Steps to reproduce:

Create new Nightly profile, right click on tab bar - Customize - drag Sidebar icon to the left of the left arrow, exit customize mode
Left click on sidebar icon, close the sidebar by clicking on close button
Close Nightly
Launch Nightly



Actual results:

Wrong sidebar icon: the one displayed on mouse hove


Expected results:

Sidebar icon without mouse hover

Comment 1

[:Gijs (he/him)]

Brian, this looks like a regression from bug 1360282.

Comment 2

[Brian Grinstead [:bgrins]]

OK, maybe we should not be persisting checked state to the DOM and allow the call to show() to manage this instead.  I'll take a look

Comment 3

[Brian Grinstead [:bgrins]]

Ah, from: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/persist. "Persistence will not remember the absence of an attribute, so for boolean attributes like checked where absence means false, you will need to explicitly set the attribute to false before the window closes (bug 15232)."

I believe if we just stop persisting 'checked' this will remain a problem for nightly users who already have a persisted 'checked' state.  Another option would be to continue to persist but set checked to false (which I actually like a little less but doesn't have this migration problem).

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: Bug 1372954 - Do not persist sidebar checked state to document;

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

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8879257 [details]
Bug 1372954 - Do not persist sidebar checked state to document;

https://reviewboard.mozilla.org/r/150544/#review155240

::: browser/base/content/browser-sidebar.js:61
(Diff revision 1)
>      this._reversePositionButton = document.getElementById("sidebar-reverse-position");
>      this._switcherPanel = document.getElementById("sidebarMenu-popup");
>      this._switcherTarget = document.getElementById("sidebar-switcher-target");
>      this._switcherArrow = document.getElementById("sidebar-switcher-arrow");
>  
> +    // Migrate users who hit Bug 1372954, where the checked state was persisted

Unfortunately unless if we can remove the entry from the xulstore this migration would need to stay around forever, I guess.

Unless if there was a way to rename the attribute on the box and make observes element map from sidebar-box[checked2] to the sidebar-button[checked] here: https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizableWidgets.jsm#607-609.  I don't think there's a way to do that though.

Comment 6

[:Gijs (he/him)]

Comment on attachment 8879257 [details]
Bug 1372954 - Do not persist sidebar checked state to document;

https://reviewboard.mozilla.org/r/150544/#review155304

You can remove items from the XUL store. See the code in nsBrowserGlue.js that does this. We can just remove the persisted thing, and then remove that code in 1-2 nightly cycles.

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 8879257 [details]
Bug 1372954 - Do not persist sidebar checked state to document;

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

Comment 8

[:Gijs (he/him)]

Comment on attachment 8879257 [details]
Bug 1372954 - Do not persist sidebar checked state to document;

https://reviewboard.mozilla.org/r/150544/#review155330

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e5fb1c36d5d6
Do not persist sidebar checked state to document;r=Gijs

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/e5fb1c36d5d6

Comment 11

[Azmina]

I have reproduced this bug with Nightly 56.0a1 (2017-06-14) on Windows 8.1 (64 bit).

This bug's fix is verified on Latest Nightly 56.0a1.

Build ID  : 20170623045418
User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:56.0) Gecko/20100101 Firefox/56.0

Comment 12

[Fahima Zulfath]

The issue is no longer reproducible on Firefox 56.0a1. Tests were performed under Windows 10x64.
Build ID: 20170628030206
[bugday-20170628]