1354536 - Add a 'Recent Activity' list to the Library panel

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, enhancement, P1)

Description

[Mike de Boer [:mikedeboer]]

This is a newly created list that features one of the following two options (TBD):

1. A list of recently bookmarked items, mixed with recently downloaded items, sorted by date. Clicking these items will activate the appropriate command as if they were in the 'Recent Bookmarks' menu or 'Downloads' lists.

2. A list obtained from Activity Stream, as-is.

It is, of course, possible to first implement 1) and go for 2) when and after the Activity Stream project is fully integrated into Fx.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Cc'ing the relevant Activity Stream folks for (2) so they know what's up.

Comment 2

[Justin Dolske [:Dolske]]

Moving to reserve backlog for now.

Comment 3

[:Gijs (out for now; he/him)]

Kate, do you have ideas about what APIs Activity Stream provides that we can use for this, if they're in nightly already (or if not, which bugs track them landing) and/or anything else that might be useful for implementing this?

Comment 4

[Mike de Boer [:mikedeboer]]

Or Tim, perhaps?

Comment 5

[Tim Spurway [:tspurway]]

:k88hudson definitely could answer this better than I. :-)

Comment 6

[Kate Hudson :k88hudson]

Currently in nightly, we have a Top Sites query (sorted by frecency) and we will be planning on landing a "Highlights" query by 57. This will include a mixture of recent activity as well as bookmarks, so that doesn't sound like exactly what you need – if you want a query that just returns recent items unfortunately that might be something you need to implement yourself.

If you'd like to see what we've done for Top Sites you can take a look here: http://searchfox.org/mozilla-central/source/toolkit/modules/NewTabUtils.jsm#935

Comment 7

[Mike de Boer [:mikedeboer]]

I'm taking this to
1. Land the l10n changes necessary and the pref behind I'll work on this
2. Investigate and do an initial implementation using `NewTabUtils.getHighlights()`.

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1354536 - Part 1 - Introduce a string for the 'Recent Highlights' section soon the be added to the Library view.

Review commit: https://reviewboard.mozilla.org/r/179842/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/179842/

Comment 9

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1354536 - Part 2 - Add an option to ActivityStreamLinks.getHighlights() to allow results to be returned including favicons.

Review commit: https://reviewboard.mozilla.org/r/179844/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/179844/

Comment 10

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1354536 - Part 3 - When the Library view is shown, populate a new 'Recent Highlights' section with at most 12 items as in about:newtab.

Review commit: https://reviewboard.mozilla.org/r/179846/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/179846/

Comment 11

[Mike de Boer [:mikedeboer]]

Comment on attachment 8908154 [details]
Bug 1354536 - Part 3 - When the Library view is shown, populate a new 'Recent Highlights' section with at most 12 items as in about:newtab.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179846/diff/1-2/

Comment 12

[:Gijs (out for now; he/him)]

Comment on attachment 8908152 [details]
Bug 1354536 - Part 1 - Introduce a string for the 'Recent Highlights' section soon the be added to the Library view.

https://reviewboard.mozilla.org/r/179842/#review185084

Comment 13

[Ed Lee :Mardak]

Comment on attachment 8908153 [details]
Bug 1354536 - Part 2 - Add an option to ActivityStreamLinks.getHighlights() to allow results to be returned including favicons.

https://reviewboard.mozilla.org/r/179844/#review185090

