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

VERIFIED FIXED in Firefox 56

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: ajfhajf, Assigned: bgrins)

Tracking

({regression})

56 Branch
Firefox 56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55 unaffected, firefox56 verified)

Details

(Whiteboard: [reserve-photon-structure])

Attachments

(2 attachments)

Reporter

Description

2 years ago
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
Reporter

Updated

2 years ago
Component: Untriaged → Toolbars and Customization

Comment 1

2 years ago
Brian, this looks like a regression from bug 1360282.
Blocks: 1360282
Flags: needinfo?(bgrinstead)
Keywords: regression
Assignee

Comment 2

2 years ago
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)

Updated

2 years ago
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]
Assignee

Comment 3

2 years ago
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 hidden (mozreview-request)
Assignee

Comment 5

2 years ago
mozreview-review
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

2 years ago
mozreview-review
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 hidden (mozreview-request)

Comment 8

2 years ago
mozreview-review
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+

Comment 9

2 years ago
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e5fb1c36d5d6
Do not persist sidebar checked state to document;r=Gijs

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e5fb1c36d5d6
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Iteration: --- → 56.1 - Jun 26

Comment 11

2 years ago
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]

Updated

2 years ago
Flags: qe-verify? → qe-verify+
QA Contact: gwimberly

Comment 12

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

Updated

2 years ago
Status: RESOLVED → VERIFIED
Flags: qe-verify+

Updated

2 years ago
Depends on: 1407737
You need to log in before you can comment on or make changes to this bug.