1933158 - Shortcuts with adaptive SVG favicons have poor contrast with dark new tab page background

Status: VERIFIED FIXED

(Toolkit :: Places, defect, P3)

Description

[Kestrel]

Attachment: CBS icon lacks contrast with dark new tab background

STR:

1. Enable the Light theme.
2. Pin https://www.cbsnews.com to the new tab page shortcuts.
3. Change the new tab background color/image to dark.

Expected:
CBS icon stays black against white background.

Actual:
CBS icon is white against white background unless hovered.

The image was previously PNG but recently changed to SVG.

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=47b1ae23989b8389b90ec9fed715d9eae6455ad7&tochange=ec6364b47f312809587d58ec8fe38e02d7f78976

Regressed by Bug 1494016.

Comment 1

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

:yazan, since you are the author of the regressor, bug 1494016, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 2

[Marco Bonardo [:mak]]

Hm, this is a mask-icon, as defined by <link rel="shortcut icon mask-icon", and we're not supporting them yet. We should not store it.

Apparently the check at https://searchfox.org/mozilla-central/rev/7987501f2c2ed1914e5c682bd328ace9c4a7c6cd/browser/actors/LinkHandlerChild.sys.mjs#115 is wrong? mask icons have a "color" attribute, not a "mask" attribute... though it may be safer to check if rel includes "mask-icon". The color is supposed to be "#101010", but since we're not supporting it, it's broken here.

Unfortunately even after fixing the bug the icon may stay broken for a few days.

We should really fix Bug 1337397 at a certain point.

Comment 3

[Marco Bonardo [:mak]]

To clarify, this means bug 1494016 just uncovered an existing bug.

Comment 4

[Yazan Al Macki [:yazan]]

Attachment: Bug 1933158 - Prevent mask icons from being saved and remove existing mask icons from DB. r?#places-reviewers

Comment 5

[Yazan Al Macki [:yazan]]

Attachment: Bug 1933158 - Prevent mask icons from being stored. r?mak

Comment 6

[Yazan Al Macki [:yazan]]

Attachment: Bug 1933158 - Prevent mask icons from being stored.

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

Comment 7

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: Mask-icon svg favicons will incorrectly remain to be displayed, causing the user to see cropped/incorrectly colored icons in components like new tab and history sidebar.
• Code covered by automated testing: yes
• Fix verified in Nightly: no
• Needs manual QE test: yes
• Steps to reproduce for manual QE testing: Enable the Light theme. Pin https://www.cbsnews.com to the new tab page shortcuts. Then change the new tab background color/image to dark. Icon should be black against a white background.
• Risk associated with taking this patch: Low
• Explanation of risk level: We are not directly modifying any pre-existing icons in the user's DB, we are preventing incorrect icons from re-stored and hence the previously stored ones will naturally expire in a few days.
• String changes made/needed: N/A
• Is Android affected?: no

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d6bb285060b6
Prevent mask icons from being stored. r=mak

Comment 9

[Serban Stanca [:SerbanS]]

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

Comment 10

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/0f9628723836

Comment 11

[Giorgia Nichita, Release Desktop QA]

Reproduced this issue on Win 10 using an affected nightly build from 2024-11-25 and following the steps from Comment 0.
Verified as fixed on Firefox 135.0a1 (20241209213901) and Firefox 134.0b8 (20241209091406) on Win 10, macOS 11 and Ubuntu 22.04.