1534213 - Icon glitch when opening articles from Top Sites using context menu

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect, P1)

Description

[Sorina Florean [:sflorean]]

Environment:
Device:

• Huawei MediaPad M2 (Android 5.1.1);
• Samsung Galaxy S8 (Android 8.0)
• Nexus 6P (Android 8.1.0)

Build: Nightly 67.0a1 (2019-03-10);

Steps to reproduce:

1. Go to Top Sites Panel.
2. Tap on three dots menu from the Pocket first article.
3. Choose "Open in a new tab".
4. Observe the article icon.

Expected result:
No visual effects are displayed.

Actual result:
An icon glitch is displayed after the article is opened in a new tab.

Notes:

• Reproducible also for Highlights section.
• If one article from "Recommended by Pocket" is listed in the Highlights section, the glitch is displayed for both entries.
• Probably due to bug 1511946 because on Beta 66 this is not reproducible, and "Switch to tab" is not displayed.

Video: https://drive.google.com/open?id=1tPyG2XNBtsw6dMyhqR3ajQ4z4ykgcBia

Comment 1

[Eliza Balazs]

Hi!

I tested this on Samsung Galaxy S8 (Android 8) and I found a regression for this issue.

Last good revision: 4f44987ada9476da27ebe098e583d1653e5e871f
First bad revision: 8ca888f573652270684dc7b85d1a796fbd4c5f0b

Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=4f44987ada9476da27ebe098e583d1653e5e871f&tochange=8ca888f573652270684dc7b85d1a796fbd4c5f0b

• Issue caused by Bug 1511946 - Switch to tab icon and text doesn`t appear on Top Sites

Comment 2

[Pascal Chevrel:pascalc (PTO until April 26)]

Petru, could you look at this regression caused by bug 1511946 please? Thanks

Comment 3

[Petru-Mugurel Lingurar [:petru]]

If I understand correctly the "glitch" is that small flicker for the just opened article?

I disagree with the expected result - "No visual effects are displayed".
Modifying the visuals of that articles is the intended purpose of the patch from bug 1511946.
We will refresh the layout of that article to show or hide the new "switch to tab" text whenever the status of it's Tab gets changed because the actual height of the article in the list gets changed.
Will check though if it's possible to reduce the number of refreshes in the event that a Tab for that article is already opened / not all Tabs for that article are closed because in this cases indeed, refreshing the layout serves no purpose.

Comment 4

[Neha Kochar [:neha]]

Petru, is this on target to get resolved for 67?

Comment 5

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1534213 - Reduce the number of WebpageItemRow layout refreshes; r?JanH

Previously, WebpageItemRow's layout would be updated everytime a new Tab for
it's url would be ADDED/CLOSED/LOCATION_CHANGED to force a recheck of the need
to show the "Switch to tab hint".
To prevent multiple layout refreshes for every such event we will check the
current display status of the "Switch to tab hint" against the newly computed
value after an ADDED/CLOSED/LOCATION_CHANGED event was received.
Eagerly changing the value for 'switchToTabHintShown' along with informing
about the need for layout refresh to prevent any race conditions between
receiving more events and actualy refreshing the layout.
(In my tests I saw ADDED/LOCATION_CHANGE refreshing the same layout needlessly)

Comment 6

[Petru-Mugurel Lingurar [:petru]]

(In reply to Neha Kochar [:neha] from comment #4)

Petru, is this on target to get resolved for 67?

Will probably request an uplift to Beta 67 given the small risk of the minor change above.

Comment 7

[Neha Kochar [:neha]]

[Tracking Requested - why for this release]:
Based on comment 6 from :petru.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0c5057ecdb64
Reduce the number of WebpageItemRow layout refreshes; r=JanH

Comment 9

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/0c5057ecdb64

Comment 10

[Sorina Florean [:sflorean]]

Verified as fixed on latest Nightly build (03/22/2019).
Devices:

• Huawei MediaPad M2 (Android 5.1.1)
• Huawei P9 Lite (Android 6.0.1)
• HTC 10 (Android 8.0.0)
• Xiaomi Mi4i (Android 5.0.2)

I will remove the qe-verify+ flag after we test on Beta 67.

Comment 11

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 9051973 [details]
Bug 1534213 - Reduce the number of WebpageItemRow layout refreshes; r?JanH

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: Bug 1511946
• User impact if declined: Small flicker of a TopSite item when opening it in a new Tab. Caused by a repeatedly refreshing it's layout.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: - Go to Top Sites Panel.

• Tap on three dots menu from the Pocket first article.
• Choose "Open in a new tab".
• Observe article row

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Not risky because of the small change needed, already verified in Nightly.
• String changes made/needed:

Comment 12

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9051973 [details]
Bug 1534213 - Reduce the number of WebpageItemRow layout refreshes; r?JanH

Fix for a fennec P1 regression, verified on Nightly, approved for 67 beta 5, thanks

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/8c19d4132c70

Comment 14

[Sorina Florean [:sflorean]]

Verified as fixed on Firefox 67.0b5.
Devices:

• Xiaomi Mi4i (Android 5.0.2)
• Google Pixel(Android 9)
• Huawei P9 Lite (Android 6.0)
• Motorola Nexus 6 (Android 7.1.1)
• Samsung Galaxy Note 8 (Android 9)