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

Assigned To

Authored By

	Nomsterio
	Dec 3 2020, 9:05 AM

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

AC	Status	Details
1	❓	T269338#6686493
2	❓	T269338#6686493
3	✅	T269338#6684167

Details

Related Changes in Gerrit:

	Subject	Repo	Branch	Lines +/-
	Parse template HTML only once, as HTML parsing is expensive	mediawiki/extensions/Popups	master	+118 -62

Customize query in gerrit

Related Objects

Mentioned Here: T269297: Performance optimization for popup rendering: Remove title attributes at init

Event Timeline

Nomsterio created this task.Dec 3 2020, 9:05 AM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 3 2020, 9:05 AM

• Gilles added a subscriber: phuedx.Dec 3 2020, 9:31 AM

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

gerritbot added a project: Patch-For-Review.Dec 3 2020, 1:44 PM

Jdlrobson added a project: Web-Team-Backlog-Archived (Kanbanana-FY-2020-21).Dec 3 2020, 4:13 PM

Jdlrobson moved this task from Needs Analysis to Code Review on the Web-Team-Backlog-Archived (Kanbanana-FY-2020-21) board.Dec 3 2020, 8:20 PM

Jdlrobson reassigned this task from Nomsterio to Edtadros.Dec 7 2020, 10:40 PM

Jdlrobson moved this task from Code Review to QA on the Web-Team-Backlog-Archived (Kanbanana-FY-2020-21) board.

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

ReleaseTaggerBot added a project: MW-1.36-notes (1.36.0-wmf.21; 2020-12-08).Dec 8 2020, 12:00 AM

Maintenance_bot removed a project: Patch-For-Review.Dec 8 2020, 12:10 AM

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)

Edtadros updated the task description. (Show Details)Dec 11 2020, 2:30 AM

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.

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.

✅ 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)Dec 12 2020, 3:33 PM

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 moved this task from QA to QA in Prod on the Web-Team-Backlog-Archived (Kanbanana-FY-2020-21) board.Jan 4 2021, 6:14 PM

ovasileva moved this task from QA in Prod to Ready for Signoff on the Web-Team-Backlog-Archived (Kanbanana-FY-2020-21) board.Jan 4 2021, 6:14 PM

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

Jdlrobson closed this task as Resolved.Jan 11 2021, 11:18 PM

	F33942359: Screen Recording 2020-12-10 at 6.17.01 PM.mov.gif
	Dec 11 2020, 2:28 AM

	F33942361: Screen Recording 2020-12-10 at 6.23.44 PM.mov.gif
	Dec 11 2020, 2:28 AM

	F33942368: Screen Recording 2020-12-10 at 6.26.07 PM.mov.gif
	Dec 11 2020, 2:28 AM

Popup extension: use templates instead of HTML parseClosed, ResolvedPublicActions

Description

QA Results - Beta

Details

Related Objects

Event Timeline

Test Result - Beta

QA Steps

Test Result - Beta

QA Steps

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