1113896 - UITour: Hello panel isn't closing on icon click after being opened using showMenu("loop")

Status: VERIFIED FIXED

(Firefox :: Tours, defect)

Description

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

In development of the Hello product page - which also interacts with the Hello icon - we've come across an issue where the panel doesn't close as expected.

STR:

# Run in updated Nightly
# Set about:config values (requireSecure=false, browser.uitour.testingOrigins=https://www-demo2.allizom.org)
# Browse to https://www-demo2.allizom.org/en-US/firefox/hello/
# Click on the "TRY FIREFOX HELLO" CTA to bring down the Hello panel
# You can also open the Web Console to run the function manually

Issue: can't close panel by clicking on the Hello icon, clicking in the chrome, refreshing browser, changing tabs or clicking in the document

Expected behavior: should close like a standard panel.

:agibson can confirm, but we're not seeing this in the FTUE prototypes so far.

:dolske, how would you rate this on priority/ease to fix? The product page is 100% landing in GA 35.

Comment 1

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

Note: this happens both for loop.gettingStarted.seen = true and false.

Comment 2

[Alex Gibson [:agibson]]

> Issue: can't close panel by clicking on the Hello icon, clicking in the chrome, refreshing browser, changing tabs or clicking in the document

It is normal for UiTour to create "sticky" menus when calling showMenu(), this is by design as they have auto-hide set to false. The browser will automatically close the menu while changing tabs, however the web page can handle most other cases:

- Use Page Visibility API to hide the menu when the tab is not visible or the browser is minimized (this will also take care if refreshing the page etc). 

- Use a one time click handler on the document to close the menu once it is opened

The only way the Hello panel behaves any differently from calling say showMenu('appMenu') is that the panel does not close if you click the Hello icon. This I would consider a bug.

Hope that makes sense

Comment 3

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

Thanks Alex - this make sense.

Jon, can we add Page Visibility API calls, and an event handler for document clicks in case this bug isn't fixed before Jan 13th?

+ Arcadio to keep informed.

Comment 4

[Jonathan Petto [:jpetto]]

(In reply to Cory Price [:ckprice] from comment #3)
> Thanks Alex - this make sense.
> 
> Jon, can we add Page Visibility API calls, and an event handler for document
> clicks in case this bug isn't fixed before Jan 13th?

Yep, I've got these checks built in now.

Comment 5

[Matthew N. [:MattN]]

An alternative fix is to provide an optional argument to showMenu to disable sticky menus which would also fix this issue and is useful for things like the product page and snippets that don't want sticky menus. Then those pages don't need to handle clicks on the page themselves.

Btw. there isn't currently an API to close the Hello panel with the SocialFrame stuff but you can close it with a reference to the panel.

Comment 6

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

Putting this on the radar for RC 36. The campaign will be much more widespread (PR, snippets, etc).

Comment 7

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

Marking this as a P2 for Hello 36 tours. Although it will not impede development, we're at risk for a much larger audience being exposed to this UX.

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: add an option to auto-hide a panel that is shown with UITour.showMenu

Did someone already mention once that UITour-lib.js is using single quotes for string literals, tabs for indentation and contains Windows newlines?
Just sayin'.

Comment 9

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2: adjust unit tests to not use 'noautohide' when showing the Loop panel

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Mike de Boer [:mikedeboer] from comment #8)
> Did someone already mention once that UITour-lib.js is using single quotes
> for string literals, tabs for indentation and contains Windows newlines?
> Just sayin'.

Blame http://hg.mozilla.org/mozilla-central/rev/491b452af425 for introducing the tabs for indentation. http://hg.mozilla.org/mozilla-central/rev/ca251a28d3dd was supposed to fix the Windows line endings, but I can't see where they were added back in. Want to add a patch to this bug that fixes these issues (a secondary patch)? I don't think it's worth it as a good-first-bug, since there is no way for the new contributor to confirm or test our their changes.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8558465 [details] [diff] [review]
Patch v1: add an option to auto-hide a panel that is shown with UITour.showMenu

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

::: browser/components/uitour/UITour.jsm
@@ +1410,5 @@
>        aOpenCallback(event);
>      }
>  
>      if (aMenuName == "appMenu") {
> +      aWindow.PanelUI.panel.setAttribute("noautohide", !aAutohide);

We should only set noautohide if aAutohide is true, otherwise we should remove the attribute. It's common in XUL to expect that the presence of an attribute is analogous to the value being true.

@@ +1435,5 @@
>          return;
>        }
>  
>        let panel = aWindow.document.getElementById("loop-notification-panel");
> +      panel.setAttribute("noautohide", !aAutohide);

Ditto.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8558465 [details] [diff] [review]
Patch v1: add an option to auto-hide a panel that is shown with UITour.showMenu

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

::: browser/components/uitour/UITour.jsm
@@ +1410,5 @@
>        aOpenCallback(event);
>      }
>  
>      if (aMenuName == "appMenu") {
> +      aWindow.PanelUI.panel.setAttribute("noautohide", !aAutohide);

Sorry, I meant "we should only set noautohide if aAutohide is false. But the flip from noautohide (a poorly named attribute continued through legacy) to "autohide" is confusing. I'm usually against having booleans with a negation attached to them, but in this case I'd prefer that the argument be `aNoAutohide` since that should be pretty common to anybody working with XUL panels.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8558486 [details] [diff] [review]
Patch 2: adjust unit tests to not use 'noautohide' when showing the Loop panel

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

::: browser/components/uitour/test/browser_UITour_loop.js
@@ +35,5 @@
>        return loopButton.open;
>      }, "Menu should be visible after showMenu()");
>  
>      ok(loopPanel.hasAttribute("noautohide"), "@noautohide should be on the loop panel");
> +    is(loopPanel.getAttribute("noautohide"), "false", "@noautohide should be set to |false|");

