936635 - Defect - Tapping the 'X' of a tab in the tab strip does not close the tab if the side-scroll-arrows are visible in the tab strip

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: General, defect, P2)

Description

[Karen Rudnitski [:kar]]

I'm using Nightly. I have many tabs open (I've just been following link after link from skype). I want to clean it up, so I want to close down some tabs.

I swipe from the top to see my tab tray. Great.

Being an Android user, I try to 'swipe' them off the screen. Doesn't work. Doh. 

I then look to press the 'x'. And this is where the frustrating bit comes. You can easily tap on the 'x' provided the full thumbnail is displayed. But if the row of thumbnails start a little 'off' the screen, then I try and press an 'x' and it jumps and I end up launching that tab. I go in circles and circles, until I (just this moment) realized that it's fine if I move the tray of thumbnails so that the furthest right one is not cut off. And then after that, even if one is cut off, the 'x's behave.

But I didn't even think about moving the tab tray across until I was writing this bug, and I was just getting more frustrated. Something is wonky here!

Comment 1

[Jim Mathies [:jimm]]

(In reply to Karen Rudnitski [:kar] from comment #0)
> I'm using Nightly. I have many tabs open (I've just been following link
> after link from skype). I want to clean it up, so I want to close down some
> tabs.
> 
> I swipe from the top to see my tab tray. Great.
> 
> Being an Android user, I try to 'swipe' them off the screen. Doesn't work.
> Doh. 

You can do this, you just edge swipe down again. This is a standard win8 gesture so we followed it with our context ui.

> I then look to press the 'x'. And this is where the frustrating bit comes.
> You can easily tap on the 'x' provided the full thumbnail is displayed. But
> if the row of thumbnails start a little 'off' the screen, then I try and
> press an 'x' and it jumps and I end up launching that tab. I go in circles
> and circles, until I (just this moment) realized that it's fine if I move
> the tray of thumbnails so that the furthest right one is not cut off. And
> then after that, even if one is cut off, the 'x's behave.
> 
> But I didn't even think about moving the tab tray across until I was writing
> this bug, and I was just getting more frustrated. Something is wonky here!

Yeah our tab tray interaction needs work. Thanks for filing.

Comment 2

[Matt Brubeck (:mbrubeck)]

(In reply to Jim Mathies [:jimm] from comment #1)
> > Being an Android user, I try to 'swipe' them off the screen. Doesn't work.
> > Doh. 
> 
> You can do this, you just edge swipe down again. This is a standard win8
> gesture so we followed it with our context ui.

The edge gesture will show/hide the tab tray.  But what Karen wants (and what Firefox for Android allows) is to tap-and-drag a single tab thumbnail and "cross-slide" it to close the tab.

Comment 3

[Karen Rudnitski [:kar]]

For v1, we need to detect and ensure that when the 'x' is pressed to close a tab, that the tab doesn't hop and does, indeed, close.

To reproduce, have lots of tabs open. When you pull down the tab tray, you'll see that some of the thumbnails fall off the right edge (if they fit perfectly, the problem doesn't occur). Tap on the 'x' of one of the tabs (without moving the tab tray left or right - just keep it static as-is). The 'x' jumps after you've pressed it, the tab tray repositions, and it thinks you wanted to view a tab instead of close it.

For v2, I'd love to see us do a 'swipe to close tab' interaction. A new bug will need to be filed to follow that work.

Comment 4

[Matt Brubeck (:mbrubeck)]

I have not managed to reproduce this in nightly on a Surface Pro 2 or Thinkpad Carbon X1 Touch.

Comment 5

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

STR:
  1. Open a bunch of tabs (more than enough that they cannot all be viewed at one time in the tab strip)
  2. Scroll the tab strip so that the last/rightmost tab is visible, but is cut off by the edge of the tab strip
  3. Close any visible tab using the 'X'

At this point, another tab should shuffle into the place of the one that you just closed, so that its 'X' is exactly in the location that you just tapped.

To see the correct/expected behavior, do the following:
  1. Same step 1 as above
  2. Scroll the tab strip so that the last/rightmost tab is either entirely visible (not cut off at all) or is entirely invisible
  3. Same step 3 as above

Comment 6

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

I'll move the STR in comment 5 to a new bug since they describe a different problem (I'll also update the titles).

STR for the issue mentioned in comment 0:
  1. Open enough tabs that the side-scroll-arrows appear in the tab strip when you move the mouse
  2. Move the mouse, so that the side-scroll-arrows appear
  3. Tap (using touch input) on one of the 'X's in the tab strip

Because you've tapped the screen, metroFx switches from the mouse-friendly tab strip with side-scroll-arrows to a touch-friendly tab strip with no arrows. This causes your tap to go to the wrong place, so you end up selecting a tab instead of closing a tab.

Comment 7

[Sam Foster [:sfoster] (he/him)]

Attachment: Adjust tab strip paddings, don't set scrollbuttons to hidden not collapse in inprecise input mode

When the user taps or moves the mouse the input mode is switched between precise and inprecise mode. We currently have CSS rules that show and hide (via visibility:collapse) the scrollbuttons for the tab strip. This changes the layout in the tab strip and is what is causing the tab buttons to jump about. 

In this patch I'm proposing we just toggle visibility:hidden, which fixes the jumping-about problem. To claw back some of the horizontal space that is now permanently given over to the arrows (visible or not) I've adjusted some of the surrounding padding values. Before and after screenshots: 

