Page MenuHomePhabricator

HtmlFormatter incorrectly removes partial classname matches in "xenomobile" or "not-an-navbox"
Closed, ResolvedPublic

Description

I’m not sure whether this is a bug or a feature, but in any case it is a behaviour that (to my knowledge) is not documented and therefore not expectable. If I assign the CSS class nomobile to an element, either via TemplateStyles or via inline styles, it will not only be hidden in the mobile version of a wiki page, but even deleted entirely from the HTML source. Fine. But apparently this happens also if the class is called something-nomobile, as it happened to me recently when using TemplateStyles (per convention on deWP, I use a unique identifier before all my individual class names, in this case charts-, which is supposed to prevent such class conflicts)! According to my tests, somethingnomobile or nomobilesomething don’t cause any trouble, only the version with the hyphen is apparently equalised with the actual nomobile class. I feel like class names that only contain nomobile should never trigger this behaviour or is there any particular reason for it?

QA steps

Create a new page with the text

<div class="xe-nomobile">I should display on mobile (xe-nomobile)</div>
<div class="nomobile">I should not display on mobile (nomobile) </div>

<div class="not-navbox">I should display on mobile (not-navbox)</div>
<div class="navbox">I should not display on mobile (navbox) </div>

Expected: In desktop all four elements should be visible. In mobile site only the two mobile elements should display

QA Results - Beta

ACStatusDetails
1T231160#6349269
2T231160#6349269

QA Results - Prod

ACStatusDetails
1T231160#6363130
2T231160#6363130

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 25 2019, 12:20 PM
Jdlrobson renamed this task from CSS nomobile class conflict to HtmlFormatter incorrectly removes partial matches for elements with hypens.EditedAug 26 2019, 3:18 PM
Jdlrobson removed a project: MobileFrontend.
Jdlrobson added a subscriber: Jdlrobson.

Adding a class nomobile will delete content from the mobile domain and is used and documented in a variety of places.

I was surprised to hear that classes that match nomobile and tried a few that match and can see that the hypen does cause problems so this is a bug that will probably have to be worked around for the time being by not using -

The issue is in https://github.com/wikimedia/html-formatter/blob/master/src/HtmlFormatter.php#L183 which is bundled in core via composer. I'm not sure how to tag that correctly... but this is indeed a bug. @Aklapper do you know which tags would be appropriate?

Maybe this is a good motivation for adopting Remex?

Alright, I have now added a warning to the German WP project pages. In my case I have simply renamed the class in hidemobile for the moment.

Anomie added a subscriber: Anomie.

The issue is in https://github.com/wikimedia/html-formatter/blob/master/src/HtmlFormatter.php#L183 which is bundled in core via composer. I'm not sure how to tag that correctly... but this is indeed a bug. @Aklapper do you know which tags would be appropriate?

It doesn't look like we have a tag for the HtmlFormatter library, which would be the correct tag for it.

This analysis seems to be correct. The linked line should be something like

$elements = $xpath->query( '//*[contains(concat(" ", normalize-space(@class), " "), " ' . $classToRemove . ' ")]' );

Other XPath code in that library should probably also be checked for similar issues.

Dinoguy1000 renamed this task from HtmlFormatter incorrectly removes partial matches for elements with hypens to HtmlFormatter incorrectly removes partial matches for elements with hyphens.Aug 28 2019, 9:37 AM
WDoranWMF triaged this task as Low priority.Sep 11 2019, 2:34 PM
AMooney changed the task status from Open to Stalled.Mar 12 2020, 1:22 PM
Krinkle renamed this task from HtmlFormatter incorrectly removes partial matches for elements with hyphens to HtmlFormatter incorrectly removes partial classname matches in "xenomobile" or "not-an-navbox".Jul 21 2020, 3:08 PM
Krinkle added a subscriber: Krinkle.

Ran into the same problem today with a sidebar template being corruped due to one portion of a larger infobox being stripped out due to containing the sequence navbox despite not matching the selector .navbox.

Krinkle changed the task status from Stalled to Open.Jul 21 2020, 3:32 PM
Krinkle claimed this task.

