Page MenuHomePhabricator

[subtask] ext.popups uses a CSS selector not recognized by old browsers
Closed, ResolvedPublic1 Estimated Story PointsBUG REPORT

Description

Steps to replicate the issue (include links if applicable):

What happens?:
SyntaxError: '#mw-content-text a[href][title]:not(.extiw, .mw-selflink, .image, .new, .internal, .external, .mw-cite-backlink a, .oo-ui-buttonedElement-button, .ve-ce-surface a, .cancelLink a)' is not a valid selector

What should have happened instead?:
No JS error

Developer notes

I regularly test code with an antique browser as code that works on an old browser tends to work fine on a modern browser as well, but not the other way around. Prevents me from accidentally using features that may not be widely supported.

IIRC support for this style of CSS selector isn't quite ubiquitous yet.

browser stack: Replicable in Firefox 68 on Windows 10

QA Results - Prod

ACStatusDetails
1T324514#8473036

Event Timeline

Jdlrobson subscribed.

I think the short and quick answer is we don't support these browsers ( https://www.mediawiki.org/wiki/Compatibility#Browser_support_matrix ) and it's non-issue because:

  1. We might be limiting this to ES6 browsers in future as part of moving the extension off of Webpack.
  2. Most people are not using Firefox 68 to access the mobile domain (it's meant for mobile devices)
  3. The page previews feature is limited to non-touch devices so most mobile traffic [1] shouldn't need it.
  4. If it was an issue we'd be seeing significant Wikimedia-production-error events in our logs.

You have me curious though, since caniuse is talking about CSS support, and this code is used inside jQuery which shouldn't have a browser compatibility issue AFAIT? (https://api.jquery.com/not-selector/)

Either change the selector or, if it's not important, fail gracefully

What exactly is happening here and how is this code even loading on mobile site? At worse a JS error does sound like failing gracefully, provided it is not being logged as a #production-error and not breaking user experience? Are you seeing something off outside your developer console or is it breaking experience in some way? Also, how is this tied to the EditNoticesOnMobile gadget (can this be replicated without the gadget or is it tied to the gadget in some way?)

[1] https://github.com/wikimedia/mediawiki-extensions-Popups/blob/master/resources/ext.popups/index.js#L6

I provided FF68 as an example for reproduction, as can be seen on caniuse most browsers started supporting this about a year ago so FF83 should be affected too. (and IE, Opera Mini, UC browser for Android, QQ browser and KaiOS browser don't support it either according to caniuse) I don't know how many people use a browser that doesn't support this feature.

Just moving the mouse around, hovering over links, causes this error to be produced at a rapid rate. I collected >300 errors in no time at all.

"If it was an issue we'd be seeing significant Wikimedia-production-error events in our logs."
No you wouldn't. I can't reproduce this on production, only on beta cluster.

"Are you seeing something off outside your developer console or is it breaking experience in some way? Also, how is this tied to the EditNoticesOnMobile gadget (can this be replicated without the gadget or is it tied to the gadget in some way?)"
I don't see anything obvious breaking, but I don't know what the code is supposed to be doing.

I was testing ENOM but it's unrelated. https://commons.m.wikimedia.beta.wmflabs.org/wiki/User_talk:Foo_nonexistent_user also triggers the error.

I was on the mobile site to test ENOM, but actually the same issue exists on https://en.wikipedia.beta.wmflabs.org/wiki/Bert (a desktop site). Here I notice page previews don't work.

Jdlrobson triaged this task as Unbreak Now! priority.Dec 6 2022, 5:02 AM
Jdlrobson updated the task description. (Show Details)

No you wouldn't. I can't reproduce this on production, only on beta cluster.

Okay this worried me so I looked a bit closer (errors on beta are usually a signal of errors to come in production).
You threw be a bit with the talk of the mobile domain and the gadget callback. I can replicate this on the desktop domain without either of those and have simplified up the replication steps.

This relates to T233099 where we switched from using the jQuery match method to native method. As you've pointed out that won't be able to handle not selectors in older browsers.

I'm not sure how widespread this error is going to be, but probably worth a backport to play it safe to revert back to the jQuery method to avoid blocking the train. Probably safest to switch back to the jQuery method we were using before with an inline comment. I'll do that tomorrow.

$('body').is('body:not(.foo,.bar)')

Thanks for the report.

"I'm not sure how widespread this error is going to be"

Considering that the use of Object.values (which I believe to be far more widely supported) in ENOM caused 600+ log events in 12 hours ( T318249 ), my educated guess is: "very".

Thanks for looking at this!

Change 865098 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/Popups@master] Avoid syntax error on hover in grade C browsers

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

Change 865098 merged by jenkins-bot:

[mediawiki/extensions/Popups@master] Avoid syntax error on hover in grade C browsers

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

Change 865128 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/Popups@wmf/1.40.0-wmf.13] Avoid syntax error on hover in grade C browsers

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

Change 865128 merged by jenkins-bot:

[mediawiki/extensions/Popups@wmf/1.40.0-wmf.13] Avoid syntax error on hover in grade C browsers

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

Mentioned in SAL (#wikimedia-operations) [2022-12-06T19:19:54Z] <ladsgroup@deploy1002> Started scap: Backport for [[gerrit:865128|Avoid syntax error on hover in grade C browsers (T324514)]]

Mentioned in SAL (#wikimedia-operations) [2022-12-06T19:21:47Z] <ladsgroup@deploy1002> ladsgroup and jdlrobson: Backport for [[gerrit:865128|Avoid syntax error on hover in grade C browsers (T324514)]] synced to the testservers: mwdebug1002.eqiad.wmnet, mwdebug1001.eqiad.wmnet, mwdebug2002.codfw.wmnet, mwdebug2001.codfw.wmnet

Mentioned in SAL (#wikimedia-operations) [2022-12-06T19:32:37Z] <ladsgroup@deploy1002> Finished scap: Backport for [[gerrit:865128|Avoid syntax error on hover in grade C browsers (T324514)]] (duration: 12m 43s)

Jdlrobson renamed this task from ext.popups uses a CSS selector not recognized by old browsers to [subtask] ext.popups uses a CSS selector not recognized by old browsers.Dec 6 2022, 9:41 PM
Jdlrobson assigned this task to Edtadros.
Jdlrobson lowered the priority of this task from Unbreak Now! to Low.
Jdlrobson moved this task from Code Review to QA on the Web-Team-Backlog (Kanbanana-2022-23-Q2) board.
LGoto set the point value for this task to 1.Dec 8 2022, 6:19 PM
Edtadros subscribed.

Test Result - Prod

Status: ✅ PASS
Environment: enwiki
OS: Windows 10
Browser: Firefox 68
Device: PC (Browserstack)
Emulated Device:NA

Test Artifact(s):

QA Steps

✅ AC1: Hover over a page preview and confirm no error appears.

Screenshot 2022-12-15 at 6.49.17 PM.png (1×1 px, 797 KB)