Various menu items should be disabled when Tab Groups/Panorama is shown

RESOLVED WONTFIX

Status

defect
P2
normal
RESOLVED WONTFIX
8 years ago
3 years ago

People

(Reporter: heycam, Unassigned)

Tracking

({ux-consistency, ux-mode-error})

Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 -)

Details

Attachments

(1 attachment, 6 obsolete attachments)

This probably applies (partially or completely) to other platforms too, but there are various items in the menu that IMO should be disabled when the focused window is a browser window with Tab Groups/Panorama showing:

  File > Open Location
  File > Open File (or it should remain enabled, but close Tab Groups and open
    a new tab when you choose the file to open)
  File > Save Page As
  File > Send Link
  File > Page Setup
  File > Print
  Edit > Select All (when an edit box isn't focused; this causes all of the
    text on the Tab Groups page to be selected)
  View > Toolbars
  View > Sidebar
  View > Reload
  View > Zoom
  View > Page Style
  View > Character Encoding
  View > Page Source
  History > Back
  History > Forward
  Bookmarks > Bookmark this Tab
  Tools > Web Search (though maybe this should exit Tab Groups and then focus
    the Search box)
  Tools > Page Info
  Help > Report Web Forgery
I think this issue should be handled under TabCandy.
Component: Menus → TabCandy
QA Contact: menus → tabcandy
Hardware: x86 → All
Version: unspecified → Trunk

Comment 2

8 years ago
Hopefully will get fixed by bug 587276, but this is a great list.
Blocks: 585689
Depends on: 587276
Priority: -- → P2

Updated

8 years ago
Duplicate of this bug: 621029
Ideally we can get this by beta 11.
Blocks: 627096
No longer blocks: 585689
Assignee: nobody → tim.taubert
Status: NEW → ASSIGNED
OS: Mac OS X → All
Taking the baton...
Assignee: tim.taubert → mitcho
Blocks: 587276
No longer depends on: 587276
Posted patch Patch v1.1, WIP (obsolete) — Splinter Review
Dāo, can you comment on the general approach here? Should we be writing a controller instead?
Attachment #505961 - Attachment is obsolete: true
Attachment #506070 - Flags: feedback?(dao)
Comment on attachment 506070 [details] [diff] [review]
Patch v1.1, WIP

>+    cmds.forEach(function (cmd) goSetCommandEnabled(cmd, !disabled));