Change 615238 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[HtmlFormatter@master] tests: Add regression tests for ignoring "foo" in "telefoon"

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

Change 615239 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[HtmlFormatter@master] Fix incorrect matching of ".foo" on <span class="foo-something">

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

(Not sure who maintains this library but it's not reading web)

Krinkle added a comment.EditedJul 23 2020, 10:38 PM

It was created in Reading Web's MobileFrontend and today is only used by the MobileFrontend and TextExtracts extensions. It was moved from the MF repo to a library repo in 2013 to allow re-use in TextExtracts and later CirussSearch (the latter no longer uses it).

At what point did you decide to transfer it to a different team, and who did it move to? :)

@Jdlrobson This is a bug in MobileFrontend either way. It could also be fixed by not using HtmlFormatter, for example.

It was created in Reading Web's MobileFrontend and today is only used by the MobileFrontend and TextExtracts extensions

Please remember, Reading web != mobile web team (the team that created that extension). The "mobile web" team no longer exists and hasn't for about 5 years. Looking at the history, it looks like Max Semenik was the last person to touch this code and he hasn't been in the team for just as long, so maintenance responsibilities for this likely slipped through the net.

At what point did you decide to transfer it to a different team, and who did it move to? :)

Well the reality is we are not maintaining it and never have been :). I think the point here is that nobody has asked "reading web" to maintain it and it shouldn't be assumed we are just because there was once someone in our team who created it. FWIW I don't think we'd be good maintainers for the same reasons we're not good maintainers of TextExtracts given T256505.

Change 615238 merged by jenkins-bot:
[HtmlFormatter@master] tests: Add regression tests for ignoring "foo" in "telefoon"

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

Change 615239 merged by jenkins-bot:
[HtmlFormatter@master] Fix incorrect matching of ".foo" on <span class="foo-something">

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

We can help with qa and sign off since this impacts mobile html.

I've just cut the next HtmlFormatter release.

Jdlrobson reassigned this task from Krinkle to Edtadros.Jul 27 2020, 5:09 PM
Jdlrobson updated the task description. (Show Details)

Change 616562 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] Update wikimedia/html-formatter to 2.0.0

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

Change 616563 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/vendor@master] Update wikimedia/html-formatter to 2.0.0

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

Change 616563 merged by jenkins-bot:
[mediawiki/vendor@master] Update wikimedia/html-formatter to 2.0.0

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

Change 616562 merged by jenkins-bot:
[mediawiki/core@master] Update wikimedia/html-formatter to 2.0.0

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

Test Result - Beta

Status: ✅ PASS
Environment: beta
OS: macOS Catalina
Browser: Chrome
Device: MBP
Emulated Device: iPad

Test Artifact(s):

QA steps

Create a new page with the text

<div class="xenomobile">I should display on mobile</div>
<div class="nomobile">I should not display on mobile</div>

✅ AC1: In desktop both elements should be visible.

✅ AC2: In mobile site only 1st element should display

Edtadros updated the task description. (Show Details)Jul 30 2020, 4:16 PM
Edtadros updated the task description. (Show Details)

The test case is flawed I think because xenomobile was never an issue in production, e,g. comparing the same on production in a Sandbox also works as expected today. The issue was when a dash is in the mix. For example xe-nomobile was wrongly seen as nomobile and not-navbox wrongly seen as navbox.

Consider these instead, which are currently broken in production:

<div class="xe-nomobile">I should display on mobile (xe-nomobile)</div>
<div class="nomobile">I should not display on mobile (nomobile) </div>

<div class="not-navbox">I should display on mobile (not-navbox)</div>
<div class="navbox">I should not display on mobile (navbox) </div>

@Edtadros apologies for the flawed test case. Can we re-run on beta cluster with @Krinkle suggestion ?

@Jdlrobson will do. @Krinkle thanks for the clarification.

Test Result - Beta

Status: ✅ PASS
Environment: beta
OS: macOS Catalina
Browser: Chrome
Device: MBP
Emulated Device: iPad

