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.

Hopefully will get fixed by bug 587276, but this is a great list.

Ideally we can get this by beta 11.

Taking the baton...

Dāo, can you comment on the general approach here? Should we be writing a controller instead?

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.

(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.

(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.

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.

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.

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.

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.

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.

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.

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?

(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

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.

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?

(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)

Uses the whitelist approach for menu items and keeps the keys whitelist in UI.js

(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

bugspam

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").

(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

@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!

Comment on attachment 537965 [details] [diff] [review] v5 Passed Try http://tbpl.mozilla.org/?tree=Try&rev=69f00b98ef07

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.

bugspam (Fx7 was branched, removing open bugs from Fx7 meta bug, we won't create new meta bugs for upcoming Fx versions)

Do we still want this?

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.

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.