Closed Bug 903740 Opened 11 years ago Closed 11 years ago

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

Categories

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

25 Branch
defect

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 26

People

(Reporter: weddi.eddy, Assigned: jimm)

References

Details

(Whiteboard: [preview] feature=defect c=tbd u=tbd p=1)

Attachments

(1 file, 4 obsolete files)

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.
Summary: Single Click to Close Tab Menu → Defect - Single Click to Close Tab Menu
Whiteboard: [preview-triage] feature=defect c=tbd u=tbd p=0
Blocks: metrov1backlog
No longer blocks: metrov1defect&change
Summary: Defect - Single Click to Close Tab Menu → Defect - Clicking the overlay back button doesn't hide the tab strip
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Summary: Defect - Clicking the overlay back button doesn't hide the tab strip → [MP] Defect - Clicking the overlay back button doesn't hide the tab strip
Whiteboard: [preview-triage] feature=defect c=tbd u=tbd p=0 → [preview] feature=defect c=tbd u=tbd p=0
Component: General → App Bar
Assignee: nobody → jmathies
Blocks: metrov1it13
Whiteboard: [preview] feature=defect c=tbd u=tbd p=0 → [preview] feature=defect c=tbd u=tbd p=1
Attached patch fix (obsolete) — Splinter Review
easy peasy, one line patch.
Attachment #790355 - Flags: review?(mbrubeck)
No longer blocks: metrov1backlog
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
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.
Attachment #790355 - Flags: review?(mbrubeck) → review+
(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
ahh, it's on Elements.browsers.
Attached patch fix plus tests (obsolete) — Splinter Review
Need to do a bit of dog fooding on this.
Attachment #790355 - Attachment is obsolete: true
Attached patch fix plus tests (obsolete) — Splinter Review
removed misc debugging gunk.
Attachment #792370 - Attachment is obsolete: true
Blocks: 907042
Attached patch fix plus tests (obsolete) — Splinter Review
I think I have everything covered here in terms of filtering out elements we should ignore. See anything I might have missed?
Attachment #792371 - Attachment is obsolete: true
Attachment #792718 - Flags: review?(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);
Attachment #792718 - Flags: review?(mbrubeck) → review-
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.
(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).
(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.
Yeah. Like Bing or Google Images.
Blocks: 909217
(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.
(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.
Attachment #792718 - Attachment is obsolete: true
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
Attachment #795570 - Flags: review?(mbrubeck)
Blocks: metrov1it14
No longer blocks: metrov1it13
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?
Attachment #795570 - Flags: review?(mbrubeck) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/35112ff85c60
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
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
Status: RESOLVED → VERIFIED
I think this also needs some device verification.
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: