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

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: verdi, Assigned: dao)

Tracking

(Blocks 1 bug)

52 Branch
Firefox 54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed, firefox53 fixed, firefox54 fixed)

Details

Attachments

(1 attachment)

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.
Assignee: nobody → dao+bmo
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 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
Attachment #8832145 - Flags: review?(gijskruitbosch+bugs)
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.
(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 on attachment 8832145 [details]
Bug 1335451 - Tweak thumbnail placeholder colors and make placeholder letters uppercase.

https://reviewboard.mozilla.org/r/108502/#review110096
Attachment #8832145 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/133dcc512b73
Tweak thumbnail placeholder colors and make placeholder letters uppercase. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/133dcc512b73
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
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]: /
Attachment #8832145 - Flags: approval-mozilla-beta?
Attachment #8832145 - Flags: approval-mozilla-aurora?
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.
Attachment #8832145 - Flags: approval-mozilla-beta?
Attachment #8832145 - Flags: approval-mozilla-beta+
Attachment #8832145 - Flags: approval-mozilla-aurora?
Attachment #8832145 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.