Page MenuHomePhabricator

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

Assigned To
Authored By
kaldari
Sep 17 2018, 11:30 PM
Referenced Files
F28664266: Screen Shot 2019-04-15 at 9.24.21 AM.png
Apr 15 2019, 4:36 PM
F28664263: Screen Shot 2019-04-15 at 9.22.55 AM.png
Apr 15 2019, 4:36 PM
F28664489: Screen Shot 2019-04-15 at 9.33.50 AM.png
Apr 15 2019, 4:36 PM
F28664264: Screen Shot 2019-04-15 at 9.23.12 AM.png
Apr 15 2019, 4:36 PM
F28664265: Screen Shot 2019-04-15 at 9.24.09 AM.png
Apr 15 2019, 4:36 PM
F28664487: Screen Shot 2019-04-15 at 9.33.26 AM.png
Apr 15 2019, 4:36 PM
F28664486: Screen Shot 2019-04-15 at 9.33.15 AM.png
Apr 15 2019, 4:36 PM
F28664488: Screen Shot 2019-04-15 at 9.33.38 AM.png
Apr 15 2019, 4:36 PM
Tokens
"Like" token, awarded by alexhollender_WMF.

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).

Screen Shot 2018-09-17 at 7.21.12 PM.png (347×583 px, 240 KB)

Screen Shot 2018-09-17 at 7.22.14 PM.png (315×530 px, 182 KB)

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:

Screen Shot 2018-12-14 at 11.51.20 AM.png (283×235 px, 29 KB)

This reveals the pokey is nowhere to be seen!

Screen Shot 2018-12-14 at 11.55.45 AM.png (293×479 px, 132 KB)

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)

Screen Shot 2018-12-14 at 12.08.12 PM.png (279×453 px, 83 KB)

and then width added:
Screen Shot 2018-12-14 at 11.57.35 AM.png (308×216 px, 94 KB)

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:::

Screen Shot 2018-12-14 at 1.18.59 PM.png (612×469 px, 261 KB)

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:

Screen Shot 2019-04-01 at 10.51.39 AM.png (390×553 px, 226 KB)

Screen Shot 2019-04-01 at 10.51.20 AM.png (398×278 px, 154 KB)

Screen Shot 2019-04-01 at 10.51.28 AM.png (378×494 px, 194 KB)

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

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
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

Screen Shot 2018-09-19 at 1.38.02 PM.png (546×958 px, 530 KB)

https://en.wikipedia.org/wiki/Theodore_of_Tarsus - Hover over Ecclesiastical History of the English
Screen Shot 2018-09-19 at 1.38.24 PM.png (558×976 px, 669 KB)

https://en.wikipedia.org/wiki/Main_Page - Hover over Eliud Kipchoge
Screen Shot 2018-09-19 at 1.36.08 PM.png (596×1 px, 551 KB)

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

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:

Screen Shot 2018-09-20 at 3.41.10 PM.png (326×539 px, 216 KB)

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 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 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:

Screenshot from 2019-02-11 16-03-22.png (912×1 px, 473 KB)

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

Screen Shot 2019-02-12 at 20.17.00.png (966×886 px, 554 KB)

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:

Screen Shot 2019-03-19 at 5.28.27 PM.png (286×490 px, 147 KB)

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

Screen Shot 2019-03-19 at 5.49.18 PM.png (288×535 px, 165 KB)

Let's now consider RTL mode:

Screen Shot 2019-03-19 at 5.31.59 PM.png (311×490 px, 190 KB)

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)

Screen Shot 2019-03-19 at 5.51.33 PM.png (365×522 px, 95 KB)

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

Screen Shot 2019-03-19 at 5.45.37 PM.png (331×569 px, 139 KB)

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

Screen Shot 2019-03-19 at 5.52.41 PM.png (315×522 px, 202 KB)

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.

Screen Shot 2019-03-20 at 11.22.50.png (932×1 px, 121 KB)

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!

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

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

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

Screen Shot 2019-04-15 at 9.24.21 AM.png (757×610 px, 334 KB)

Screen Shot 2019-04-15 at 9.24.09 AM.png (757×610 px, 320 KB)

Screen Shot 2019-04-15 at 9.23.12 AM.png (757×716 px, 323 KB)

Screen Shot 2019-04-15 at 9.22.55 AM.png (757×716 px, 282 KB)

✅ AC2: RTL

Screen Shot 2019-04-15 at 9.33.50 AM.png (623×642 px, 302 KB)

Screen Shot 2019-04-15 at 9.33.38 AM.png (623×642 px, 315 KB)

Screen Shot 2019-04-15 at 9.33.26 AM.png (623×953 px, 373 KB)

Screen Shot 2019-04-15 at 9.33.15 AM.png (623×953 px, 423 KB)

Single pokeys all around!