Sidebar switcher button stays in active state after it's clicked with the intention to hide the switcher panel

RESOLVED FIXED in Firefox 57

Status

()

Firefox
General
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: nhnt11, Assigned: nhnt11)

Tracking

Trunk
Firefox 57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

4 months ago
STR:

1. Click sidebar switcher button to open the switcher panel
2. Click the switcher button again to close it

Expected:

Switcher button is no longer in active state

Actual:

Switcher button remains in active state after panel is closed


The panel closes when you click outside of it, so when you click the button, it closes, and then the code to toggle it is called - we end up calling showSwitcherPanel, which is bad.
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED

Comment 2

4 months ago
mozreview-review
Comment on attachment 8893359 [details]
Bug 1387022 - Don't attempt to show the sidebar switcher panel if it's closing.

https://reviewboard.mozilla.org/r/164454/#review169730

::: browser/base/content/browser-sidebar.js:80
(Diff revision 1)
>      if (this._switcherPanel.state == "open" || this._switcherPanel.state == "showing") {
>        this.hideSwitcherPanel();
> -    } else {
> +    } else if (this._switcherPanel.state == "closed") {
>        this.showSwitcherPanel();
>      }

Can we simplify to:

```
if (.state == 'closed') {
  show();
} else {
  hide();
}
```

or does that break stuff? Either way, r=me
Attachment #8893359 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 3

4 months ago
(In reply to :Gijs from comment #2)
> Comment on attachment 8893359 [details]
> Bug 1387022 - Don't attempt to show the sidebar switcher panel if it's
> closing.
> 
> https://reviewboard.mozilla.org/r/164454/#review169730
> 
> ::: browser/base/content/browser-sidebar.js:80
> (Diff revision 1)
> >      if (this._switcherPanel.state == "open" || this._switcherPanel.state == "showing") {
> >        this.hideSwitcherPanel();
> > -    } else {
> > +    } else if (this._switcherPanel.state == "closed") {
> >        this.showSwitcherPanel();
> >      }
> 
> Can we simplify to:
> 
> ```
> if (.state == 'closed') {
>   show();
> } else {
>   hide();
> }
> ```
> 
> or does that break stuff? Either way, r=me

We'd still need to check that it's not closing in the else, so I'm going to leave it how it is.

Comment 4

4 months ago
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b5e38bafb4ed
Don't attempt to show the sidebar switcher panel if it's closing. r=Gijs

Comment 5

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b5e38bafb4ed
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.