Closed Bug 456088 Opened 12 years ago Closed 12 years ago

Ctrl+Tab / All Tabs revision

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 3.1b2

People

(Reporter: dao, Assigned: dao)

References

(Depends on 1 open bug)

Details

(Keywords: perf, ue, verified1.9.1, Whiteboard: [icon-3.1])

Attachments

(1 file, 6 obsolete files)

Attached patch patch (obsolete) — Splinter Review
This is a rather small update for All Tabs but a major one for Ctrl+Tab, removing the scrolling and adding pagination. Performance is improved by using the canvas element directly instead of toDataURL and <xul:image/>.
Attachment #339497 - Flags: review?(mconnor)
(In reply to comment #0)
> Performance is improved by using
> the canvas element directly instead of toDataURL and <xul:image/>.

This also fixes bug 447896.
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #339497 - Attachment is obsolete: true
Attachment #341778 - Flags: review?(mconnor)
Attachment #339497 - Flags: review?(mconnor)
Flags: in-litmus?
Attached patch patch v3 (obsolete) — Splinter Review
synced with bug 436304
Attachment #341778 - Attachment is obsolete: true
Attachment #342064 - Flags: review?(gavin.sharp)
Attachment #341778 - Flags: review?(mconnor)
Flags: blocking-firefox3.1+
Target Milestone: --- → Firefox 3.1b2
A few comments WRT the latest try-server build (on WinXP):

* This is much better than what we've previously had! I was even delighted to find that clicking on the page markers worked as expected.

* The page title remaining fixed at the top of the panel makes it harder to follow both the previews _and_ their titles, as they don't remain at a fixed distance.

* Holding Ctrl and then pressing Tab and Esc doesn't dismiss the panel and leaves no obvious way to do so (less obvious are clicking on a preview or Alt+Tabbing to Minefield and then hitting Ctrl+Tab again, but technically both of these aren't "dismissing", or focusing Minefield and hitting Esc again). I've seen a Mac-specific bug to that end. Have I missed the Windows one?

* The previews aren't updated until the panel is already visible (cf. bug 436304 comment #41), neither is the title. Visual noise...

