Page MenuHomePhabricator

Please add role=button in Collapsible Elements to a.mw-collapsible-text
Closed, ResolvedPublicBUG REPORT

Description

Using mw-collapsible class as described in https://www.mediawiki.org/wiki/Manual:Collapsible_elements auto-generates an anchor element <a class="mw-collapsible-text">Expand</a> to toggle collapse or expand the section.

This anchor element however is reported as an error by Google Lighthouse / Google Pagespeed https://pagespeed.web.dev/ . Error "Links are not crawlable". The anchor tag is auto-generated with JavaScript, but Lighthouse / Pagespeed renders this JavaScript before evaluating the page.

This error is due to Lighthouse needing a href attribute because it default interprets every anchor as a link. If no href attribute is present it registers the anchor as an error because it supposes it to be a crawlable link.

The implementation in MediaWiki is not wrong by HTML standards but it's not best practice and it might impact Lighthouse performance which is not optimal.

Our suggestion: Please add the attribute role="button" to the auto-generated link. The result would be <a class="mw-collapsible-text" role="button">Expand</a>. This is cleaner by HTMl standards and best practices and also prevents any Lighthouse errors which was already tested with Lighthouse locally.

Event Timeline

I don't think this is a good idea, because this element is already wrapped in a role="button" element. The markup looks like this: <span class="mw-collapsible-toggle mw-collapsible-toggle-default mw-collapsible-toggle-collapsed" role="button" tabindex="0" aria-expanded="false"><a class="mw-collapsible-text">Expand</a></span>

The inner a should already be treated as presentational, according to https://www.w3.org/TR/wai-aria-1.2/#tree_exclusion, since the button role has "Children Presentational: True". In addition, according to https://html.spec.whatwg.org/multipage/text-level-semantics.html#the-a-element, an a tag without href like this does not represent a link in the first place. Can you report this as a bug in Lighthouse?

It might be okay to use role="none" on the a instead, but I don't know whether that will have any effect on Lighthouse.

I feel like the correct fix, if there is a fix necessary, would be to just make it a button instead.......? Apply CSS that makes it look like it does today if absolutely necessary....

(See also T31118: Add HTML 5 semantic elements 'details' and 'summary' to Sanitizer whitelist I guess.)

In response to matmarex' reply we would agree that this is not technically a bug with Collapsible Elements regarding HTML validity. Also we support the role="none" solution. We tested this already and it worked.

We also agree with Izno that <button class="mw-collapsible-text">Expand</button> might be the better solution altogether. And CSS is already used to add the brackets before and after. So it would just be changing the CSS and not introducing more complexity by adding CSS where there was none before.

An [a] typically represents a link, a download or a jump label (with the name attribute before this solution was generally replaced by ids as jump labels). [a] is not designed to be used as an interactive element. So while syntactically correct and valid the use of [a] could be considered wrong here regarding context and use case.

Change 908372 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/core@master] jquery.makeCollapsible: Use <button> instead of <a>, but styled like a link

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

Change 908372 merged by jenkins-bot:

[mediawiki/core@master] jquery.makeCollapsible: Use <button> instead of <a>, but styled like a link

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

The brackets now always color full black, instead of inheriting the color of the surrounding text (because buttons have UA default colors). Do we want to remedy this and set them to inherit ?

Sure, if you want to give them color: inherit or something, I'd be happy to merge that patch.

Change 929012 had a related patch set uploaded (by Tacsipacsi; author: Tacsipacsi):

[mediawiki/core@master] jquery.makeCollapsible: Use `unset: all` on buttons

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

Sure, if you want to give them color: inherit or something, I'd be happy to merge that patch.

I’ve uploaded one.

Change 929012 merged by jenkins-bot:

[mediawiki/core@master] jquery.makeCollapsible: Use `unset: all` on buttons

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

Change 929968 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/core@master] Revert "jquery.makeCollapsible: Use `unset: all` on buttons"

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

Change 929969 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/core@wmf/1.41.0-wmf.13] Revert "jquery.makeCollapsible: Use `unset: all` on buttons"

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

Change 929969 merged by jenkins-bot:

[mediawiki/core@wmf/1.41.0-wmf.13] Revert "jquery.makeCollapsible: Use `unset: all` on buttons"

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

Mentioned in SAL (#wikimedia-operations) [2023-06-14T08:40:06Z] <tgr@deploy1002> Started scap: Backport for [[gerrit:929969|Revert "jquery.makeCollapsible: Use unset: all on buttons" (T333357 T338927)]]

Mentioned in SAL (#wikimedia-operations) [2023-06-14T08:41:51Z] <tgr@deploy1002> tgr: Backport for [[gerrit:929969|Revert "jquery.makeCollapsible: Use unset: all on buttons" (T333357 T338927)]] synced to the testservers: mwdebug1002.eqiad.wmnet, mwdebug1001.eqiad.wmnet, mwdebug2002.codfw.wmnet, mwdebug2001.codfw.wmnet

Mentioned in SAL (#wikimedia-operations) [2023-06-14T08:48:21Z] <tgr@deploy1002> Finished scap: Backport for [[gerrit:929969|Revert "jquery.makeCollapsible: Use unset: all on buttons" (T333357 T338927)]] (duration: 08m 14s)

Change 929968 merged by jenkins-bot:

[mediawiki/core@master] Revert "jquery.makeCollapsible: Use `unset: all` on buttons"

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