1123518 - [ReadingList] Add images/thumbnails to items in the Reading List sidebar

Status: VERIFIED FIXED

(Firefox Graveyard :: Reading List, defect, P1)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

In the Reading List sidebar, we want items to display a thumbnail and favicon to the left of the title/url. The mockup in bug 1120007 has the favicon displaying in the bottom right corner of the thumbnail block, which is 64x40 pixels.

Open question: Currently, the thumbnail service captures and stores thumbnails with a size of around 1/3 of the main screen. Which means we'll be downscaling potentially hundreds of thumbnails in the Reading List. Will this scale performance-wise, or do we need the thumbnail service to store pre-downscaled copies?

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Oh, to note: The thumbnail service may merely be a fallback. The thumbnail may come from the synced list data, although that likely still means we'll need to capture it when we add it to the list.

Also, screen captures of the page may not be the best option, especially given the small size. There are various standards for defining summary metadata for a page (although OpenGraph seems to be the prevailing one), that include a representative/useful image.

Comment 2

[Bryan Clark (DevTools PM) [:clarkbw]]

michael is going to create a spec for what kind of information to grab from the page that'll probably pull from the opengraph data set.

Just to note the social api does a lot of the work to grab the open graph data which is likely to be useful here. The OpenGraphEvent is meant for social api sidebars but there are other more global events happening.  https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Social_API/Bookmarks#OpenGraphData_DOM_Event

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Keeping this bug as UI-only, and have filed bug 1126115 as a breakdown for the rest.

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Bryan Clark (Firefox PM) [:clarkbw] from comment #2)
> michael is going to create a spec for what kind of information to grab from
> the page that'll probably pull from the opengraph data set.

Any update on this? (I'm starting to get blocked on waiting on these types of details)

Comment 5

[Justin Dolske [:Dolske]]

rnewman: not sure if you're the right person to ask, but does mobile already have a plan (or code!) wrt comment 4? Presumably we'll want to have shared code for fetching the summary and article image...

Comment 6

[Bryan Clark (DevTools PM) [:clarkbw]]

I thought bug 1126115 comment 1 by margaret had details about the code available. If so, maybe michael and I can go over the data available from that to see if it works for the design.

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Yea, we have some code. Three components that do this actually, Social API and ReaderMode both do this, and we have PageThumbs too. If Mobile also has its own thing, that's four ways of doing it inconsistently (and somewhat wastefully).

This is where a spec comes in handy :) I'm happy to just write up a spec myself, but comment 2 said Michael was already on it.

Comment 8

[Richard Newman [:rnewman]]

We have thumbnailing on Android, but that's probably not the same thing as "give me a picture to represent this article". I doubt there's anything to reuse there.

Bug 959297 added the excerpting, which is JS code that's now in toolkit:

toolkit/components/reader/content/Readability.js

-- see _getExcerpt.

Comment 9

[Michael Maslaney [:mmaslaney] ([e-mail])]

Thumbnail data scraping Spec:

http://invis.io/QT23PUEMV

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Mass change of ReadingList bugs, moving to their own component. Filter bugspam on the following quote:

“Reading is to the mind what exercise is to the body.”
― Joseph Addison

Comment 11

[Justin Dolske [:Dolske]]

P1 based on this being a pretty key piece of UX for the sidebar.

Comment 12

[Stephen Horlander [:shorlander]]

Attachment: placeholder-silhouette-24.svg — Nightly/Unbranded

Comment 13

[Stephen Horlander [:shorlander]]

Attachment: placeholder-silhouette-24.svg – Official

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Are we dropping "thumbnails" from v1?

Comment 15

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #14)
> Are we dropping "thumbnails" from v1?

The thumbnails are key piece of the experience.  If you feel this work is going to block us from making 38 we can discuss that.

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

I'm working under the assumption we need this. Saying that, it's probably one of the P1's at most risk, and is dependent on bug 1140194.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

(In reply to Bryan Clark (Firefox PM) [:clarkbw] from comment #15)
> If you feel this work is going to block us from making 38 we can discuss that.

I don't know offhand, dolske would have a better sense. We can discuss it next week.

Comment 18

[Madhava Enros [:madhava]]

Hey - just in case there's miscommunication, I also think that this is a P1.

Comment 19

[Justin Dolske [:Dolske]]

Leaving on the P1 list as it's important UX, but still need to understand the engineering risk as to if this is feasible in time. Fallback would be to just show the logo placeholder ala http://cl.ly/image/1b1F0e3H2B3c (as we would do when the page doesn't have any preview image anyway).

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Depends on the patch for bug 1133429.

Comment 21

[Florian Quèze [:florian]]

Comment on attachment 8580278 [details] [diff] [review]
Patch

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

sidebar.css was renamed to sidebar.inc.css

