1696236 - Sidebars button indicates active state, even though the sidebar is closed

Status: NEW

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

Description

[Alice0775 White]

Steps to Reproduce:

1. Start browser with new profile
2. Put Sidebars button on Navbar from customize toolbar
3. Click the button to open sidebar
4. Quit browser and Restart browser
   --- The sidebars button indicates the active state as expected.
5. Close the sidebar
   --- The sidebars button indicates the inactive state as expected.
6. Open a new window(Ctrl+N)
   --- bug appears

Actual results:
The sidebars button indicates active state, even though the sidebar is closed.

Expected results:
The sidebars button indicates actual state(i.e. inactive)

Regression window:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=943c2c2a1daf5cd0d3287c1974eb510615d5ed5b&tochange=b778b6cb49c8ed1dab244c00dc4dc3cf88b25ffe

Suspect:
0cd2b311c3fb2321ad5df14d0d0470f76363c8fe Gijs Kruitbosch — Bug 1391280 - store last sidebar command irrespective of whether sidebar was open, r=bgrins,mixedpuppy

Comment 1

[:Gijs (he/him)]

When the sidebar is opened or closed, this value is not persisted every time - it is added/removed but not persisted. It is persisted only when the window is closed.

When a new window is opened from an existing window, the sidebar state is determined based on the sidebar state in the opening window, but the active/inactive state of the button is determined by the checked value - that wasn't persisted when it changed in the originating window, so we end up using the value that was persisted when we last quit / closed a window. The code that determines sidebar state does not overwrite persisted sidebarcommand or checked attributes, which is how this mismatch occurs.

We could try fixing this in 2 ways - either persist the sidebar checked / not checked attribute every time it changes, or fixing the hiding of the sidebar state to overwrite persisted attributes that are wrong. I'm not sure which will work better / not break other flows without trying, though I would start with the latter approach. Either way we should add tests for this flow. We can't add tests that quit Firefox, but I'm fairly sure that closing and then reopening windows would be able to trigger the same bug, and that is possible within tests.

User workaround is toggling the sidebar on and off.

Comment 3

[Alan Trevino]

Attachment: Bug 1696236 - Prevents adopted window to have a different state on sidebar icon. r?Gijs

Comment 4

[Alan Trevino]

Hi, I saw this bug and gave a try to fix it, I did a small change in the adoptFromWindow function from browser/base/content/browser-sidebar.js afterwards I tried to reproduce the bug and wasn't there anymore... to me it doesn't look like a good fix but it does the job. However this doesn't contain a test for it as I'm not sure where or how to do it (still reading the Contribution documentation and otherss), hoping to get some guidance on how to fix this properly tho.

Thanks in advance!

Comment 5

[:Gijs (he/him)]

(In reply to Alan Trevino from comment #4)

Hi, I saw this bug and gave a try to fix it, I did a small change in the adoptFromWindow function from browser/base/content/browser-sidebar.js afterwards I tried to reproduce the bug and wasn't there anymore... to me it doesn't look like a good fix but it does the job. However this doesn't contain a test for it as I'm not sure where or how to do it (still reading the Contribution documentation and otherss), hoping to get some guidance on how to fix this properly tho.

Thanks in advance!

Thanks for the patch! Let's continue this conversation on phabricator. :-)

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.