Teodora Vermesan (:TeoVermesan) Reporter
	Description • 10 years ago

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.

	Aaron Train [:aaronmt]
	Comment 1 • 10 years ago

Please check fx-team for a range.

	Teodora Vermesan (:TeoVermesan) Reporter
	Comment 2 • 10 years ago

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.

	Aaron Train [:aaronmt]
	Comment 3 • 10 years ago

themes.xml change huh?

	Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]
	Comment 4 • 9 years ago

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

	Kevin Brosnan [Ex-Mozilla]
	Comment 5 • 9 years ago

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

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 6 • 9 years ago

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!

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 7 • 9 years ago

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

	Martyn Haigh (:mhaigh) Assignee
	Comment 8 • 9 years ago

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.

	Martyn Haigh (:mhaigh) Assignee
	Comment 9 • 9 years ago

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.

	Richard Newman [:rnewman]
	Comment 10 • 9 years ago

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);

	Richard Newman [:rnewman]
	Comment 11 • 9 years ago

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.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 12 • 9 years ago

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

	Richard Newman [:rnewman]
	Comment 13 • 9 years ago

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

Please file that follow-up!

	Martyn Haigh (:mhaigh) Assignee
	Comment 14 • 9 years ago

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

	Martyn Haigh (:mhaigh) Assignee
	Comment 15 • 9 years ago

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

	Richard Newman [:rnewman]
	Comment 16 • 9 years ago

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

	Martyn Haigh (:mhaigh) Assignee
	Comment 17 • 9 years ago

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

	Richard Newman [:rnewman]
	Comment 18 • 9 years ago

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!

	Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]
	Comment 19 • 9 years ago

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

	Teodora Vermesan (:TeoVermesan) Reporter
	Comment 20 • 9 years ago

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.

	Martyn Haigh (:mhaigh) Assignee
	Comment 21 • 9 years ago

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

	Martyn Haigh (:mhaigh) Assignee
	Comment 22 • 9 years ago

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

	Martyn Haigh (:mhaigh) Assignee
	Comment 23 • 9 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 24 • 9 years ago

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

	Pulsebot
	Comment 25 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 26 • 9 years ago

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

	Sylvestre Ledru [:Sylvestre]
	Comment 27 • 9 years ago

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

	:Margaret Leibovic
	Comment 28 • 9 years ago

mhaigh, what's the status on this bug?

	Kate Glazko
	Comment 29 • 9 years ago

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.

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 30 • 9 years ago

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

	Richard Newman [:rnewman]
	Comment 31 • 9 years ago

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

	Martyn Haigh (:mhaigh) Assignee
	Comment 32 • 9 years ago

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.

	:Margaret Leibovic
	Comment 33 • 9 years ago

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

	:Margaret Leibovic
	Comment 34 • 9 years ago

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?

	Martyn Haigh (:mhaigh) Assignee
	Comment 35 • 9 years ago

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.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 36 • 9 years ago

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.

	jen strickland :jenstrickland
	Comment 37 • 9 years ago

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?

	:Margaret Leibovic
	Comment 38 • 9 years ago

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

	Richard Newman [:rnewman]
	Comment 39 • 9 years ago

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