::: toolkit/modules/NewTabUtils.jsm:1334
(Diff revision 1)
>        }
>      }
>  
> +    if (aOptions.withFavicons) {
> +      return ActivityStreamProvider._faviconBytesToDataURI(
> +        await ActivityStreamProvider._addFavicons(results));

Looks like the (now) 2 callers all just want to get favicon as data uri. I guess we might want to just simplify that as an arg to addFavicons. Although then again that might change with using some favicon protocol uri.

Comment 14

[:Gijs (out for now; he/him)]

Comment on attachment 8908154 [details]
Bug 1354536 - Part 3 - When the Library view is shown, populate a new 'Recent Highlights' section with at most 12 items as in about:newtab.

https://reviewboard.mozilla.org/r/179846/#review185086

I haven't had a chance to test this yet, but here's some initial feedback.

For the library panel itself, do you think we should populate from within the mousedown handler of the library button, and then show the popup after populating it? In the hamburger panel we could add a blocker. That would solve any race conditions the current implementation might have, depending on how quick AS returns the data here...

::: browser/components/customizableui/content/panelUI.js:522
(Diff revision 2)
> +    // Since the library is the first view shown, we don't want to add a blocker
> +    // to the event, which would make PanelMultiView wait to show it.
> +    let container = this.clearLibraryRecentHighlights();
> +    if (!this.libraryRecentHighlightsEnabled)
> +      return;

I think you can do the clearing after the early return, right? If the pref is off, we would have cleaned it up when the pref got turned off.

::: browser/components/customizableui/content/panelUI.js:529
(Diff revision 2)
> +    let container = this.clearLibraryRecentHighlights();
> +    if (!this.libraryRecentHighlightsEnabled)
> +      return;
> +
> +    let highlights = await NewTabUtils.activityStreamLinks.getHighlights({ withFavicons: true });
> +    if (!highlights.length) {

Probably a good idea to check whether the popup is still open (and the view still shown) when this returns?

::: browser/components/customizableui/content/panelUI.js:538
(Diff revision 2)
> +    container.hidden = container.previousSibling.hidden =
> +      container.previousSibling.previousSibling.hidden = false;
> +    let fragment = document.createDocumentFragment();
> +    for (let highlight of highlights) {
> +      let button = document.createElement("toolbarbutton");
> +      let icon = highlight.favicon || highlight.preview_image_url;

Hm, is there documentation of what preview_image_url is? I don't think we want to try to render huge images into the panel's tiny favicon spaces...

::: browser/components/customizableui/content/panelUI.js:545
(Diff revision 2)
> +      button.classList.toggle("subviewbutton-iconic", !!icon);
> +      let title = highlight.title || highlight.url;
> +      button.setAttribute("label", title);
> +      button.setAttribute("tooltiptext", title);
> +      button.setAttribute("type", "highlight-" + highlight.type);
> +      button.setAttribute("onclick", "PanelUI.onLibraryHighlightClick(event)");

Is there a particular reason not to have an event listener on the `<toolbaritem>` that just handles clicks on any of the item, and to add it with an `addEventListener()` call in the `ensureInitialized` code? That would seem cleaner.

::: browser/themes/shared/customizableui/panelUI.inc.css:1298
(Diff revision 2)
> +  -moz-context-properties: fill;
> +  content: url("chrome://browser/skin/bookmark-hollow.svg");
> +  fill: GrayText;
> +  float: right;
> +  opacity: .5;
> +  /* Centers the icon and resizes it to 12px square. */
> +  transform: translateY(2px) scaleX(.75);

I spy that all of these are duplicated 3 times, besides the content: bit. Can you deduplicate them?

Comment 15

[:Gijs (out for now; he/him)]

(In reply to Ed Lee :Mardak from comment #13)
> Comment on attachment 8908153 [details]
> Bug 1354536 - Part 2 - Add an option to ActivityStreamLinks.getHighlights()
> to allow results to be returned including favicons.
> 
> https://reviewboard.mozilla.org/r/179844/#review185090
> 
> ::: toolkit/modules/NewTabUtils.jsm:1334
> (Diff revision 1)
> >        }
> >      }
> >  
> > +    if (aOptions.withFavicons) {
> > +      return ActivityStreamProvider._faviconBytesToDataURI(
> > +        await ActivityStreamProvider._addFavicons(results));
> 
> Looks like the (now) 2 callers all just want to get favicon as data uri. I
> guess we might want to just simplify that as an arg to addFavicons. Although
> then again that might change with using some favicon protocol uri.

Can we not just use page-icon: URIs? Maybe not in AS because it's not privileged, but the library could, I think?

Comment 16

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs from comment #14)
> For the library panel itself, do you think we should populate from within
> the mousedown handler of the library button, and then show the popup after
> populating it? In the hamburger panel we could add a blocker. That would
> solve any race conditions the current implementation might have, depending
> on how quick AS returns the data here...

Not a fan... and since we clear and build the list consecutively, I'm not too worried about the edgy raciness here.

> I think you can do the clearing after the early return, right? If the pref
> is off, we would have cleaned it up when the pref got turned off.

We could, *if* I make the nodes hidden by default, in case we're gonna have this pref'ed off.

> Hm, is there documentation of what preview_image_url is? I don't think we
> want to try to render huge images into the panel's tiny favicon spaces...

No, but IME it displays just fine resized to 16x16px. But remember: with the favicons code in there this is _very_ rare.

> Is there a particular reason not to have an event listener on the
> `<toolbaritem>` that just handles clicks on any of the item, and to add it
> with an `addEventListener()` call in the `ensureInitialized` code? That
> would seem cleaner.

I was thinking about wanting to prevent smelly add/remove event listener for each node... I can set it on the toolbaritem, but that's similar. This is a one-liner and I thought it looked pretty clean, leak-less. I can change it, of course!

> I spy that all of these are duplicated 3 times, besides the content: bit.
> Can you deduplicate them?

Pseudo elements are a piece of evil in the Web Platform! I couldn't get it to work differently :(

Comment 17

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs from comment #15)
> Can we not just use page-icon: URIs? Maybe not in AS because it's not
> privileged, but the library could, I think?

Good thought, will do.

Comment 18

[:Gijs (out for now; he/him)]

(In reply to Mike de Boer [:mikedeboer] from comment #16)
> > Hm, is there documentation of what preview_image_url is? I don't think we
> > want to try to render huge images into the panel's tiny favicon spaces...
> 
> No, but IME it displays just fine resized to 16x16px. But remember: with the
> favicons code in there this is _very_ rare.

OK, so what are these? Screenshots of webpages? Something else? I think that's the main thing I'm not clear about...

> > I spy that all of these are duplicated 3 times, besides the content: bit.
> > Can you deduplicate them?
> 
> Pseudo elements are a piece of evil in the Web Platform! I couldn't get it
> to work differently :(

Oh, that's surprising. I would expect this to work:

.subviewbutton[type="highlight-bookmark"]::after {
  content: url("chrome://browser/skin/bookmark-hollow.svg");
}

.subviewbutton[type="highlight-download"]::after {
  content: url("chrome://browser/skin/downloads/download-icons.svg#arrow-with-bar");
}

.subviewbutton[type="highlight-history"]::after {
  content: url("chrome://browser/skin/history.svg");
}

.subviewbutton[type="highlight-bookmark"]::after,
.subviewbutton[type="highlight-download"]::after,
.subviewbutton[type="highlight-history"]::after {
  -moz-context-properties: fill;
  content: url("chrome://browser/skin/history.svg");
  fill: GrayText;
  float: right;
  opacity: .5;
  /* Centers the icon and resizes it to 12px square. */
  transform: translateY(2px) scaleX(.75);
}

but maybe I'm missing something?

Comment 19

[:Gijs (out for now; he/him)]

Err, without the content: property line in the last rule, of course. It's late, I probably shouldn't be writing code for you to use...

Comment 20

[Ed Lee :Mardak]

preview_image_url are from og:image, etc. meta tags from the page, so they can be huge. Added by bug 1393924.

Activity Stream uses the thumbnailer to shrink / crop and cache those images. Thumbnailing added by bug 1397390.

The thumbnailing happens only when we know the image should be shown. https://searchfox.org/mozilla-central/source/browser/extensions/activity-stream/lib/HighlightsFeed.jsm#105

Comment 21

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/16edc2f1ada8f6f22ae8c36dda417522ac4fa25c
Bug 1354536 - Part 1 - Introduce a string for the 'Recent Highlights' section soon the be added to the Library view. r=Gijs

Comment 22

[Mike de Boer [:mikedeboer]]

(In reply to Mike de Boer [:mikedeboer] from comment #16)
> (In reply to :Gijs from comment #14)
> > For the library panel itself, do you think we should populate from within
> > the mousedown handler of the library button, and then show the popup after
> > populating it? In the hamburger panel we could add a blocker. That would
> > solve any race conditions the current implementation might have, depending
> > on how quick AS returns the data here...
> 
> Not a fan... and since we clear and build the list consecutively, I'm not
> too worried about the edgy raciness here.

It was late for me too, I guess, because I'd liked to elaborate here: I think for the library view it's prudent to show it as fast as possible, because the most important items are in fact the first static ones.
It's OK for the highlights to appear later (while in fact they're loaded super fast atm, but may change later when Activity Stream starts using a more impressive API) and appear with a short delay.
So I'm not in favor of adding a blocker for this one. That doesn't solve the potential race, so I'll add a guard for that.

Comment 23

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs from comment #18)
> Oh, that's surprising. I would expect this to work:
> 
> .subviewbutton[type="highlight-bookmark"]::after {
>   content: url("chrome://browser/skin/bookmark-hollow.svg");
> }
> 
> .subviewbutton[type="highlight-download"]::after {
>   content:
> url("chrome://browser/skin/downloads/download-icons.svg#arrow-with-bar");
> }
> 
> .subviewbutton[type="highlight-history"]::after {
>   content: url("chrome://browser/skin/history.svg");
> }
> 
> .subviewbutton[type="highlight-bookmark"]::after,
> .subviewbutton[type="highlight-download"]::after,
> .subviewbutton[type="highlight-history"]::after {
>   -moz-context-properties: fill;
>   content: url("chrome://browser/skin/history.svg");
>   fill: GrayText;
>   float: right;
>   opacity: .5;
>   /* Centers the icon and resizes it to 12px square. */
>   transform: translateY(2px) scaleX(.75);
> }
> 
> but maybe I'm missing something?

Works! \o/

Comment 24

[Mike de Boer [:mikedeboer]]

Comment on attachment 8908152 [details]
Bug 1354536 - Part 1 - Introduce a string for the 'Recent Highlights' section soon the be added to the Library view.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179842/diff/1-2/

Comment 25

[Mike de Boer [:mikedeboer]]

Comment on attachment 8908153 [details]
Bug 1354536 - Part 2 - Add an option to ActivityStreamLinks.getHighlights() to allow results to be returned including favicons.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179844/diff/1-2/

Comment 26

[Mike de Boer [:mikedeboer]]

Comment on attachment 8908154 [details]
Bug 1354536 - Part 3 - When the Library view is shown, populate a new 'Recent Highlights' section with at most 12 items as in about:newtab.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179846/diff/2-3/

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/16edc2f1ada8

Comment 28

[:Gijs (out for now; he/him)]

Comment on attachment 8908154 [details]
Bug 1354536 - Part 3 - When the Library view is shown, populate a new 'Recent Highlights' section with at most 12 items as in about:newtab.

https://reviewboard.mozilla.org/r/179846/#review185710

So, the code looks sane to me, but how do I test this effectively?

Nightly doesn't seem to ever change the 'highlights' when I navigate to places, and I only ever see two items in the library menu. Worse, "highlights" in the new tab page shows 1 item, and in the library I also see a second item, so the two aren't in sync. The second item is a history item, which shows up as a history item in the new tab page but not a highlight. The highlight that is both in the library and on the new tab page isn't one of my 6 "top sites", so I don't understand why it's a "highlight" (and the other 5 "top sites" items aren't). Existing bookmarks also don't show up, though deleting a bookmark and recreating it makes it show up as a highlight. Downloads don't show up as highlights either (neither older or newly created ones). So at the moment I don't understand what is generating these, it just looks really buggy and not like something we can ship.

::: browser/components/customizableui/content/panelUI.js:555
(Diff revision 3)
> +      button.setAttribute("label", title);
> +      button.setAttribute("tooltiptext", title);
> +      button.setAttribute("type", "highlight-" + highlight.type);
> +      button.setAttribute("onclick", "PanelUI.onLibraryHighlightClick(event)");
> +      if (highlight.favicon) {
> +        button.setAttribute("image", highlight.favicon);

I think we should show a globe for history/bookmark items that have no favicon. I don't know what we currently do for downloads because I can't get any downloads to show up in the highlights menu.

::: browser/components/customizableui/content/panelUI.js:590
(Diff revision 3)
> +    if (event.button > 1 || !button._highlight) {
> +      return;
> +    }
> +    let url = button._highlight.url;
> +    let where = window.whereToOpenLink(event, false, true);
> +    window.openUILinkIn(url, where, { allowPopups: url.startsWith("javascript:") });

Can you use openUILink() and pass the event to simplify this a teeeeeny bit? Or does that not work for some reason?

Comment 29

[:Gijs (out for now; he/him)]

On a fresh clean profile, clicking the 'facebook' tile makes that show up in the menu, but not in the new tab page's "highlights"... I am... puzzled.

Comment 30

[Ed Lee :Mardak]

Activity Stream removes duplicates so that there should only be one link to a url on that page across all of its sections (top sites, stories, highlights).

Highlights from history must have a description and preview_image_url, so easiest way is to navigate to some news sites as they usually have og:description and og:image tags.

Comment 31

[:Gijs (out for now; he/him)]

(In reply to Ed Lee :Mardak from comment #30)
> Activity Stream removes duplicates so that there should only be one link to
> a url on that page across all of its sections (top sites, stories,
> highlights).

OK, but then doesn't "highlights" on the new tab page effectively just end up being an additional row of "top sites"? What will be the difference?

> Highlights from history must have a description and preview_image_url, so
> easiest way is to navigate to some news sites as they usually have
> og:description and og:image tags.

But the menu doesn't use that information, it just uses page titles and favicons. Additionally, none of that information will be available for upgrading users who already have history, so highlights will be empty there. Presumably this explains why my test profile has almost no "highlights", because I haven't visited places with these tags in that profile since highlights landed...

Will we do some kind of one-off migration for those cases, or just leave things as they are?

I also still don't understand how to get downloads to show up in the highlights.

Comment 32

[Ed Lee :Mardak]

(In reply to :Gijs from comment #31)
> (In reply to Ed Lee :Mardak from comment #30)
> OK, but then doesn't "highlights" on the new tab page effectively just end
> up being an additional row of "top sites"? What will be the difference?
Highlights show recent bookmarks (with or without metadata) and recent history with metadata. Top Sites shows the highest frecency pages.

> Will we do some kind of one-off migration for those cases, or just leave
> things as they are?
I don't think there will be any migration as the data doesn't exist in Firefox unless we proactively request those pages again to see if they have the desired metadata. Given that Highlights is supposed to be recent stuff, I believe it's fine to just wait for the user to visit things.

> I also still don't understand how to get downloads to show up in the
> highlights.
Activity Stream's Highlights doesn't include downloads.

Comment 33

[Mike de Boer [:mikedeboer]]

Comment on attachment 8908154 [details]
Bug 1354536 - Part 3 - When the Library view is shown, populate a new 'Recent Highlights' section with at most 12 items as in about:newtab.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179846/diff/3-4/

Comment 34

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1354536 - Part 2 - Add an option to ActivityStreamLinks.getHighlights() to allow results to be returned including favicons.

Review commit: https://reviewboard.mozilla.org/r/181292/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181292/

Comment 35

[Mike de Boer [:mikedeboer]]

Comment on attachment 8908154 [details]
Bug 1354536 - Part 3 - When the Library view is shown, populate a new 'Recent Highlights' section with at most 12 items as in about:newtab.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179846/diff/4-5/

Comment 36

[Mike de Boer [:mikedeboer]]

Ed, sorry, my MozReview-fu gave up on me and I pushed to wrong set of changes... which means you'll have to r+ the same patch again :/

Comment 37

[:Gijs (out for now; he/him)]

Comment on attachment 8908154 [details]
Bug 1354536 - Part 3 - When the Library view is shown, populate a new 'Recent Highlights' section with at most 12 items as in about:newtab.

https://reviewboard.mozilla.org/r/179846/#review186528

Alright, let's do it. Thanks! :-)

Comment 38

[:Gijs (out for now; he/him)]

Should we have a followup bug to add downloads?

Comment 39

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs from comment #38)
> Should we have a followup bug to add downloads?

I don't think so - we're relying on the Activity Stream data source as we are now and they have plans to make changes to it for Fx 58. We'll see what's included in there; if there's Downloads included, we'll add an icon at that point, I guess.
Since Ed is watching this bug: could you make sure to keep the frontend team - other than AS - in the loop about changes in this space? Thanks!

Comment 40

[Ed Lee :Mardak]

Comment on attachment 8909800 [details]
Bug 1354536 - Part 2 - Add an option to ActivityStreamLinks.getHighlights() to allow results to be returned including favicons.

https://reviewboard.mozilla.org/r/181292/#review186534

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ea4c53366f8d
Part 2 - Add an option to ActivityStreamLinks.getHighlights() to allow results to be returned including favicons. r=Mardak
https://hg.mozilla.org/integration/autoland/rev/df4f51e18090
Part 3 - When the Library view is shown, populate a new 'Recent Highlights' section with at most 12 items as in about:newtab. r=Gijs

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ea4c53366f8d
https://hg.mozilla.org/mozilla-central/rev/df4f51e18090

Comment 43

[Azmina]

I have seen this "Add a 'Recent Activity' list to the Library panel" feature has been implemented with Latest Nightly 57.0a1 on Windows 8.1, 64 Bit.

Build ID : 20170921100141
User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:57.0) Gecko/20100101 Firefox/57.0

Comment 44

[Mohammad Maruf Rahman]

This bug was about adding a 'Recent Activity' list to the "Library panel" and I have seen this feature being implemented with Latest Nightly 57.0a1 on Ubuntu 16.04 LTS!

Build ID :   20170921100141
User Agent : Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0

[bugday-20170920]

Comment 45

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

I have reproduced this issue using Firefox  55.0a1 (20170410030221) on Win 8.1 x64.
I can confirm this feature was implemented, I verified using Firefox 57.0b6 on Win 8.1 x64, Mac OS X 10.11 and Ubuntu 14.04 x64.