Remove history thumbnail/title replacing functionality

VERIFIED FIXED in Firefox 38

Status

()

defect
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: Mardak, Assigned: emtwo)

Tracking

Trunk
Firefox 39
Points:
3
Dependency tree / graph

Firefox Tracking Flags

(firefox38 verified, firefox39 verified)

Details

(Whiteboard: .002)

Attachments

(2 attachments)

Reporter

Description

4 years ago
The Enhnaced Tiles functionality has been causing confusion without much adoption because we change the image and text without breaking the user's expected behavior of going to a specific link.

It also causes bugs that need to be fixed like bug 1105360 where some enhanced tiles should not show up as directory or some directory tiles with rollover images should not show up as enhanced.
Reporter

Updated

4 years ago
Depends on: 1030832
Assignee

Updated

4 years ago
Assignee: nobody → msamuel
Reporter

Comment 1

4 years ago
This might not be necessary for the affiliate suggested tiles depending on how we implement it. I was originally concerned that the rollover images on suggested tiles would confuse the enhanced tiles implementation.

But if we do keep around enhanced tiles with affiliate suggested tiles, we would need to rethink bug 1126186 where the menu is simpler without needing to support enhanced tiles. If we do keep enhanced, we would either 1) add an extra menu item or 2) use the same "suggestions" menu item to turn off enhanced tiles. Both have drawbacks of confusing users or causing the user to unnecessarily turn off suggested tiles.
Assignee

Updated

4 years ago
Attachment #8584735 - Attachment is patch: true
Reporter

Comment 3

4 years ago
Comment on attachment 8584735 [details] [diff] [review]
v1: Remove thumbnail/title replacing functionality for history tiles

>+++ b/browser/base/content/newtab/sites.js
>   _render: function Site_render() {
>-    let enhanced = gAllPages.enhanced && DirectoryLinksProvider.getEnhancedLink(this.link);
>+    let enhanced = gAllPages.enhanced && this.link.type != "history" &&
>+      DirectoryLinksProvider.getEnhancedLink(this.link);
An alternative approach that might be simpler is to not add in links to the data structure behind getEnhancedLink. A directory tile and suggested tile with a enhancedImageURI should still have that attribute on the link. If we add nothing to the data structure, nothing will become additionally enhanced, i.e., history tiles, but already enhanced directory/suggested tiles should just work. (I haven't touched that code for a while, so I'm not sure what this approach might break...)
Reporter

Comment 4

4 years ago
Prioritizing this removal of enhancing history functionality because it's causing user confusion and customer confusion.
Whiteboard: .? → .002
Comment on attachment 8584735 [details] [diff] [review]
v1: Remove thumbnail/title replacing functionality for history tiles

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

I like Ed's idea, but I think it's not even necessary to keep the _enhancedLinks structure up-to-date.  We could do that -- DirectoryLinksProvider already observes PlacesProvider -- or, since getEnhancedLink is the one public access point to _enhancedLinks, and it takes the original link object, how about just moving the link.type != "history" check into getEnhancedLink?  Then you don't have to update callers.

i.e., in Site._render, keep it as it is now:

let enhanced = gAllPages.enhanced && DirectoryLinksProvider.getEnhancedLink(this.link);

But change getEnhancedLink like:

getEnhancedLink: function DirectoryLinksProvider_getEnhancedLink(link) {
  return link.type == "history" ? null :
         link.enhancedImageURI && link ? link :
         this._enhancedLinks.get(NewTabUtils.extractSite(link.url));
},

Wouldn't that work?  But I'm OK with this patch too, so up to you, r+ either way.
Attachment #8584735 - Flags: review?(adw) → review+
Reporter

Comment 7

4 years ago
Turns out the history type check wasn't enough. This causes a suggested tile to get enhanced by a directory tile.
Reporter

Comment 8

4 years ago
I tested with this json:

https://people.mozilla.org/~elee/suggested2.json

Note the suggested tile should have a title with ".. Suggest!" but I only see it get a title of the directory tile.


This attachment is of a slightly different json where the directory link has a different domain to allow the directory tile to not be excluded by the one-tile-per-domain logic. But conceptually, we should be able to have a suggested link and a directory link with different images/titles. Where the current landed patch replaces the suggested images/title with the enhanced directory one (because suggested type is not "history").
Reporter

Comment 9

4 years ago
Let's do the follow up fixes per comment 7, comment 8 in bug 1105360.
Iteration: --- → 39.3 - 30 Mar
Points: --- → 3
https://hg.mozilla.org/mozilla-central/rev/9f25f9cf6ce1
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Reporter

Updated

4 years ago
Blocks: 1148859
Assignee

Comment 11

4 years ago
(In reply to Ed Lee :Mardak from comment #8)
> But conceptually, we should be able to have a
> suggested link and a directory link with different images/titles.

Do you mean that we should consider two links with the same URL as separate (and can appear simultaneously on a page) if one is directory and another is suggested?
Reporter

Comment 12

4 years ago
We can just let the existing new tab logic work, i.e., new tab page only shows one link for a given site. So if there's a suggested tile, it'll hide the directory one. Arguably, the longer term desired logic is bug 1139052 where there shouldn't be any suggested tiles shown if directory tiles are visible.
Verified on Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit using:
- Firefox 38 beta 4, build ID: 20150413143743;
- latest Firefox 39.0a2, build ID: 20150414004003
Status: RESOLVED → VERIFIED

Updated

4 years ago
Blocks: 1155443

Updated

4 years ago
No longer blocks: 1155443
You need to log in before you can comment on or make changes to this bug.