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

VERIFIED FIXED in Firefox 26

Status

P2
normal
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: weddi.eddy, Assigned: jimm)

Tracking

25 Branch
Firefox 26
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

5 years ago
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.

Updated

5 years ago
Blocks: 859003
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

Updated

5 years ago
Blocks: 838081
No longer blocks: 859003
(Assignee)

Updated

5 years ago
Summary: Defect - Single Click to Close Tab Menu → Defect - Clicking the overlay back button doesn't hide the tab strip
(Assignee)

Updated

5 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 1

5 years ago
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.

Updated

5 years ago
Blocks: 899390
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
(Assignee)

Updated

5 years ago
Component: General → App Bar
(Assignee)

Updated

5 years ago
Assignee: nobody → jmathies
Blocks: 898799
Whiteboard: [preview] feature=defect c=tbd u=tbd p=0 → [preview] feature=defect c=tbd u=tbd p=1
(Assignee)

Comment 2

5 years ago
Created attachment 790355 [details] [diff] [review]
fix

easy peasy, one line patch.
Attachment #790355 - Flags: review?(mbrubeck)

Updated

5 years ago
No longer blocks: 838081
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+
(Assignee)

Comment 4

5 years ago
(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
(Assignee)

Comment 5

5 years ago
ahh, it's on Elements.browsers.
(Assignee)

Comment 6

5 years ago
Created attachment 792370 [details] [diff] [review]
fix plus tests

Need to do a bit of dog fooding on this.
Attachment #790355 - Attachment is obsolete: true
(Assignee)

Comment 7

5 years ago
Created attachment 792371 [details] [diff] [review]
fix plus tests

removed misc debugging gunk.
Attachment #792370 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Blocks: 907042
(Assignee)

Comment 8

5 years ago
Created attachment 792718 [details] [diff] [review]
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?
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-
(Reporter)

Comment 10

5 years ago
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).
(Assignee)

Comment 12

5 years ago
(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.
(Reporter)

Comment 13

5 years ago
Yeah. Like Bing or Google Images.
(Assignee)

Updated

5 years ago
Blocks: 909217
(Assignee)

Comment 14

5 years ago
(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.
(Assignee)

Comment 15

5 years ago
(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.
(Assignee)

Comment 16

5 years ago
Created attachment 795570 [details] [diff] [review]
fix plus tests v.2
Attachment #792718 - Attachment is obsolete: true
(Assignee)

Comment 17

5 years ago
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)

Updated

5 years ago
Blocks: 904269
No longer blocks: 898799
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+
(Assignee)

Comment 19

5 years ago
(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
Last Resolved: 5 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.