Page MenuHomePhabricator

Display Page Preview images when using the RESTBase endpoint
Closed, ResolvedPublic5 Story Points

Description

Now that RESTBase returns the original image with each page summary endpoint request, we should make use of it to construct thumbnails on the client with the dimensions we need. Originally, RESTBase would only return images up to 300px wide, which considering device DPI would be hidden because of some logic in Page Previews.

Here is the new format from RESTBase:

{
    "title": "Barack Obama",
    "extract": "Barack Hussein Obama II (US /bษ™หˆrษ‘หk huหหˆseษชn oสŠหˆbษ‘หmษ™/ bษ™-RAHK hoo-SAYN oh-BAH-mษ™; born August 4, 1961) is an American politician who served as the 44th President of the United States from 2009 to 2017. He was the first African American to serve as president, as well as the first born outside the contiguous United States. He previously served in the U.S. Senate representing Illinois from 2005 to 2008, and in the Illinois State Senate from 1997 to 2004.\nObama was born in Honolulu, Hawaii, two years after the territory was admitted to the Union as the 50th state. He grew up mostly in Hawaii, but also spent one year of his childhood in Washington State and four years in Indonesia.",
    "thumbnail": {
        "source": "https://upload.wikimedia.org/wikipedia/commons/thumb/8/8d/President_Barack_Obama.jpg/256px-President_Barack_Obama.jpg",
        "width": 256,
        "height": 320,
        "original": "http://upload.wikimedia.org/wikipedia/commons/8/8d/President_Barack_Obama.jpg"
    },
    "originalimage": {
        "source": "https://upload.wikimedia.org/wikipedia/commons/8/8d/President_Barack_Obama.jpg",
        "width": 2687,
        "height": 3356
    },
    "lang": "en",
    "dir": "ltr",
    "timestamp": "2017-01-31T08:42:35Z",
    "description": "44th President of the United States of America"
}

Notice the new originalimage object. We should use this to construct thumbnails of width used in the existing MediaWiki API end point.

Test Plan


Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

RESTBase is already setup on the beta cluster: example.

We need to enabled the config variable now so that we can write up the testing plan.

Change 339475 had a related patch set uploaded (by Phuedx):
Make Page Previews use RESTBase on Beta Cluster

https://gerrit.wikimedia.org/r/339475

Over to @pmiazga who'll add a testing plan while rOMWC512cb69e83c4: Make Page Previews use RESTBase on Beta Cluster is reviewed, merged, and deployed.

phuedx removed pmiazga as the assignee of this task.
phuedx added a subscriber: pmiazga.
phuedx claimed this task.Feb 28 2017, 10:52 AM
phuedx moved this task from To Do to Doing on the Reading-Web-Sprint-92-๐Ÿœ board.

Change 339475 merged by jenkins-bot:
Make Page Previews use RESTBase on Beta Cluster

https://gerrit.wikimedia.org/r/339475

phuedx reassigned this task from phuedx to pmiazga.

