Closed Bug 1354536 Opened 7 years ago Closed 7 years ago

Add a 'Recent Activity' list to the Library panel

Categories

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

Product:
Firefox
Component:
Toolbars and Customization
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 57
Iteration:
57.3 - Sep 19
Tracking Flags:
Tracking Status
firefox57 --- verified

People

(Reporter: mikedeboer, Assigned: mikedeboer)

References

Details

(Whiteboard: [reserve-photon-structure])

Attachments

(2 files, 2 obsolete files)

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.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1354536 - Part 2 - Add an option to ActivityStreamLinks.getHighlights() to allow results to be returned including favicons.
59 bytes, text/x-review-board-request
Mardak
: review+
Details 
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.
Cc'ing the relevant Activity Stream folks for (2) so they know what's up.
No longer depends on: 1354534
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
Whiteboard: [photon] → [photon-structure]
Moving to reserve backlog for now.
Priority: P2 → P3
Whiteboard: [photon-structure] → [reserve-photon-structure]
Priority: P3 → P2
Whiteboard: [reserve-photon-structure] → [photon-structure]
See Also: → 1375806
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?
Flags: needinfo?(khudson)
Or Tim, perhaps?
Flags: needinfo?(tspurway)
:k88hudson definitely could answer this better than I. :-)
Flags: needinfo?(tspurway)
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
Flags: needinfo?(khudson)
Blocks: 1387512
Whiteboard: [photon-structure] → [photon-structure][photon-l10n-risk]
Priority: P2 → P4
Whiteboard: [photon-structure][photon-l10n-risk] → [reserve-photon-structure][photon-l10n-risk]
Priority: P4 → P5
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 57.3 - Sep 19
Priority: P5 → P1
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 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
Attachment #8908152 - Flags: review?(gijskruitbosch+bugs) → review+
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.
Attachment #8908153 - Flags: review?(edilee) → review+
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?
Attachment #8908154 - Flags: review?(gijskruitbosch+bugs)
(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?
(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 :(
(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.
(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?
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...
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
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
Keywords: leave-open
(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.
(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/
Keywords: leave-open
Whiteboard: [reserve-photon-structure][photon-l10n-risk] → [reserve-photon-structure]
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?
Attachment #8908154 - Flags: review?(gijskruitbosch+bugs)
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.
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.
(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.
Flags: needinfo?(edilee)
(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.
Flags: needinfo?(edilee)
Attachment #8908152 - Attachment is obsolete: true
Attachment #8908153 - Attachment is obsolete: true
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 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! :-)
Attachment #8908154 - Flags: review?(gijskruitbosch+bugs) → review+
Should we have a followup bug to add downloads?
Flags: needinfo?(mdeboer)
(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!
Flags: needinfo?(mdeboer)
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
Attachment #8909800 - Flags: review?(edilee) → review+
Pushed by mdeboer@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/ea4c53366f8d
https://hg.mozilla.org/mozilla-central/rev/df4f51e18090
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Depends on: 1402023
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
QA Whiteboard: [bugday-20170920]
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]
Status: RESOLVED → VERIFIED
Depends on: 1402176
Depends on: 1402690
Depends on: 1402849
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.
status-firefox57: fixed → verified
Flags: qe-verify+
See Also: → 1644975
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: