Page MenuHomePhabricator

Popup extension: use templates instead of HTML parse
Closed, ResolvedPublic

Description

$.parseHTML is a performance bottleneck. Replacing it with cloned templates have shown to be more performant.
The template element, which would be ideal, is not available on IE11, but it's possible to do this without it.

QA steps

When JS is disabled a browser-based tooltip should display on hover.
When page previews is disabled a browser-based tooltip should display on hover.
the browser-based tooltip should not display on hover when page previews is enabled

QA Results - Beta

ACStatusDetails
1T269338#6686493
2T269338#6686493
3T269338#6684167

Event Timeline

Change 645074 had a related patch set uploaded (by Noam Rosenthal; owner: Noam Rosenthal):
[mediawiki/extensions/Popups@master] Parse template HTML only once, as HTML parsing is expensive

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

Change 645074 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Parse template HTML only once, as HTML parsing is expensive

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

Edtadros added a subscriber: Edtadros.

Test Result - Beta

Status: ❌ FAIL
Environment: beta
OS: macOS Big Sur
Browser: Chrome
Device: MBP
Emulated Device: NA

Test Artifact(s):

QA Steps

❌ AC1: When JS is disabled a browser-based tooltip should display on hover.
I do not get a tooltip.

Screen Recording 2020-12-10 at 6.17.01 PM.mov.gif (1×890 px, 297 KB)

❌ AC2: When page previews is disabled a browser-based tooltip should display on hover.
I do not get a tooltip.

Screen Recording 2020-12-10 at 6.26.07 PM.mov.gif (1×890 px, 355 KB)

✅ AC3: the browser-based tooltip should not display on hover when page previews is enabled

Screen Recording 2020-12-10 at 6.23.44 PM.mov.gif (1×890 px, 435 KB)

Jdlrobson added a subscriber: Jdlrobson.

I think the former 2 QA steps were because of the now reverted changes in T269297

Can you try QAing this one again?

Test Result - Beta

Status: ❓ Need more information
Environment: beta
OS: macOS Big Sur
Browser: Chrome
Device: MBP
Emulated Device: NA

Test Artifact(s):

QA Steps

❓ AC1: When JS is disabled a browser-based tooltip should display on hover.
I see the tool tip now.

Screen Recording 2020-12-10 at 6.17.01 PM.mov.gif (1×890 px, 297 KB)

However, if I move the browser to a different monitor, the tooltip doesn't follow. It appears on my original monitor but at some scaled ratio of coordinates. For example, if I move my browser to the left of my secondary monitor, the tooltip will move to the left, but not pixel per pixel. My secondary monitor is an ultrawide monitor. Here is a grab of me showing the tooltip on my main monitor, then I move the browser to the secondary monitor and you still see the tooltip on my primary. This may have been the issue with the first QA attempt.
ezgif.com-gif-maker.gif (513×600 px, 2 MB)

❓ AC2: When page previews is disabled a browser-based tooltip should display on hover.
I see the tool tip now. This also behaves as above where the tooltip only shows up on the primary monitor.

Screen Recording 2020-12-10 at 6.26.07 PM.mov.gif (1×890 px, 355 KB)

✅ AC3: the browser-based tooltip should not display on hover when page previews is enabled
I re-verified that this still works.

Edtadros updated the task description. (Show Details)

@Jdlrobson I'm leaving this in QA but assigning it to you for some clarification. I noticed some odd behavior with the tooltips and multi-monitor setups. You can see an example of this in the second screengrab in T269338#6686493. It appears the tooltip doesn't like to go onto a second monitor. This might be a browser issue or just an issue with my particular setup, but I wanted to highlight it for you to decide if this is or is not in the scope of this task.

@edtrados this is fine. As long as it's showing I wouldn't worry about additional monitor behaviour. That was likely a pre-existing condition.

Jdlrobson triaged this task as Medium priority.Jan 6 2021, 6:07 PM