Test Artifact(s):

QA steps

Create a new page with the text

<div class="xe-nomobile">I should display on mobile (xe-nomobile)</div>
<div class="nomobile">I should not display on mobile (nomobile) </div>

<div class="not-navbox">I should display on mobile (not-navbox)</div>
<div class="navbox">I should not display on mobile (navbox) </div>

✅ AC1: In desktop four elements should be visible.

✅ AC2: In mobile site only the two mobile elements should display

Edtadros updated the task description. (Show Details)Jul 30 2020, 4:56 PM
Edtadros updated the task description. (Show Details)
Edtadros reassigned this task from Edtadros to ovasileva.Aug 5 2020, 3:51 PM
Edtadros added a subscriber: Edtadros.

Test Result - Prod

Status: ✅ PASS
Environment: testwiki
OS: macOS Catalina
Browser: Chrome
Device: MBP
Emulated Device: NA

Test Artifact(s):

QA steps

Create a new page with the text

<div class="xe-nomobile">I should display on mobile (xe-nomobile)</div>
<div class="nomobile">I should not display on mobile (nomobile) </div>

<div class="not-navbox">I should display on mobile (not-navbox)</div>
<div class="navbox">I should not display on mobile (navbox) </div>

✅ AC1: In desktop four elements should be visible.

✅ AC2: In mobile site only the two mobile elements should display

Edtadros updated the task description. (Show Details)Aug 5 2020, 3:52 PM
ovasileva closed this task as Resolved.Aug 6 2020, 2:07 PM

Change 619485 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Hide vertical nav-boxes on mobile domain

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

Jdlrobson reopened this task as Open.Aug 11 2020, 3:12 PM

This has revealed vertical navboxes on tablet which was an unexpected side effect, that was relying on this bug:


I've submitted a fix.

@Jdlrobson I also filed a bug report for that this morning - https://phabricator.wikimedia.org/T260170 - feel free to close as a duplicate if this fix is currently being tracked here.

Jdlrobson raised the priority of this task from Low to High.Aug 11 2020, 6:33 PM

Upping priority since this is quite a visible problem now for end users.

Change 619380 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@wmf/1.36.0-wmf.3] Hide vertical nav-boxes on mobile domain

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

Change 619381 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@wmf/1.36.0-wmf.4] Hide vertical nav-boxes on mobile domain

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

Change 619485 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Hide vertical nav-boxes on mobile domain

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

Change 619380 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@wmf/1.36.0-wmf.3] Hide vertical nav-boxes on mobile domain

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

Change 619381 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@wmf/1.36.0-wmf.4] Hide vertical nav-boxes on mobile domain

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

Change 619592 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[operations/mediawiki-config@master] Update wgMFRemovableClasses

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

Mentioned in SAL (#wikimedia-operations) [2020-08-11T23:34:52Z] <urbanecm@deploy1001> Synchronized php-1.36.0-wmf.4/extensions/MobileFrontend/extension.json: 81d54b0ec82d0b78f723f9400031e918a4a143aa: Hide vertical nav-boxes on mobile domain (T231160) (duration: 01m 05s)

Change 619592 merged by jenkins-bot:
[operations/mediawiki-config@master] Update wgMFRemovableClasses

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

Mentioned in SAL (#wikimedia-operations) [2020-08-11T23:36:35Z] <urbanecm@deploy1001> Synchronized php-1.36.0-wmf.3/extensions/MobileFrontend/extension.json: c22d65ff9b2439f484ab8ccffed87b00e78c3ad2: Hide vertical nav-boxes on mobile domain (T231160) (duration: 01m 03s)

Mentioned in SAL (#wikimedia-operations) [2020-08-11T23:39:32Z] <urbanecm@deploy1001> Synchronized wmf-config/InitialiseSettings.php: 0f238f71c95c7bd7534c28abfac759fbb47f674f: Update wgMFRemovableClasses (T231160) (duration: 01m 03s)

ovasileva closed this task as Resolved.Aug 12 2020, 11:32 AM

All fixed now, thanks @Jdlrobson!