1036299 - Show enhanced content image when the tile is unhovered

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect)

Description

[Ed Lee :Mardak]

For directory tiles with enhanced (unhovered) content, show that image and switch to the regular image when hovered.

Comment 1

[Ed Lee :Mardak]

Attachment: v1

Comment 2

[Ed Lee :Mardak]

Attachment: v1 screenshot

Screenshot with a sample engadget enhanced sponsored tile (with all the other directory tiles removed).

Comment 3

[Ed Lee :Mardak]

Attachment: v1.1

Added an enhanced content for one of the directory tiles. (webmaker)
Also updated to latest m-c so including bug 1031942.

Comment 4

[Ed Lee :Mardak]

Attachment: v1.1 screenshot

https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-689ef8f6dd25/try-macosx64/firefox-33.0a1.en-US.mac.dmg

Comment 5

[Ed Lee :Mardak]

Attachment: v2

Was going to do the enhanced history replacement in a separate patch and leave this bug for showing just enhanced directory, but it was just as simple to do both.

Comment 6

[Ed Lee :Mardak]

Attachment: v2 screenshot (unhovered)

Comment 7

[Ed Lee :Mardak]

Attachment: v2 screenshot (hovered)

Comment 8

[Ed Lee :Mardak]

clarkbw says the non-enhanced logo should replace the thumbnail as well.

Comment 9

[Ed Lee :Mardak]

Attachment: v3

Save an enhanced link instead of just the enhanced image so we can replace both the unhovered and hovered images with data from the enhanced link.

Comment 10

[Drew Willcoxon :adw]

Comment on attachment 8455440 [details] [diff] [review]
v3

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

Sorry for the delay.

This looks good, but it could probably use a newtab test that tests hovering.

I wonder how much performance will be impacted by drawing two images per enhanced tile.  If it becomes a problem, we could probably switch background-images on hover on the normal .newtab-thumbnail instead of having a separate .enhanced-content and hiding it.  It could maybe even be done entirely in CSS using attr(), but that would require storing these giant image strings as DOM attributes, which seems not great -- but turning those strings into ArrayBuffers and using blob URIs would fix that.

