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

VERIFIED FIXED in Firefox 38

Status

defect
P1
normal
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: Unfocused, Assigned: jaws)

Tracking

(Depends on 2 bugs, Blocks 1 bug)

Trunk
Firefox 39
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox38 verified, firefox39 verified)

Details

Attachments

(1 attachment, 5 obsolete attachments)

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?
Flags: firefox-backlog+
Flags: qe-verify+
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.
Points: 3 → 5
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
Depends on: 1126115
Keeping this bug as UI-only, and have filed bug 1126115 as a breakdown for the rest.
(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)
Flags: needinfo?(mmaslaney)
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...
Flags: needinfo?(rnewman)
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.
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.
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.
Flags: needinfo?(rnewman)
Thumbnail data scraping Spec:

http://invis.io/QT23PUEMV
Flags: needinfo?(mmaslaney)
Blocks: 1132074
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
Component: General → Reading List
P1 based on this being a pretty key piece of UX for the sidebar.
Priority: -- → P1
Are we dropping "thumbnails" from v1?
Flags: needinfo?(clarkbw)
Assignee: nobody → florian
Status: NEW → ASSIGNED
Iteration: --- → 39.2 - 23 Mar
Summary: [ReadingList] Add thumbnails/favicons to items in the Reading List sidebar → Add representative images/thumbnails/favicons to items in the Reading List sidebar
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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.
Flags: needinfo?(clarkbw)
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.
Depends on: 1140194
(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.
Hey - just in case there's miscommunication, I also think that this is a P1.
QA Contact: andrei.vaida
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).
Assignee: florian → jaws
Posted patch Patch (obsolete) — Splinter Review
Depends on the patch for bug 1133429.
Attachment #8574096 - Attachment is obsolete: true
Attachment #8574097 - Attachment is obsolete: true
Attachment #8580278 - Flags: review?(florian)
Summary: Add representative images/thumbnails/favicons to items in the Reading List sidebar → Add images/thumbnails to items in the Reading List sidebar
Summary: Add images/thumbnails to items in the Reading List sidebar → [ReadingList] Add images/thumbnails to items in the Reading List sidebar
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.
Attachment #8580278 - Flags: review?(florian) → feedback+
Are you going to take care of the favicon as a separate bug?
Posted patch Patch v1.1 (obsolete) — Splinter Review
Rebased and using a getter now.
Attachment #8580278 - Attachment is obsolete: true
Attachment #8580338 - Flags: review?(florian)
(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 on attachment 8580338 [details] [diff] [review]
Patch v1.1

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

Thanks!
Attachment #8580338 - Flags: review?(florian) → review+
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.
Posted patch Patch for check-in (obsolete) — Splinter Review
Attachment #8580338 - Attachment is obsolete: true
Attachment #8580357 - Flags: review+
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.
Flags: needinfo?(jaws)
(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`.
Flags: needinfo?(jaws)
Removing the property now if it is not present.
Attachment #8580357 - Attachment is obsolete: true
Attachment #8580798 - Flags: review+
(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.
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
https://hg.mozilla.org/mozilla-central/rev/b6d0e75d064e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Depends on: 1146348
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.
Status: RESOLVED → VERIFIED
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.
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.