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

RESOLVED FIXED in Firefox 9

Status

Firefox Graveyard
Panorama
RESOLVED FIXED
7 years ago
a year ago

People

(Reporter: mitcho, Assigned: raymondlee)

Tracking

Trunk
Firefox 9

Details

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

Attachments

(2 attachments, 3 obsolete attachments)

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.

Comment 1

7 years ago
Mozilla/5.0 (Windows NT 5.1; rv:2.0b11pre) Gecko/20110131 Firefox/4.0b11pre

I can confirm the problem with the above build.
(Assignee)

Comment 2

6 years ago
Created attachment 552622 [details] [diff] [review]
v1
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-
Created attachment 553040 [details]
screencast with patch v1
Version: unspecified → Trunk
(Assignee)

Comment 5

6 years ago
Created attachment 553129 [details] [diff] [review]
v2

(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+
(Assignee)

Comment 7

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

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+
(Assignee)

Comment 9

6 years ago
Created attachment 553989 [details] [diff] [review]
[checked-in] Patch for checkin

(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
(Assignee)

Updated

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Whiteboard: [a11y][ux][polish][fixed-in-fx-team] → [a11y][ux][polish]

Comment 12

6 years ago
@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.