Closed
Bug 456088
Opened 16 years ago
Closed 16 years ago
Ctrl+Tab / All Tabs revision
Categories
(Firefox :: General, defect)
Firefox
General
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)
(deleted),
patch
|
mconnor
:
review+
|
Details | Diff | 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)
Assignee | ||
Comment 1•16 years ago
|
||
(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.
Assignee | ||
Comment 2•16 years ago
|
||
Attachment #339497 -
Attachment is obsolete: true
Attachment #341778 -
Flags: review?(mconnor)
Attachment #339497 -
Flags: review?(mconnor)
Updated•16 years ago
|
Flags: in-litmus?
Assignee | ||
Comment 3•16 years ago
|
||
synced with bug 436304
Attachment #341778 -
Attachment is obsolete: true
Attachment #342064 -
Flags: review?(gavin.sharp)
Attachment #341778 -
Flags: review?(mconnor)
Updated•16 years ago
|
Flags: blocking-firefox3.1+
Target Milestone: --- → Firefox 3.1b2
Comment 4•16 years ago
|
||
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.
Assignee | ||
Comment 5•16 years ago
|
||
(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.
Assignee | ||
Updated•16 years ago
|
Attachment #342064 -
Flags: review?(gavin.sharp) → review?(mconnor)
Comment 6•16 years ago
|
||
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.
Comment 7•16 years ago
|
||
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?
Assignee | ||
Comment 8•16 years ago
|
||
(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.
Assignee | ||
Comment 9•16 years ago
|
||
synced with bug 436304
Attachment #342064 -
Attachment is obsolete: true
Attachment #342880 -
Flags: review?(mconnor)
Attachment #342064 -
Flags: review?(mconnor)
Assignee | ||
Comment 10•16 years ago
|
||
updated to trunk
Attachment #342880 -
Attachment is obsolete: true
Attachment #343232 -
Flags: review?(mconnor)
Attachment #342880 -
Flags: review?(mconnor)
Assignee | ||
Updated•16 years ago
|
Assignee | ||
Updated•16 years ago
|
Assignee | ||
Updated•16 years ago
|
Assignee | ||
Updated•16 years ago
|
Assignee | ||
Comment 11•16 years ago
|
||
Attachment #342473 -
Attachment is obsolete: true
Attachment #343232 -
Attachment is obsolete: true
Attachment #345201 -
Flags: review?(mconnor)
Attachment #343232 -
Flags: review?(mconnor)
Assignee | ||
Updated•16 years ago
|
Attachment #345201 -
Attachment description: patch → patch (ctrl-tab only)
Comment 12•16 years ago
|
||
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]
Assignee | ||
Updated•16 years ago
|
Assignee | ||
Updated•16 years ago
|
Comment 13•16 years ago
|
||
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+
Updated•16 years ago
|
Whiteboard: [needs status update] → [needs status update][icon-3.1]
Assignee | ||
Comment 14•16 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Keywords: ue
Resolution: --- → FIXED
Whiteboard: [needs status update][icon-3.1] → [icon-3.1]
Comment 15•16 years ago
|
||
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.
Comment 16•16 years ago
|
||
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.
Comment 17•16 years ago
|
||
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.
Updated•16 years ago
|
Keywords: fixed1.9.1
Comment 18•16 years ago
|
||
This has been landed a while back before it was removed from 1.9.1 branch. Marking as verified1.9.1.
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
Comment 19•16 years ago
|
||
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.
Description
•