1146348 - PageMetadata poorly chooses a preview image when none is specified by the page

Status: RESOLVED INVALID

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

Description

[Andrei Vaida [:avaida]]

Attachment: screenshot from Ubuntu 14.04 (x64)

Reproducible on:
Nightly 39.0a1 (2015-03-23)

Affected platforms:
Windows 7 (x64), Ubuntu 14.04 (x64), Mac OS X 10.9.5

Preconditions:
* browser.readinglist.enabled = true (default)
* a profile with a few items available in Reading List

Steps to reproduce:
1. Launch Firefox.
2. Enable the Reading List sidebar, via toolbar → Show your bookmarks → Reading List → Show Reading List Sidebar.
3. Check the thumbnails displayed in the sidebar, for items available in the Reading List.

Expected result:
The thumbnails are scaled and displayed properly.

Actual result:
Thumbnails are displayed as blurry/low-res.

Comment 1

[Florian Quèze [:florian]]

It looks to me like this is specific to blog.mozilla.org, and caused by the reader mode selecting an image it should ignore as the main article image. Gijs, is this a known issue?

Comment 2

[:Gijs (he/him)]

(In reply to Florian Quèze [:florian] [:flo] from comment #1)
> It looks to me like this is specific to blog.mozilla.org, and caused by the
> reader mode selecting an image it should ignore as the main article image.
> Gijs, is this a known issue?

No, and I don't know how this relates to reader mode. Can you clarify?

Comment 3

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

Andrei, how did you add these items to the reading list? Did you use the button in the location bar or the button from within Reader Mode?

Comment 4

[Florian Quèze [:florian]]

(In reply to :Gijs Kruitbosch from comment #2)
> (In reply to Florian Quèze [:florian] [:flo] from comment #1)
> > It looks to me like this is specific to blog.mozilla.org, and caused by the
> > reader mode selecting an image it should ignore as the main article image.
> > Gijs, is this a known issue?
> 
> No, and I don't know how this relates to reader mode. Can you clarify?

I was assuming, probably wrongly, that the image URL used for the preview was something readability returned. But now I think it's more likely from separate metadata extraction code.

Comment 5

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

(In reply to Florian Quèze [:florian] [:flo] from comment #4)
> (In reply to :Gijs Kruitbosch from comment #2)
> > (In reply to Florian Quèze [:florian] [:flo] from comment #1)
> > > It looks to me like this is specific to blog.mozilla.org, and caused by the
> > > reader mode selecting an image it should ignore as the main article image.
> > > Gijs, is this a known issue?
> > 
> > No, and I don't know how this relates to reader mode. Can you clarify?
> 
> I was assuming, probably wrongly, that the image URL used for the preview
> was something readability returned. But now I think it's more likely from
> separate metadata extraction code.

It depends. If the readinglist item is added through reader mode, then the image is chosen by Readability. If the readinglist item is added through the location bar, then the image is chosen by ReadingList via PageMetadata.

Comment 6

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

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> It depends. If the readinglist item is added through reader mode, then the
> image is chosen by Readability. If the readinglist item is added through the
> location bar, then the image is chosen by ReadingList via PageMetadata.

Why the discrepancy? Why not always use PageMetadata?

Comment 7

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

That's a good question. Right now the short answer is that ReaderParent.jsm calls ReadingList.addItem directly with the article object that was passed along from AboutReader.jsm.

We should be using PageMetadata wherever possible, and probably find a way for Readability to use it so that the logic can be shared on Android and iOS too.

I've filed bug 1146773 to track fixing this.

Comment 8

[Andrei Vaida [:avaida]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> Andrei, how did you add these items to the reading list? Did you use the
> button in the location bar or the button from within Reader Mode?

I used the button from the Location Bar, as the one from Reader View is not working (as of Bug 1146358).

Comment 9

[Justin Dolske [:Dolske]]

Can you recheck this with tomorrow's nightly (with the fix for bug 1146773)? Seems like that will likely fix this?

Comment 10

[Justin Dolske [:Dolske]]

Actually, nevermind, because that fix shouldn't change the behavior here (when clicking Add from the URL bar).

Sounds like the issue is that PageMetadata is making a poor selection for the page's image when there's no metadata specifying one (and that it should ignore tiny images entirely). In particular, _getImageUrls() is just blindly returning the first 5 images from the page. >_<

For Reading List we might even just want to just have it return nothing (or at least distinguish between page-specified and out-best-guess), as our placeholder image is probably better than a random page image, most of the time.

Comment 11

[Rares Doghi, Desktop QA]

Closing this as invalid since a lot of the behavior has changed over the years, but we can reopen with different steps if it starts to reoccur.