Closed Bug 600665 Opened 14 years ago Closed 9 years ago

App tabs in Panorama should be focusable

Categories

(Firefox Graveyard :: Panorama, defect, P2)

defect

Tracking

(blocking2.0 -)

RESOLVED WONTFIX
Tracking Status
blocking2.0 --- -

People

(Reporter: aza, Unassigned)

References

Details

(Keywords: polish)

Attachments

(1 file, 2 obsolete files)

This is interdependent with a couple other bugs like 595374 and 596729 as they are artifacts of app tabs not being focusable (i.e., having that little glow around them).
Assignee: nobody → ian
Blocks: 597043
blocking2.0: --- → ?
Depends on: 595374, 596729
Priority: -- → P2
Target Milestone: --- → Firefox 4.0
Status: NEW → ASSIGNED
Blocks: 596729
No longer depends on: 596729
Attached patch patch v1 (obsolete) — Splinter Review
Ok, we finally need to make app tabs their own object. To facilitate the selection code, I've created an interface class, TabRef, for the objects that represent tabs in the Panorama UI. 

Note that I'm also fixing bug 596729 as part of this patch. 

Still to do: 

* Iron out some edge cases
* Add a test
Attachment #485430 - Flags: feedback?(raymond)
Comment on attachment 485430 [details] [diff] [review]
patch v1

Aza, you might see if the keyboard implementation here is pleasing to you. The arrow keys pretty much do the right thing, though one interesting side effect of the fact that we're basing it on centers is that when you go right from the right-most tab in a group, you won't necessarily end up at the top app tab on the right column... it'll be which ever app tab is closest to the center of the tab you just left.

I improved the tab key considerably.... before it just went around within a single group, but now you can tab through all tabs and app tabs in every group plus orphaned tabs. Also, the tab key is limited just to one group if that group is an expanded stack.

You may be able to apply this patch directly to your local repo... my current mq stack is rather large, but I don't know that there are any actual conflicts.
Attachment #485430 - Flags: ui-review?(aza)
Comment on attachment 485430 [details] [diff] [review]
patch v1

Looks good!
Attachment #485430 - Flags: feedback?(raymond) → feedback+
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #485430 - Attachment is obsolete: true
Attachment #485430 - Flags: ui-review?(aza)
Comment on attachment 486766 [details] [diff] [review]
Patch v2

Ok, now this patch is ready. Major changes since last patch: 

* Some API tweaks on TabRef to keep it in line with TabItem
* Now supporting the "next group" key combo for app tabs (also added support in there for orphan tabs, which was missing)
* Now has a test
* Now properly selecting an AppTab after zooming out of it
* Misc fixes

By the way, you can get an interdiff between patch v2 and patch v1 by going to the diff for patch v2 and selecting patch v1 in the drop down at the top of the page and hitting "diff"... I hadn't noticed that before, but it's pretty slick!
Attachment #486766 - Flags: feedback?(raymond)
Blocks: 595020
Blocks: 608389
Flags: in-litmus?(marcia)
Blocks: 588597
No longer blocks: 588597
Blocks: 603721
Comment on attachment 486766 [details] [diff] [review]
Patch v2

Looks good!
Attachment #486766 - Flags: feedback?(raymond) → feedback+
Comment on attachment 486766 [details] [diff] [review]
Patch v2

Dietrich, I realize this bug isn't blocking, but it blocks two blockers (bug 608389 and bug 603721), so perhaps it should be?
Attachment #486766 - Flags: review?(dietrich)
Blocks: 608407
Attachment #486766 - Flags: review?(dietrich) → review?(dolske)
Dolske, nudge. :)
Realizing my nudge to dolske would probably be more effective if he was actually cc'ed on this bug.
blocking2.0: ? → betaN+
Blocks: 598154
Beta8 has 1 bug left on it, moving our blockers to b9
No longer blocks: 597043
I think this sucks, but doesn't need to block.
blocking2.0: betaN+ → -
Keywords: polish
Whiteboard: [d?]
Blocks: 608153
Attached patch Patch v3Splinter Review
Newly unrotted patch. 

This bug has been languishing in Dolske's review queue for a couple months now... could you take a look at it, Dao?
Attachment #486766 - Attachment is obsolete: true
Attachment #501482 - Flags: review?(dao)
Attachment #486766 - Flags: review?(dolske)
No longer blocks: 595020
bugspam (moving b9 to b10)
Blocks: 608028
bugspam (removing b9)
No longer blocks: 598154
This would be great to have.
Assignee: ian → nobody
Blocks: 627096
No longer blocks: 608028
Too late for this for Fx4; it's a major patch, and it's been waiting for review for 3 months.
No longer blocks: 627096
Target Milestone: Firefox 4.0 → Future
No longer blocks: 603721
Tim, can you take this over? It's in the same boat as bug 595020. It's got a lot of goodness that we should get in, but it's also a very large patch. Take a look and see what you think.
Assignee: nobody → tim.taubert
Blocks: 603789
Target Milestone: Future → ---
No longer blocks: 603789
bugspam
No longer blocks: 653099
bugspam
Blocks: 660175
Attachment #501482 - Flags: review?(dao)
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
Blocks: 733103
Blocks: 749217
Will not work on this anytime soon.
Assignee: ttaubert → nobody
Status: ASSIGNED → NEW
Flags: in-litmus?(mozillamarcia.knous)
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.