Closed Bug 1133770 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 39
Tracking Status
firefox36 --- wontfix
firefox37 + verified
firefox38 + fixed
firefox39 --- fixed
fennec 37+ ---

People

(Reporter: u421692, Assigned: mcomella)

References

(Blocks 2 open bugs)

Details

(Keywords: regression, verifyme)

Attachments

(3 files, 1 obsolete file)

Attached video 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
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.
Assignee: nobody → michael.l.comella
Status: NEW → ASSIGNED
Summary: Tab strip loses focus after changing orientation from landscape to portrait → Selected tab in tab strip (tablet) not always visible after changing orientation
/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 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
Attachment #8566239 - Flags: review?(mhaigh)
Attachment #8566239 - Flags: review?(mhaigh) → review+
Comment on attachment 8566239 [details]
MozReview Request: bz://1133770/mcomella

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

Ship It!
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
Attachment #8566239 - Flags: approval-mozilla-aurora?
Just making sure this is tracked, this is particularly bad.
tracking-fennec: --- → ?
Attachment #8566239 - Flags: approval-mozilla-aurora?
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.
Attachment #8566239 - Flags: review+ → review?(mhaigh)
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 on attachment 8566239 [details]
MozReview Request: bz://1133770/mcomella

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

Ship It!
Attachment #8566239 - Flags: review?(mhaigh) → review+
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
Attachment #8566239 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/be22a65f0614
https://hg.mozilla.org/mozilla-central/rev/5e6ac1912e5a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Flags: needinfo?(flaviu.cos)
Keywords: verifyme
The bug is still reproducible on latest nightly (2015-02-24);
Device: Google Nexus 7 (Android 5.0.2).
Flags: needinfo?(flaviu.cos)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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).
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Comment on attachment 8566239 [details]
MozReview Request: bz://1133770/mcomella

37 is now Beta. Need a Beta nom.
Attachment #8566239 - Flags: approval-mozilla-beta?
Approving (and tracking) for beta  and aurora because it's a recent regression.
Comment on attachment 8566239 [details]
MozReview Request: bz://1133770/mcomella

Based on comment 16, approving for 37 and 38.
Attachment #8566239 - Flags: approval-mozilla-beta?
Attachment #8566239 - Flags: approval-mozilla-beta+
Attachment #8566239 - Flags: approval-mozilla-aurora?
Attachment #8566239 - Flags: approval-mozilla-aurora+
tracking-fennec: ? → 37+
Verifying as fixed on Firefox 37 Beta 2 (except the scenario from bug 1134408)
Attachment #8566239 - Attachment is obsolete: true
Attachment #8619497 - Flags: review+
Attachment #8619498 - Flags: review+
You need to log in before you can comment on or make changes to this bug.