Show enhanced content image when the tile is unhovered

VERIFIED FIXED in Firefox 33

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Mardak, Assigned: Mardak)

Tracking

(Blocks 1 bug)

Trunk
Firefox 34
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox33 verified, firefox34 verified)

Details

Attachments

(3 attachments, 5 obsolete attachments)

For directory tiles with enhanced (unhovered) content, show that image and switch to the regular image when hovered.
Flags: firefox-backlog+
Posted patch v1 (obsolete) — Splinter Review
Assignee: nobody → edilee
Status: NEW → ASSIGNED
Attachment #8452929 - Flags: review?(adw)
Posted image v1 screenshot (obsolete) —
Screenshot with a sample engadget enhanced sponsored tile (with all the other directory tiles removed).
QA Whiteboard: [qa+]
Iteration: 33.3 → ---
Posted patch v1.1 (obsolete) — Splinter Review
Added an enhanced content for one of the directory tiles. (webmaker)
Also updated to latest m-c so including bug 1031942.
Attachment #8452929 - Attachment is obsolete: true
Attachment #8452929 - Flags: review?(adw)
Attachment #8453191 - Flags: review?(adw)
Posted patch v2 (obsolete) — Splinter Review
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.
Attachment #8453191 - Attachment is obsolete: true
Attachment #8453191 - Flags: review?(adw)
Attachment #8453456 - Flags: review?(adw)
Attachment #8453249 - Attachment is obsolete: true
clarkbw says the non-enhanced logo should replace the thumbnail as well.
Posted patch v3Splinter Review
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.
Attachment #8453456 - Attachment is obsolete: true
Attachment #8453456 - Flags: review?(adw)
Attachment #8455440 - Flags: review?(adw)
QA Contact: cornel.ionce
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.
Attachment #8455440 - Flags: review?(adw) → review+
(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.
(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?
(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?
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.  :-) :-/
Er, the page is updated because DirectoryLinksProvider notifies onManyLinksChanged, but nodes are not reused, so, yes, nevermind.
Depends on: 1036284
Blocks: 1042204
https://hg.mozilla.org/mozilla-central/rev/a66707ee8f36
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Iteration: --- → 34.1
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;
Blocks: 1043525
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?
Flags: needinfo?(edilee)
(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).
Flags: needinfo?(edilee)
Depends on: 1044067
(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.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa+] → [qa!]
Blocks: 1045760
Depends on: 1030892
Depends on: 1050705
Depends on: 1053937
Blocks: 1057602
Uplift has been managed in bug 1057602
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.
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?
Also the new looking new tab page tabs/containers in FF33 are very kid looking, the old way was more profesional looking.
Depends on: 1071088
You need to log in before you can comment on or make changes to this bug.