965550 - Tab stip doesn't touch scroll if last tab is partially visible

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: App Bar, defect, P1)

Description

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

STR:
1. Open as many tabs as necessary to have the last one partially visible (that's 6 tabs on the X1 carbon and 5 tabs on the surface pro)
2. Scroll the tab strip to the beginning and select the first tab.
3. After the tab stip dismisses, being it up again.

Now you can't touch scroll the tab strip. All tabs are still selectable. Adding a new tab fixes it.

Comment 1

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

I was trying to find any of our front-end code that could be related to this with no luck. It happens in -metrodesktop too, which makes it easier to debug. I'm starting to think it could be something deeper.

Comment 2

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

TouchModule check for dragability[1] compares the target bounding rect with the scroller size. The overgrown scroll arrows[2] are not discounted from the bounding rect, thus it will only scroll if the scroller width is larger than the tabs viewable are plus both scroll buttons (64px each).

We used to collapse the arrow buttons when in imprecise mode but it caused it's own issues (bug 8346255).I'll try to see if I can get a better solution. 

[1] - http://dxr.mozilla.org/mozilla-central/source/browser/metro/base/content/input.js#519
[2] - http://dxr.mozilla.org/mozilla-central/source/browser/metro/theme/browser.css#62

Comment 3

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

Attachment: Patch v1

Adding padding to the size of the arrow buttons and compensating with negative margin fixes it. This is a very special case where we resize the arrow buttons on a scrollbox, we can investigate a more generic fix if it comes back somewhere. It's very targeted and low risk for uplift too.

Comment 4

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

https://hg.mozilla.org/integration/fx-team/rev/dc386405bf17

Comment 5

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/dc386405bf17

Comment 6

[Marco Mucci [:MarcoM]]

For testing and verification.  Reopen if any defects found.

Comment 7

[Kamil Jozwiak [:kjozwiak]]

Went through the verification process with the latest Nightly build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-03-02-03-02-03-mozilla-central/

I reproduced the original issue using the following build before going through verification:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-01-30-03-02-02-mozilla-central/

- Went through the original test case from comment #0 and ensured it was working correctly (couldn't reproduce the original issue)
- Opened over 15 different tabs and ensured that I can swipe and scroll through the Tab App Bar
- Ensured that all the tabs under the Tab App Bar are selectable
- Ensured that you can close all the opened tab's using the "X"
- Ensured that you can scroll using the arrows on each side of the Tab App Bar (quickly clicking through & clicking and holding)
- Ensured that you can scroll through the tabs using the mouse wheel
- Went through all of the above test cases using different variations of snapped view
- Tested with both the X1 Carbon and the Surface Pro 2

Comment 8

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

Comment on attachment 8381545 [details] [diff] [review]
Patch v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): It was uncovered by bug 936635
User impact if declined: Tab strip will not be touch scrollable if you have the last tab partially visible.
Testing completed (on m-c, etc.): on m-c since 2/27, QA tested - see comment 7.
Risk to taking this patch (and alternatives if risky): Very low, CSS change impacting Metro only.
String or IDL/UUID changes made by this patch: none.

Comment 9

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6a3684aee3e3

Comment 10

[Kamil Jozwiak [:kjozwiak]]

Changing the issue back to resolved until the other channels have been tested and verified.

Comment 11

[Kamil Jozwiak [:kjozwiak]]

Went through the verification process using the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-03-05-00-40-02-mozilla-aurora/

- Went through the original test case from comment #0
- Went through the test cases added in comment #7 (used both X1 Carbon/Surface Pro 2)

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8381545 [details] [diff] [review]
Patch v1

Will take this on Beta even though it doesn't meet the typical criteria for this stage. It's low risk and Metro only so we'll go for it to help Metro be its best on first public release.

Comment 13

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f7953ebceb60

Comment 14

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/f7953ebceb60

Comment 15

[Manuela Muntean [Away]]

Verified as fixed with Firefox 28 beta 9 on Dell XPS 12 ultrabook with Windows 8 64-bit.