Page MenuHomePhabricator

Buttons in MMV are not really buttons and are thus not semantic
Closed, ResolvedPublic

Description

In fixing T58471: Close and zoom buttons have no tooltips or speakable title (title attribute) I noticed that a bunch of buttons in MMV are actually divs. or as We should convert all of them to buttons with a title attribute and keep the tipsy wherever it is already there. For example:

  • $downloadButton
  • $copyButton
  • $button in stripeButtons
  • $close in mmv/ui/mmv.ui.permission.js

Instead of making this change, one could also T111159: Convert MultimediaViewer to use OOUI instead of MW UI buttons and icons (which has a much larger scale)

Event Timeline

dr0ptp4kt moved this task from Backlog to Needs Analysis on the MediaViewer board.

@Prtksxna would it be sufficient to add role=button et al. to make these accessible in the usual way? It's not clear what specific issue you're looking to solve.

@Prtksxna would it be sufficient to add role=button et al. to make these accessible in the usual way? It's not clear what specific issue you're looking to solve.

Yep, adding role=button would be sufficient to make it accessible, but turning it into a button would be better:

If you can use a native HTML element [HTML51] or attribute with the semantics and behaviour you require already built in, instead of re-purposing an element and adding an ARIA role, state or property to make it accessible, then do so.

+1 to @Prtksxna comment, there is no obvious reason not to use <button> elements with browser rendering support nowadays.

Hello all!
This would be my first time contributing to MediaWiki.
I can do this issue,I know Jquery and Php but the only thing is that I am not able to understand is that how do I understatement how the code is working in these files. I mean what different variables represent and all. I would like any one of you to guide me on my first issue so that I can learn more and Start contributing more to MediaWiki MultimediaViewer extension.

Thank you

Hi @prakamya, please see https://www.mediawiki.org/wiki/How_to_become_a_MediaWiki_hacker for setting up the development environment and installing extensions.

I am facing " Error: Execution of '/usr/bin/apt-get -q -y -o DPkg::Options::=--force-confold install apache2' returned 100: Reading package lists..." while doing vagrant up. can you help @Prtksxna?

@prakamya: This seems to be a general Vagrant setup problem and not tied to this specific task about MMV buttons. Please see https://www.mediawiki.org/wiki/How_to_become_a_MediaWiki_hacker#Feedback.2C_questions_and_support for places where to get general support - it's off-topic for this very task, sorry :)

I am facing " Error: Execution of '/usr/bin/apt-get -q -y -o DPkg::Options::=--force-confold install apache2' returned 100: Reading package lists..." while doing vagrant up.

That sounds like half of an error message (the less useful half, unfortunately).

Hey, I'm Hunter, and I'm one of five senior CS students that are on a team wanting to contribute to MediaWiki accessibility. We are all new to open-source contributing. We have gone through the Vagrant development environment setup and read through the MediaWiki hacking onboarding doc. We were looking through the open tasks to find one to work on and thought this would be an excellent place to start.

Any guidance or suggestions on how to go about this would be highly appreciated. Thanks.

Any guidance or suggestions on how to go about this would be highly appreciated. Thanks.

Install the mediaviewer role in Vagrant, locate the code and find where the variables mentioned in the task description are created (somewhere in /vagrant/mediawiki/extensions/MultimediaViewer/modules/mmv/ui/; grepping is probably the best way to locate them), change it so that the buttons are <button>s and not <div>s, update the CSS if needed, check the tests (see JavaScript unit testing), update them if needed, submit the change to gerrit.

This comment was removed by JHunterH.

@JHunterH: Please consider asking general development questions (not related to the specific task) in https://discourse-mediawiki.wmflabs.org/ - thanks!

Change 408577 had a related patch set uploaded (by HunterH; owner: HunterH):
[mediawiki/extensions/MultimediaViewer@master] Change <a> and <div> tags to <button>s

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

If a <button> already has a tittle property, is there any reason to add an aria-label as well?

@JHuntherH There's actually no strong reason to output the msg in aria-label, especially when older screenreader support is in question as well. aria-label had for example issues in NVDA 2014.1 with IE.
I'd rather put the text into the button and use something like text-indent: -9999em & overflow: hidden as long as the width is a fixed value for widest possible support.
Edited: Removed mention of core's .mixin-screen-reader-text() as it would need another DOM element.

Hey, I've been waiting for a patch review for this issue for about a week. Could someone help me out in trying to get the patch reviewed? Please and thank you in advance.

@Volker_E So I changed the approach from using msg in aria-label to using text-indent: -9999em & overflow: hidden for the patch set 5 but then I came across this documentation (https://www.mediawiki.org/wiki/Accessibility_guide_for_developers) that says don't it that way. Should I still go this route or do something different? Thanks.

Here are screenshots of the buttons that were converted. The top row is before and the bottom row is after.
The buttons that were converted were the copy button in the embed and share dialog, as well as in the download-pane.

Change 408577 merged by jenkins-bot:
[mediawiki/extensions/MultimediaViewer@master] Fix buttons in Multimedia Viewer to be semantic

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

@Aklapper The conversation went on at Gerrit.
@JHunterH Congratulations, good work so far!

@JHunterH $submitDiv from the task description in mmv/ui/mmv.ui.viewingOptions.js left untouched. Do you plan on amending this?

@Volker_E Yep, I missed that...how should I go about amending? Create a different Gerrit submission with this bug tagged or a sub task or...?

@JHunterH Gerrit patch set against this task is fine.

@Volker_E I would suggest that the $SubmitDiv should stay as a <div>. I could be misunderstanding the codebase, but I believe that $SubmitDiv creates a container for the SubmitButton and CancelButton (which are both <button> elements). To me, $SubmitDiv is not an interactive element, but a container for two interactive elements that are tagged appropriately. Please let me know if I am misinterpreting this. Thanks.

$SubmitDiv
SubmitButton
CancelButton
Volker_E removed a project: Patch-For-Review.
Volker_E updated the task description. (Show Details)

@JHunterH Yes, that's correct and you've accomplished everything as expected. Was just skimming through the task, therefore the confusion.