1399683 - Rotating device re-layouts Activity Stream panel to fit half the screen sometimes

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect, P1)

Description

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

I don't have an STR, only saw this once, and neglected to take a screenshot but I rotated my Nexus S and the top sites tiles overlapped one another: in particular, the bottom row overlapped the top. This made the top sites grid unusable and it looked bad.

Filing to see if QA might be aware of such an issue (NI Bogdan).

I wonder...
- if the animation just froze but it all would have turned out just fine.
- if this is for just small devices (my Nexus S is small)
- if this is for just slow devices (my Nexus S is slow)

Comment 1

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

If we can't reproduce this after a little trying, it's probably not worth tracking down.

Comment 2

[Bogdan Surd, QA [:bsurd, NI]]

Hello I managed to reproduce this.

Exact STR:
 1. Go into landscape mode;
 2. Long tap on the second or third tile to open the menu (reproducible ~70% of the times);
 3. Rotate the device back in portrait mode.
 4. Close the menu.

Expected results:
 - Tiles are displayed normally.

Actual results:
 - Tiles are overlapping eachother.

Notes:
Sometimes the issue does not reproduce at all, or you have to open the menu in portrait close it in landscape and when rotating back to portrait you can see the icond overlapping.

Video - https://goo.gl/wwzpmg

Comment 3

[Bogdan Surd, QA [:bsurd, NI]]

Attachment: log.txt

Extra info + log:
When constantly rotating between landscape and portrait mode an icon is extended to the right in landscape mode, when going back in portrait mode the icons overlap.

I've attached a log from a HTC 10 (Android 7.0)

Comment 4

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

Attachment: Final frame from repro video

In this final frame, the right side of the home page doesn't reach the right edge of the phone, which causes the top sites to overlap and more content on the screen to shrink in width.

However, I wonder if this is the final state or just that the video ended before the final state could be shown.

Comment 5

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

Bogdan, when you're able to reproduce, do the top sites eventually fix themselves or does it stay stuck overlapping?

Comment 6

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

(In reply to Michael Comella (:mcomella) from comment #5)
> Bogdan, when you're able to reproduce, do the top sites eventually fix
> themselves or does it stay stuck overlapping?

I repro'd on my N5 and it's stuck this way.

Note: I have a patch applied that adjusts the tile sizes but considering the top stories are also affected, I think my patch did not affect the outcome.

Comment 7

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

This could be more critical for tablets (since you're more likely to use landscape) but we have higher priority issues.

Comment 8

[Bogdan Surd, QA [:bsurd, NI]]

Hello,

I've got some new info regarding this, if you go to Settings>General>Home tap on Top Sites and disable all of the additional content this issue is 100% rep for all devices after they rotate the display one time.

Opening a new tab will not fix the issue. Closing and repoening fennec will fixes this, or opening any page.

Comment 9

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

Seems more important than before: re-adding to triage.

Comment 10

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

Now that this is 100% reproducible, if this is easy and doesn't have a high risk of breaking other things, let's fix it. Otherwise, we'll accept it as is.

Comment 11

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

Attachment: Screenshot: UI hierarchy

It looks like both pages of the top sites are getting smushed into the same screen.

Comment 12

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

Attachment: Screenshot: UI hierarchy with Pocket

It looks like the RecyclerView containing the top sites content is fitted to half the screen, which the second top sites page is sitting outside of.

I can reproduce this when I don't have a second page of top sites so I think the RecyclerView (or its parents) is probably at fault here.

Comment 13

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

The layout with one page (and the broken layout) is:

HomePager
 TabMenuStrip
 ActivityStreamHomeScreen <- full width
  RecyclerView <- half width
   ViewPager
    TopSitesPage
 FrameLayout (bookmarks)
 LinearLayout (history)

The views we care about (HomePager, ASHomescreen, RV, VP, TSP) are all MATCH_PARENT in the width and nothing suspicious appears from the margin parameters.

The RecyclerView is mMeasuredWidth = 534, which is less than half of the ASHomeScreen, with getWidth = 1080.

Comment 14

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

I think the problem is that we're modifying layout parameters (padding) in onSizeChanged and non-child layout changes in onSizeChanged do not request a new layout.

---

In this code [1], called when we measure the children views (including the RecyclerView) from ActivityStreamHomePanel.onMeasure:

6568    protected void measureChildWithMargins(View child,
6569            int parentWidthMeasureSpec, int widthUsed,
6570            int parentHeightMeasureSpec, int heightUsed) {
6571        final MarginLayoutParams lp = (MarginLayoutParams) child.getLayoutParams();
6572
6573        final int childWidthMeasureSpec = getChildMeasureSpec(parentWidthMeasureSpec,
6574                mPaddingLeft + mPaddingRight + lp.leftMargin + lp.rightMargin
6575                        + widthUsed, lp.width);
6576        final int childHeightMeasureSpec = getChildMeasureSpec(parentHeightMeasureSpec,
6577                mPaddingTop + mPaddingBottom + lp.topMargin + lp.bottomMargin
6578                        + heightUsed, lp.height);
6579
6580        child.measure(childWidthMeasureSpec, childHeightMeasureSpec);
6581    }

On rotation, parentWidthMeasureSpec = the size of the whole screen while childWidthMeasure = a reduced size. It looks like mPaddingLeft is non-zero so I think when we measure, we're using the padding we set on the ASHomePanel for the landscape orientation despite measuring for portrait mode. When we later modify the padding parameters in onSizeChanged to the desired values, onMeasure is not called again so we're stuck with a width for the RecyclerView that was measured for a portrait width but with landscape padding.

[1]: http://androidxref.com/8.0.0_r4/xref/frameworks/base/core/java/android/view/ViewGroup.java#6573

Comment 15

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

Possible solutions:
1) Updating MarginLayoutParams, rather than padding, in onSizeChanged may cause a re-layout
2) We can directly update the width of the RecyclerView to the width we desire
3) We can override onMeasure (or whichever method) to provide more accurate measurements of our desired width, which should affect the measurement process, unlike onSizeChanged which seems to be a callback for "I'm all done measuring and laying out!"

