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
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
Brian, this looks like a regression from bug 1360282.
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
Summary: Wrong sidebar icon after startup → Sidebar button looks pressed/checked/hovered after restarting Firefox if sidebar was opened previously
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 firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/e5fb1c36d5d6 Do not persist sidebar checked state to document;r=Gijs
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]
The issue is no longer reproducible on Firefox 56.0a1. Tests were performed under Windows 10x64. Build ID: 20170628030206 [bugday-20170628]
You need to log in before you can comment on or make changes to this bug.