Closed Bug 1132508 Opened 5 years ago Closed 4 years ago

Last tab is cut off in tab tray after rotation

Categories

(Firefox for Android :: General, defect)

38 Branch
ARM
Android
defect
Not set

Tracking

()

RESOLVED WONTFIX
Tracking Status
firefox35 --- unaffected
firefox36 --- unaffected
firefox37 --- unaffected
firefox38 --- wontfix
firefox39 --- wontfix
firefox40 --- wontfix
firefox41 --- wontfix

People

(Reporter: TeoVermesan, Assigned: mhaigh)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

Tested with:
Device: Nexus 5 (Android 5.0.1)
Build: Firefox for Android 38.0a1 (2015-02-12)

Steps to reproduce:
1. Open 4 tabs in portrait
2. Rotate to landscape
3. Return to portrait

Expected results:
- Tabs are displayed correctly in tab tray.

Actual results:
- The last tab is cut off.
Please check fx-team for a range.
tracking-fennec: --- → ?
Flags: needinfo?(teodora.vermesan)
tracking-fennec: ? → 38+
Assignee: nobody → mhaigh
Regression window:
fx-team:

good build: 1423674568
bad build: 1423674869
pushlog: http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=fdcb26aac28c&tochange=e1035da2a8dd

Bug 1097337.
Flags: needinfo?(teodora.vermesan)
themes.xml change huh?
ping mhaigh! We should try to get this into 38.
Flags: needinfo?(mhaigh)
[Tracking Requested - why for this release]: This needs to be uplifted to 38.
Tracking for 38 and 39 since it looks like a recent regression. 

mfinkle: is there someone who can work on this so that a fix can be uplifted to 38? thanks!
Flags: needinfo?(mark.finkle)
We are fairly saturated at the moment. I think Martyn is our best bet.
Flags: needinfo?(mark.finkle)
It seems that the tabs panel isn't cut off, it's the chrome which is being pushed down too far.  See attached screenshots of before and after rotation.
Flags: needinfo?(mhaigh)
We ran in to a race condition - the window height in TabsPanel.getTabContainerHeight was being incorrectly reported as we were trying to grab it when other ui elements were still being created.  Delaying the tabs panel from showing until those other elements have finished being inflated fixes this.
Attachment #8586861 - Flags: review?(rnewman)
Status: NEW → ASSIGNED
Comment on attachment 8586861 [details] [diff] [review]
Fix tabs panel from showing until chrome UI has finished inflating

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

I don't doubt that this works, but I'm slightly concerned that this is happening in the first place.

We shouldn't be reaching this code until the views are fully inflated, right? All of this is supposed to happen on the UI thread, serially. This bug implies that either things are happening in the wrong order, in which case the fix is to correct the ordering, or things are happening on the wrong thread.

So what's the root cause?

