1132508 - Last tab is cut off in tab tray after rotation

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[Teodora Vermesan (:TeoVermesan)]

Attachment: Screenshot_2015-02-12-18-35-42.png

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.

Comment 1

[Aaron Train [:aaronmt]]

Please check fx-team for a range.

Comment 2

[Teodora Vermesan (:TeoVermesan)]

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.

Comment 3

[Aaron Train [:aaronmt]]

themes.xml change huh?

Comment 4

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

ping mhaigh! We should try to get this into 38.

Comment 5

[Kevin Brosnan [Ex-Mozilla]]

[Tracking Requested - why for this release]: This needs to be uplifted to 38.

Comment 6

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

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!

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

We are fairly saturated at the moment. I think Martyn is our best bet.

Comment 8

[Martyn Haigh (:mhaigh)]

Attachment: Screenshot before and after rotation

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.

Comment 9

[Martyn Haigh (:mhaigh)]

Attachment: Fix tabs panel from showing until chrome UI has finished inflating

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.

Comment 10

[Richard Newman [:rnewman]]

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 11

[Richard Newman [:rnewman]]

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.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Let's consider getting the "fix" on Beta ASAP. We can ruminate on the underlying issue in a new bug.

Comment 13

[Richard Newman [:rnewman]]

Comment on attachment 8586861 [details] [diff] [review]
Fix tabs panel from showing until chrome UI has finished inflating

Please file that follow-up!

Comment 14

[Martyn Haigh (:mhaigh)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=897230ad620c

Comment 15

[Martyn Haigh (:mhaigh)]

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

Comment 16

[Richard Newman [:rnewman]]

I can't repro with current Nightly. Over to Teo to see if she can still repro.

Comment 17

[Martyn Haigh (:mhaigh)]

Attachment: Last tab is cut off in tab tray after rotation

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

Comment 18

[Richard Newman [: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!

Comment 19

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

(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?

Comment 20

[Teodora Vermesan (:TeoVermesan)]

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.

Comment 21

[Martyn Haigh (:mhaigh)]

(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.

Comment 22

[Martyn Haigh (:mhaigh)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e89830631af

Comment 23

[Martyn Haigh (:mhaigh)]

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

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/80a2fed814d0

Comment 25

[Pulsebot]

Backout:
https://hg.mozilla.org/mozilla-central/rev/725af2a560d8

Comment 26

[Carsten Book [:Tomcat]]

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

Comment 27

[Sylvestre Ledru [:Sylvestre]]

Too late for 38 but we could take a patch for 39 if it lands soon.

Comment 28

[:Margaret Leibovic]

mhaigh, what's the status on this bug?

Comment 29

[Kate Glazko]

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.

Comment 30

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

Too late to take this in 39. We could still uplift to 40.

Comment 31

[Richard Newman [:rnewman]]

Martyn: still tracking 41. Should we track a later release or not at all?

Comment 32

[Martyn Haigh (: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.

Comment 33

[:Margaret Leibovic]

[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.

Comment 34

[:Margaret Leibovic]

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?

Comment 35

[Martyn Haigh (: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.

Comment 36

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 37

[jen strickland :jenstrickland]

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?

Comment 38

[:Margaret Leibovic]

(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.

Comment 39

[Richard Newman [:rnewman]]

Triage: we decided that this is something we'll live with.