Before patch: https://dl.dropboxusercontent.com/u/1719101/share/tabs-arrows-collapse.png
After patch: https://dl.dropboxusercontent.com/u/1719101/share/tabs-arrows-visible.png

.. In the after-patch screenshot, in touch/inprecise mode, the left and right scroll arrows are simply hidden; the space the occupy in the layout is preserved.

I dont see the tabs, scrollbuttons or + control aligning to any obvious grid at the moment. So to my eyes and fingers at least these changes leave enough space for distinct touch targets, while leaving alignment at least as good as it was before?

Comment 8

[Michael Maslaney [:mmaslaney] ([e-mail])]

Attachment: Screen Shot 2013-12-10 at 7.00.43 PM.png

Sam, the padding looks good and should provide enough room for touch/click. 

As an enhancement, Could we provide a vertical rule (much like we do in desktop, see attached) to visually indicate a cut-off point, and at the same time not condense the tab shape?

Comment 9

[Sam Foster [:sfoster] (he/him)]

I think we should be able to work that into the tab-arrows image? I've filed bug 948778 for it
(Sorry, :kar, no particular info needed from you, I'm clearing the flag)

Comment 10

[Michael Maslaney [:mmaslaney] ([e-mail])]

Attachment: Padding.png

I would make the left and right padding equidistant.

Comment 11

[Michael Maslaney [:mmaslaney] ([e-mail])]

Attachment: Horizontal_rule.png

Also, the vertical rule can be super subtle.

We can try something like this:

.Vertical_Rule {
  background-color: rgb( 90, 91, 95 );
  position: absolute;
  left: 102px;
  top: 5px;
  width: 1px;
  height: 260px;
  z-index: 8;
}

Comment 12

[Sam Foster [:sfoster] (he/him)]

Attachment: Even up tabstrip scroll arrow spacing and fix scroff offset problem when toggling between precise/imprecise input

To fix the main issue here I'm leaving the left scroll arrow in the layout and just toggling visibility: hidden/visible between precise/imprecise input modes. 
I tried doing the same with the right arrow but with the 'x' close tab button there as well it ends up being a lot of dead space when the arrow is hidden.

Then I've adjusted and evened up the spacing per mmaslaney's comments

Comment 13

[Rodrigo Silveira [:rsilveira] [:rodms]]

Comment on attachment 8346255 [details] [diff] [review]
Even up tabstrip scroll arrow spacing and fix scroff offset problem when toggling between precise/imprecise input

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

Because the right arrow still shifts the tab thumbnails, this STR still fails:
1. With many tabs open scroll the tab strip all the way to the end using the mouse
2. Touch on an x to close a tab

You end up selecting it instead of closing. This is a super edge case and I'm totally OK not fixing/moving to other bug.

Comment 14

[Sam Foster [:sfoster] (he/him)]

I'll land this (tree currently closed) and leave it open for now. You're right, its not quite fixed. I'm open to ideas. I feel like it needs a design solution rather than a technical one. I'm not even sure what a technical solution would look like - manually adjusting scrollLeft when toggling imprecise input mode? - that seems fugly.

Comment 15

[Sam Foster [:sfoster] (he/him)]

Landed on fx-team: https://hg.mozilla.org/integration/fx-team/rev/9968e99bbdf6

Lets keep this ticket open for now and take a look at this in the nightly to see if further fixes are warranted.

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9968e99bbdf6

Comment 17

[Marco Mucci [:MarcoM]]

Hey Sam, can this bug be marked as resolved?

(In reply to Sam Foster [:sfoster] from comment #15)
> Landed on fx-team:
> https://hg.mozilla.org/integration/fx-team/rev/9968e99bbdf6
> 
> Lets keep this ticket open for now and take a look at this in the nightly to
> see if further fixes are warranted.

Comment 18

[Sam Foster [:sfoster] (he/him)]

Lets re-triage this now that the first fix has landed. The problem described in Comment 13 persists. Do we want to block on this?

Comment 19

[Sam Foster [:sfoster] (he/him)]

Decision in the meeting was to close this, and look at follow-up work under bug 948709, in v2.

Comment 20

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8346255 [details] [diff] [review]
Even up tabstrip scroll arrow spacing and fix scroff offset problem when toggling between precise/imprecise input

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Long-standing issue in Metro tabstrip feature
User impact if declined: User clicks to close a tab may cause it to be selected rather than closed under some circumstances
Testing completed (on m-c, etc.): Tested good on m-c
Risk to taking this patch (and alternatives if risky): low risk, only Metro
code is affected
String or IDL/UUID changes made by this patch: None

Comment 21

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/de4a49ad1731

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Regaring the in-qa-testsuite flag which has been set here, I don't think that a Mozmill test is necessary. Instead a mochitest (http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/tests/mochitest/) should perfectly work here. Sam, could you work on such a test?

Comment 23

[Sam Foster [:sfoster] (he/him)]

Yeah I can add a mochitest for this, do you want me to re-land this patch or just create a new bug and patch and reference it here?

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Its perfect if you do it as a follow-up patch. There is nothing we would have to backout here, as long as the landing hasn't caused a major regression. So thank you for working on that test!

Comment 25

[Sam Foster [:sfoster] (he/him)]

Filed bug 952663 as follow-up for tests to cover this.

Comment 26

[Cornel Ionce [:noni] [Hubs QA]]

Verified as fixed on Latest Aurora (build ID: 20140217004001) and on Firefox 28 beta 3 (build ID: 20140213172947) with the STR from description and comment 5 using a Microsoft Surface Pro 2 device.