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

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[(inactive) Nihanth Subramanya [:nhnt11]]

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 1

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1387022 - Don't attempt to show the sidebar switcher panel if it's closing.

Review commit: https://reviewboard.mozilla.org/r/164454/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/164454/

Comment 2

[:Gijs (he/him)]

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

Comment 3

[(inactive) Nihanth Subramanya [:nhnt11]]

(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

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b5e38bafb4ed