Page MenuHomePhabricator

Vector: Middle click on <a href="#"> in tabs should not open "{current url}#" in a new window
Closed, ResolvedPublic

Description

Patch for Vector.php

Skin Vector contains <a href="#"></a> for variants and for actions. This generates a annoying link to # when clicking with a middle click or when JavaScript is disabled.

Please remove href="#".


Version: unspecified
Severity: normal

attachment Vector.php.patch ignored as obsolete

Details

Reference
bz42241

Event Timeline

bzimport raised the priority of this task from to Normal.Nov 22 2014, 12:44 AM
bzimport added a project: Vector.
bzimport set Reference to bz42241.
bzimport added a subscriber: Unknown Object (MLST).
Fomafix created this task.Nov 18 2012, 1:04 PM

Removing href="#" does not solve the problem. That will instead create a link to the current url (and the behaviour is unstable in older browsers).

This is likely a case of a forgotten call to e.preventDefault().

Also, these anchor tags on the headings shouldn't exist afaik. Are they still there? I thought we're styling the <h5> directory now, or at least adding them dynamically.

With e.preventDefault() it is not possible to prevent a middle click to open a new tab at least in Firefox 16 and Opera 12. And when JavaScript is disabled the link is always active. This is annoying.

Why is there a <a href="#"></a>? Would a simple <div></div> be enough?

(In reply to comment #3)

With e.preventDefault() it is not possible to prevent a middle click to open a
new tab at least in Firefox 16 and Opera 12. And when JavaScript is disabled
the link is always active. This is annoying.
Why is there a <a href="#"></a>? Would a simple <div></div> be enough?

Yes, as I said already:

(comment #2)

[..] these anchor tags on the headings shouldn't exist.

They are only there for style (blue links, and pointy cursor). Which is already being handled on the javascript side. Without javascript, the links don't belong there.

Patch for Bug 42241

Replace <a href="#"></a> by <div></div>

attachment bug42241.patch ignored as obsolete

Fomafix:
You are welcome to use Developer access

https://www.mediawiki.org/wiki/Developer_access

to submit this as a Git branch directly into Gerrit:

https://www.mediawiki.org/wiki/Git/Tutorial

Putting your branch in Git makes it easier for us to review it quickly.

Patch for Bug 42241

Rebased patch with <h3> instead of <h5>.

Simplified because display: block is default for <div>.

With <div> there is no workaround for Internet Explorer 6 necessary. Tested with Internet Explorer 6.

Attached:

sumanah wrote:

Fomafix, can you submit your change in Git?

Unfortunately, this patch does not apply cleanly to master.

Fomafix, you can also submit patches using the web-based uploader at https://tools.wmflabs.org/gerrit-patch-uploader . Please use 'Bug: 42241' in your commit message to link it to this bug.

Change 136711 had a related patch set uploaded by Gerrit Patch Uploader:
Vector: Use <div> instead of <a> for action menu

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

Change 139795 had a related patch set uploaded by Gerrit Patch Uploader:
Use <a> instead of <a href="#"> for JavaScript click events

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

Change 154453 had a related patch set uploaded by Gerrit Patch Uploader:
Use <div> instead of <a> for action menu

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

Change 136711 abandoned by Bartosz Dziewoński:
Vector: Use <div> instead of <a> for action menu

Reason:
Thank you.

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

Change 154453 abandoned by Krinkle:
Use <div> instead of <a> for action menu

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

matmarex set Security to None.
Edokter closed this task as Declined.May 2 2015, 9:47 AM
Edokter claimed this task.
Isarra added a subscriber: Isarra.May 2 2015, 4:02 PM

Why was this declined?

Edokter removed Edokter as the assignee of this task.May 2 2015, 4:40 PM

Because most patches are abandoned, and any solution will involve too much payload just to solve an edge case.

matmarex reopened this task as Open.Jun 3 2015, 4:31 PM

That may be true for https://gerrit.wikimedia.org/r/139795, but that patch isn't really related to this bug, to be honest.

The actual patch for this is https://gerrit.wikimedia.org/r/154453 and it doesn't have this problem (in fact it decreases the payload ever so slightly), although it needs some work. It seems like a worthwhile thing to do, though.

Change 154453 had a related patch set uploaded (by Bartosz Dziewoński):
Use <div> instead of <a> for action menu

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

Not convinced this needs action. It will also break any script interacting with the menu... again.

Also, as @Krinkle hase pointed out in the patch, divs cannot have focus and thus cannot be navigated using the keyboard. That hurts accessability more then the purported middle-click would.

@Edokter: You are wrong. <div>s can have a focus and can navigated using the keyboard by tabindex="0". @Krinkle pointed out that the tabindex="0" is added by JavaScript and not in HTML.

Then I was partly wrong, and it still presents a problem.

One of the criteria of MediaWiki, is that basic functionality must work without JavaScript. If you can't tab to basic links (without JavaScript), then that functionality is broken.

When JavaScript is disabled the menu is currently not accessible by keyboard only. Was this ever possible? This change does not change the current behavior.

Sipun added a subscriber: Sipun.Jun 24 2015, 7:52 AM
Jdlrobson changed the task status from Open to Stalled.Sep 24 2015, 12:03 AM
Jdlrobson added a subscriber: Jdlrobson.

Patch is waiting on a response from @Edokter

Isarra moved this task from Backlog to Bugs on the Vector board.Apr 7 2016, 1:21 AM
Krinkle removed a subscriber: Krinkle.Apr 7 2016, 11:50 PM
Jdlrobson lowered the priority of this task from Normal to Lowest.Jul 21 2016, 8:38 PM

Given the arguments on the patches and history.

I propose we go ahead and merge https://gerrit.wikimedia.org/r/#/c/154453/
That patch does seem to fix this problem.
Any objections?

Change 154453 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Use <div> instead of <a> for action menu

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

matmarex closed this task as Resolved.Jun 21 2017, 10:53 PM
matmarex assigned this task to Fomafix.
matmarex removed a project: Patch-For-Review.