This line should be removed and the one above it adjusted to use `!loopPanel.hasAttribute()`

Comment 14

[Matthew N. [:MattN]]

Comment on attachment 8558465 [details] [diff] [review]
Patch v1: add an option to auto-hide a panel that is shown with UITour.showMenu

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

(In reply to Mike de Boer [:mikedeboer] from comment #8)
> Did someone already mention once that UITour-lib.js is using single quotes
> for string literals

The library was originally trying to match www.mozilla.org style for the quotes.

::: browser/components/uitour/UITour-lib.js
@@ +196,5 @@
>  
>  		_sendEvent('showMenu', {
>  			name: name,
>  			showCallbackID: showCallbackID,
> +			autohide: autohide

I don't think we need another argument to control this. We just need to fix clicks on the Hello button to toggle visibility instead of always trying to show the panel. This is what we do with the appMenu button.

Comment 15

[Mike de Boer [:mikedeboer]]

Attachment: Patch v2: toggle the Loop panel upon clicking the toolbar button

Aaaah, much nicer. You were so right, Matt.

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 8560422 [details] [diff] [review]
Patch v2: toggle the Loop panel upon clicking the toolbar button

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

::: browser/base/content/browser-loop.js
@@ +41,5 @@
>  
> +    togglePanel: function(event, tabId = null) {
> +      if (this.panel.state == "open") {
> +        this.panel.hidePopup();
> +      } else {

IMO this would be nicer down a level in PanelFrame so other consumers benefit as it seems like something they would all benefit from. Since we want this for 36 and it needs to get uplifted very soon, I won't block on this.

Comment 17

[Mike de Boer [:mikedeboer]]

Pushed to fx-team as: https://hg.mozilla.org/integration/fx-team/rev/90ee83dcb817

Will work on moving this logic to PanelFrame.jsm in a follow-up that can be tracked on trunk.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/90ee83dcb817

Comment 19

[Mike de Boer [:mikedeboer]]

Comment on attachment 8560422 [details] [diff] [review]
Patch v2: toggle the Loop panel upon clicking the toolbar button

Approval Request Comment
[Feature/regressing bug #]: UITour for Fx36
[User impact if declined]: The user won't be able to close the Loop panel once it's been opened via the UITour. This patch also improves the general Loop toolbarbutton UX.
[Describe test coverage new/current, TreeHerder]: landed on m-c, tests pass.
[Risks and why]: minor
[String/UUID change made/needed]: n/a.

Comment 20

[Mike de Boer [:mikedeboer]]

Pushed to release branches:

https://hg.mozilla.org/releases/mozilla-aurora/rev/baab7423b067
https://hg.mozilla.org/releases/mozilla-beta/rev/3287468f6460

Comment 21

[Catalin Varga [QA][:VarCat]]

I verified this bug using the following environment:

FF 38
Build Id:20150219070749
Test Environment: https://www-demo2.allizom.org
OS: Win 7 x64, Mac Os X 10.10, Ubuntu 12.04 x86

Comment 22

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Setting as qe-verify- since this was verified once on the demo environment.

Comment 23

[Matthew N. [:MattN]]

(In reply to Florin Mezei, QA (:FlorinMezei) from comment #22)
> Setting as qe-verify- since this was verified once on the demo environment.

Shouldn't it be marked as verified instead then? qe-verify- means it doesn't need to be verified which is different.

Comment 24

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

(In reply to Matthew N. [:MattN] from comment #23)
> Shouldn't it be marked as verified instead then? qe-verify- means it doesn't
> need to be verified which is different.

Wanted to set qe-verify- so this no longer shows up on our lists of bugs that need verification. But you have a good point there Matthew. Could we perhaps keep the VERIFIED status and remove the qe-verify flag? That way we will know this was verified and it will no longer show up in our lists for fixes that need verification.

Comment 25

[Matthew N. [:MattN]]

(In reply to Florin Mezei, QA (:FlorinMezei) from comment #24)
> Could we
> perhaps keep the VERIFIED status and remove the qe-verify flag? That way we
> will know this was verified and it will no longer show up in our lists for
> fixes that need verification.

I haven't seen the qe-verify flag cleared before when verifying. The list of bugs to verify should exclude bugs with the VERIFIED status.

Comment 26

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

(In reply to Matthew N. [:MattN] from comment #25)
> I haven't seen the qe-verify flag cleared before when verifying. The list of
> bugs to verify should exclude bugs with the VERIFIED status.

We don't exclude bugs with the Verified status because that includes bugs verified for example on Nightly and then uplifted, so that means bugs that would still need verification on Aurora or Beta (Beta usually being more important). To not generate more bugmail on this, I'll just keep a note on this and it's fine to keep it this way. Thank you Matthew!

Comment 27

[Catalin Varga [QA][:VarCat]]

Verified as fixed using:

FF 37 RC2
Build Id: 20150326190726
OS: Win 7 x64, Ubuntu 14.04 x86, Mac Os X 10.9.5

Comment 28

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Verified on Firefox 38 according to comment 21.