Open Bug 1146348 Opened 5 years ago Updated 4 years ago

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

Categories

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

39 Branch
defect

Tracking

(firefox39 affected)

Tracking Status
firefox39 --- affected

People

(Reporter: avaida, Unassigned)

References

Details

(Whiteboard: [reader-ui])

Attachments

(1 file)

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.
Flags: qe-verify+
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?
(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?
Flags: needinfo?(florian)
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?
Flags: needinfo?(florian) → needinfo?(andrei.vaida)
(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.
(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.
(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?
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.
(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).
Flags: needinfo?(andrei.vaida)
Can you recheck this with tomorrow's nightly (with the fix for bug 1146773)? Seems like that will likely fix this?
Flags: needinfo?(andrei.vaida)
Priority: -- → P3
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.
Flags: needinfo?(andrei.vaida)
Summary: The thumbnails associated to Reading List items appear low-res/blurry in the sidebar → PageMetadata poorly chooses a preview image when none is specified by the page
Whiteboard: [reader-ui]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.