Page MenuHomePhabricator

Popups: Double pokey on some page preview pop-ups due to SVG mask being larger than thumbnail
Closed, ResolvedPublic5 Estimate Story Points

Description

Sometimes I get page preview pop-ups with two pokeys. These screenshots are from the Main Page on English Wikipedia in safe mode (https://en.wikipedia.org/wiki/Main_Page?safemode=1).


Developer notes

Captured and immortalised in this jsfiddle:
https://jsfiddle.net/raw3y1jf/3/

The question is - why?

The problem is that the image thumbnail width is less than the expected 200px
https://github.com/wikimedia/mediawiki-extensions-Popups/blob/master/src/ui/thumbnail.js#L167
and the clip-path mask is a fixed width, so although the pokey is there - the image needs to be 200px to be aligned with it!

When changing this make sure to use the results of T205989 to verify that the output is behaving as expected.
The issue can be replicated here on landscape - Thin thumbnail

Add a green background to the SVG to make it clear where the mask is:

This reveals the pokey is nowhere to be seen!

For an image that's 129px, to get the pokey to show correctly the image needs to be displaced (via x attribute) by 200-129px (71px)


and then width added:

Acceptance criteria

  • The method setThumbnailClipPath needs to consider the displacement (200px - the actual width) and set a matrix:

e.g. for the example above where the image is 129px, rather than 200px...

matrix(1 0 0 1 -73 0)

setThumbnailClipPath will likely need to be passed a new parameter to deal with this, but is fully unit tested!

Tada:::

QA steps

(testing environment: Production)
For each test case test the 4 combos:

  • where the pokey is on the right of the preview
  • where the pokey is on the left
  • where pokey is above link
  • where pokey is below link

This can be achieved by resizing the window horizontally and making the link at either the bottom or top of page like so:



QA Note: To make the pokey appear on the right side of the preview on the Special:Search pages linked above, it helps to make the browser window very narrow.

QA Results

ACStatusDetails
1T204627#5112286
2T204627#5112286

Details

Related Gerrit Patches:
mediawiki/extensions/Popups : masterImproving RTL support for the Storybook app
mediawiki/extensions/Popups : masterFix double pokey bug

Event Timeline

kaldari created this task.Sep 17 2018, 11:30 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 17 2018, 11:30 PM
ovasileva triaged this task as Medium priority.Sep 18 2018, 10:19 PM
ovasileva added a project: Readers-Web-Backlog.
ovasileva moved this task from Backlog to Needs Analysis on the Page-Previews board.
ovasileva added a subscriber: ovasileva.
Jdlrobson added a subscriber: Jdlrobson.

I can't reproduce this. To analyse this I'm going to need to know browser, screen resolution, browser version, operating system version and some steps to reproduce reliably. @ovasileva can we arrange someone to dig into this some more?

ovasileva added a subscriber: ABorbaWMF.

I can't reproduce this. To analyse this I'm going to need to know browser, screen resolution, browser version, operating system version and some steps to reproduce reliably. @ovasileva can we arrange someone to dig into this some more?

@ABorbaWMF - do you think you'd have some time to look into reproducing this? @kaldari - could you add some more details whenever you get a chance?

I was able to reproduce it using Chrome and Safari on Mac. I have been able to reproduce it on a couple links and so far it has not happened on any sites other than English Wikipedia.

https://en.wikipedia.org/wiki/Main_Page - Hover over Theodore of Tarsus


https://en.wikipedia.org/wiki/Theodore_of_Tarsus - Hover over Ecclesiastical History of the English

https://en.wikipedia.org/wiki/Main_Page - Hover over Eliud Kipchoge

Thanks @ABorbaWMF! Moving along to needs analysis.

I still can't replicate this (even now trying Safari), can you please provide more information per https://phabricator.wikimedia.org/T204627#4596374 as maybe this is due to my browser/operating system combo?

How are you hovering over these links? Where are the links in relation to the window edges when you hover?
A video would be helpful if you can replicate this consistently and might give some clues.

Is it just me that can't replicate this?

Is it just me that can't replicate this?

I can't either

kaldari added a comment.EditedSep 20 2018, 5:03 PM

I can reproduce in Safari, Chrome, and Firefox, logged in or logged out, on MacOS. I'm hovering by hovering the cursor on a laptop (not touch-screen). Bug appears regardless of whether the pop-up is above or below the cursor. Only happens for certain links though. Window size doesn't seem to affect it.

oddly enough, today I can reproduce:

Chrome 68.0.3440.106
OSX 10.11.6

ovasileva set the point value for this task to 8.Oct 2 2018, 4:50 PM

During discussion we pointed out that T205989 might lower the risk of making such changes (and also help us understand this issue better)

Jdlrobson renamed this task from Double pokey on some page preview pop-ups to Popups: Double pokey on some page preview pop-ups.Nov 15 2018, 7:03 PM
Jdlrobson moved this task from Triaged but Future to Upcoming on the Readers-Web-Backlog board.
Jdlrobson updated the task description. (Show Details)Dec 14 2018, 7:44 PM
Jdlrobson renamed this task from Popups: Double pokey on some page preview pop-ups to Popups: Double pokey on some page preview pop-ups due to SVG mask being larger than thumbnail.Dec 14 2018, 8:12 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)
Jdlrobson removed ovasileva as the assignee of this task.Dec 14 2018, 9:19 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson removed the point value for this task.
Jdlrobson added a subscriber: Jdrewniak.

