TabView should set its visibility with tab-view-deck.selectedPanel rather than selectedIndex

RESOLVED FIXED in Firefox 6

Status

Firefox Graveyard
Panorama
P3
normal
RESOLVED FIXED
7 years ago
a year ago

People

(Reporter: kmag, Assigned: raymondlee)

Tracking

4.0 Branch
Firefox 6

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

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

Updated

7 years ago
Target Milestone: --- → Future

Comment 1

7 years ago
Most of our API work for Tab Candy is being schedule for post ff-4, fyi,.
OS: Linux → Windows CE
Priority: -- → P3
(Reporter)

Comment 2

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

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

Comment 4

7 years ago
Created attachment 490021 [details] [diff] [review]
Patch
(Assignee)

Updated

7 years ago
Attachment #490021 - Flags: feedback?(ian)
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
Attachment #490021 - Flags: feedback?(ian) → feedback-
(Assignee)

Updated

6 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: TabView should set its visibility with tab-view-deck.selectedItem rather than selectedIndex → TabView should set its visibility with tab-view-deck.selectedPanel rather than selectedIndex
(Assignee)

Comment 6

6 years ago
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
Assignee: nobody → raymond
Attachment #490021 - Attachment is obsolete: true
Attachment #525236 - Flags: feedback?(tim.taubert)
(Assignee)

Updated

6 years ago
OS: Windows CE → All
Hardware: x86_64 → All
(Assignee)

Updated

6 years ago
Version: unspecified → 4.0 Branch
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.
Attachment #525236 - Flags: feedback?(tim.taubert) → feedback+
(Assignee)

Comment 8

6 years ago
Created attachment 525395 [details] [diff] [review]
v3

Thanks Tim.  I missed the bit in the browser-tabview.js
Attachment #525236 - Attachment is obsolete: true
Attachment #525395 - Flags: review?(ian)
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 9

6 years ago
Comment on attachment 525395 [details] [diff] [review]
v3

Passed Try
http://tbpl.mozilla.org/?tree=MozillaTry&rev=1156b1885571
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. :)
Attachment #525395 - Flags: review?(ian) → review+
(Assignee)

Comment 11

6 years ago
Created attachment 525912 [details] [diff] [review]
Patch for checkin

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

Done
Attachment #525395 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
http://hg.mozilla.org/projects/cedar/rev/784db2f4b6ac
Keywords: checkin-needed
Whiteboard: fixed-in-cedar
Target Milestone: Future → Firefox 6
http://hg.mozilla.org/mozilla-central/rev/784db2f4b6ac
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.