::: browser/components/readinglist/sidebar.js
@@ +133,5 @@
>      itemNode.setAttribute("title", `${item.title}\n${item.url}`);
>  
>      itemNode.querySelector(".item-title").textContent = item.title;
>      itemNode.querySelector(".item-domain").textContent = item.domain;
> +    if (item._properties.image) {

_properties isn't supposed to be available outside of ReadingList.jsm. I think you'll want to add a getter in ReadingListItem.prototype.

Comment 22

[Florian Quèze [:florian]]

Are you going to take care of the favicon as a separate bug?

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.1

Rebased and using a getter now.

Comment 24

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Florian Quèze [:florian] [:flo] from comment #22)
> Are you going to take care of the favicon as a separate bug?

Yes, I've filed bug 114509 for tracking the favicon.

Comment 25

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Florian Quèze [:florian] [:flo] from comment #22)
> Are you going to take care of the favicon as a separate bug?

Yes, I've filed bug 1145409 for tracking the favicon.

Comment 26

[Florian Quèze [:florian]]

Comment on attachment 8580338 [details] [diff] [review]
Patch v1.1

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

Thanks!

Comment 27

[Florian Quèze [:florian]]

Comment on attachment 8580338 [details] [diff] [review]
Patch v1.1

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

::: browser/themes/shared/readinglist/sidebar.inc.css
@@ +50,4 @@
>    border: 1px solid white;
>    box-shadow: 0px 1px 2px rgba(0,0,0,.35);
>    margin: 5px;
> +  background-color: #EBEBEB;

As discussed, making this white looks better.

Comment 28

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for check-in

Comment 29

[Justin Dolske [:Dolske]]

Comment on attachment 8580357 [details] [diff] [review]
Patch for check-in

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

::: browser/components/readinglist/sidebar.js
@@ +141,5 @@
>      itemNode.querySelector(".item-domain").textContent = item.domain;
> +    if (item.preview) {
> +      itemNode.querySelector(".item-thumb-container").style.backgroundImage =
> +        "url(" + item.preview + ")";
> +    }

Tiny driveby:

Can an item be updated to _remove_ a preview image? If so we'd need an |else| here to ensure any previously-set image is removed.

But seems like that isn't super useful, and at worst is a temporary glitch (eg restarting the browser would fix it).

::: browser/themes/shared/readinglist/sidebar.inc.css
@@ +50,5 @@
>    border: 1px solid white;
>    box-shadow: 0px 1px 2px rgba(0,0,0,.35);
>    margin: 5px;
> +  background-color: #fff;
> +  background-size: contain;

Hmm, should this be |cover|?

I think the desired UX here is that it's more important to show the thumbnails at a consistent size, versus showing the whole EG, we don't want to letterbox the top or sizes of images with differing aspect ratios, but instead scale enough to fill the thumbnail region and crop any excess.

Comment 30

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Justin Dolske [:Dolske] from comment #29)
> Comment on attachment 8580357 [details] [diff] [review]
> Patch for check-in
> 
> Review of attachment 8580357 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/readinglist/sidebar.js
> @@ +141,5 @@
> >      itemNode.querySelector(".item-domain").textContent = item.domain;
> > +    if (item.preview) {
> > +      itemNode.querySelector(".item-thumb-container").style.backgroundImage =
> > +        "url(" + item.preview + ")";
> > +    }
> 
> Tiny driveby:
> 
> Can an item be updated to _remove_ a preview image? If so we'd need an
> |else| here to ensure any previously-set image is removed.
> 
> But seems like that isn't super useful, and at worst is a temporary glitch
> (eg restarting the browser would fix it).

This is something simple that we could fix by removing the inline style if the value is not truthy.
 
> ::: browser/themes/shared/readinglist/sidebar.inc.css
> @@ +50,5 @@
> >    border: 1px solid white;
> >    box-shadow: 0px 1px 2px rgba(0,0,0,.35);
> >    margin: 5px;
> > +  background-color: #fff;
> > +  background-size: contain;
> 
> Hmm, should this be |cover|?
> 
> I think the desired UX here is that it's more important to show the
> thumbnails at a consistent size, versus showing the whole EG, we don't want
> to letterbox the top or sizes of images with differing aspect ratios, but
> instead scale enough to fill the thumbnail region and crop any excess.

We can't use `cover` here. Florian and I looked at it and it is not working as the MDN docs on background-size:cover may lead people to believe. With cover, many thumbnails will simply be the top corner of the large image. Unless there is something funky about using `cover` that we don't know about and some other CSS property has to be enabled to get it to work, unlike `contain`.

Comment 31

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for check-in v1.1

Removing the property now if it is not present.

Comment 32

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/b6d0e75d064e

Comment 33

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #30)
> > Hmm, should this be |cover|?
> > 
> > I think the desired UX here is that it's more important to show the
> > thumbnails at a consistent size, versus showing the whole EG, we don't want
> > to letterbox the top or sizes of images with differing aspect ratios, but
> > instead scale enough to fill the thumbnail region and crop any excess.
> 
> We can't use `cover` here. Florian and I looked at it and it is not working
> as the MDN docs on background-size:cover may lead people to believe. With
> cover, many thumbnails will simply be the top corner of the large image.
> Unless there is something funky about using `cover` that we don't know about
> and some other CSS property has to be enabled to get it to work, unlike
> `contain`.

Actually, I think I was wrong here. I think we just needed to tweak background-origin. I'll try it out when my build finishes.

Comment 34

[Jared Wein [:jaws] (please needinfo? me)]

Yeah, background-size:cover works here. I'm not sure why it didn't work for me or Florian before (using separate dev environments).

Landed as https://hg.mozilla.org/integration/fx-team/rev/81d3bcf24237

Comment 35

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

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

Comment 36

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/81d3bcf24237

Comment 37

[Andrei Vaida [:avaida]]

Verified fixed on Nightly 39.0a1 (2015-03-23), using Ubuntu 14.04 (x64), Mac OS X 10.9.5 and Windows 7 (x64), with one issue found and filed separately - Bug 1146348.

Comment 38

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b0e7f69c3590
https://hg.mozilla.org/releases/mozilla-aurora/rev/7d452f002a02

Comment 39

[Andrei Vaida [:avaida]]

Verified fixed on Aurora 38.0a2 (2015-03-29) as well, using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5.

Please note that Bug 1147571 is currently affecting this scenario.