Closed Bug 628887 Opened 9 years ago Closed 8 years ago

When in an expanded stack, arrow keys should only move between those

Categories

(Firefox Graveyard :: Panorama, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 9

People

(Reporter: mitcho, Assigned: raymondlee)

References

Details

(Whiteboard: [a11y][ux][polish])

Attachments

(2 files, 3 obsolete files)

Right now when we are in an expanded stack and we use the arrow keys, it moves between all tabs, including those which happen to be behind the expander tray.
Mozilla/5.0 (Windows NT 5.1; rv:2.0b11pre) Gecko/20110131 Firefox/4.0b11pre

I can confirm the problem with the above build.
Attached patch v1 (obsolete) — Splinter Review
Assignee: nobody → raymond
Attachment #552622 - Flags: review?(tim.taubert)
Comment on attachment 552622 [details] [diff] [review]
v1

Review of attachment 552622 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch!

Alas, this doesn't work because getClosestTabBy() gets the closest tab in the desired direction. So if you have a tab below the expanded group that will be selected. With your patch applied the navigation is blocked - I'll attach a screencast to to clarify what I mean.

To solve this bug we'd need to actually modify getClosestTabBy() to include only the tabs of the currently expanded group (if there's one).
Attachment #552622 - Flags: review?(tim.taubert) → review-
Version: unspecified → Trunk
Attached patch v2 (obsolete) — Splinter Review
(In reply to Tim Taubert [:ttaubert] from comment #3)
> Comment on attachment 552622 [details] [diff] [review]
> v1
> 
> Review of attachment 552622 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch!
> 
> Alas, this doesn't work because getClosestTabBy() gets the closest tab in
> the desired direction. So if you have a tab below the expanded group that
> will be selected. With your patch applied the navigation is blocked - I'll
> attach a screencast to to clarify what I mean.
> 
> To solve this bug we'd need to actually modify getClosestTabBy() to include
> only the tabs of the currently expanded group (if there's one).

Updated the getClosestTabBy().
Attachment #552622 - Attachment is obsolete: true
Attachment #553129 - Flags: review?(tim.taubert)
Comment on attachment 553129 [details] [diff] [review]
v2

Review of attachment 553129 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!

Can you please add a test for this? Maybe a little stacked group that is expanded and you make sure that the focus doesn't jump to a group right of the expanded one. After that we should collapse the group and make sure it _does_ jump.

r- only because of the missing test. This is pretty basic functionality, we should make sure this doesn't regress.

::: browser/base/content/tabview/ui.js
@@ +1090,5 @@
>            [[item.bounds.center(), item]
> +            for each(item in TabItems.getItems()) if (!item.parent.hidden &&
> +                                                       (!activeTabGroup.expanded ||
> +                                                       activeTabGroup.id == item.parent.id))];
> +        let myCenter = activeTab.bounds.center();

Nit: The formatting looks really odd because of this big generator expression. Could you please maybe add a new line before the "if (!item.parent.hidden ..." and see how that looks like?

@@ +1095,5 @@
>          let matches = centers
>            .filter(function(item){return norm(item[0], myCenter)})
>            .sort(function(a,b){
>              return myCenter.distance(a[0]) - myCenter.distance(b[0]);
>            });

Optional: (If you feel like doing this, great! If not we should tackle this in a follow-up.)

The whole getClosestTabBy() function could be refactored to handle all those items in a simple forEach() call. If the item meets the condition we just calculate the myCenter.distance() and if it's lower than the value we found so far, store it and the corresponding item. If we found anything, return it after the forEach() call.

So we don't iterate three times over all available tabItems.
Attachment #553129 - Flags: review?(tim.taubert)
Attachment #553129 - Flags: review-
Attachment #553129 - Flags: feedback+
Attached patch v3 (obsolete) — Splinter Review
Re-factored the getClosestTabBy() and added test for this patch.
Attachment #553129 - Attachment is obsolete: true
Attachment #553418 - Flags: review?(tim.taubert)
Comment on attachment 553418 [details] [diff] [review]
v3

Review of attachment 553418 [details] [diff] [review]:
-----------------------------------------------------------------

Great work, thanks!

r=me with the one little improvement addressed.

::: browser/base/content/tabview/ui.js
@@ +1096,5 @@
> +            let itemCenter = item.bounds.center();
> +
> +            if (norm(itemCenter, myCenter) &&
> +                 (!match || myCenter.distance(match[0]) > myCenter.distance(itemCenter)))
> +                match = [itemCenter, item];

Instead of storing "itemCenter" in match[0] we could rather store "myCenter.distance(itemCenter)" because myCenter doesn't change. So we don't need to recalculate the distance when comparing.
Attachment #553418 - Flags: review?(tim.taubert) → review+
(In reply to Tim Taubert [:ttaubert] from comment #8)
> Instead of storing "itemCenter" in match[0] we could rather store
> "myCenter.distance(itemCenter)" because myCenter doesn't change. So we don't
> need to recalculate the distance when comparing.

Addressed.

Sent it to try and waiting for the results.
http://tbpl.mozilla.org/?tree=Try&rev=d61a71b65986
Attachment #553418 - Attachment is obsolete: true
Attachment #553989 - Attachment description: v4 → Patch for checkin
http://hg.mozilla.org/integration/fx-team/rev/edeed3c6065c
Status: NEW → ASSIGNED
Whiteboard: [a11y][ux][polish] → [a11y][ux][polish][fixed-in-fx-team]
Target Milestone: Future → Firefox 9
Comment on attachment 553989 [details] [diff] [review]
[checked-in] Patch for checkin

http://hg.mozilla.org/mozilla-central/rev/edeed3c6065c
Attachment #553989 - Attachment description: Patch for checkin → [checked-in] Patch for checkin
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [a11y][ux][polish][fixed-in-fx-team] → [a11y][ux][polish]
@Tim Taubert Can you set the MIME type of the screencast to video/ogg? I'd like to view it in Fx.
Attachment #553040 - Attachment mime type: application/octet-stream → video/ogg
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.