Closed Bug 1534213 Opened 7 months ago Closed 7 months ago

Icon glitch when opening articles from Top Sites using context menu

Categories

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

Firefox 67
ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 68
Tracking Status
firefox66 --- unaffected
firefox67 + verified
firefox68 --- verified

People

(Reporter: sflorean, Assigned: petru)

References

Details

(Keywords: regression)

Attachments

(1 file)

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

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

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

Blocks: 1511946
Flags: needinfo?(petru.lingurar)

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.

Assignee: nobody → petru.lingurar
Status: NEW → ASSIGNED
Flags: needinfo?(petru.lingurar)

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

Flags: needinfo?(petru.lingurar)

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)

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

Flags: needinfo?(petru.lingurar)

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

Has Regression Range: --- → yes
Has STR: --- → yes
Priority: -- → P1
Keywords: checkin-needed

Pushed by nerli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0c5057ecdb64
Reduce the number of WebpageItemRow layout refreshes; r=JanH

Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
Flags: qe-verify+

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 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:
Attachment #9051973 - Flags: approval-mozilla-beta?
Flags: qe-verify+ → qe-verify?

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

Attachment #9051973 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify? → qe-verify+
QA Whiteboard: [qa-triaged]

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)
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.