::: mobile/android/base/tabs/TabsPanel.java
@@ +127,2 @@
>          } else {
> +            return LayoutInflater.from(context).inflate(R.layout.tabs_panel, this);

No need for else with return, and NewTabletUI.isEnabled now really means "is this a tablet?", so maybe just:

  final int layout = NewTabletUI.isEnabled(context) ? R.layout.tabs_panel_default : R.layout.tabs_panel;
  return LayoutInflater.from(context).inflate(layout, this);
Comment on attachment 8586861 [details] [diff] [review]
Fix tabs panel from showing until chrome UI has finished inflating

Punting this back to you for further thoughts, M.
Flags: needinfo?(mhaigh)
Attachment #8586861 - Flags: review?(rnewman)
Let's consider getting the "fix" on Beta ASAP. We can ruminate on the underlying issue in a new bug.
Comment on attachment 8586861 [details] [diff] [review]
Fix tabs panel from showing until chrome UI has finished inflating

Please file that follow-up!
Attachment #8586861 - Flags: review+
It's last thing on Friday and this patch stopped working (possibly - could have been a build issue) and then started working again.  I don't suppose you could test out this build and tell me if you still see this behaviour?

https://dl.dropboxusercontent.com/u/7163922/bug1132508.apk
Flags: needinfo?(mhaigh) → needinfo?(rnewman)
I can't repro with current Nightly. Over to Teo to see if she can still repro.
Flags: needinfo?(rnewman) → needinfo?(teodora.vermesan)
I can still reproduce on nightly.  Last patch wasn't working it seems, so here's a new one. 

Currently we're seeing this flow [1]:
- the height of the tabs panel is calculated, 
- the TwoWayView and all children are measured and laid out,
- the TwoWayView and all children are again measured and laid out.

We want to calculate the height only after the TwoWayView has been measured.  We introduce a couple of VTOs to assist:

We place one VTO to prevent the height of the tabs panel from being calculated until the adapter has inflated the views and added them to the TwoWayView, but by itself it doesn't fix the issue and we see another round of measurements and layout for the TwoWayView after the size of the tab panel has been measured, this second round of measurement alters the dimensions of the TwoWayView [2].

Another VTO is placed in the refresh function, the effect of this, along with the previous VTO, is that we delay the TwoWayView from being measured and laid out multiple times [3] instead it goes through just one round of measurement and layout and we then take the height of the tabs panel.

The reason this works is because we delay the measurement of the tabs panel until such a time that the entire upwards view hierarchy has completed it's own measure and layout cycles.  The tabs panel height is dependant on these other views.

One side effect of this fix is that we delay calling onTabsLayoutChange in BrowserApp which can cause a slight visual stutter when redrawing.

1: https://etherpad.mozilla.org/1132508-1
2: https://etherpad.mozilla.org/1132508-2
3: https://etherpad.mozilla.org/1132508-3
Attachment #8586861 - Attachment is obsolete: true
Attachment #8591727 - Flags: review?(rnewman)
Comment on attachment 8591727 [details] [diff] [review]
Last tab is cut off in tab tray after rotation

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

This doesn't seem insane to me. If it works on the usual collection of devices, ship it!
Attachment #8591727 - Flags: review?(rnewman) → review+
(In reply to Martyn Haigh (:mhaigh) from comment #17)
> One side effect of this fix is that we delay calling onTabsLayoutChange in
> BrowserApp which can cause a slight visual stutter when redrawing.

When redrawing after what action? Rotation in the tabs tray?
Flags: needinfo?(mhaigh)
I can still reproduce the issue on latest Nightly (2015-04-14)

I open 4 tabs in portrait => Tabs are displayed correctly in tab tray
I rotate to landscape with the tab tray opened and return to portrait => The last tab is cut off.
Flags: needinfo?(teodora.vermesan)
(In reply to Michael Comella (:mcomella) from comment #19)
> (In reply to Martyn Haigh (:mhaigh) from comment #17)
> > One side effect of this fix is that we delay calling onTabsLayoutChange in
> > BrowserApp which can cause a slight visual stutter when redrawing.
> 
> When redrawing after what action? Rotation in the tabs tray?

Yeah,  it's because we now wait until the majority of the layout has been done before we can work out the height that the tabs panel needs to be, and hence the y value that the main layout needs to be translated by.

I'm going to land this, but lets keep an eye on it.
Flags: needinfo?(mhaigh)
https://hg.mozilla.org/mozilla-central/rev/80a2fed814d0
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
sorry had to back this out , seems this was causing frequent rc2 test failures across the trees like https://treeherder.mozilla.org/logviewer.html#?job_id=2721770&repo=fx-team
Status: RESOLVED → REOPENED
Flags: needinfo?(mhaigh)
Resolution: FIXED → ---
Target Milestone: Firefox 40 → ---
tracking-fennec: 38+ → 39+
Too late for 38 but we could take a patch for 39 if it lands soon.
tracking-fennec: 39+ → 41+
mhaigh, what's the status on this bug?
I spoke to mhaigh- this is currently not a top priority bug, patch still affects tests. https://bugzilla.mozilla.org/show_bug.cgi?id=1161638 could remove the need for this to be fixed.
Flags: needinfo?(mhaigh)
Too late to take this in 39. We could still uplift to 40.
Martyn: still tracking 41. Should we track a later release or not at all?
Flags: needinfo?(mhaigh)
I'm erring on not tracking at all.  I'm working on removing the chrome (bug 1161638) and ironing a few issues out with that surrounding tab previews - once I have those fixed then I'll start landing and this won't be an issue anymore.
Flags: needinfo?(mhaigh)
[Tracking Requested - why for this release]: I don't think we need to track this for a release.

Hopefully Martyn can fix this as part of some upcoming tabs tray improvements.
tracking-fennec: 41+ → ?
However... looking at the history in this bug, it seems like this was just backed out for some test failures. Martyn, did you ever look into those? Can you reproduce them locally?
Flags: needinfo?(mhaigh)
I didn't look in to this.  To be honest it's a nasty race condition which is causing this issue and the fix introduces a fair amount more complexity and some visual artifacts which is, I suspect, why the tests were failing.  

As mentioned before, this issue is edge case and not very common. I would be happier not landing this fix and instead live with it until the tab tray improvements land.
Flags: needinfo?(mhaigh)
I thought we decided to make this a lower priority bug (maybe tracking +) given that it's an edge case. We were also thinking that any new refactoring in the Tabs Tray could take this issue into account.
Since this is an edge case, and needs some refactoring and significant testing, I have a sense we shouldn't track it for 38, 39, which we won't fix anyway. I'm not clear if there's actually a desire to track for 40+, since it's a lower priority as an edge case. 

Is tracking still requested for this?
(In reply to jen strickland :jenstrickland from comment #37)
> Since this is an edge case, and needs some refactoring and significant
> testing, I have a sense we shouldn't track it for 38, 39, which we won't fix
> anyway. I'm not clear if there's actually a desire to track for 40+, since
> it's a lower priority as an edge case. 
> 
> Is tracking still requested for this?

I re-nomed as a way to make sure clearing tracking was on release management's radar, but I can just clear it myself.
Triage: we decided that this is something we'll live with.
Status: REOPENED → RESOLVED
tracking-fennec: ? → ---
Closed: 5 years ago4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.