* The currently selected tab's URL is displayed in the status bar behind the panel. IMO that's unneeded distraction, as the status bar is definitively not where you're looking and the change is perceived nonetheless.
(In reply to comment #4)
> * Holding Ctrl and then pressing Tab and Esc doesn't dismiss the panel

This is because bug 451300 added noauthide="true", because of bug 457997.
Attachment #342064 - Flags: review?(gavin.sharp) → review?(mconnor)
I did a test for this feature with the given try server build on bug 436304 and have to say it looks great! Thanks Dao. But further I've also noticed following issues (at least on OS X):

* The outline of the selected tab is not blurred. I can only see this partly applied on the first line. It has holes inside and the 2nd and 3rd line misses it at all.

* Hitting Ctrl+Tab really fast shouldn't show the dialog. I think to remember we already had this topic on another bug. Probably the one which handled the initial implementation.
Attached image broken outline (obsolete) —
This shows the broken outline on the first line.

In the "All tabs" panel we have a focus rect around the currently selected tab. Shouldn't it be also used for Ctrl-Tab?
(In reply to comment #7)
> Created an attachment (id=342473) [details]
> broken outline
> 
> This shows the broken outline on the first line.

Probably worth a separate bug. Core:GFX I would guess.

> In the "All tabs" panel we have a focus rect around the currently selected tab.
> Shouldn't it be also used for Ctrl-Tab?

The "All Tabs" panel highlights both the hovered and the focused tab, thus the extra rect in order to differentiate between them.
Attached patch patch v3 (obsolete) — Splinter Review
synced with bug 436304
Attachment #342064 - Attachment is obsolete: true
Attachment #342880 - Flags: review?(mconnor)
Attachment #342064 - Flags: review?(mconnor)
Attached patch patch v3 (obsolete) — Splinter Review
updated to trunk
Attachment #342880 - Attachment is obsolete: true
Attachment #343232 - Flags: review?(mconnor)
Attachment #342880 - Flags: review?(mconnor)
Depends on: 460240
Blocks: 436304
No longer depends on: 436304
Attachment #342473 - Attachment is obsolete: true
Attachment #343232 - Attachment is obsolete: true
Attachment #345201 - Flags: review?(mconnor)
Attachment #343232 - Flags: review?(mconnor)
Attachment #345201 - Attachment description: patch → patch (ctrl-tab only)
Blocks: 447896
Mconnor, could you please give a status update for this bug? The freeze is coming soon and QA needs some time to run verification tests on this feature. Even none of the patches were reviewed yet. Doing so will probably cause remaining work which has to be done asap.

Thanks!
Whiteboard: [needs status update]
Blocks: 449929, 450138
Blocks: 445495
Blocks: 445854
Comment on attachment 345201 [details] [diff] [review]
patch (ctrl-tab only)

looks good overall.  Could use a bit more spacing and comments throughout, but code looked pretty straightforward.

still want to test this hands on, and get a ui-r from beltzner before landing this and the all tabs patch today.  tryserver builds are in progress though.
Attachment #345201 - Flags: review?(mconnor) → review+
Whiteboard: [needs status update] → [needs status update][icon-3.1]
http://hg.mozilla.org/mozilla-central/rev/9dcff436bd1b
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Keywords: ue
Resolution: --- → FIXED
Whiteboard: [needs status update][icon-3.1] → [icon-3.1]
I emailed a few notes, I'll put them here too:

I'm running the latest build from Bug 436304 and Bug 456088 and see a few problems.  Here's the three biggest ones:

1. Control Tab and All Tabs are still separate items.  I believe mconnor made the call that we were creating only one preview mode.  Since both now have 12 tabs, the I think the distinction between them makes even less sense now.  The design that is posed to land is one Control+Tab with a search bar and pagination and 12 previews.

2. When the preview panes appear and disappear is broken.  Right now Control+Tab sometimes disappears when you let go of the keys, and sometimes not.  All tabs stays until a selection is made.  Here is my recommendation, assuming only one Control+Tab panel:

- If Control+Tab is called via the chrome button, it is permanent until a selection is made or the user clicks away from the screen.  

- If the user calls the window with Control+Tab, it is temporary and is dismissed if the keys are released.  This is true unless the user begins typing or moves the cursor over the window, in which case the window becomes permanent unless a selection is made or the user clicks off the screen.

3. If the user presses Control+Tab and then Tabs to a preview and releases, Control+Tab should go to that window and be dismissed.  Right now, often nothing happens if the user tabs over.
So are we going to merge with Ctrl+Tab and lose All Tabs widget?
Many guys use it to survey their tabs, and they are in the same order they positioned on a tabbar strip.

Visualization of tabs brought by new Ctrl+Tab behavior is good, but it messes up the order of tabs you still have on said tab strip.

I'm [was] using keyconfig to invoke 'List All Tabs' widgets with keyboard shortcut, and for one would not like to loose this ability to have a simple flat list of tabs to observe.

Very difference of Ctrl+Tab & 'List ALL Tabs' entity suggest that we keep them separate & with distinct workings.

Yes there's 80% of the market to take, but not at the cost of embarrassing your loyal 20% home base.
Re Comment 16 -

The difference I was referring to between Control+Tab and All Tabs is the ordering of the tabs: tab order vs. MRU.  Since both now order tabs in the same way, there's no longer a distinction between them and they are instead two panels of the same feature.  So this at least is resolved.

Re having order - bug 463569 has been resolved.
This has been landed a while back before it was removed from 1.9.1 branch. Marking as verified1.9.1.
Status: RESOLVED → VERIFIED
Marcia, how can we solve the litmus flag? Which tests you want to add?
You need to log in before you can comment on or make changes to this bug.