1171746 - Some panels stay open when switching or closing tabs

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect, P4)

Description

[Erik Rose [:erik][:erikrose]]

1. Open several tabs (as if intending to Pocket them all).
2. Click the Pocket menu. Wait till "Saved to Pocket" appears.
3. Close the tab with command-W.

Actual:
The Pocket menu stays open, saying "Saved to Pocket", as if the newly exposed tab was also saved to pocket.

Expected:
The menu closes when the tab is closed.

Comment 1

[Erik Rose [:erik][:erikrose]]

In fact, all of the toolbar menus do this. None of them should. It's equally weird for the Privacy Badger menu to keep showing info about Slashdot after the Slashdot tab is closed.

Comment 2

[Shane Caraveo (:mixedpuppy)]

While the STR is on the pocket button, any toolbar button that has a panel will behave the same.  Moving to general since it is not pocket specific.

IMO cmd-w should close the panel and leave the tab alone.

Comment 4

[:Gijs (he/him)]

I'm not sure this isn't a feature in the general case. As a whole we seem to be heading towards keeping them open more rather than less - I know I saw a bug about this for either web extensions or the identity popup, but I can't find it right now. Kris or Philipp, can you comment?

Comment 5

[Dietrich Ayala (:dietrich)]

(In reply to :Gijs Kruitbosch from comment #4)
> I'm not sure this isn't a feature in the general case. As a whole we seem to
> be heading towards keeping them open more rather than less

This is a very weak justification for a per-tab UI element staying open across tab open/close/switch events. How exactly is it a feature?

If the intent is for panels to not be per-tab UI, then panels should be visually anchored to something that's *not* part of the tab+urlbar visual space. (I've always thought that the bookmarks and hamburger menu items were sorely out of place for this reason.)

That said, in the Pocket case the panel is *clearly* a tab-tied UI element. Well, unless all your tabs had the same URL you just saved to Pocket ;)

Our designs have gone to great lengths over the years, making controversial designs like tabs-on-top, in order to build a strong visual demarcation between per-tab and global spaces in the interface. This shows the visual division, along with the exceptions: https://i.imgur.com/VrWu1Yj.png

The panel not behaving in a per-tab way is frustrating because it is in the way of the user, is visually incorrect (Pocket panel for URL x, showing for URL y), and also because the behavior breaks this piece of the browser window design.

Related, I scratched my own itch last week, and released an add-on that takes a panel-free approach to Pocket integration: https://autonome.wordpress.com/2016/02/19/doublestitch-a-simpler-and-cleaner-pocket-for-firefox

Comment 6

[:Gijs (he/him)]

