848155 - Change - Add Colored Bar with Text to Bottom of Topsites Thumbnails

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: Firefox Start, defect, P1)

Description

[Allison Naaktgeboren :ally]

follow up from https://bugzilla.mozilla.org/show_bug.cgi?id=794028#c26

Sadly, this may block the story from completing. Sadness

Comment 1

[Matt Brubeck (:mbrubeck)]

Bug 794028 comment 26 says we want the "thumbnail dominant color" but I wonder if we should use the favicon dominant color instead.

For example, attachment 617342 [details] shows hulu.com with a "Hulu green" background.  The actual dominant color in the Hulu thumbnail will probably change depending on the page content.  Using the favicon we could more reliably find a color associated with the brand and its logo.

(Also, the mozIColorAnalizer code is designed for favicons, so its heuristics or performance might not be tuned as well for thumbnails.  I haven't actually tested to see if this is an issue in practice.)

Stephen, any comments?

Comment 2

[Stephen Horlander [:shorlander]]

(In reply to Matt Brubeck (:mbrubeck) from comment #1)
> Bug 794028 comment 26 says we want the "thumbnail dominant color" but I
> wonder if we should use the favicon dominant color instead.
> 
> For example, attachment 617342 [details] shows hulu.com with a "Hulu green"
> background.  The actual dominant color in the Hulu thumbnail will probably
> change depending on the page content.  Using the favicon we could more
> reliably find a color associated with the brand and its logo.
> 
> (Also, the mozIColorAnalizer code is designed for favicons, so its
> heuristics or performance might not be tuned as well for thumbnails.  I
> haven't actually tested to see if this is an issue in practice.)
> 
> Stephen, any comments?

That's a good point! I would be fine using the favicon's dominant color.

Comment 3

[Allison Naaktgeboren :ally]

(In reply to Stephen Horlander from comment #2)
> (In reply to Matt Brubeck (:mbrubeck) from comment #1)
> > Stephen, any comments?
> 
> That's a good point! I would be fine using the favicon's dominant color.

sgtm!

Comment 4

[Allison Naaktgeboren :ally]

Attachment: wip functional

Couple notes:
- functionally complete*
* in the case that there is no favicon, the default colors defined in the css sheet win (as in the patch skips over them). We didnt formally spec for that case, so I made an executive decision. If something else is desired, let me know.
- I left the test code in there in case you want to apply it & play with it
- How can we test?

Comment 5

[Matt Brubeck (:mbrubeck)]

Comment on attachment 727453 [details] [diff] [review]
wip functional

Review of attachment 727453 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  We might want to make it share some code with metro/base/content/bookmarks.js, which caches favicon colors to avoid recalculating them.  Maybe the common code (and some of the color code here) could move into the richgriditem binding, or just into a utility file somewhere.

Some ideas for testing:

For testing the actual calculation, you could pull the calculation and comparison into a "pure" function like "isLighterThan(aColor, aThresholdColor)" that doesn't touch the DOM or depend on any browser code.  Then we can test that part in an xpcshell test (or if the code lives in a file that is hard to load in xpcshell, then in a browser-chrome test similar to browser_canonizeURL.js).

If you want to test the DOM parts (without depending on the entire browser UI), your test could create a temporary richgriditem with some hard-coded properties, pass it to TopSites.colorizeTextBar (or directly to setTextColorsForContrast), then make sure the expected styles are set.

> * in the case that there is no favicon, the default colors defined in the
> css sheet win (as in the patch skips over them).

Sounds good to me.  I expect this won't be too common in the wild anyway.

Comment 6

[Allison Naaktgeboren :ally]

Attachment: draft 0, part 1/1

Couple notes: 
- refactoring of the colorized code to share between bookmarks & topsites will be part of 826556
- includes the change to bookmarks.js attribute setting so that this patch works in immersive mode. 
- I also removed stray comments I left on the original topsites patch in platform.css
- test bug is 855451

Comment 7

[Matt Brubeck (:mbrubeck)]

Comment on attachment 730336 [details] [diff] [review]
draft 0, part 1/1

Review of attachment 730336 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!  Further refinements can continue in those follow-up bugs.

::: browser/metro/base/content/bookmarks.js
@@ +236,1 @@
>        aItem.color = colorStr;

Now that bug 808770 has landed, you can replace "aItem.color = colorStr;" with:

  if (aItem.refresh)
    aItem.refresh();

refresh() does the same thing as the code above, but makes the intent slightly clearer.

Comment 8

[Marco Mucci [:MarcoM]]

If bug is resolved this evening I'll move it back to Iteration #4.  Otherwise, it can close out early in Iteration #5.

Comment 9

[Matt Brubeck (:mbrubeck)]

Comment on attachment 730336 [details] [diff] [review]
draft 0, part 1/1

Unfortunately it looks like bug 808770 has badly bitrotted this patch, and removed some code it depended on, so it can't land in its current state...  :(

Comment 10

[Allison Naaktgeboren :ally]

Attachment: fix to 826556, post sam's changes, utility library now

outstanding item - whether or not we should save the icon uri for a topsite, as we no longer save that in the items (it is part of all other tiles, but we have no explicit use for it right now)

annotations code removed for perf reasons (discussed offline)

Comment 11

[Allison Naaktgeboren :ally]

Comment on attachment 732117 [details] [diff] [review]
fix to 826556, post sam's changes, utility library now

there's a couple stray printfs, and a couple questions about isolating the primary favicon color code (my feeling at this point is somewhat 'no', but I could make it work) and saving the iconuri.

Comment 12

[Allison Naaktgeboren :ally]

Attachment: screenshot

disregard the annoying 2x4-not-3x3 layout bug. bookmark tiles have light or dark text colors, and topsites have colored bars with contrasting text

(note that fb & twitter use https, so there is no thumb for them)

Comment 13

[Matt Brubeck (:mbrubeck)]

Comment on attachment 732117 [details] [diff] [review]
fix to 826556, post sam's changes, utility library now

Review of attachment 732117 [details] [diff] [review]:
-----------------------------------------------------------------

r=mbrubeck.  Feel free to land with the various nits and OCD-ish comments addressed, and with the debug code/comments removed.  Or I can glance over the final draft if you like.

> diff --it a/browser/metro/base/content/colorUtils.js b/browser/metro/base/content/colorUtils.js
> new file mode 100644

It looks like this empty file got left in the patch by accident.

::: browser/metro/base/content/TopSites.js
@@ +226,5 @@
>    updateTile: function(aTileNode, aSite, aArrangeGrid) {
>      if (this._useThumbs) {
>        aSite.backgroundImage = 'url("'+PageThumbs.getThumbnailURL(aSite.url)+'")';
> +      //TODO is there still an iconURI field on item that should be set?
> +      PlacesUtils.favicons.getFaviconURLForPage(Util.makeURI(aSite.url), function(iconURLfromSiteURL) {

Yes, there's an "iconURI" attribute (and associated "iconSrc" property) that is used for displaying the icon in non-thumbnail tiles.

I think we want to move this code out of the "if (this._useThumbs)" statement and set "aTileNode.iconSrc = iconURLfromSiteURL;" so the icon and color will appear for non-thumbnail tiles (in snapped view).

@@ +227,5 @@
>      if (this._useThumbs) {
>        aSite.backgroundImage = 'url("'+PageThumbs.getThumbnailURL(aSite.url)+'")';
> +      //TODO is there still an iconURI field on item that should be set?
> +      PlacesUtils.favicons.getFaviconURLForPage(Util.makeURI(aSite.url), function(iconURLfromSiteURL) {
> +        if(iconURLfromSiteURL) {

Nit: add a space after "if"

Also, some Mozilla developers are obsessed with using early returns in code like this, i.e., "if (!var) return;"

Personally I don't really care, but please modify this to use an early return, for consistency and to appease our other reviewers. :)

@@ +230,5 @@
> +      PlacesUtils.favicons.getFaviconURLForPage(Util.makeURI(aSite.url), function(iconURLfromSiteURL) {
> +        if(iconURLfromSiteURL) {
> +          //this gets the image locally, not from network
> +          let faviconURL = (PlacesUtils.favicons.getFaviconLinkForIcon(iconURLfromSiteURL)).spec;
> +          let xpFaviconURL = Util.makeURI(faviconURL.replace("moz-anno:favicon:",""));

Naming suggestion: We're not 100% consistent about it, but I find it a useful signal to use "URI" instead of "URL" when a variable refers to an nsIURI object.

::: browser/metro/base/content/bookmarks.js
@@ +197,5 @@
> +    if (aIconUri) {
> +      ColorUtils.getForegroundAndBackgroundIconColors(aIconUri, function(foregroundColor, backgroundColor) {
> +        aItem.color = backgroundColor; //set custom color of tile
> +        aItem.style.color = foregroundColor; //color the text fixes bug 826556
> +        });

Whitespace nit: I personally prefer the "});" to be indented one level less.  No real rationale, just aesthetics/consistency.

::: browser/metro/modules/Makefile.in
@@ +16,1 @@
>    $(NULL)

Whitespace nit: Looks like this adds a real tab, which is generally fine in Makefile code, but doesn't match the surrounding lines.

You might need to ":set et" and then ":retab" to convince Vim to add spaces here instead.

::: browser/metro/modules/colorUtils.jsm
@@ +14,5 @@
> +  // Takes an icon and a success callback (who is expected to handle foreground color
> +  //  & background color as is desired)
> +  // aSuccessCallback(foregroundColor, backgroundColor)
> +  getForegroundAndBackgroundIconColors: function getForegroundAndBackgroundIconColors(aIconURI, aSuccessCallback) {
> +    if(aIconURI) {

Nit: space after "if"; convert to early return.

@@ +29,5 @@
> +    }
> +  },
> +  // TODO can we isolate out the primary favicon color code?
> +  getIconPrimaryColor: function getIconPrimaryColor(aIcon) {
> +  },

I agree with your comment that factoring this out further is not worth it; mozIColorAnalyzer already encapsulates this pretty well.

@@ +37,5 @@
> +  bestTextColorForContrast: function bestTextColorForContrast(aColor) {
> +    let r = (aColor & 0xff0000) >> 16;
> +    let g = (aColor & 0x00ff00) >> 8;
> +    let b = (aColor & 0x0000ff);
> +    let w3cContrastValue = ((r*299)+(g*587)+(b*114))/1000;

Could you add a comment linking to the W3C document where this algorithm is defined?

@@ +41,5 @@
> +    let w3cContrastValue = ((r*299)+(g*587)+(b*114))/1000;
> +    w3cContrastValue = Math.round(w3cContrastValue);
> +    let textColor = "rgb(255,255,255)";
> +
> +    if( w3cContrastValue > 125) {

Nit: space after "if" but not after "("

Comment 14

[Allison Naaktgeboren :ally]

Attachment: draft 2, part 1/1

Comment 15

[Allison Naaktgeboren :ally]

Comment on attachment 732558 [details] [diff] [review]
draft 2, part 1/1

Since you generously offered to do another round.. :)

Comment 16

[Matt Brubeck (:mbrubeck)]

Attachment: follow-up patch for an XBL issue

>+        aTileNode.style.color = foreground; //color text
>+        let textBox = document.getAnonymousElementByAttribute(aTileNode, "class", "richgrid-item-desc");
>+        textBox.style.backgroundColor = background; // color box behind text

While test-driving this patch, I found a weird case where this code was throwing a "textBox is null" exception.  Steps to reproduce:

1. In the options flyout, change the startup option to "Tabs from last time".
2. Open a page other than about:start.  Close all other tabs.
3. Quit Metro Firefox (Alt+F4, or drag down from top edge).
4. Re-launch Metro Firefox.
5. Open a new tab.

When this happens, the foreground color gets applied but the background color doesn't.

In the process of trying to reproduce and debug this, I was close enough to fixing it that I figured I'd just finish up my patch and attach it here.  It looks like this is happening because the XBL binding isn't attached yet (yes, again!) and so we can solve it using refresh (again).

Comment 17

[Allison Naaktgeboren :ally]

Attachment: combined patch(what will actually land)

Comment 18

[Allison Naaktgeboren :ally]

Comment on attachment 732615 [details] [diff] [review]
follow-up patch for an XBL issue

Review of attachment 732615 [details] [diff] [review]:
-----------------------------------------------------------------

\o/ I rolled it into my patch

There is a possible glitch. when we have https (or other sites we dont capture) we get a solid colored background. Personally I like this change from the blank white tiles, and I'm inclined to land it that way (when the tree opens anyway)

Comment 19

[Allison Naaktgeboren :ally]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ecd66e8c854f

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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

Comment 21

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (Windows NT 6.2; WOW64; rv:23.0) Gecko/20130404 Firefox/23.0

Verified that for a white top site tile background, the foreground text is black. Text is readable. 

Also verified that for black top site tile background, the text is white and ok to read.

For https sites, the tab bar isn't noticeable because the whole thumbnail box has the same color as the bar. I assume that's ok.

Comment 22

[Virgil Dicu [:virgil] [QA]]

Attachment: Top sites thumbnail for https sites

Comment 23

[Samvedana (:Samvedana)]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130825030201
Built from http://hg.mozilla.org/mozilla-central/rev/01576441bdc6

WFM
Tested on windows 8 using latest nightly for iteration-12. I observed colored bar with text to bottom of Topsites thumbnails.