903740 - [MP] Defect - Clicking the overlay back button doesn't hide the tab strip

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: App Bar, defect, P2)

Description

[Eddi]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0 (Beta/Release)
Build ID: 20130805030205

Steps to reproduce:

Open any webpage. Right click to open the tab menu.  Single click back on the main webpage.


Actual results:

The tab menu does not disappear.


Expected results:

When you single click on the webpage, the tab menu should disappear.  The currently works only with right clicks.

Comment 1

[Jim Mathies [:jimm]]

I've morphed this a bit - single click on a web page hides the tab strip. I think a fix for this landed in another bug.

Comment 2

[Jim Mathies [:jimm]]

Attachment: fix

easy peasy, one line patch.

Comment 3

[Matt Brubeck (:mbrubeck)]

Comment on attachment 790355 [details] [diff] [review]
fix

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

Could you add a quick test to browser_context_ui.js too?

I wonder if this should be more generic.  Do we also want to hide the tab strip when clicking on other chrome elements (e.g. buttons in the navbar)?  If so then we could expand the mousedown listener in ContextUI.js rather than adding dismiss() calls to each separate button action.

Comment 4

[Jim Mathies [:jimm]]

(In reply to Matt Brubeck (:mbrubeck) from comment #3)
> Comment on attachment 790355 [details] [diff] [review]
> fix
> 
> Review of attachment 790355 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Could you add a quick test to browser_context_ui.js too?
> 
> I wonder if this should be more generic.  Do we also want to hide the tab
> strip when clicking on other chrome elements (e.g. buttons in the navbar)? 
> If so then we could expand the mousedown listener in ContextUI.js rather
> than adding dismiss() calls to each separate button action.

Hmm, good point. I don't understand why this code isn't taking care of this for us as is - 

http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/ContextUI.js#274

Comment 5

[Jim Mathies [:jimm]]

ahh, it's on Elements.browsers.

Comment 6

[Jim Mathies [:jimm]]

Attachment: fix plus tests

Need to do a bit of dog fooding on this.

Comment 7

[Jim Mathies [:jimm]]

Attachment: fix plus tests

removed misc debugging gunk.

Comment 8

[Jim Mathies [:jimm]]

Attachment: fix plus tests

I think I have everything covered here in terms of filtering out elements we should ignore. See anything I might have missed?

Comment 9

[Matt Brubeck (:mbrubeck)]

Comment on attachment 792718 [details] [diff] [review]
fix plus tests

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

Hey, sorry to send you around on this again after you expanded the scope based on my suggestion, but there are a few logic and stylistic issues that I'd like to see addressed:

::: browser/metro/base/content/ContextUI.js
@@ +257,5 @@
> +    while (elem != null) {
> +      if (elem == Elements.tabs ||
> +          elem == Elements.flyouts ||
> +          elem == Elements.contextappbar ||
> +          elem == Elements.autocomplete) {

Are you sure all of these elements should be on the whitelist?  I feel like perhaps any interaction outside the tab strip should dismiss the tab strip, unless there's a specific reason not to.  (On the other hand, we could land this patch now which makes a minimal change to current behavior, and then polish it later.)

Note: If we remove the flyout panels from the whitelist, then we should probably dismiss on the MozFlyoutPanelShowing event rather than waiting for a mousedown/touchstart.

@@ +260,5 @@
> +          elem == Elements.contextappbar ||
> +          elem == Elements.autocomplete) {
> +        return;
> +      }
> +      elem = elem.parentNode;

Optional style nit: Instead of looping through the ancestor chain ourselves, we could use .contains:

let whitelist = [Elements.tabs,
                 Elements.flyouts,
                 Elements.contextappbar,
                 Elements.autocomplete];

if (whitelist.some(elem => elem.contains(aEvent.target))) {
  return;
}

(though this might become less efficient if there were a large number of elements in the whitelist)

@@ +267,5 @@
> +    // Don't let a click on an infobar button dismiss the appbar or navbar.
> +    // Note the notification box should always hover above these other two
> +    // bars.
> +    let box = Browser.getNotificationBox();
> +    if (box.contains(aEvent.originalTarget)) {

For simplicity you can add Browser.getNotificationBox() to the whitelist above rather than handling it separately here.

@@ +279,5 @@
> +    }
> +
> +    // content, dismiss anything visible
> +    if (aEvent.originalTarget.ownerDocument == Browser.selectedBrowser.contentDocument) {
> +      this.dismiss();

I think this check is incorrect for elements in frames or iframes.  Something like this should work:

if (aEvent.target.ownerDocument.defaultView.top == Browser.contentWindow)

::: browser/metro/base/content/NavButtonSlider.js
@@ +71,5 @@
>      this._updateVisibility();
>      Services.prefs.addObserver(kNavButtonPref, this, false);
>    },
>  
> +  observe: function observe(aSubject, aTopic, aData) {

You can also just get rid of the name after "function" here -- it's no longer useful:
http://javascript-reverse.tumblr.com/post/34328529526/javascript-function-name-inference-aka-stop-function

::: browser/metro/base/content/browser-ui.js
@@ +47,5 @@
>    ["findbar",            "findbar"],
>    ["contentViewport",    "content-viewport"],
>    ["progress",           "progress-control"],
>    ["progressContainer",  "progress-container"],
> +  ["flyouts",            "about-flyoutpanel"],

This is misleadingly named.  It should be "aboutFlyout" or similar.  If we want to whitelist all the flyoutpanels then we'll need to add the other two explicitly.

::: browser/metro/base/tests/mochitest/browser_context_ui.js
@@ +133,5 @@
> +
> +    navTo(getpage(1));
> +    yield waitForCondition(function () { return tab.browser.currentURI.spec == getpage(1); });
> +    yield waitForCondition(function () { return pageShowEvent; });
> +    pageShowEvent = false;

Style nit: you could move this logic into a "yield navTo(url);" helper function in head.js, to avoid repeating it.

Instead of the pageShowEvent boolean, you could use waitForEvent:

    let urlChange = waitForCondition(function() Browser.selectedBrowser.currentURI.spec == url);
    let pageShow = waitForEvent(Browser.selectedBrowser, "pageshow");
    BrowserUI.goToURI(url);
    yield Promise.all(urlChange, pageShow);

Comment 10

[Eddi]

Also, would it make sense to have a timer where if the user doesn't press anything and the tab bar is open, it'll close automatically.  It's rare, but there are picture websites where you can't really click anywhere.  With the tab bar open, it's hard to close it.

Comment 11

[Matt Brubeck (:mbrubeck)]

(In reply to Eddi from comment #10)
> Also, would it make sense to have a timer where if the user doesn't press
> anything and the tab bar is open, it'll close automatically.  It's rare, but
> there are picture websites where you can't really click anywhere.  With the
> tab bar open, it's hard to close it.

Hmm... let's open a separate bug if you want to propose this.  Also note that as an alternative or workaround, you can close the tab strip with the Esc key or Win+Z (if you have a keyboard) or an edge swipe gesture (if you have a touch screen).

Comment 12

[Jim Mathies [:jimm]]

(In reply to Eddi from comment #10)
> Also, would it make sense to have a timer where if the user doesn't press
> anything and the tab bar is open, it'll close automatically.  It's rare, but
> there are picture websites where you can't really click anywhere.  With the
> tab bar open, it's hard to close it.

It should close when you tap the image in content. I guess if the image is linked, that would be a problem.

Comment 13

[Eddi]

Yeah. Like Bing or Google Images.

Comment 14

[Jim Mathies [:jimm]]

(In reply to Matt Brubeck (:mbrubeck) from comment #9)
> ::: browser/metro/base/content/ContextUI.js
> @@ +257,5 @@
> > +    while (elem != null) {
> > +      if (elem == Elements.tabs ||
> > +          elem == Elements.flyouts ||
> > +          elem == Elements.contextappbar ||
> > +          elem == Elements.autocomplete) {
> 
> Are you sure all of these elements should be on the whitelist?

In testing each of these had side effects that we didn't want:

Elements.tabs - switching tabs in the tab bar closed the tab bar and didn't switch tabs.
Elements.flyouts - clicking anything in a flyout hid the tab bar. I'll try to add MozFlyoutPanelShowing instead.
Elements.contextappbar - I added this exclusion for some reason but now I'm not sure why. We can land without it and see if something gets filed.

> @@ +260,5 @@
> > +          elem == Elements.contextappbar ||
> > +          elem == Elements.autocomplete) {
> > +        return;
> > +      }
> > +      elem = elem.parentNode;
> 
> Optional style nit: Instead of looping through the ancestor chain ourselves,
> we could use .contains:
> 
> let whitelist = [Elements.tabs,
>                  Elements.flyouts,
>                  Elements.contextappbar,
>                  Elements.autocomplete];
> 
> if (whitelist.some(elem => elem.contains(aEvent.target))) {
>   return;
> }

Ok, will try this. 

> @@ +267,5 @@
> > +    // Don't let a click on an infobar button dismiss the appbar or navbar.
> > +    // Note the notification box should always hover above these other two
> > +    // bars.
> > +    let box = Browser.getNotificationBox();
> > +    if (box.contains(aEvent.originalTarget)) {
> 
> For simplicity you can add Browser.getNotificationBox() to the whitelist
> above rather than handling it separately here.
> 
> @@ +279,5 @@
> > +    }
> > +
> > +    // content, dismiss anything visible
> > +    if (aEvent.originalTarget.ownerDocument == Browser.selectedBrowser.contentDocument) {
> > +      this.dismiss();
> 
> I think this check is incorrect for elements in frames or iframes. 
> Something like this should work:
> 
> if (aEvent.target.ownerDocument.defaultView.top == Browser.contentWindow)
> 
> ::: browser/metro/base/content/NavButtonSlider.js
> @@ +71,5 @@
> >      this._updateVisibility();
> >      Services.prefs.addObserver(kNavButtonPref, this, false);
> >    },
> >  
> > +  observe: function observe(aSubject, aTopic, aData) {
> 
> You can also just get rid of the name after "function" here -- it's no
> longer useful:
> http://javascript-reverse.tumblr.com/post/34328529526/javascript-function-
> name-inference-aka-stop-function
> 
> ::: browser/metro/base/content/browser-ui.js
> @@ +47,5 @@
> >    ["findbar",            "findbar"],
> >    ["contentViewport",    "content-viewport"],
> >    ["progress",           "progress-control"],
> >    ["progressContainer",  "progress-container"],
> > +  ["flyouts",            "about-flyoutpanel"],
> 
> This is misleadingly named.  It should be "aboutFlyout" or similar.  If we
> want to whitelist all the flyoutpanels then we'll need to add the other two
> explicitly.
> 
> ::: browser/metro/base/tests/mochitest/browser_context_ui.js
> @@ +133,5 @@
> > +
> > +    navTo(getpage(1));
> > +    yield waitForCondition(function () { return tab.browser.currentURI.spec == getpage(1); });
> > +    yield waitForCondition(function () { return pageShowEvent; });
> > +    pageShowEvent = false;
> 
> Style nit: you could move this logic into a "yield navTo(url);" helper
> function in head.js, to avoid repeating it.
> 
> Instead of the pageShowEvent boolean, you could use waitForEvent:
> 
>     let urlChange = waitForCondition(function()
> Browser.selectedBrowser.currentURI.spec == url);
>     let pageShow = waitForEvent(Browser.selectedBrowser, "pageshow");
>     BrowserUI.goToURI(url);
>     yield Promise.all(urlChange, pageShow);

This doesn't work actually. I'm not sure why, when I do individual pageshow notifications on each nav, some never get called back. (particularly for forward and back. If I have a single page show and the boolean, everything works.

Comment 15

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #14)
> (In reply to Matt Brubeck (:mbrubeck) from comment #9)
> > Instead of the pageShowEvent boolean, you could use waitForEvent:
> > 
> >     let urlChange = waitForCondition(function()
> > Browser.selectedBrowser.currentURI.spec == url);
> >     let pageShow = waitForEvent(Browser.selectedBrowser, "pageshow");
> >     BrowserUI.goToURI(url);
> >     yield Promise.all(urlChange, pageShow);
> 
> This doesn't work actually. I'm not sure why, when I do individual pageshow
> notifications on each nav, some never get called back. (particularly for
> forward and back. If I have a single page show and the boolean, everything
> works.

I *think* I understand what was happening here. I guess onEvent isn't unique here - 

http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/tests/mochitest/head.js#287

when I changed this up to a var listener = function () {...

my missing pageshow problems went away.

Comment 16

[Jim Mathies [:jimm]]

Attachment: fix plus tests v.2

Comment 17

[Jim Mathies [:jimm]]

Comment on attachment 795570 [details] [diff] [review]
fix plus tests v.2

try run with all the requested changes:

https://tbpl.mozilla.org/?tree=Try&showall=0&rev=3f2e2df98391

Comment 18

[Matt Brubeck (:mbrubeck)]

Comment on attachment 795570 [details] [diff] [review]
fix plus tests v.2

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

Thanks!

::: browser/metro/base/content/ContextUI.js
@@ +332,5 @@
>          break;
> +      case "MozFlyoutPanelShowing":
> +        if (!BrowserUI.isStartTabVisible) {
> +          this.dismiss();
> +        } else {

Very very minor optional nit:

I find "if (x) foo; else bar;" more readable than "if (!x) bar; else foo;"

but that's mostly personal taste, so feel free to keep this unchanged if you find your original version more readable.

::: browser/metro/base/tests/mochitest/head.js
@@ +402,5 @@
>  }
>  
> +/**
> + * same as waitForCondition but with better test output. 
> + *

Any reason to keep the original waitForCondition around?

Comment 19

[Jim Mathies [:jimm]]

(In reply to Matt Brubeck (:mbrubeck) from comment #18)
> ::: browser/metro/base/tests/mochitest/head.js
> @@ +402,5 @@
> >  }
> >  
> > +/**
> > + * same as waitForCondition but with better test output. 
> > + *
> 
> Any reason to keep the original waitForCondition around?

Just that removing it would require a ton of updates in other test files. Thought about it but decided not to add that work to this bug. I'll file a follow up.

Comment 20

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/fx-team/rev/35112ff85c60

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/35112ff85c60

Comment 22

[Manuela Muntean [Away]]

Verified as fixed, with the latest Nightly (build ID: 20130915030208), for iteration #14, on both Win 8 32 and 64 bit.

I followed the STR from comment 0, and also checked that the tab menu disappears in the next situations:

- when you single click on the webpage
- when you click the "+" button
- when you hit ESC
- when you open the flyout panel

Comment 23

[Manuela Muntean [Away]]

I think this also needs some device verification.

Comment 24

[Kamil Jozwiak [:kjozwiak]]

Went through the following "Defect" for iteration #15 testing without any issues (touch device verification). Used the following build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-10-02-03-02-01-mozilla-central/

- Went through the original issue described in comment #0 without any issues
- Ensured that tapping on a blank area while on the awesome screen dismisses the "Tab" bar without any issues
- Ensured that tapping on a blank area while on a website dismisses the "Tab" app bar without any issues
- Ensured that tapping anywhere on the "Navigation" app bar dismissed the "Tab" app bar without any issues
- Ensured that the "Tab" app bar was dismissed when going into the "Options", "Sync", "About", "Help" & "Permission" via the Windows Charm
- Ensured that the "Tab" app bar was dismissed when tapping on the overlay buttons (New Tab & Back buttons)
- Ensured that all of the above test cases worked with full & filled view without any issues