(In reply to Dietrich Ayala (:dietrich) from comment #5)
> (In reply to :Gijs Kruitbosch from comment #4)
> > I'm not sure this isn't a feature in the general case. As a whole we seem to
> > be heading towards keeping them open more rather than less
> 
> This is a very weak justification for a per-tab UI element staying open
> across tab open/close/switch events. How exactly is it a feature?

I am not justifying anything. I am saying that I don't agree prima facie that it is the case that every single CUI panel ever should close on tab switch, which is the implication from comment #2 .

> If the intent is for panels to not be per-tab UI,

Some of them are, some of them are not, as your image shows.

> visually anchored to something that's *not* part of the tab+urlbar visual
> space. (I've always thought that the bookmarks and hamburger menu items were
> sorely out of place for this reason.)

That's fine, but not the subject of this bug as originally filed, right? Is there a separate bug filed for it? I have no strong opinion, I just want to keep this bug focused on what it was filed about, and not devolve into "fix all our UI ever to be consistent about this" and then never get fixed.

> That said, in the Pocket case the panel is *clearly* a tab-tied UI element.

I don't disagree. Again, I'm arguing about comment #2, which implies "all panels implemented using module X should behave this way". Especially when add-ons use panels, and their buttons get added to the navbar because that's where everybody's buttons go these days (no value judgment here, just stating the fact), closing it when switching tabs may or may not be expected. Apparently Chrome, for instance, keeps its panels open when switching windows / applications.

All I'm saying is that we don't know if a generic panel is per-tab UI or not. Right now the behaviour is inconsistent because clicking to switch tabs will close the panel, but keyboard shortcuts that switch tabs (or close them, or...) don't. We should make the behaviour consistent, but we should make a conscious decision about which way we go, so I needinfo'd a bunch of folks.

Comment 7

[Kris Maglione [:kmag]]

I think there are some obvious cases where panels should definitely close when switching tabs. Bookmarks, social API widgets, page action panels, identity panels, since those are all clearly tied to the current site.

Though, if it comes to it, that means they should close if the tab navigates too.

For other panels, it's less clear. A browser action for an extension, for instance, could easily be written to track tab changes, and do something useful when tabs are switched.

Honestly, this mostly seems like a bit of a corner case, and I'd rather not take away potentially useful functionality just to squash it.

Comment 9

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

We may be able to add a mutation observer to the panel's anchor and hide the panel if some condition changes in the anchor.

A test function could be passed to the popup binding to check if the panel should close.

We will probably need to treat each one on a case-by-case basis, but could make the default be to close the panel on a mutation.

Comment 10

[Kris Maglione [:kmag]]

I think it would be enough to add some sort of context attribute, so panels can specify whether they're page-specific, tab-specific, or generic, and then close any stale panels from onLocationChange.

Comment 11

[Shane Caraveo (:mixedpuppy)]

This certainly isn't real data, however, I have various addons installed that I play around with.  Currently have 13 buttons in nav-bar.  None have functionality that would make sense in this scenario, and I cannot think of any addon I've used (other than when debugging) that I would want remain open across tabs.

If a panel is open, it is a modal element and bindings should go to that.  In this case, cmd+w would close the panel, ctrl+tab wouldn't bubble up past the panel, etc.  This would also be consistent with mouse clicks.  This is how Safari operates, but then again, Chrome operates like Firefox currently does.

strawman:

tabpanel attribute (default true), if tabpanel=false remains open across tab switches/closes in a consistent fashion (eg. specific action such as click button again to close it).  tabpanel=true would isolate key bindings to the panel as described above.

Comment 12

[arni2033]

(In reply to Shane Caraveo (:mixedpuppy) from comment #11)
> I cannot think of any addon I've used that I would want remain open across tabs
I only have 3 addons which provide a panel, and 2 of them should probably stay open after switching tabs:  Tab Data [1] and Simplified Tab Groups [2].
[1] https://addons.mozilla.org/en-US/firefox/addon/tab-data/
[2] https://addons.mozilla.org/ru/firefox/addon/tab-groups/

Actually, [2] doesn't register switching tabs... Huh.
But it allows switching tabs via the panel. It would be sad if this bug was fixed hastily, and [2]'s panel closed after each click. This scenario should be prevented somehow. E.g. panels of those addons
which don't use new API - should behave the same as before (== panels should stay open across tabs)

Comment 14

[Erik Rose [:erik][:erikrose]]

> If a panel is open, it is a modal element and bindings should go to that.  In this case, cmd+w would close the panel

Given that panels are more like menus than windows--not having titlebars or close buttons or being movable--this would be a gross inconsistency. Command-W should always close the window or tab.

> This is how Safari operates

I'm not seeing it. The two panels in my Safari UI, brought forth by the Show All Tabs and the Show Sidebar buttons, are not dismissable with command-W; the whole tab closes instead.

Comment 15

[Shane Caraveo (:mixedpuppy)]

Safari Version 9.0.3 (11601.4.4) downloads button closes panel with cmd+w, though I discovered the share panel does not.  So it is not consistent within Safari.

Comment 16

[Erik Rose [:erik][:erikrose]]

> Safari Version 9.0.3 (11601.4.4) downloads button closes panel with cmd+w

So I went and played with that, and it's true...*if* you haven't used keyboard shortcuts in the interrim to change to a different tab. If you have, command-W closes the tab (despite the Downloads panel remaining open). Whee! :-D So I think that's not a very good precedent to follow.

Comment 19

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

Panels related specifically to the current tab should close when changing tabs. We can start by closing the Bookmarks and Pocket panels on TabSwitch. You can add an attribute to the panel that says it is tab-specific, and then in the TabSwitch event listener you could query for any panels that have this opt-in attribute as well as panel.state == "open" and hide them.

Comment 20

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

Array.from(document.querySelectorAll("panel")).filter(p => p.state == "open").forEach(p => p.hidePopup());

Comment 21

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

Array.from(document.querySelectorAll("panel"))
     .filter(p => p.state == "open")
     .forEach(p => p.hidePopup());

Comment 22

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

Array.from(document.querySelectorAll("panel[tabSpecific='true']"))
     .filter(p => p.state == "open")
     .forEach(p => p.hidePopup());

Comment 23

[Katie Broida [:ktbee]]

Attachment: Bug 1171746 - ensure tab specific panels close when you switch the tab

Adds a tabSpecific attribute to the edit bookmarks panel and the Pocket subview panel to signal that these popups should close when the user navigates away from the tab. It also specifies that the keyboard shortcut command-w should close the edit bookmarks panel and the tab (previously it didn't).

Review commit: https://reviewboard.mozilla.org/r/63714/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63714/

Comment 24

[Dão Gottwald [:dao]]

Comment on attachment 8770177 [details]
Bug 1171746 - ensure tab specific panels close when you switch the tab

>           tabSpecific="true"

DOM attributes are usually lower-case.

>            if (aEvent.metaKey && aEvent.key === "w"){

This needs support for platforms other than OS X and it needs to be localized.

nit: use == here instead of === and put a space between ) and {

>              Array.from(document.querySelectorAll("panel[tabSpecific='true']"))
>                .filter(p => p.state == "open")
>                .forEach(p => p.hidePopup());

Please make this a for-of loop:

for (let panel of document.querySelectorAll("panel[tabspecific='true']")) {
  if (panel.state == "open") {
    panel.hidePopup();
  }
}

It would also be nice to put this code in a listener in browser.js, as tabbrowser.xml doesn't need to know about this and is already complex enough.

Comment 25

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

Comment on attachment 8770177 [details]
Bug 1171746 - ensure tab specific panels close when you switch the tab

https://reviewboard.mozilla.org/r/63714/#review60694

Clearing review since Dao already left comments here.

Comment 26

[Katie Broida [:ktbee]]

Comment on attachment 8770177 [details]
Bug 1171746 - ensure tab specific panels close when you switch the tab

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63714/diff/1-2/

Comment 27

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

Comment on attachment 8770177 [details]
Bug 1171746 - ensure tab specific panels close when you switch the tab

https://reviewboard.mozilla.org/r/63714/#review61984

::: browser/base/content/browser-places.js:132
(Diff revision 2)
> +            if (aEvent.getModifierState("Control") || aEvent.getModifierState("Meta")) {
> +              let accessKey = document.getElementById("key_close")
> +                                .getAttribute("key")
> +                                .toLocaleLowerCase();
> +              if (aEvent.key == accessKey) {

Ideally we would get the key_close element, and from that we could see what key was needed as well as what modifiers were useful for it.

Maybe this is something better added to utilityOverlay.js?

Like the following?

let key = document.getElementById("key_close");
if (key && eventMatchesKey(aEvent, key)) {
  ...
}

where eventMatchesKey is a function that compares the event's modifiers and key to the key's required modifiers and specified key.

Note that this looks like it would accept Ctrl+Shift+key because your check at the beginning only looks to see if the required modifiers were pressed but doesn't exclude the event if other modifiers were also pressed.

Comment 28

[Katie Broida [:ktbee]]

Comment on attachment 8770177 [details]
Bug 1171746 - ensure tab specific panels close when you switch the tab

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63714/diff/2-3/

Comment 29

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

Comment on attachment 8770177 [details]
Bug 1171746 - ensure tab specific panels close when you switch the tab

https://reviewboard.mozilla.org/r/63714/#review62740

At a high-level this looks good, but Gijs should review the utilityOverlay code too.

::: browser/base/content/utilityOverlay.js:485
(Diff revision 3)
> + *
> + * @param aEvent
> + *        The KeyboardEvent event you want to compare against your key.
> + *        Note, this only compares keys with just one modifier.
> + *
> + * @param aEvent

@param aKey

::: browser/base/content/utilityOverlay.js:495
(Diff revision 3)
> +function eventMatchesKey(aEvent, aKey)
> +{
> +  let keyPressed = aKey.getAttribute("key").toLowerCase();
> +  let keyModifiers = aKey.getAttribute("modifiers");
> +  let eventModifier = [];
> +  let modifiers = ["Accel", "Shift", "Alt"];

Ctrl and Meta should be added to this list, right?

Comment 30

[Dão Gottwald [:dao]]

Comment on attachment 8770177 [details]
Bug 1171746 - ensure tab specific panels close when you switch the tab

Adding the TabSelect listener in XULBrowserWindow.onUpdateCurrentBrowser makes no sense.

The widget property (not the DOM attribute) should be tabSpecific (uppercase S).

Comment 31

[:Gijs (he/him)]

Comment on attachment 8770177 [details]
Bug 1171746 - ensure tab specific panels close when you switch the tab

https://reviewboard.mozilla.org/r/63714/#review62858

Sorry you're now getting feedback from 3 people, Katie. There's a number of issues with the patch I explain below. One separate thing: it would be a Really Good Idea to make sure we have tests for this code working as expected.

::: browser/base/content/browser-places.js:131
(Diff revision 3)
>                //    be enough.
>                break;
>              }
>              this.panel.hidePopup();
>              break;
> +          case 0:

Please add a comment here explaining that this is catching character-generating keypresses which have a keyCode of 0.

::: browser/base/content/utilityOverlay.js:500
(Diff revision 3)
> +  let modifiers = ["Accel", "Shift", "Alt"];
> +  let keysMatch;
> +
> +  if (aEvent.key != keyPressed) {
> +    return false;
> +  } else {

Nit: no else after a return.

::: browser/base/content/utilityOverlay.js:503
(Diff revision 3)
> +  modifiers.forEach(function(modifier) {
> +    if (aEvent.getModifierState(modifier)) {
> +      eventModifier.push(modifier);
> +    }
> +  });

You could remove the declaration of `eventModifiers` further up, as it's not used before this point, and do:

`let eventModifiers = modifiers.filter(modifier => aEvent.getModifierState(modifier));`

to filter out the correct modifiers for the event from the list of modifiers.

::: browser/base/content/utilityOverlay.js:508
(Diff revision 3)
> +  // ensure only one modifier is in use
> +  if (eventModifier.length > 1) {
> +    return false;

This seems like a restriction specific to this particular <key>, and this function is supposed to be generic. That is, there are some <key>s that will have more than one modifier, right?

::: browser/base/content/utilityOverlay.js:518
(Diff revision 3)
> +  if (!!eventModifier[0] && !keyModifiers) {
> +    return false;
> +  }
> +  // Check whether any of aKey's modifiers match aEvent's modifier
> +  if (keyModifiers) {
> +    keyModifiers = keyModifiers.split(/" "|,/);

You're splitting by either of the following:

- the string `" "`, that is, a double quote followed by a space followed by a double quote
- a comma

I think you just want a space or a comma, and we should probably deal with just any quantity of whitespace, so I would suggest doing:

```
....split(/[\s,]+/);
```

More generally though, this stuff is complicated. See the description here: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/key#Attributes . In particular, this code doesn't deal with "control". Happily, none of our code actually uses the "any" classifier which makes modifiers optional, which would make this even more complex.

Getting this stuff right is going to be quite tricky... Here's what I would do, and I *think* this works, but you'll have to check...

- remove the `eventModifiers` array
- change the `modifiers` array to have "Control" and "Meta" but not "Accel"
- from the keyModifiers array, replace "accel" with "Meta" on OS X, and "Control" on everything else, keep uppercasing the rest
- `return modifiers.every(modifier => keyModifiers.includes(modifier) == aEvent.getModifierState(modifier))`. In English: check that every modifier's state on the event is identical to whether or not it was included in the list of modifiers on the `<key>` element.

This way you also verify that keys not in the modifier list are not pressed, and that multi-modifier shortcut keys (ctrl-shift-something) still work. We'll need to do this for it to work well with l10n and on multiple platforms including OS X. In order to get that right, we need to "normalize" accel, because otherwise the `<key>` modifier attribute list will include only accel *OR* its OS-specific equivalent, and of course when you call `event.getModifierState`, *both* will be either true or false depending on whether the user pressed them.

::: browser/components/customizableui/CustomizableUI.jsm:1363
(Diff revision 3)
>        if (aWidget.disabled) {
>          node.setAttribute("disabled", true);
>        }
>        node.setAttribute("removable", aWidget.removable);
>        node.setAttribute("overflows", aWidget.overflows);
> +      node.setAttribute("tabspecific", aWidget.tabspecific);

A few things: in CustomizableUI there's a list of the default values for all properties. You didn't add aWidget.tabspecific to the list, so this will set the attribute to the raw string "undefined" if it isn't specified (for most widgets), or to the string "false" if the property is explicitly specified as false.

I think it makes more sense to do what we do for `aWidget.disabled` and only set the attribute if it's truthy. We should also include the property in the default specification with the value `false`.

Right now the fact that you later read it with `getAttribute` and don't explicitly compare with the string `true` means that, as far as I can tell, you're now always setting this attribute on panels from CustomizableUI, even the ones that don't have this property set to true on their widget specification.

::: browser/components/customizableui/content/panelUI.js:338
(Diff revision 3)
>        let tempPanel = document.createElement("panel");
>        tempPanel.setAttribute("type", "arrow");
>        tempPanel.setAttribute("id", "customizationui-widget-panel");
>        tempPanel.setAttribute("class", "cui-widget-panel");
>        tempPanel.setAttribute("viewId", aViewId);
> +      if (aAnchor.getAttribute("tabspecific")){

Nit: space before `{`.

Comment 32

[Katie Broida [:ktbee]]

Comment on attachment 8770177 [details]
Bug 1171746 - ensure tab specific panels close when you switch the tab

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63714/diff/3-4/

Comment 33

[Katie Broida [:ktbee]]

(In reply to :Gijs Kruitbosch from comment #31)
> Comment on attachment 8770177 [details]
> Bug 1171746 - ensure tab specific panels close when you switch the tab
> 
> https://reviewboard.mozilla.org/r/63714/#review62858
> 
> Sorry you're now getting feedback from 3 people, Katie. 

This is good! More feedback == more better for learning :)

> ::: browser/base/content/utilityOverlay.js:508
> (Diff revision 3)
> > +  // ensure only one modifier is in use
> > +  if (eventModifier.length > 1) {
> > +    return false;
> 
> This seems like a restriction specific to this particular <key>, and this
> function is supposed to be generic. That is, there are some <key>s that will
> have more than one modifier, right?

I had wondered if there are <key> elements that had more than one modifier so I checked the files that had <key>s (like browser-sets.inc, baseMenuOverlay.xul). It seemed like there weren't <key>s with combination modifiers, but now I'm realizing that I misunderstood how multiple modifiers were added to a <key> and that there are some with combination modifiers. Good to have a solution for that already, thank you.  

Dão, I tried adding my TabSelect event listener to gBrowserInit._delayedStartup (a possibility you suggested on irc), but I found it was only firing whenever I started a new window and not when a new tab was selected. Instead I call closeTabSpecificPanels() in XULBrowserWindow.onLocationChange, which seems like a good way to have the function called when I needed it to be called and avoid adding multiple event listeners. Let me know if I should do it differently though. 

I think I've addressed everyone's feedback with my latest patch, but Gijs I'll make you the reviewer since you requested the most changes.

Comment 34

[Dão Gottwald [:dao]]

(In reply to Katie Broida [:ktbee] from comment #33)
> Dão, I tried adding my TabSelect event listener to
> gBrowserInit._delayedStartup (a possibility you suggested on irc), but I
> found it was only firing whenever I started a new window and not when a new
> tab was selected.

Could you show that code? Sounds like you called closeTabSpecificPanels directly in _delayedStartup rather than in the event listener.

> Instead I call closeTabSpecificPanels() in
> XULBrowserWindow.onLocationChange, which seems like a good way to have the
> function called when I needed it to be called and avoid adding multiple
> event listeners. Let me know if I should do it differently though. 

This will call closeTabSpecificPanels more often than needed.

Comment 35

[:Gijs (he/him)]

Comment on attachment 8770177 [details]
Bug 1171746 - ensure tab specific panels close when you switch the tab

https://reviewboard.mozilla.org/r/63714/#review63574

Clearing review while we sort out where the event handler needs to go - I'd tend to agree with Dão that we should specifically listen for the tab switch event instead.

Thanks for the test for the event-to-key matching. Did you intend to also write a test for the tab-specific panel behaviour? You'd probably want to create two panels (one tab-specific, one not tab-specific), open both, and then switch tabs and verify one got closed and one didn't.

::: browser/base/content/test/general/browser_utilityOverlay.js:35
(Diff revision 4)
> +  let eventMatchResult;
> +  document.addEventListener("keypress", function(e) {
> +      eventMatchResult = eventMatchesKey(e, key);
> +  });
> +
> +  let key = document.getElementById("key_find");

These tests will break if the shortcuts ever change, and won't work when run against non-en-US builds. While none of those our dealbreakers per se, the test would be easier to read and understand if you created and added the <key> elements from inside the test (and removed them again afterwards). That would also ensure (and this is perhaps slightly more important) that there would be no side-effects, like opening the find bar (which could potentially break later tests).

In fact, to avoid conflicts/side-effects from the keypresses, it might be sensible to create the keyboard event manually and dispatch it against a dummy element, and stop propagation + prevent default immediately in the event handler.

Comment 36

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

Katie, do you have enough here to move forward with this work?

Comment 37

[Katie Broida [:ktbee]]

Attachment: addEventListener-test.patch

Sorry I missed your request to see the code Dão. This patch has the method I'm trying in gBrowserInit._delayedStartup:

`gBrowser.tabContainer.addEventListener("TabSelect", this.closeTabSpecificPanels);`

I was mistaken about the event firing when the window starts up. I think that was due to how I was doing some of my testing (putting `console.log("TabSelect)` as the second argument for addEventListener() ). However console messages in closeTabSpecificPanels() don't fire at all.

Comment 38

[Dão Gottwald [:dao]]

Comment on attachment 8777014 [details] [diff] [review]
addEventListener-test.patch

>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -1384,16 +1384,18 @@ var gBrowserInit = {
>       }, 5000);
> 
>       PanicButtonNotifier.init();
>     });
>     this.delayedStartupFinished = true;
> 
>     Services.obs.notifyObservers(window, "browser-delayed-startup-finished", "");
>     TelemetryTimestamps.add("delayedStartupFinished");
>+
>+    gBrowser.tabContainer.addEventListener("TabSelect", this.closeTabSpecificPanels);
>   },

this.closeTabSpecificPanels doesn't exist here. You could put closeTabSpecificPanels in the global scope or just inline it here, considering that it's very small.

Btw, this code should come before 'this.delayedStartupFinished = true;'

Comment 39

[Katie Broida [:ktbee]]

Comment on attachment 8770177 [details]
Bug 1171746 - ensure tab specific panels close when you switch the tab

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63714/diff/4-5/

Comment 40

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

Comment on attachment 8770177 [details]
Bug 1171746 - ensure tab specific panels close when you switch the tab

https://reviewboard.mozilla.org/r/63714/#review68148

r=me with the following changes

::: browser/base/content/test/tabPrompts/browser_closeTabSpecificPanels.js:10
(Diff revision 5)
> +add_task(function*() {
> +
> +let tab1 = gBrowser.addTab("http://mochi.test:8888/#0");

nit, remove this blank line at the beginning of the function and indent the body of the test by two spaces.

::: browser/base/content/test/tabPrompts/browser_closeTabSpecificPanels.js:38
(Diff revision 5)
> +
> +gBrowser.tabContainer.advanceSelectedTab(-1, true);
> +is(specificPanel.state, "closed", "specificPanel panel is closed after its tab loses focus");
> +is(generalPanel.state, "open", "generalPanel is still open after tab switch");
> +
> +generalPanel.hidePopup();

You can get rid of the hidePopup() call here and just do the following (note you should remove both panels):

specificPanel.remove();
generalPanel.remove();

::: browser/base/content/utilityOverlay.js:494
(Diff revision 5)
> +  let keysMatch;
> +
> +  if (aEvent.key == keyPressed) {
> +    keysMatch = true;
> +  } else {
> +    return false;
> +  }

Remove the keysMatch variable and just return false here if aEvent.key != keyPressed.

Then at the end of the function just return true.

::: browser/base/content/utilityOverlay.js:512
(Diff revision 5)
> +  if (keyModifiers) {
> +    keyModifiers = keyModifiers.split(/[\s,]+/);
> +    // Capitalize first letter of aKey's modifers to compare to aEvent's modifier
> +    keyModifiers.forEach(function(modifier, index) {
> +      if (modifier == "accel") {
> +        AppConstants.platform == "macosx" ? keyModifiers[index] = "Meta" : keyModifiers[index] = "Control";

This should be written differently.

keyModifiers[index] = AppConstants.platform == "macosx" ? "Meta" : "Control";

::: browser/components/customizableui/CustomizableUI.jsm:2318
(Diff revision 5)
>        currentArea: null,
>        removable: true,
>        overflows: true,
>        defaultArea: null,
>        shortcutId: null,
> +      tabspecific: false,

Here you use all lowercase "tabspecific" but other places in this file you use camelCase "tabSpecific", I think this one just needs to be fixed.

Comment 41

[Kris Maglione [:kmag]]

Can you also please file a follow-up bug in Toolkit :: WebExtensions: Frontend to add extension-specific tests for this?

Thanks.

Comment 42

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

Marking dev-doc-needed. Panels that have the "tabspecific=true" attribute will close themselves when the selected tab changes.

Comment 43

[Katie Broida [:ktbee]]

Comment on attachment 8770177 [details]
Bug 1171746 - ensure tab specific panels close when you switch the tab

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63714/diff/5-6/

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/33fcf83eefe4
ensure tab specific panels close when you switch the tab r=jaws

Comment 45

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for browser chrome failures like https://treeherder.mozilla.org/logviewer.html#?job_id=1869564&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/b4d5fa60f8e4

Comment 46

[Katie Broida [:ktbee]]

Thank you for letting me know about these failing tests, Wes. Taking a deeper look at this, I found that the keypress event listener I added to browser_utilityOverlay.js was never removed and so was affecting later tests that involved pressing keys. I've removed my listener and have found that my patch's tests are now passing:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=57f0fc1c9057

I'll submit this update for review shortly.

Comment 47

[Katie Broida [:ktbee]]

Attachment: bug1171746.patch

I had some trouble submitting this to MozReview due to the "Parent review request [being] submitted or discarded". Let me know if you have any questions on this or if I should submitted this differently.

Comment 48

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

Comment on attachment 8782549 [details] [diff] [review]
bug1171746.patch

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

Thanks for pointing out what changed between mozreview and this patch. r=me

::: browser/base/content/test/general/browser_utilityOverlay.js
@@ +66,5 @@
> +  EventUtils.synthesizeKey("VK_DELETE", {accelKey: true});
> +  is(eventMatchResult, false, "eventMatchesKey: mismatch modifiers");
> +  keyset.removeChild(key);
> +
> +  document.removeEventListener("keypress", checkEvent);

Please do like the following:

document.addEventListener("keypress", checkEvent);
try {
  ...
} finally {
  // Make sure to remove the event listener so future tests don't
  // fail when they simulate key presses.
  document.removeEventListener("keypress, checkEvent);
}

If you do it this way we can guarantee that the event listener will be removed even if an exception is thrown after adding the event listener.

Comment 49

[Katie Broida [:ktbee]]

Attachment: Ensures event listener is removed from test when finished.

Comment 50

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/947400884306
Ensure tab-specific panels close when you switch the tab. r=jaws

Comment 51

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/947400884306

Comment 52

[Tanvir Rahman]

I have reproduced this bug with Nightly 41.0a1(2015-06-04) on Windows 10, 64 bit.

The Bug's fix is now verified on latest Nightly 51.0a1

Build ID 	20160820030224
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0

[bugday-20160817]

Comment 53

[Erik Vold]

(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #41)
> Can you also please file a follow-up bug in Toolkit :: WebExtensions:
> Frontend to add extension-specific tests for this?
> 
> Thanks.

did this ever happen?