1133770 - Selected tab in tab strip (tablet) not always visible after changing orientation

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[u421692]

Attachment: tabs.mp4

Device: Nexus 7(Android 5.0.2)
Build: Nightly 38.0a1(2015-02-17)

Steps to reproduce:
1. Put the device in landscape mode and open more tabs than will fit on screen
2. Rotate device to portrait mode

Expected result:
Focused tab in the tab strip should be visible

Actual result:
Tabs are shifted, and focused tab is no longer visible to the user(see attached video)

Note: I don't think this is a regression

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Lucas put some code in to do this easily, TabStripView.ensurePositionIsVisible:

https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tabs/TabStripView.java#242

and TabStripView.ensureSelectedPosition.

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: bz://1133770/mcomella

/r/4019 - Bug 1133770 - Display the selected tab in the tab strip on device rotation. r=mhaigh

Pull down this commit:

hg pull review -r f9071610c50fc344d55e54876252c7e2cdca09bd

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8566239 [details]
MozReview Request: bz://1133770/mcomella

/r/4019 - Bug 1133770 - Display the selected tab in the tab strip on device rotation. r=mhaigh

Pull down this commit:

hg pull review -r f9071610c50fc344d55e54876252c7e2cdca09bd

Comment 4

[Martyn Haigh (:mhaigh)]

Comment on attachment 8566239 [details]
MozReview Request: bz://1133770/mcomella

https://reviewboard.mozilla.org/r/4017/#review3189

Ship It!

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8566239 [details]
MozReview Request: bz://1133770/mcomella

Not important enough to risk uplifting to 36, imo.

Approval Request Comment
[Feature/regressing bug #]: New tablet UI (bug 1014156)

[User impact if declined]:
  Users who rotate the device may not see their selected tab (i.e. polish).

[Describe test coverage new/current, TreeHerder]: None

[Risks and why]: 
  Worst case, one of the views we access is null and we crash when the device is rotated. However, real risk is low - we just call some pre-existing (and thus tested) methods on device rotation.

[String/UUID change made/needed]: None

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/integration/fx-team/rev/5e195355a1b9

https://treeherder.mozilla.org/logviewer.html#?job_id=2035320&repo=fx-team

Comment 8

[Aaron Train [:aaronmt]]

Just making sure this is tracked, this is particularly bad.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

re comment 7: the bustage is because the TabStrip class is excluded on API 9 devices so we can't specify the type of that class. Better to use an interface, I guess.

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8566239 [details]
MozReview Request: bz://1133770/mcomella

/r/4019 - Bug 1133770 - Display the selected tab in the tab strip on device rotation. r=mhaigh
/r/4099 - Bug 1133770 - Use Refreshable interface instead of TabStrip in BrowserApp to allow builds on API 9. r=mhaigh

Pull down these commits:

hg pull review -r 06d5c1293536c0ea6b4990527b91a03d7e900cf3

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Saving Sheriff Ryan: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f031bcd276b3

Comment 12

[Martyn Haigh (:mhaigh)]

Comment on attachment 8566239 [details]
MozReview Request: bz://1133770/mcomella

https://reviewboard.mozilla.org/r/4017/#review3265

Ship It!

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I blame Newman: https://treeherder.mozilla.org/#/jobs?repo=try&revision=255dfd020418

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Okay, that was my fault: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8843f256a48

Forgot to remove the import.

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

remote:   https://hg.mozilla.org/integration/fx-team/rev/be22a65f0614
remote:   https://hg.mozilla.org/integration/fx-team/rev/5e6ac1912e5a

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8566239 [details]
MozReview Request: bz://1133770/mcomella

To be clear, this should be in 37 and above.

(same as comment 6:)

Not important enough to risk uplifting to 36, imo.

Approval Request Comment
[Feature/regressing bug #]: New tablet UI (bug 1014156)

[User impact if declined]:
  Users who rotate the device may not see their selected tab (i.e. polish).

[Describe test coverage new/current, TreeHerder]: None

[Risks and why]: 
  Worst case, one of the views we access is null and we crash when the device is rotated. However, real risk is low - we just call some pre-existing (and thus tested) methods on device rotation.

[String/UUID change made/needed]: None

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/be22a65f0614
https://hg.mozilla.org/mozilla-central/rev/5e6ac1912e5a

Comment 18

[Flaviu Cos]

The bug is still reproducible on latest nightly (2015-02-24);
Device: Google Nexus 7 (Android 5.0.2).

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Flaviu Cos, QA [:flaviu] from comment #18)
> The bug is still reproducible on latest nightly (2015-02-24);
> Device: Google Nexus 7 (Android 5.0.2).

Ahh, I only put this in the commit comment:

"This does not always work in the case that one of the last few tabs (to the
right) are selected and the device is rotated from landscape to portrait.
Filed bug 1134408 to track this."

It'd take too much work to diagnose and totally fix this issue, particularly for uplifting to 37. Please reopen if you think that my work did not fix the issue (with the exception of the STR described above).

Comment 20

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8566239 [details]
MozReview Request: bz://1133770/mcomella

37 is now Beta. Need a Beta nom.

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

Approving (and tracking) for beta  and aurora because it's a recent regression.

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8566239 [details]
MozReview Request: bz://1133770/mcomella

Based on comment 16, approving for 37 and 38.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/eb261fd50770
https://hg.mozilla.org/releases/mozilla-beta/rev/e7319d343f20

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a3214625cd30
https://hg.mozilla.org/releases/mozilla-aurora/rev/abeee313fed0

Comment 25

[Catalin Suciu [:csuciu]]

Verifying as fixed on Firefox 37 Beta 2 (except the scenario from bug 1134408)

Comment 27

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1133770 - Use Refreshable interface instead of TabStrip in BrowserApp to allow builds on API 9. r=mhaigh

Comment 28

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1133770 - Display the selected tab in the tab strip on device rotation. r=mhaigh