Thanks to @Jdrewniak story book adventures in T205989, I understand this bug. Given the new information, I think this should work out a lot easier than the 8 story points we originally proposed.

It sounds like this work is more approachable now that T205989 is complete and could be considered for a future kanbanana.

ovasileva set the point value for this task to 5.Jan 16 2019, 5:18 PM

I don't know if this is the same issue or not (it goes back at least as far as StoryPops 30c9de1) but I see pretty bad double pokeys in the StoryPops "RTL thumbnails" section:

Jdrewniak added a comment.EditedFeb 12 2019, 7:17 PM

@Niedzielski It might be an issue with the storybook implementation, that specific preview looks fine in production...

Had a quick look here and I'm pretty sure it's because the lang attribute is not set on the storybook iframe <html> element. Setting lang='he' and dir='rtl' on the English wikipedia has the same effect, probably because some of the popup elements are display inline-block.

Change 495758 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/extensions/Popups@master] Improving RTL support for the Storybook app

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

Change 495759 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/extensions/Popups@master] Fix double pokey bug

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

@Niedzielski wrote:
Can you add a comment here why the Math.min( thumbnail.width - SIZES.portraitImage.w, 0 ) logic isn't necessary for RTL?

Consider an image which is 193px. SVG mask is always 203px. I've made the SVG green to get a sense of what's going on. The green SVG

LTR mode:
We want to move the pokey to the left a little. The size of the image matters in this case, as the SVG mask might be a little too large.
matrix being used: matrix(1 0 0 1 -10 0) gives:


here translateX is Math.min( thumbnail.width - SIZES.portraitImage.w, 0 ) = Math.min( 193-200, 0 )
If the SVG mask is exactly the same as the image, no translation needed (0)

Here's the image without the SVG mask

Let's now consider RTL mode:


matrix being used: matrix(-1 0 0 1 203 0)
Here we rotate the entire map 180 degrees. This puts the pokey in the bottom left/top left corner and given its a mirror image, the image mask is to the left of the image... so we must move the mask the full width of the image (as the mask is always 203px).

We thus always want to move the full width of the mask (as that's fixed) only the image is variable
here translateX = SIZES.portraitImage.w = 203.

In case it helps:
The SVG mask with matrix(-1 0 0 1 0 0)

Here's the same preview with this matrix:
matrix(-1 0 0 1 53 0)

and with matrix(-1 0 0 1 367 0)

Change 495759 merged by Jdlrobson:
[mediawiki/extensions/Popups@master] Fix double pokey bug

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

matrix being used: matrix(-1 0 0 1 203 0)
Here we rotate the entire map 180 degrees. This puts the pokey in the bottom left/top left corner and given its a mirror image,

Something that I didn't realize before, (and that @Jdlrobson alluded to above) was that when we change the translateX to -1, the transform-origin of the mask is actually set to 0 0, being the top-left of the mask, not the centre of it. So the mask does't flip "in-place", but rather like the image below, Which is why we need to shift it back to 203 when we flip it.

Wow, that's super complicated. Have y'all considered just using a PNG pokey like the Echo overlay?

Wow, that's super complicated. Have y'all considered just using a PNG pokey like the Echo overlay?

Yup. You'll notice the pokey contains the image itself rather than a white background. That was a design constraint and why this is more complicated than Echo!

Overachievers! ;)

Jdrewniak updated the task description. (Show Details)Mar 21 2019, 10:55 AM
Jdrewniak reassigned this task from Jdrewniak to Jdlrobson.Mar 25 2019, 12:34 PM

Change 495758 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Improving RTL support for the Storybook app

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

Jdlrobson reassigned this task from Jdlrobson to Edtadros.Apr 1 2019, 5:53 PM
Jdlrobson updated the task description. (Show Details)
Edtadros reassigned this task from Edtadros to ovasileva.Apr 15 2019, 4:36 PM
Edtadros added a subscriber: Edtadros.

Test Result

Status: ✅ PASS
OS: macOS Mojave
Browser: Chrome
Device: MBP

Test Artifact(s):

QA Steps

(testing environment: Production)
For each test case test the 4 combos:

where the pokey is on the right of the preview
where the pokey is on the left
where pokey is above link
where pokey is below link

✅ AC1: LTR

✅ AC2: RTL

Edtadros updated the task description. (Show Details)
ovasileva closed this task as Resolved.Apr 16 2019, 9:51 AM

Single pokeys all around!