1874693 - Flickering favicon on https://dgtized.github.io/shimmers/#/sketches

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect, P3)

Description

[Mayank Bansal]

Go to https://dgtized.github.io/shimmers/#/sketches and keep this tab in the foreground

AR: The favicon is flickering
ER: Not so

Regressed by:
Bug 1845035 - Invalid favicon causes the page to inherit favicon from the previously visited page. r=dao,tabbrowser-reviewers

Differential Revision: https://phabricator.services.mozilla.com/D194758

Comment 1

[Mayank Bansal]

Attachment: about:support

Comment 2

[Marco Bonardo [:mak]]

It looks like this is a page that continuously replaces the favicon for digital art purposes. It's an edge case, but it works in other browsers.
It's imo less problematic than the original bug, as it's more confusing and dangerous to get the wrong favicon than a flickering one.

The only workaround I could think about so far, is adding a "purpose" argument to setIcon to distinguish a page load from a page icon replacement, maybe reusing again beforePageShow from FaviconLoader. Anything after pageshow would not clear the attribute and thus not flicker. It should be as easy as letting .beforePageShow pass-through.
Otherwise we should undo Bug 1845035 and rather modify the image loading procedure to clear the image if the new one is unreadable, but I suspect that may affect the Web as it's loading the "image" attribute.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1845035

Comment 4

[Donal Meehan [:dmeehan]]

:mak/:dao could this be triaged?
Is there anything you could do in a patch that we could take in a ride-long for a Fx122 dot release?

Comment 5

[Marco Bonardo [:mak]]

(In reply to Donal Meehan [:dmeehan] from comment #4)

:mak/:dao could this be triaged?
Is there anything you could do in a patch that we could take in a ride-long for a Fx122 dot release?

Do you think this is critical to be fixed in 122, or are you just asking for us to assess that?
I don't see this as something users may hit easily, thus I think we could just work on a patch for the next version. Unless Dao disagrees, as he's the module owner here, so let's await for his evaluation.
I could work on a patch implementing comment 2.

Comment 6

[Donal Meehan [:dmeehan]]

....or are you just asking for us to assess that?

Thanks for double-checking, yes I'm asking for your assessment on impact.

Comment 7

[Marco Bonardo [:mak]]

Attachment: Bug 1874693 - Setting favicon repeatedly causes flicker. r=dao

The test is updated to take the new behavior into account (empty icon must be
set before pageshow), but it doesn't cover the flicker case, as that's complex.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e038f030b6e8
Setting favicon repeatedly causes flicker. r=dao,tabbrowser-reviewers

Comment 9

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/e038f030b6e8

Comment 10

[Marco Bonardo [:mak]]

Attachment: Bug 1874693 - Setting favicon repeatedly causes flicker. r=dao

The test is updated to take the new behavior into account (empty icon must be
set before pageshow), but it doesn't cover the flicker case, as that's complex.

Original Revision: https://phabricator.services.mozilla.com/D199092

Comment 11

[Phabricator Automation]

Uplift Approval Request

• Risk associated with taking this patch: low
• String changes made/needed: none
• Explanation of risk level: We are partially restoring the pre-regression behavior, the code change is simple
• User impact if declined: Tab favicon flashing on certain Web pages
• Is Android affected?: no
• Fix verified in Nightly: no
• Code covered by automated testing: no
• Steps to reproduce for manual QE testing: Steps are in the opening comment
• Needs manual QE test: yes

Comment 12

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/910cf83cb449

Comment 13

[Daniel Bodea [:danibodea]]

I have reproduced the issue in Release v122.0. This issue no longer occurs in Beta v123.0b2 or Nightly v124.0a1. This issue was reproduced and verified in Windows 10 and MacOS 11.