Closed Bug 1372954 Opened 3 years ago Closed 3 years ago

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

Categories

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

56 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 56
Iteration:
56.1 - Jun 26
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 --- verified

People

(Reporter: ajfhajf, Assigned: bgrins)

References

Details

(Keywords: regression, Whiteboard: [reserve-photon-structure])

Attachments

(2 files)

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
Component: Untriaged → Toolbars and Customization
Brian, this looks like a regression from bug 1360282.
Blocks: 1360282
Flags: needinfo?(bgrinstead)
Keywords: regression
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
Assignee: nobody → bgrinstead
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(bgrinstead)
Summary: Wrong sidebar icon after startup → Sidebar button looks pressed/checked/hovered after restarting Firefox if sidebar was opened previously
Whiteboard: [photon-structure][triage]
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [photon-structure][triage] → [reserve-photon-structure]
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 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 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.
Attachment #8879257 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8879257 [details]
Bug 1372954 - Do not persist sidebar checked state to document;

https://reviewboard.mozilla.org/r/150544/#review155330
Attachment #8879257 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e5fb1c36d5d6
Do not persist sidebar checked state to document;r=Gijs
https://hg.mozilla.org/mozilla-central/rev/e5fb1c36d5d6
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Iteration: --- → 56.1 - Jun 26
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
QA Whiteboard: [bugday-20170621]
Flags: qe-verify? → qe-verify+
QA Contact: gwimberly
The issue is no longer reproducible on Firefox 56.0a1. Tests were performed under Windows 10x64.
Build ID: 20170628030206
[bugday-20170628]
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1407737
You need to log in before you can comment on or make changes to this bug.