982620 - UITour: menu button remains in active state after aborting tour

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Sören Hentzschel]

Attachment: screenshot

STR:

1. Help → Nightly Tour
2. click "Let's go"
3. close the tab

Actual result:

The menu button has still its active state, see screenshot

Expected result:

The menu button should not have an active state.

Comment 1

[Matthew N. [:MattN]]

I can reproduce this.

Thanks for filing Sören.

Comment 2

[:Gijs (he/him)]

Looking...

Comment 3

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cafe909f7e07&tochange=c8cd1f6b6d2d

Maybe https://hg.mozilla.org/mozilla-central/rev/d591aee0b095 ? That's the only UITour bug, but I'm not sure how that'd cause this issue...

Comment 4

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #3)
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=cafe909f7e07&tochange=c8cd1f6b6d2d
> 
> Maybe https://hg.mozilla.org/mozilla-central/rev/d591aee0b095 ? That's the
> only UITour bug, but I'm not sure how that'd cause this issue...

Backing this out locally fixes it.

Comment 5

[:Gijs (he/him)]

Attachment: Australis' UITour doesn't hide the panel on teardown, causing recreatePopup to break later hideMenu calls,

Please doublecheck that this doesn't break anything else... I don't know if we have magical behaviour that depends on us not closing the panel at that point in teardown, or if this messes up on Windows. Works on OS X, though.

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8391578 [details] [diff] [review]
Australis' UITour doesn't hide the panel on teardown, causing recreatePopup to break later hideMenu calls,

Review of attachment 8391578 [details] [diff] [review]:
-----------------------------------------------------------------

I'll attach a test in a few minutes. Please confirm this doesn't regress bug 966072.

Comment 7

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] (away March 10 - 11) from comment #6)
> Comment on attachment 8391578 [details] [diff] [review]
> Australis' UITour doesn't hide the panel on teardown, causing recreatePopup
> to break later hideMenu calls,
> 
> Review of attachment 8391578 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'll attach a test in a few minutes. Please confirm this doesn't regress bug
> 966072.

It doesn't in my testing.

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8391578 [details] [diff] [review]
Australis' UITour doesn't hide the panel on teardown, causing recreatePopup to break later hideMenu calls,

Review of attachment 8391578 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/modules/UITour.jsm
@@ +554,5 @@
>        this.hideHighlight(aWindow);
>        this.hideInfo(aWindow);
>        aWindow.PanelUI.panel.removeAttribute("noautohide");
> +      // Ensure panel is hidden before calling recreatePopup
> +      aWindow.PanelUI.hide();

Actually, change this to:
this.hideMenu(aWindow, "appMenu");
and remove the line after

Comment 9

[Matthew N. [:MattN]]

Attachment: v.1 test

Here's a test for the fix.

Comment 10

[:Gijs (he/him)]

Comment on attachment 8391603 [details] [diff] [review]
v.1 test

Review of attachment 8391603 [details] [diff] [review]:
-----------------------------------------------------------------

rs=me

Comment 11

[Matthew N. [:MattN]]

   https://hg.mozilla.org/integration/fx-team/rev/242281e4bcd0
   https://hg.mozilla.org/integration/fx-team/rev/60da49f4d200

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 8391578 [details] [diff] [review]
Australis' UITour doesn't hide the panel on teardown, causing recreatePopup to break later hideMenu calls,

Note that this patch was replace with a different approach in comment 8 and landed as https://hg.mozilla.org/integration/fx-team/rev/242281e4bcd0

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug  966072
User impact if declined: If a user leaves the tour tab while the menu panel is open, the menu panel button will remain in the depressed state.
Testing completed (on m-c, etc.): Locally by Gijs and MattN on Linux and OS X.
Risk to taking this patch (and alternatives if risky): Low risk tour-specific additional cleanup
String or IDL/UUID changes made by this patch: None

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/242281e4bcd0
https://hg.mozilla.org/mozilla-central/rev/60da49f4d200

Comment 14

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/57845d9b79e5