Page Previews now uses RESTBase on the Beta Cluster (and it's pretty darn snappy). However, while I was writing the testing notes for QA, I observed that the thumbnail URL doesn't include the all-important "thumb" part, e.g. when I hover over the "With tall image" link on https://en.wikipedia.beta.wmflabs.org/wiki/Special:WhatLinksHere/With_tall_image, I see a request for

https://upload.wikimedia.org/wikipedia/commons/8/8f/Fossil_Bay_Seascape.jpg/300px-Fossil_Bay_Seascape.jpg

and not

https://upload.wikimedia.org/wikipedia/commons/thumb/8/8f/Fossil_Bay_Seascape.jpg/300px-Fossil_Bay_Seascape.jpg

Change 340313 had a related patch set uploaded (by Phuedx; owner: Phuedx):
WIP: Add test case for T156800#3061082

https://gerrit.wikimedia.org/r/340313

phuedx claimed this task.Feb 28 2017, 3:01 PM

^ The Selenium tests now pass but now the "node" tests are failing because downloading PhatomJS is timing out.

Provisionally putting this back in Needs Code Review. @bmansurov noted a bug in the scaling of the resized thumbnails yesterday (hence T156800#3064050) and I believe I've fixed that but haven't yet set up my local testing environment for Page Previews with RESTBase.

Change 340313 merged by Phuedx:
[mediawiki/extensions/Popups] restbase: Use thumbnail when generating thumbnail

https://gerrit.wikimedia.org/r/340313

Tested here: https://en.wikipedia.beta.wmflabs.org/wiki/Dog, images appear as expected. @ABorbaWMF - over to you to reconfirm.

ABorbaWMF added a comment.EditedMar 2 2017, 6:53 PM

Did some testing across a few different OS and browser combinations. Mostly everything is working, but I noticed a problem with Tall images on IE browsers only.

URLs:
https://en.wikipedia.beta.wmflabs.org/wiki/Dog (hover over hunting and/or Carnivora)

Systems:
Windows 10 - IE11
Windows 8.1 - IE11
Windows 8 - IE10
Windows 7 - IE8
Windows 7x64 - IE9

Does not occur on:
MS Edge browser
Firefox
Chrome
Opera
Safari

phuedx added a comment.Mar 2 2017, 7:25 PM

@ABorbaWMF: Do you see the same behaviour on the staging server, which isn't configured to use RESTBase?

@phuedx I don't think the same page previews code is running on staging. Right now I don't see images at all in the previews for 'with image' and 'with tall image' on this page http://reading-web-staging.wmflabs.org/wiki/Category:Popups_corpus

These worked in the past. I remember @ovasileva mentioned that different code was running there.

I also noticed this odd behavior:
On this search results page, the tall image displays the issue:
https://en.wikipedia.beta.wmflabs.org/w/index.php?search=page+previews&title=Special:Search&go=Go&searchToken=a25lpixkamznh1742737584p0

On this categories page, the tall image is correct:
https://en.wikipedia.beta.wmflabs.org/wiki/Category:Page_Previews

Here is the same 'hunting dog' image on a category page:
https://en.wikipedia.beta.wmflabs.org/w/index.php?title=Category:Articles_with_invalid_date_parameter_in_template&pagefrom=Elaine+%28Legend%29%0AElaine+%28legend%29#mw-pages

One of the differences between those previews is the position of the pokey:

  1. the pokey is at the bottom-right of the preview.
  2. the pokey is at the bottom-left of the preview.
  3. the pokey is at the top-right of the preview.

In 2 and 3 the thumbnail is fully or partially clipped and in both cases, the pokey is being used to clip the image.

Obviously, this needs a more thorough investigation. Thanks, @ABorbaWMF!

phuedx added a comment.EditedMar 3 2017, 11:13 AM

Using IE11 on Windows 10 I see the following when dwelling on the "Donald Trump" link on http://reading-web-staging.wmflabs.org/wiki/Special:AllPages?debug=true:

So this rendering issue occurs regardless of the API being used (RESTBase on the Beta Cluster, the MediaWiki API on the staging server).

Change 341327 had a related patch set uploaded (by phuedx):
[mediawiki/extensions/Popups] rest: Don't scale unscalable thumbnails

https://gerrit.wikimedia.org/r/341327

Left a nit. @phuedx that patch is the solution to the images not showing some times, right?

Change 341365 had a related patch set uploaded (by phuedx):
[mediawiki/extensions/Popups] renderer: Ensure images don't overflow container

https://gerrit.wikimedia.org/r/341365

Change 341366 had a related patch set uploaded (by phuedx):
[mediawiki/extensions/Popups] renderer: Ensure settings cog visible in IE9-11

https://gerrit.wikimedia.org/r/341366

With rEPOP46b918a924bf: renderer: Ensure images don't overflow container and rEPOP6327844bd991: renderer: Ensure settings cog visible in IE9-11 applied I see the following:

IE9 on Windows 7

IE10 on Windows 7

IE11 on Windows 10

Chrome (56.0.2924.87) on macOS Sierra (10.12.3)

^ I've responded to the question but I'm not sure it's the response you're looking for ๐Ÿ˜„

Change 341365 merged by jenkins-bot:
[mediawiki/extensions/Popups] renderer: Ensure images don't overflow container

https://gerrit.wikimedia.org/r/341365

Change 341366 merged by jenkins-bot:
[mediawiki/extensions/Popups] renderer: Ensure settings cog visible in IE9-11

https://gerrit.wikimedia.org/r/341366

phuedx added a comment.Mar 8 2017, 4:28 PM

^ For context, @Jhernandez spotted a neat little bug where small SVG images weren't being scaled properly/the thumbnail URL 404'd. Here's the test page that he created to prove the point: https://en.wikipedia.beta.wmflabs.org/wiki/Popups-small-svg-thumbnail.

Merged.

Can be tested in beta cluster when it updates: https://en.wikipedia.beta.wmflabs.org/wiki/T156800

  • Verify that the thumbnails show up properly work properly
    • Popups-small-svg-thumbnail: Red cross
    • PreviewsNonFreeImage/sandbox: Fedora logo
Jhernandez updated the task description. (Show Details)Mar 8 2017, 6:06 PM
Jhernandez updated the task description. (Show Details)

Change 341327 merged by jenkins-bot:
[mediawiki/extensions/Popups] rest: Don't scale unscalable thumbnails

https://gerrit.wikimedia.org/r/341327

Popups-small-svg-thumbnail: Red cross

  • works as expected

PreviewsNonFreeImage/sandbox: Fedora logo

  • works as expected

Barack Obama

  • Image is centered incorrectly

@ABorbaWMF - could you confirm?

Firefox 51.0.1
https://en.wikipedia.beta.wmflabs.org/wiki/T156800


https://en.wikipedia.org/wiki/Michelle_Obama

Change 342191 had a related patch set uploaded (by Phuedx):
[mediawiki/extensions/Popups] rest: Always scale thumbnail's largest dimension

https://gerrit.wikimedia.org/r/342191

rEPOP1a37366c6701: rest: Always scale thumbnail's largest dimension fixes T156800#3087507 by scaling the thumbnail's largest dimension to the desired size. Note well that in @ovasileva's example above, the dimensions of the thumbnail returned by the PageImages API are 240px x 300px.

When reviewing and testing rEPOP1a37366c6701: rest: Always scale thumbnail's largest dimension, make sure you include the titles that @Jhernandez and @ovasileva used in their testing (see T156800#3087507) โ€“ at the very least!

Change 342191 merged by jenkins-bot:
[mediawiki/extensions/Popups] rest: Always scale thumbnail's largest dimension

https://gerrit.wikimedia.org/r/342191

Barack Obama looks good and is consistent on beta cluster with production now.

The cross is a little clipped and I'm not sure if that's acceptable:


I realise that we didn't clearly define what "works as expected" means. Is this expected?
TBH I'm finding this task extremely hard to follow and I'd appreciate subtasks capturing bugs for the specific issues we are finding to understand the behaviour we are expecting here and how I'm meant to be testing them.

Appears to be fixed on the target page: https://en.wikipedia.beta.wmflabs.org/wiki/T156800

Tested on the following:
Windows 10 - Chrome 56 - Edge - IE11 - Firefox 51
Windows 8.1 - Chrome 55 - Firefox 50 - Opera 43
Mac OSX 10.12 - Firefox 51 - Safari 10 - Chrome 57

TBH I'm finding this task extremely hard to follow and I'd appreciate subtasks capturing bugs for the specific issues we are finding to understand the behaviour we are expecting here and how I'm meant to be testing them.

Likely something for the retro.

phuedx removed phuedx as the assignee of this task.Mar 13 2017, 11:15 AM
phuedx assigned this task to ovasileva.

I think this is yours to sign off @ovasileva.

ovasileva closed this task as Resolved.Mar 14 2017, 4:16 PM

Looks good from here.