Page MenuHomePhabricator

Popups calls mw.Uri on javascript: links, causing "bad constructor arguments" error
Closed, ResolvedPublic

Description

I'm seeing this problem at
https://www.mediawiki.org/wiki/Talk:Flow?debug=1
when a link with href javascript:void(0); is passed used in a call like new mw.Uri( this.href ):
https://github.com/wikimedia/mediawiki-extensions-Popups/blob/e21d08e0d0f5197420bab3bbc0bb311bf2501e0f/resources/ext.popups.core.js#L157

The element is this:
<a data-flow-menu-target="&lt; .flow-board-navigation .flow-board-sort-menu" data-flow-interactive-handler="menuToggle" title="You are currently reading the most recently active topics first. Click for more sorting options." data-tooltip-pointing="down" class="flow-board-navigator-last flow-ui-tooltip-target" href="javascript:void(0);">

Event Timeline

He7d3r created this task.Apr 6 2015, 8:18 PM
He7d3r raised the priority of this task from to Needs Triage.
He7d3r updated the task description. (Show Details)
He7d3r added a subscriber: He7d3r.
Restricted Application added a project: Collaboration-Team-Triage. · View Herald TranscriptApr 6 2015, 8:18 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Popups should not be processing links with the javascript protocol.

Mattflaschen-WMF renamed this task from Flow javascript links causes JavaScript error in Popups: Bad constructor arguments to Popups calls mw.Uri on javascript: links, causing "bad constructor arguments" error.Apr 6 2015, 9:22 PM
Mattflaschen-WMF set Security to None.
Se4598 added a subscriber: Se4598.EditedApr 6 2015, 10:31 PM

old bugs reintroduced. I'm not innocent on this one.
Are try-catch in JS code nice?

Note: keep this in mind at https://gerrit.wikimedia.org/r/199694

He7d3r added a comment.Apr 7 2015, 5:28 PM

And why is flow using javascript URLs?

EBernhardson triaged this task as High priority.Apr 7 2015, 5:40 PM
EBernhardson added a subscriber: EBernhardson.

Change 202976 had a related patch set uploaded (by Prtksxna):
core: getTitle: Return undefined for JavaScript links

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

TheDJ added a subscriber: TheDJ.Apr 9 2015, 2:14 PM

Eh good point, why is Flow even using javascript links like that ? Chrome is actively killing this option if I remember correctly, and they might be problematic for screenreader users as well...

In some cases (where no-JS makes sense), we have real links that are augmented with JavaScript (i.e. if you have no-JS, you go to a separate page, if you have JS it does something dynamic).

In others (JS-only), we need a link that doesn't really go anywhere. In this case, we sometimes use javascript:void(0). One alternative is <a href=""></a> (the prevent default is not the issue, that can and generally is done in the handler).

However, the latter (empty string) isn't really any more semantic. It means "go to the current page", which is not what we really mean.

Do you have a link for Chrome killing this feature?

TheDJ added a comment.EditedApr 15 2015, 9:33 AM

If you have a link that doesn't really go anywhere and has no JS fallback, then it's not a link, so you should use <a> without href attribute (which is functionally the same as a <span>, since it no longer has 'interactive' behavior/keyhandlers etc). Better might be a button, because then you don't need to manually add back keyboard handling and put role="button" tabindex="0" attributes on it.

Basically, don't abuse <a> elements because you want stuff to look like a link but to behave like a button. Only use it for when it IS a link fallback, that is overridden by a button behavior.
https://www.mediawiki.org/wiki/Accessibility_guide_for_developers#Don.27t

TheDJ added a comment.Apr 15 2015, 9:48 AM

On the support for the javascript protocol: It seems that chrome is only blocking you from entering javascript protocol in the URL bar for now. But I would not be surprised if Chrome devs axe Javascript protocol from anchors at some point in the future.

Also consider webpages are sandboxed inside something like the Twitter app for instance. For security reasons, they will block on anything but http and https protocols.

So basically your suggestion is to use <a role="button" tabindex="0">Hide</a> (i.e. an anchor with button role), then add back desired behavior (cursor, etc.)?

This is really a separate task, and probably going to be a low priority. However, if it's causing actual accessibility problems that would increase the urgency.

Change 202976 merged by jenkins-bot:
core: getTitle: Return undefined for non URI links

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

Mattflaschen-WMF closed this task as Resolved.Apr 25 2015, 2:24 AM
Mattflaschen-WMF assigned this task to Prtksxna.
Quiddity moved this task from Backlog to Done on the Page-Previews board.Jun 21 2015, 10:48 PM