Last Comment Bug 610996 - TabView should set its visibility with tab-view-deck.selectedPanel rather than selectedIndex
: TabView should set its visibility with tab-view-deck.selectedPanel rather tha...
Status: RESOLVED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: 4.0 Branch
: All All
: P3 normal
: Firefox 6
Assigned To: Raymond Lee [:raymondlee]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-11-10 07:12 PST by Kris Maglione [:kmag]
Modified: 2016-04-12 14:00 PDT (History)
3 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch (2.00 KB, patch)
2010-11-11 20:04 PST, Kris Maglione [:kmag]
ian: feedback-
Details | Diff | Splinter Review
v2 (3.73 KB, patch)
2011-04-11 17:32 PDT, Raymond Lee [:raymondlee]
ttaubert: feedback+
Details | Diff | Splinter Review
v3 (6.31 KB, patch)
2011-04-12 08:41 PDT, Raymond Lee [:raymondlee]
ian: review+
Details | Diff | Splinter Review
Patch for checkin (6.27 KB, patch)
2011-04-13 20:31 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review

Description Kris Maglione [:kmag] 2010-11-10 07:12:44 PST
User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:2.0b8pre) Gecko/20101108 Firefox/4.0b8pre
Build Identifier: Mozilla/5.0 (X11; Linux x86_64; rv:2.0b8pre) Gecko/20101108 Firefox/4.0b8pre

Currently TabView toggles its visibility by toggling the selectedIndex of the #tab-view-deck element between 0 and 1. This unfortunately makes it difficult or impossible to overlay the tab-view-deck element, because the TabView iframe is created dynamically. Therefore, if any overlay appends an item to the deck, that item is selected rather than TabView itself.

This problem would be obviated by using gTabViewDeck.selectedItem = document.getElementById("tab-view") (gTabViewFrame) instead.



Reproducible: Always

Steps to Reproduce:
1. Append an element to #tab-view-deck before Panorama is intialized
2. Toggle Panorama's visibility

The element appended in step one is visible rather than Panorama.
Comment 1 Kevin Hanes 2010-11-10 09:43:39 PST
Most of our API work for Tab Candy is being schedule for post ff-4, fyi,.
Comment 2 Kris Maglione [:kmag] 2010-11-10 10:03:21 PST
This isn't an API issue. I'm not especially interested in interacting with Panorama at all, at the moment. But the stack element is useful to extensions, and I've talked to several other authors interested in overlaying it. Doing so should not break Panorama.
Comment 3 Kevin Hanes 2010-11-11 15:18:14 PST
(In reply to comment #2)
> This isn't an API issue. I'm not especially interested in interacting with
> Panorama at all, at the moment. But the stack element is useful to extensions,
> and I've talked to several other authors interested in overlaying it. Doing so
> should not break Panorama.

Right. Sorry for the confusion. When I say API, I mean specifically how extensions interact with Panorama. 

I agree that it would be a useful and non-breaking change, but it isn't a current dev priority. That said, we'd certainly take a patch and review it :).
Comment 4 Kris Maglione [:kmag] 2010-11-11 20:04:45 PST
Created attachment 490021 [details] [diff] [review]
Patch
Comment 5 Ian Gilman [:iangilman] 2010-11-12 11:29:03 PST
Comment on attachment 490021 [details] [diff] [review]
Patch

Looks like a good start. Also need to update: 

/browser/base/content/browser-tabview.js

/browser/base/content/test/tabview/browser_tabview_launch.js
Comment 6 Raymond Lee [:raymondlee] 2011-04-11 17:32:30 PDT
Created attachment 525236 [details] [diff] [review]
v2

The is the same as the previous patch and it works fine.  I don't see the need to update browser-tabview.js and browser_tabview_launch.js for the code in trunk.

Passed Try
http://tbpl.mozilla.org/?tree=MozillaTry&rev=1b4826896422
Comment 7 Tim Taubert [:ttaubert] 2011-04-12 05:26:49 PDT
Comment on attachment 525236 [details] [diff] [review]
v2

We definitely need to update browser-tabview.js:

>  isVisible: function() {
>    return (this._deck ? this._deck.selectedIndex == 1 : false);
>  },

Panorama being index 1 in the deck can be a false assumption if some add-on modifies the deck order.

browser_tabview_launch.js seems fixed already (or was never wrong), so no need to touch this:

>  if (deck.selectedPanel == iframe) {

F+ with the fix in browser-tabview.js.
Comment 8 Raymond Lee [:raymondlee] 2011-04-12 08:41:54 PDT
Created attachment 525395 [details] [diff] [review]
v3

Thanks Tim.  I missed the bit in the browser-tabview.js
Comment 9 Raymond Lee [:raymondlee] 2011-04-12 19:42:25 PDT
Comment on attachment 525395 [details] [diff] [review]
v3

Passed Try
http://tbpl.mozilla.org/?tree=MozillaTry&rev=1156b1885571
Comment 10 Ian Gilman [:iangilman] 2011-04-13 18:05:27 PDT
Comment on attachment 525395 [details] [diff] [review]
v3

Nice cleanup!

One super-small nit:

>+var gBrowserPanel =  gWindow.document.getElementById("browser-panel");

Kill the extra space after the equal sign. :)
Comment 11 Raymond Lee [:raymondlee] 2011-04-13 20:31:02 PDT
Created attachment 525912 [details] [diff] [review]
Patch for checkin

(In reply to comment #10)
> Kill the extra space after the equal sign. :)

Done

Note You need to log in before you can comment on or make changes to this bug.