::: browser/base/content/newtab/sites.js
@@ +164,3 @@
>      thumbnail.style.backgroundImage = 'url("' + uri + '")';
> +
> +    if (link.enhancedImageURI) {

Is it possible for this to go the other way: DirectoryLinksProvider.getEnhancedLink() returns undefined for this link when previously it had returned a link with enhancedImageURI?  Seems like it is.

Comment 11

[Ed Lee :Mardak]

(In reply to Drew Willcoxon :adw from comment #10)
> This looks good, but it could probably use a newtab test that tests hovering.
What would this check for in the test? Right now it's just css fading out the unhovered image on hover.

> we could probably switch background-images on hover on the normal .newtab-thumbnail 
That could be interesting with your attr() idea, and the image URIs will probably switch to urls down the line, so it won't be huge strings.

The current implementation does allow for the opacity fading transition as well as making sure the image is already loaded.

> ::: browser/base/content/newtab/sites.js
> > +    if (link.enhancedImageURI) {
> Is it possible for this to go the other way:
> DirectoryLinksProvider.getEnhancedLink() returns undefined for this link
> when previously it had returned a link with enhancedImageURI?  Seems like it
> is.
When getEnhancedLinks() returns undefined, the caller (refreshThumbnail) then defaults to the current link, which would not have an enhancedImageURI.

Comment 12

[Drew Willcoxon :adw]

(In reply to Ed Lee :Mardak from comment #11)
> What would this check for in the test? Right now it's just css fading out
> the unhovered image on hover.

Well, the feature is that when you hover over a tile, the image changes.  Seems like that deserves a test, no matter how it's implemented.  But, nevermind, let's forget I said it.

> When getEnhancedLinks() returns undefined, the caller (refreshThumbnail)
> then defaults to the current link, which would not have an enhancedImageURI.

I get that part, but in that case, the node will keep its type="enhanced" attribute, and .enhanced-content will keep its background image, which is wrong, no?

Comment 13

[Ed Lee :Mardak]

(In reply to Drew Willcoxon :adw from comment #12)
> > When getEnhancedLinks() returns undefined, the caller (refreshThumbnail)
> > then defaults to the current link, which would not have an enhancedImageURI.
> I get that part, but in that case, the node will keep its type="enhanced"
> attribute, and .enhanced-content will keep its background image, which is
> wrong, no?
Ahhh. I see what you're getting at. For a cell that was previously enhanced but now used for something that is not enhanced. It looks like this doesn't happen as new sites are created when necessary. Is there a case you're thinking of where an existing site node is reused?

Comment 14

[Drew Willcoxon :adw]

Sorry, I should have stated it plainly.  Directory links are periodically refreshed in the background, right?  So if a link has an enhanced image at one point, and it appears in a particular newtab page, and then links are refreshed and that link no longer has an enhanced image but its tile still appears on that same newtab page...

... but existing newtab pages are not updated when new directory links are fetched, so I was imagining a case that doesn't exist.  Nevermind, again.  :-) :-/

Comment 15

[Drew Willcoxon :adw]

Er, the page is updated because DirectoryLinksProvider notifies onManyLinksChanged, but nodes are not reused, so, yes, nevermind.

Comment 16

[Carsten Book [:Tomcat]]

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

Comment 17

[Alfred Kayser]

A small optimization in the .css would be to replace:
.newtab-site[type=affiliate] .newtab-thumbnail,
.newtab-site[type=organic] .newtab-thumbnail,
.newtab-site[type=sponsored] .newtab-thumbnail,
.newtab-site[type=enhanced] .newtab-thumbnail{
    background-position: center;
with:
.newtab-site:-moz-any([type=affiliate],[type=organic],[type=sponsored],[type=enhanced]) .newtab-thumbnail {
    background-position: center center;

Comment 18

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:34.0) Gecko/20100101 Firefox/34.0
Mozilla/5.0 (X11; Linux i686; rv:34.0) Gecko/20100101 Firefox/34.0

Verified on latest Nightly, Build ID: 20140725030202. 
The directory tiles content changes accordingly (hovered/unhovered state).

The only thing I would like to mention is that "Get to know Mozilla - Mozilla" tile is not displayed. Tried with a clean profile and deleted tiles until the last one became empty but "Get to know Mozilla - Mozilla" was not displayed.
Any thoughts?

Comment 19

[Ed Lee :Mardak]

(In reply to Cornel Ionce [QA] from comment #18)
> The only thing I would like to mention is that "Get to know Mozilla -
> Mozilla" tile is not displayed.
Ah, sorry for the confusion! That tile is actually a history tile not a directory tile. I got that by typing "m" in the location bar and selecting the first result (which is the bookmark to the about page).

Comment 20

[Cornel Ionce [:noni] [Hubs QA]]

(In reply to Ed Lee :Mardak from comment #19)
> Ah, sorry for the confusion! That tile is actually a history tile not a
> directory tile. I got that by typing "m" in the location bar and selecting
> the first result (which is the bookmark to the about page).

Thank you!
In this case I'll mark this issue verified.

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Uplift has been managed in bug 1057602

Comment 22

[Ed Lee :Mardak]

https://hg.mozilla.org/releases/mozilla-aurora/rev/35f5ce3cfaaa

Comment 23

[Cornel Ionce [:noni] [Hubs QA]]

Verified on nightly based on comment 20.

Tested with latest Aurora, build ID: 20140828004002 on Windows 7 64bit, Mac OS X 10.8.5 and Ubuntu 14.04 32bit and everything looks good.

Comment 24

[noitidart]

I only have 6 pinned tabs. The other 3 from my history are always junk. I would like to set theese 3 to be enhanced tiles, is that posible?

Comment 25

[noitidart]

Also the new looking new tab page tabs/containers in FF33 are very kid looking, the old way was more profesional looking.