Closed Bug 936635 Opened 6 years ago Closed 6 years ago

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

Categories

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

x86_64
Windows 8.1
defect

Tracking

(firefox28 verified, firefox29 verified)

VERIFIED FIXED
Firefox 28
Tracking Status
firefox28 --- verified
firefox29 --- verified

People

(Reporter: krudnitski, Assigned: sfoster)

References

Details

(Whiteboard: [beta28] feature=defect c=tbd u=tbd p=3)

Attachments

(4 files, 1 obsolete file)

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!
Summary: closing open tabs is frustrating when multiple are open and the tray pulls down with the furthest right one cut off → Defect - Closing open tabs is frustrating when multiple are open and the tray pulls down with the furthest right one cut off
Whiteboard: [triage] → [triage] feature=defect c=tbd u=tbd p=0
(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.
(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.
Whiteboard: [triage] feature=defect c=tbd u=tbd p=0 → [release28] feature=defect c=tbd u=tbd p=0
Whiteboard: [release28] feature=defect c=tbd u=tbd p=0 → [beta28] feature=defect c=tbd u=tbd p=0
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.
I have not managed to reproduce this in nightly on a Surface Pro 2 or Thinkpad Carbon X1 Touch.
Assignee: nobody → sfoster
Blocks: metrov1it21
No longer blocks: metrov1backlog
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
Whiteboard: [beta28] feature=defect c=tbd u=tbd p=0 → [beta28] feature=defect c=tbd u=tbd p=3
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
Summary: Defect - Closing open tabs is frustrating when multiple are open and the tray pulls down with the furthest right one cut off → Defect - Tabs in tab strip do not rearrange correctly if user closes a tab while the last/rightmost tab is partially obscured
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.
Summary: Defect - Tabs in tab strip do not rearrange correctly if user closes a tab while the last/rightmost tab is partially obscured → 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
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?
Attachment #8345649 - Flags: feedback?(mmaslaney)
Flags: needinfo?(krudnitski)
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?
Flags: needinfo?(sfoster)
Flags: needinfo?(sfoster)
Flags: needinfo?(sfoster)
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)
Flags: needinfo?(sfoster)
Flags: needinfo?(krudnitski)
Attached image Padding.png
I would make the left and right padding equidistant.
Attached image 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;
}
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
Attachment #8345649 - Attachment is obsolete: true
Attachment #8346255 - Flags: review?(rsilveira)
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.
Attachment #8346255 - Flags: review?(rsilveira) → review+
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.
Whiteboard: [beta28] feature=defect c=tbd u=tbd p=3 → [beta28] feature=defect c=tbd u=tbd p=3 [leave open]
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.
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.
Flags: needinfo?(sfoster)
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?
Flags: needinfo?(sfoster)
Whiteboard: [beta28] feature=defect c=tbd u=tbd p=3 [leave open] → [beta28] feature=defect c=tbd u=tbd p=3 [leave open] [triage]
Decision in the meeting was to close this, and look at follow-up work under bug 948709, in v2.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [beta28] feature=defect c=tbd u=tbd p=3 [leave open] [triage] → [beta28] feature=defect c=tbd u=tbd p=3
Target Milestone: --- → Firefox 29
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
Attachment #8346255 - Flags: approval-mozilla-aurora?
Attachment #8346255 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Target Milestone: Firefox 29 → Firefox 28
Flags: in-qa-testsuite?(hskupin)
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?
Flags: needinfo?(sfoster)
Flags: in-testsuite?
Flags: in-qa-testsuite?(hskupin)
Flags: in-qa-testsuite-
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?
Flags: needinfo?(sfoster)
Depends on: 952297
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!
Depends on: 952663
Filed bug 952663 as follow-up for tests to cover this.
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.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.