Closed Bug 1399683 Opened 2 years ago Closed 2 years ago

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

Categories

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

All
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 58
Iteration:
1.30
Tracking Status
fennec + ---
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- verified
firefox58 --- verified

People

(Reporter: mcomella, Assigned: mcomella)

Details

(Keywords: regression, Whiteboard: [mobileAS])

Attachments

(5 files)

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)
If we can't reproduce this after a little trying, it's probably not worth tracking down.
Flags: needinfo?(bogdan.surd)
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
Flags: needinfo?(bogdan.surd)
Attached file 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)
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.
Bogdan, when you're able to reproduce, do the top sites eventually fix themselves or does it stay stuck overlapping?
Flags: needinfo?(bogdan.surd)
(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.
Flags: needinfo?(bogdan.surd)
Summary: Rotating device re-layouts top sites to overlap on Nexus S → Rotating device re-layouts top sites to overlap sometimes
Summary: Rotating device re-layouts top sites to overlap sometimes → Rotating device re-layouts Activity Stream panel to fit half the screen sometimes
This could be more critical for tablets (since you're more likely to use landscape) but we have higher priority issues.
tracking-fennec: ? → +
Rank: 2
Priority: -- → P2
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.
Severity: normal → major
Seems more important than before: re-adding to triage.
tracking-fennec: + → ?
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.
tracking-fennec: ? → +
Rank: 2
Priority: P2 → P1
Iteration: --- → 1.30
Assignee: nobody → michael.l.comella
It looks like both pages of the top sites are getting smushed into the same screen.
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.
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.
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
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.
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/
Attachment #8910969 - Flags: review?(liuche)
(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 ^.
Attachment #8910969 - Flags: review?(liuche) → review?(s.kaspari)
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.
Attachment #8910969 - Flags: review?(s.kaspari) → review+
Pushed by s.kaspari@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/75c688fefb30
Change padding of content view rather than self in onSizeChanged. r=sebastian
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]: -
Attachment #8910969 - Flags: approval-mozilla-beta?
(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. :)
https://hg.mozilla.org/mozilla-central/rev/75c688fefb30
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
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
Attachment #8910969 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I was no longer able to reproduce this, top sites tiles no longer overlap. Marking as verified for Nightly.
Verified in 57.0b3, issue is not reproducible.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.