This looks like it will enable commands even if they would normally be disabled.
Attachment #506070 - Flags: feedback?(dao) → feedback-
(In reply to comment #8)
> Comment on attachment 506070 [details] [diff] [review]
> Patch v1.1, WIP
> 
> >+    cmds.forEach(function (cmd) goSetCommandEnabled(cmd, !disabled));
> 
> This looks like it will enable commands even if they would normally be
> disabled.

Indeed, that is a concern. How would you recommend resolving this? Could you point me to someplace in the codebase that currently does the same kind of thing? I would really appreciate it as I am less familiar with the XUL world.
I guess we could store the disabled state of those commands and items before disabling them (entering Panorama).   When we exit Panorama UI, we enable the appropriate ones based on the previously stored values.
(In reply to comment #10)
> I guess we could store the disabled state of those commands and items before
> disabling them (entering Panorama).   When we exit Panorama UI, we enable the
> appropriate ones based on the previously stored values.

Dāo, could you comment on this approach? It feels kind of brute-force-y to me, but would probably work. :/
Nominating for blocking. Choosing menu items which should be unavailable can result in untested, unsupported behavior and errors.
blocking2.0: --- → ?
(In reply to comment #11)
> (In reply to comment #10)
> > I guess we could store the disabled state of those commands and items before
> > disabling them (entering Panorama).   When we exit Panorama UI, we enable the
> > appropriate ones based on the previously stored values.
> 
> Dāo, could you comment on this approach? It feels kind of brute-force-y to me,
> but would probably work. :/

This doesn't sound sane. Command states could have changed, especially if panorama switches tabs, but even if it doesn't.

Comment 14

8 years ago
This sounds like this needs a beta? Is there a resolution on how this should work? Can we triage it in/out?
Spoke to gavin last night and NeilAway now... here's the plan:

1. create a broadcaster for panoramaDisabled or somesuch.
2. for commands which observe something, when Panorama is shown, remove that observes and replace it to make it observe panoramaDisabled.
3. for commands which aren't observing anything, add the panoramaDisabled observer.
4. for any keys or menuitems which don't use a command, make it use a command instead.

Gavin, Dāo, Neil, please let us know what you think of this plan, or if I got anything wrong.
(In reply to comment #15)
> Spoke to gavin last night and NeilAway now... here's the plan:
> 
> 1. create a broadcaster for panoramaDisabled or somesuch.
> 2. for commands which observe something, when Panorama is shown, remove that
> observes and replace it to make it observe panoramaDisabled.
> 3. for commands which aren't observing anything, add the panoramaDisabled
> observer.
> 4. for any keys or menuitems which don't use a command, make it use a command
> instead.
> 
> Gavin, Dāo, Neil, please let us know what you think of this plan, or if I got
> anything wrong.

I agree with point 4. But in order to correctly lock and restore the disabled state, you need to do the following:
1. Save off the command/observes attribute for the menuitems/keys that you need to disable
2. Disable them
Then when exiting Tab Candy,
3. Remove the disabled attribute
4. Restore the command/observes attribute (which will restore the disabled attribute where necessary.)
Not a blocker, no.
blocking2.0: ? → -
Posted patch Patch v1.2, WIP (obsolete) — Splinter Review
I'd hoped to make more progress on this before submitting for feedback, but I've come down with a cold and haven't been able to focus on anything all day.

Known issues:
- Select All is often available in Panorama, even though it shouldn't be
- GoBack and GoForward buttons don't revert state correctly
- not tested on non-Mac platforms

Commands and items which are controlled via observer are handled as we agreed upon. I haven't yet modified many (just one, if I recall) of the other commands and items to use observers.

Neil, Dāo, Ian, I would appreciate any quick feedback you may have on the approach.
Attachment #506070 - Attachment is obsolete: true
Attachment #508960 - Flags: feedback?(neil)
Attachment #508960 - Flags: feedback?(ian)
Attachment #508960 - Flags: feedback?(dao)
Comment on attachment 508960 [details] [diff] [review]
Patch v1.2, WIP

>-    goSetCommandEnabled("Browser:BookmarkAllTabs",
Note: goSetCommandEnabled actually works on most elements, not just <command>s.

>+          if (child.tagName != 'OBSERVES' ||
tagName is a bad idea in XML documents, since it can contain the prefix. Also tag names are only uppercased in HTML documents, so this will always fail.

>+        let observes = document.createElement('observes');
>+        observes.setAttribute('element', this._removedObserves[id].element);
>+        observes.setAttribute('attribute', 'disabled');
Looks like you forgot to add the element back to the DOM.

>+    broadcasterControlled: [
...
>+    ],
>+    direct: [
Note: <command> elements work somewhat like broadcasters, except that they use the command= attribute rather than the observes= attribute/tag. So you could implement support for that attribute and then list the affected elements under broadcasterControlled instead.
Attachment #508960 - Flags: feedback?(neil) → feedback-
Comment on attachment 508960 [details] [diff] [review]
Patch v1.2, WIP

I don't have any comments on the disabling/enabling mechanism, as the other reviewers are more familiar with that code. As for how it's called, however: 

>+  show: function TabView_show() {
>     if (this.isVisible())
>       return;
>+
>+    this._disableUnavailable();

... this is the wrong place (hide as well); it needs to go in UI.showTabView and UI.hideTabView; plenty of things trigger those routines besides TabView.show and TabView.hide.
Attachment #508960 - Flags: feedback?(ian) → feedback-
Ok, this is a big patch that touches a lot of things, and it's looking like we're not going to get it for beta 12, so I'm going to go ahead and punt it. We definitely want to get bug 587276 in, though.
No longer blocks: 587276, 627096
Depends on: 587276
Target Milestone: --- → Future
Blocks: 640592
Blocks: 640594
Assignee: mitcho → nobody

Updated

8 years ago
Blocks: 603789
Target Milestone: Future → ---
Blocks: 640602
No longer blocks: 603789
Assignee: nobody → raymond
Posted patch v2 (obsolete) — Splinter Review
Now, the elements with command= attribute and the observes= attribute/tag are in the broadcasterControlled object.  Also, removed the whitelist introduced in bug 587276 because we don't need that anymore.
Attachment #508960 - Attachment is obsolete: true
Attachment #530594 - Flags: feedback?(tim.taubert)
Attachment #530594 - Flags: feedback?(neil)
Attachment #508960 - Flags: feedback?(dao)
Comment on attachment 530594 [details] [diff] [review]
v2

>+      if (element.hasAttribute("observes")) {
>+        this._removedObserves[id] = {
>+          element: element.getAttribute("observes"),
>+          type: "observes",
>+          attr: true
>+        };
>+        element.removeAttribute("command");
Presumably this should say "observes"

>+          if (child.nodes != "observes" ||
Not sure what .nodes is supposed to be, maybe you want .localName

>+      let wasDisabled = element.hasAttribute("disabled") ?
>+                          element.getAttribute("disabled") : false;
element.getAttribute("disabled") suffices here, since if it is not set it will fail to compare equal to "true" later on anyway.

>+      if (!element)
>+        return;
How likely is this?

>+      if (this._removedObserves[id]["attr"]) {
>+        if (this._removedObserves[id]["observes"])
You can write this as this._removedObserves[id].attr and this._removedObserves[id].observes respectively.

>+      if (!element || !element.hasAttribute("wasDisabled"))
>+        return;
Is this useful?
Attachment #530594 - Flags: feedback?(neil) → feedback+
Posted patch v3 (obsolete) — Splinter Review
(In reply to comment #23)
> Comment on attachment 530594 [details] [diff] [review] [review]
> v2
> 
> >+      if (element.hasAttribute("observes")) {
> >+        this._removedObserves[id] = {
> >+          element: element.getAttribute("observes"),
> >+          type: "observes",
> >+          attr: true
> >+        };
> >+        element.removeAttribute("command");
> Presumably this should say "observes"

Fixed

> 
> >+          if (child.nodes != "observes" ||
> Not sure what .nodes is supposed to be, maybe you want .localName

Fixed

> 
> >+      let wasDisabled = element.hasAttribute("disabled") ?
> >+                          element.getAttribute("disabled") : false;
> element.getAttribute("disabled") suffices here, since if it is not set it
> will fail to compare equal to "true" later on anyway.

Yes, you are right.  Fixed.

> 
> >+      if (!element)
> >+        return;
> How likely is this?

Some elements only exist in some platforms so we need this

> 
> >+      if (this._removedObserves[id]["attr"]) {
> >+        if (this._removedObserves[id]["observes"])
> You can write this as this._removedObserves[id].attr and
> this._removedObserves[id].observes respectively.

Updated
> 
> >+      if (!element || !element.hasAttribute("wasDisabled"))
> >+        return;
> Is this useful?

Some elements only exist in some platforms so we need this.  Removed !element.hasAttribute("wasDisabled").


Pushed to try.  Waiting for the results
http://tbpl.mozilla.org/?tree=Try&rev=071959798806
Attachment #530594 - Attachment is obsolete: true
Attachment #530982 - Flags: review?(ian)
Attachment #530594 - Flags: feedback?(tim.taubert)
Comment on attachment 530982 [details] [diff] [review]
v3

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

Sorry for the late feedback. The patch looks good with the one nit fixed.

One thing: I wondered about the blacklist approach we're doing here. Isn't it much easier to just create a whitelist of all allowed commands/keys and just handle every command/key that isn't listed there? This would disable commands and keys from extensions, too (which might be good and bad). Also it seems unnecessary to divide everything into "direct" and "broadcasterControlled" because we can detect the type via checking if attributes "command" or "observes" exist?

::: browser/base/content/browser-tabview.js
@@ +463,5 @@
> +  // Enables search.
> +  enableSearch: function TabView_enableSearch() {
> +    if (this._window)
> +      this._window.UI.enableSearch();
> +  },

We should enable search only when Panorama is really shown. I know you're actually checking in the command but this is kind of an internal API and we should make sure it doesn't break here.
(In reply to comment #24)
> (In reply to comment #23)
> > (From update of attachment 530594 [details] [diff] [review])
> > >+      if (!element)
> > >+        return;
> > How likely is this?
> Some elements only exist in some platforms so we need this
Then maybe "continue;" would be better?
Comment on attachment 530982 [details] [diff] [review]
v3

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

This is outside my realm of expertise. Please have one of the browser peers review it once the feedback cycle has completed.
Attachment #530982 - Flags: review?(ian)
Neil:  what do you think about Tim's whitelist suggestion in comment 25?
How do you get a list of all menuitems/keys/commands that aren't whitelisted?
(In reply to comment #29)
> How do you get a list of all menuitems/keys/commands that aren't whitelisted?

querySelectorAll can get you the menu items. You don't need the non-whitelisted keys if you capture all key events and stop them unless they're on the whitelist.
By the way, now that the Fx4 pressure is off, maybe we should look at how to solve this systemically? What would have to change in the browser to make this sort of "we're in a special mode, so disable a number of menus and keys" scenario straight-forward?
Duplicate of this bug: 656013
(In reply to comment #31)
> By the way, now that the Fx4 pressure is off, maybe we should look at how to
> solve this systemically? What would have to change in the browser to make
> this sort of "we're in a special mode, so disable a number of menus and
> keys" scenario straight-forward?

That sounds like a good idea. If it's so hard for us to disable incompatible menu items we should maybe look for a better solution. Now that the <deck> element is introduced it's an integration point for add-ons, too. So probably they will suffer the same problems as we do now.
(In reply to comment #33)
> (In reply to comment #31)
> > By the way, now that the Fx4 pressure is off, maybe we should look at how to
> > solve this systemically? What would have to change in the browser to make
> > this sort of "we're in a special mode, so disable a number of menus and
> > keys" scenario straight-forward?
> 
> That sounds like a good idea. If it's so hard for us to disable incompatible
> menu items we should maybe look for a better solution. Now that the <deck>
> element is introduced it's an integration point for add-ons, too. So
> probably they will suffer the same problems as we do now.

Dao: do you have any recommendations how we tackle this?
I have no concrete recommendation, but:
- I wouldn't advise extensions to use the deck
- a safe approach would be based on a whitelist (and that you're removing the keys whitelist seems like a step backwards)
Posted patch v4 (obsolete) — Splinter Review
Uses the whitelist approach for menu items and keeps the keys whitelist in UI.js
Attachment #530982 - Attachment is obsolete: true
Attachment #534430 - Flags: feedback?(tim.taubert)
(In reply to comment #36)
> Created attachment 534430 [details] [diff] [review] [review]
> v4
> 
> Uses the whitelist approach for menu items and keeps the keys whitelist in
> UI.js

I was working on bug 656488 and I see there is a problem in the current implementation of key events whitelist in _setTabViewFrameKeyHandlers() in UI.js/ (I mean in the m-c).   "Hide Others" menu item for Mac has alt+cmd+H key combination, however, we could not be able to detect it in the current implementation as alt+h would have different charCode than h.  The only solution I see is to replace the current implementation with a whitelisted <key> elements approach (disable key elements which are not in the whitelist).

What do you think?
Alt (event.altKey) should always go through, afaik.
(In reply to comment #38)
> Alt (event.altKey) should always go through, afaik.

Yes, Alt always go through. However, the ctrl + alt + key (e.g h) combination has different charCode compared to ctrl + key combination.  Therefore, we can't tell that the "h" key is pressed.
Why do you care about the h when anything involving Alt should go through?
(In reply to comment #40)
> Why do you care about the h when anything involving Alt should go through?

Ah, I misunderstood it. I am going to update the patch for bug 656488 to allow Alt + any key to go through.
bugspam
No longer blocks: 653099
Comment on attachment 534430 [details] [diff] [review]
v4

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

The approach looks good but there are some issues that should be fixed.

::: browser/base/content/browser-tabview.js
@@ +48,5 @@
>    _browserKeyHandlerInitialized: false,
>    _isFrameLoading: false,
>    _initFrameCallbacks: [],
> +  _removedObserves: {},
> +  _whitelistEements: [

Typo -> "_whitelistElements"

@@ +430,5 @@
> +  },
> +
> +  // ----------
> +  // Disabled the non-whitelisted elements on menu bar.
> +  disableNonWhitelistedElements: function TabView_disableUnavailable() {

Please update the function name.

@@ +444,5 @@
> +      if (element.id &&
> +          self._whitelistEements.indexOf(element.id) > -1)
> +        continue;
> +
> +      let found = false;

This variable is unused.

@@ +449,5 @@
> +      if (element.hasAttribute("observes")) {
> +        if (!element.id)
> +          element.setAttribute("id", "temp_" + i);
> +
> +        this._removedObserves[element.id] = {

I think _removedObserves should be an array and the element itself should be added to the object that is pushed. So we avoid temporary IDs as they seem a bit hacky. (They're not even temporary as they don't get removed afterwards)

@@ +450,5 @@
> +        if (!element.id)
> +          element.setAttribute("id", "temp_" + i);
> +
> +        this._removedObserves[element.id] = {
> +          element: element.getAttribute("observes"),

We should rename ".element" because we need it for the element and because it actually stores "observe", "command" and "element" attribute values.

@@ +465,5 @@
> +
> +        if (!element.id)
> +          element.setAttribute("id", "temp_" + i);
> +
> +        this._removedObserves[element.id] = {

(see above)

@@ +483,5 @@
> +
> +            if (!element.id)
> +              element.setAttribute("id", "temp_" + i);
> +
> +            this._removedObserves[element.id] = {

(see above)

@@ +486,5 @@
> +
> +            this._removedObserves[element.id] = {
> +              observesElement: child,
> +              element: child.getAttribute("element"),
> +              attr: false

Why not remove ".attr" and set type="child" for this (or something like that)?

@@ +489,5 @@
> +              element: child.getAttribute("element"),
> +              attr: false
> +            };
> +            child.removeAttribute("element");
> +            found = true;

(see above)

@@ +502,5 @@
> +  },
> +
> +  // ---------
> +  // Enables the non-whitelisted elements on menu bar.
> +  enableWhitelistedElements: function TabView_enableUnavailable() {

Please update the function name.

@@ +512,5 @@
> +        return;
> +
> +      element.removeAttribute("disabled");
> +      if (this._removedObserves[id].attr) {
> +        if (this._removedObserves[id].observes)

This property should not exist. If your test didn't catch this please include an element with the "observes" (and "command") attribute.

You should just check for (type == "observes") here as we should now have all three types. That could be three if-branches or a switch-case statement.

::: browser/base/content/test/tabview/browser_tabview_bug621795.js
@@ +3,5 @@
> +
> +function test() {
> +  waitForExplicitFinish();
> +
> +  // pick few items for testing.

Please make sure to pick items of all three types ("observes", "command" and "child").
Attachment #534430 - Flags: feedback?(tim.taubert) → feedback-
Posted patch v5Splinter Review
(In reply to comment #44)
> Comment on attachment 534430 [details] [diff] [review] [review]
> v4
> 
> Review of attachment 534430 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> The approach looks good but there are some issues that should be fixed.
> 
> ::: browser/base/content/browser-tabview.js
> @@ +48,5 @@
> >    _browserKeyHandlerInitialized: false,
> >    _isFrameLoading: false,
> >    _initFrameCallbacks: [],
> > +  _removedObserves: {},
> > +  _whitelistEements: [
> 
> Typo -> "_whitelistElements"

Updated

> 
> @@ +430,5 @@
> > +  },
> > +
> > +  // ----------
> > +  // Disabled the non-whitelisted elements on menu bar.
> > +  disableNonWhitelistedElements: function TabView_disableUnavailable() {
> 
> Please update the function name.
> 

Updated

> @@ +444,5 @@
> > +      if (element.id &&
> > +          self._whitelistEements.indexOf(element.id) > -1)
> > +        continue;
> > +
> > +      let found = false;
> 
> This variable is unused.

Removed

> 
> @@ +449,5 @@
> > +      if (element.hasAttribute("observes")) {
> > +        if (!element.id)
> > +          element.setAttribute("id", "temp_" + i);
> > +
> > +        this._removedObserves[element.id] = {
> 
> I think _removedObserves should be an array and the element itself should be
> added to the object that is pushed. So we avoid temporary IDs as they seem a
> bit hacky. (They're not even temporary as they don't get removed afterwards)

Done

> 
> @@ +450,5 @@
> > +        if (!element.id)
> > +          element.setAttribute("id", "temp_" + i);
> > +
> > +        this._removedObserves[element.id] = {
> > +          element: element.getAttribute("observes"),
> 
> We should rename ".element" because we need it for the element and because
> it actually stores "observe", "command" and "element" attribute values.
> 
> @@ +465,5 @@
> > +
> > +        if (!element.id)
> > +          element.setAttribute("id", "temp_" + i);
> > +
> > +        this._removedObserves[element.id] = {
> 
> (see above)
> 
> @@ +483,5 @@
> > +
> > +            if (!element.id)
> > +              element.setAttribute("id", "temp_" + i);
> > +
> > +            this._removedObserves[element.id] = {
> 
> (see above)
> 
> @@ +486,5 @@
> > +
> > +            this._removedObserves[element.id] = {
> > +              observesElement: child,
> > +              element: child.getAttribute("element"),
> > +              attr: false
> 
> Why not remove ".attr" and set type="child" for this (or something like
> that)?
> 
> @@ +489,5 @@
> > +              element: child.getAttribute("element"),
> > +              attr: false
> > +            };
> > +            child.removeAttribute("element");
> > +            found = true;
> 
> (see above)
> 
> @@ +502,5 @@
> > +  },
> > +
> > +  // ---------
> > +  // Enables the non-whitelisted elements on menu bar.
> > +  enableWhitelistedElements: function TabView_enableUnavailable() {
> 
> Please update the function name.

Updated

> 
> @@ +512,5 @@
> > +        return;
> > +
> > +      element.removeAttribute("disabled");
> > +      if (this._removedObserves[id].attr) {
> > +        if (this._removedObserves[id].observes)
> 
> This property should not exist. If your test didn't catch this please
> include an element with the "observes" (and "command") attribute.
> 
> You should just check for (type == "observes") here as we should now have
> all three types. That could be three if-branches or a switch-case statement.
> 

Done

> ::: browser/base/content/test/tabview/browser_tabview_bug621795.js
> @@ +3,5 @@
> > +
> > +function test() {
> > +  waitForExplicitFinish();
> > +
> > +  // pick few items for testing.
> 
> Please make sure to pick items of all three types ("observes", "command" and
> "child").

Picked "observes", "command" and element with "oncommand".  However, there isn't menu item with observes element so didn't add a test but we still need the code as extensions may add such elements to the XUL overlay
Attachment #534430 - Attachment is obsolete: true
Attachment #537965 - Flags: feedback?(tim.taubert)
@Tim: could you feedback on the patch please?
Comment on attachment 537965 [details] [diff] [review]
v5

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

Sorry, that it took me so long :/ Looks good, thanks!
Attachment #537965 - Flags: feedback?(tim.taubert) → feedback+
Attachment #537965 - Flags: review?(dao)
Attachment #537965 - Flags: review?(ehsan)

Comment 49

8 years ago
Comment on attachment 537965 [details] [diff] [review]
v5

I'd be much more comfortable if a browser peer would review this.  Dao is probably the right person.  If you need more reviews here, you should talk to Gavin I think.
Attachment #537965 - Flags: review?(ehsan)
Blocks: 671809
bugspam

(Fx7 was branched, removing open bugs from Fx7 meta bug, we won't create new meta bugs for upcoming Fx versions)
No longer blocks: 660175
Do we still want this?
Status: ASSIGNED → NEW
Assignee: raymond → nobody
Comment on attachment 537965 [details] [diff] [review]
v5

>     <command id="cmd_find"
>-             oncommand="gFindBar.onFindCommand();"
>+             oncommand="if (TabView.isVisible()) TabView.enableSearch(); else gFindBar.onFindCommand();"
>              observes="isImage"/>

> function onViewToolbarsPopupShowing(aEvent, aInsertPoint) {
>   var popup = aEvent.target;
>-  if (popup != aEvent.currentTarget)
>+  if (popup != aEvent.currentTarget || TabView.isVisible())
>     return;

>@@ -347,16 +347,17 @@ PlacesViewBase.prototype = {
>         element.className = "menu-iconic bookmark-item";
> 
>         aPlacesNode._DOMElement = popup;
>       }
>       else
>         throw "Unexpected node";
> 
>       element.setAttribute("label", PlacesUIUtils.getBestTitle(aPlacesNode));
>+      element.setAttribute("observes", "tabviewState");

I don't think we want to litter non-tabview code with stuff like that.
Attachment #537965 - Flags: review?(dao) → review-
Panorama has been removed from Firefox 45, currently in Beta and scheduled for release on March 7th. As such, I'm closing all existing Panorama bugs.

If you are still using Panorama, you will see a deprecation message in Firefox 44, and when 45 is released your tab group data will be migrated to bookmarks, with a folder for each group. There are also a few addons offering similar functionality.

See https://support.mozilla.org/en-US/kb/tab-groups-removal for more info.

We're removing Panorama because it has extremely low usage (about 0.01% of users), and has a large number of bugs and usability issues. The cost of fixing all those issues is far too high to justify, and so we'll instead be focusing our time and energy on improving other parts of Firefox.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WONTFIX
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.