Last Comment Bug 628887 - When in an expanded stack, arrow keys should only move between those
: When in an expanded stack, arrow keys should only move between those
Status: RESOLVED FIXED
[a11y][ux][polish]
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 9
Assigned To: Raymond Lee [:raymondlee]
:
Mentors:
Depends on:
Blocks: 592204
  Show dependency treegraph
 
Reported: 2011-01-25 18:56 PST by Michael Yoshitaka Erlewine [:mitcho]
Modified: 2016-04-12 14:00 PDT (History)
4 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 (1.13 KB, patch)
2011-08-12 01:51 PDT, Raymond Lee [:raymondlee]
ttaubert: review-
Details | Diff | Splinter Review
screencast with patch v1 (803.09 KB, video/ogg)
2011-08-14 15:04 PDT, Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
no flags Details
v2 (1.31 KB, patch)
2011-08-15 01:54 PDT, Raymond Lee [:raymondlee]
ttaubert: review-
ttaubert: feedback+
Details | Diff | Splinter Review
v3 (5.15 KB, patch)
2011-08-16 02:51 PDT, Raymond Lee [:raymondlee]
ttaubert: review+
Details | Diff | Splinter Review
[checked-in] Patch for checkin (5.23 KB, patch)
2011-08-17 20:04 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review

Description Michael Yoshitaka Erlewine [:mitcho] 2011-01-25 18:56:48 PST
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 George Carstoiu 2011-01-31 07:54:59 PST
Mozilla/5.0 (Windows NT 5.1; rv:2.0b11pre) Gecko/20110131 Firefox/4.0b11pre

I can confirm the problem with the above build.
Comment 2 Raymond Lee [:raymondlee] 2011-08-12 01:51:38 PDT
Created attachment 552622 [details] [diff] [review]
v1
Comment 3 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-08-14 15:03:12 PDT
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).
Comment 4 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-08-14 15:04:10 PDT
Created attachment 553040 [details]
screencast with patch v1
Comment 5 Raymond Lee [:raymondlee] 2011-08-15 01:54:57 PDT
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().
Comment 6 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-08-15 07:51:51 PDT
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.
Comment 7 Raymond Lee [:raymondlee] 2011-08-16 02:51:34 PDT
Created attachment 553418 [details] [diff] [review]
v3

Re-factored the getClosestTabBy() and added test for this patch.
Comment 8 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-08-17 15:38:24 PDT
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.
Comment 9 Raymond Lee [:raymondlee] 2011-08-17 20:04:22 PDT
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
Comment 10 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-08-22 00:51:19 PDT
http://hg.mozilla.org/integration/fx-team/rev/edeed3c6065c
Comment 11 Rob Campbell [:rc] (:robcee) 2011-08-25 11:50:43 PDT
Comment on attachment 553989 [details] [diff] [review]
[checked-in] Patch for checkin

http://hg.mozilla.org/mozilla-central/rev/edeed3c6065c
Comment 12 Stefan 2011-08-25 14:16:08 PDT
@Tim Taubert Can you set the MIME type of the screencast to video/ogg? I'd like to view it in Fx.

Note You need to log in before you can comment on or make changes to this bug.