1335451 - Improve the look of tiles for which we don't have a favicon

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Verdi [:verdi] Best to slack me]

In bug 1322737 we styled tiles with an initial and a color. This implementation used a lowercase initial and a range of mostly pale greens and blues for background colors. We want to update that styling to use an uppercase initial and wider range of bolder background colors.

Comment 1

[Dão Gottwald [:dao]]

D'oh, the hue is in degrees (0-360) but the algorithm wrongly stops at 255, so a whole range of colors is missing.

Comment 2

[Dão Gottwald [:dao]]

Attachment: Bug 1335451 - Tweak thumbnail placeholder colors and make placeholder letters uppercase.

Review commit: https://reviewboard.mozilla.org/r/108502/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/108502/

Comment 3

[:Gijs (he/him)]

Comment on attachment 8832145 [details]
Bug 1335451 - Tweak thumbnail placeholder colors and make placeholder letters uppercase.

https://reviewboard.mozilla.org/r/108502/#review109814

(In reply to Dão Gottwald [:dao] from comment #1)
> D'oh, the hue is in degrees (0-360) but the algorithm wrongly stops at 255,
> so a whole range of colors is missing.

Ugh, should have spotted that in review. Sorry.

::: browser/base/content/newtab/sites.js:262
(Diff revision 1)
>      if (THUMBNAIL_PLACEHOLDER_ENABLED &&
>          link.type == "history" &&
>          link.baseDomain) {
>        let placeholder = this._querySelector(".newtab-thumbnail.placeholder");
> -      let hue = 0;
> +      let charCodeSum = 0;
> +      const colors = 8;

Nit: seems other constants here use ALL_CAPS, so this should too.

::: browser/base/content/newtab/sites.js:266
(Diff revision 1)
> -      let hue = 0;
> +      let charCodeSum = 0;
> +      const colors = 8;
>        for (let c of link.baseDomain) {
> -        hue += c.charCodeAt(0);
> +        charCodeSum += c.charCodeAt(0);
>        }
> -      hue %= 256;
> +      let hue = Math.round((charCodeSum % colors) / colors * 360);

I'm a bit confused because AIUI this only uses 8 different colours (hues). Don't we want to use more? While we will only have 6 tiles on the funnelcake, if you assume an even spread of character codes, the birthday paradox means there is a high likelihood (>90%) that at least two tiles will have the same colour, and in fact a relatively high likelihood that at least 3 will have the same colour (23%) - see http://www.wolframalpha.com/input/?i=birthday+problem+calculator&rawformassumption=%22FSelect%22+-%3E+%7B%7B%22BirthdayProblem%22%7D%7D&rawformassumption=%7B%22F%22,+%22BirthdayProblem%22,+%22n%22%7D+-%3E%226%22&rawformassumption=%7B%22F%22,+%22BirthdayProblem%22,+%22pbds%22%7D+-%3E%228%22

Comment 4

[Dão Gottwald [:dao]]

Using "more" hues doesn't really help with that, because effectively some of them would be too close together to tell apart at a glance. I'm hoping using fewer hues cleans that up a bit. Whether we should distribute them differently to reduce duplicates is a separate question. Since these placeholders aren't supposed to stay permanently (except for minority cases where we don't take screenshots of pages), it's probably not a big deal.

Comment 5

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #4)
> Using "more" hues doesn't really help with that, because effectively some of
> them would be too close together to tell apart at a glance. I'm hoping using
> fewer hues cleans that up a bit. Whether we should distribute them
> differently to reduce duplicates is a separate question. Since these
> placeholders aren't supposed to stay permanently (except for minority cases
> where we don't take screenshots of pages), it's probably not a big deal.

Some of them are easier to distinguish than others (e.g. the same difference in numeric hue between reds seems "more different" to my eyes than that same difference is between some of the blues and greens). It feels to me like upping it to 16 would still be helpful, though I agree there's diminishing returns after that. ( https://jsbin.com/melicifudi/1/edit?html,js,output for context.)

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 8832145 [details]
Bug 1335451 - Tweak thumbnail placeholder colors and make placeholder letters uppercase.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/108502/diff/1-2/

Comment 7

[:Gijs (he/him)]

Comment on attachment 8832145 [details]
Bug 1335451 - Tweak thumbnail placeholder colors and make placeholder letters uppercase.

https://reviewboard.mozilla.org/r/108502/#review110096

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/133dcc512b73
Tweak thumbnail placeholder colors and make placeholder letters uppercase. r=Gijs

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/133dcc512b73

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 8832145 [details]
Bug 1335451 - Tweak thumbnail placeholder colors and make placeholder letters uppercase.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1322737

[User impact if declined]: this is wanted for the next funnelcake (bug 1332318)

[Is this code covered by automated tests?]: no

[Has the fix been verified in Nightly?]: no

[Needs manual test from QE? If yes, steps to reproduce]: will be covered as part of the funnelcake QA

[List of other uplifts needed for the feature/fix]: /

[Is the change risky?]: no

[Why is the change risky/not risky?]: it's behind a hidden pref (browser.newtabpage.thumbnailPlaceholder)

[String changes made/needed]: /

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8832145 [details]
Bug 1335451 - Tweak thumbnail placeholder colors and make placeholder letters uppercase.

Fix should only affect planned funnelcake builds, let's uplift to aurora and beta.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e03ffc294d7a

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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