I already tried:
- Calling requestLayout in onSizeChanged - it had no effect.

Comment 16

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

Attachment: Bug 1399683: Change padding of content view rather than self in onSizeChanged.

To be honest, I'm not sure why this works. onSizeChanged seems to be a callback
to notify the user that your layout has completed and not a place to update
layout params. However, it makes *slightly* more sense that you could update
your children's layout from this view, which is what this patch changes the
code to do.

I think I could figure out why this works if I dug further into [1] but I have
other things to focus on.

I don't think this is the most correct solution, but it is likely the smallest
and least risky change we can make to fix this issue, which is ideal because
we'd like to uplift this to the 57 Beta.

A more correct solution would override the Activity Stream RecyclerView and
provide different desired measurements to its parent so that the new size is
set *during* layout rather than after it, which would make the layout process
more efficient. However, this type of code is less commonly written day-to-day
by developers so it's harder to write, harder to maintain, and I'd have to read
a lot of [1] in order to write it. :)

[1]: https://developer.android.com/guide/topics/ui/how-android-draws.html

Review commit: https://reviewboard.mozilla.org/r/182432/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/182432/

Comment 17

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

(In reply to Michael Comella (:mcomella) from comment #15)
> Possible solutions:

Actually, I ended up using neither of these and stumbled upon an accidental solution that seems to work. See comment 16 ^.

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8910969 [details]
Bug 1399683: Change padding of content view rather than self in onSizeChanged.

https://reviewboard.mozilla.org/r/182432/#review187848

LGTM.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/75c688fefb30
Change padding of content view rather than self in onSizeChanged. r=sebastian

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8910969 [details]
Bug 1399683: Change padding of content view rather than self in onSizeChanged.

Uplift request for Firefox 57 Beta.

Approval Request Comment

[Feature/Bug causing the regression]: The "Activity Stream" panel is a new feature in 57.

[User impact if declined]: See attached screenshots. The UI can get into a unpleasant/unusable state after rotating the device.

[Is this code covered by automated tests?]: No.

[Has the fix been verified in Nightly?]: The fix just landed in mozilla-central.

[Needs manual test from QE? If yes, steps to reproduce]: Bogdan will verify the fix.

[List of other uplifts needed for the feature/fix]: -

[Is the change risky?]: Very low risk.

[Why is the change risky/not risky?]: Only minor change in code that isn't risky: Instead of applying padding to the "parent view" it's applied to the "content view".

[String changes made/needed]: -

Comment 21

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Sebastian Kaspari (:sebastian) from comment #20)
> [Has the fix been verified in Nightly?]: The fix just landed in
> mozilla-central.

Um, in autoland - not yet mozilla-central. :)

Comment 22

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

https://hg.mozilla.org/mozilla-central/rev/75c688fefb30

Comment 23

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8910969 [details]
Bug 1399683: Change padding of content view rather than self in onSizeChanged.

Fix an UX issue, taking it.
Should be in 57b3

Comment 24

[Bogdan Surd, QA [:bsurd, NI]]

I was no longer able to reproduce this, top sites tiles no longer overlap. Marking as verified for Nightly.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b18dcb98aa98

Comment 26

[Bogdan Surd, QA [:bsurd, NI]]

Verified in 57.0b3, issue is not reproducible.