Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.0] [a11y] aria-describedby when field has a description #23919

Merged
merged 24 commits into from
Feb 25, 2019

Conversation

brianteeman
Copy link
Contributor

@brianteeman brianteeman commented Feb 16, 2019

The aria-described-by was in the wrong place and there was an unneeded title attribute

This was reported by @zwiastunsw #23896

I am working through the fields

fields included so far

  • list
  • number
  • text
  • textarea
  • calendar
  • url
  • tel
  • email
  • range
  • accesslevel
  • combo

NOTE - this change will need to be done for all field layouts even if core doesnt have any cases where there is a description

The aria-described-by was in the wrong place and there was an unneeded title attribute

This was reported by @zwiastunsw

I am working through the fields

fields included so far
- list
- number
- text
- textarea
@dgrammatiko
Copy link
Contributor

@brianteeman one thing to consider here: this description in J3 used to be the tooltip of each field. Some texts are way too long and probably inappropriate for an aria-describedby

Maybe for the shake of B/C we should leave this as is (and use it in the new tooltips, activated on mouse over a field, or on field focus) and introduce another attribute, eg describe ?

FWIW I'm not against what you're doing here, just thinking what will be the less painful approach here...

@brianteeman
Copy link
Contributor Author

@dgrammatiko I would much prefer it if peo0ple could SEE those crazy long tips and submit pr to change them to something other than a manual

@dgrammatiko
Copy link
Contributor

@brianteeman and we both know that some will resist those changes...

@brianteeman
Copy link
Contributor Author

@zwiastunsw
Copy link
Contributor

After the changes the descriptions are read by the screen reader. Perfectly.
@dgrammatiko : The number of characters in the aria-describedby attribute is not limited. But I agree that some descriptions need to be shortened.

@zwiastunsw
Copy link
Contributor

I have tested this item ✅ successfully on e38aaa4

I test pages: Article new/edit, Menus new/edit, Modules new/edit. Different field types


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/23919.

@zwiastunsw
Copy link
Contributor

@brianteeman Please, correct PR title: aria-describedby

@brianteeman brianteeman changed the title [4.0] [a11y] ariadescribed-by when field has a description [4.0] [a11y] aria-describedby when field has a description Feb 16, 2019
@brianteeman
Copy link
Contributor Author

@zwiastunsw Thanks for testing the work so far. I will continue on this tomorrow to make sure that all fields have this functionality even if the core doesnt use it

Quy and others added 3 commits February 16, 2019 22:56
Thanks -missed this one

Co-Authored-By: brianteeman <brian@teeman.net>
@brianteeman
Copy link
Contributor Author

@dgrammatiko

Maybe for the shake of B/C we should leave this as is (and use it in the new tooltips, activated on mouse over a field, or on field focus) and introduce another attribute, eg describe ?

Are these "new tooltips" available now? We need to resolve the search box descriptions that previously looked like
image

@dgrammatiko
Copy link
Contributor

@brianteeman the code needs some cleanup and fixing the positioning: https://codepen.io/dgrammatiko/pen/eGELjo

@brianteeman
Copy link
Contributor Author

@dgrammatiko looks good. We need this for the search tips. If you create a pr that would be awesome

@dgrammatiko
Copy link
Contributor

@brianteeman cans you please cancel the new variable?
Eg: this

$aria = $name . '-desc';
	$attr .= (!empty($description)) ? ' aria-describedby="' . $aria . '"' : '';

becomes

	$attr .= (!empty($description)) ? ' aria-describedby="' . $name . '-desc' . '"' : '';

Rule of thumb: declare a new variable only if you're gonna use it more than once, else you just adding more memory allocation for the app

@zwiastunsw
Copy link
Contributor

We also need it for buttons - font icons (without label).

@brianteeman
Copy link
Contributor Author

updated pr as requested

Co-Authored-By: brianteeman <brian@teeman.net>
@Quy
Copy link
Contributor

Quy commented Feb 18, 2019

When in an array, no need to add a leading space since it will be imploded later with a space.

This should also be fixed in layouts/joomla/form/field/url.php with all the attributes.

Quy and others added 8 commits February 18, 2019 08:48
Co-Authored-By: brianteeman <brian@teeman.net>
Co-Authored-By: brianteeman <brian@teeman.net>
Co-Authored-By: brianteeman <brian@teeman.net>
Co-Authored-By: brianteeman <brian@teeman.net>
Co-Authored-By: brianteeman <brian@teeman.net>
Co-Authored-By: brianteeman <brian@teeman.net>
@brianteeman
Copy link
Contributor Author

@wilsonge can you merge this one as well please. There won't be any more fields added to this PR as the remaining ones require js

@wilsonge
Copy link
Contributor

I don't have permission to your repo to merge in 4.0. so yes - but i need you to sync up with 4.0 please

@brianteeman
Copy link
Contributor Author

@wilsonge should be ok now

@wilsonge
Copy link
Contributor

waiting for rips to be sorted but intention is to merge this next

@brianteeman
Copy link
Contributor Author

OK - thanks

@wilsonge wilsonge merged commit af69393 into joomla:4.0-dev Feb 25, 2019
@wilsonge
Copy link
Contributor

Thanks!

@wilsonge wilsonge added this to the Joomla 4.0 milestone Feb 25, 2019
@brianteeman
Copy link
Contributor Author

Thanks

@brianteeman brianteeman deleted the aria branch February 25, 2019 11:02
@Quy
Copy link
Contributor

Quy commented Oct 15, 2019

How to make radio buttons a11y or is it already as is? There is id jform[sticky]-desc but no reference to it.

<div class="control-group">
  <div class="control-label sr-only">
    <label id="jform_sticky-lbl" > Pinned</label>
  </div>
  <div class="controls">
    <fieldset>
      <legend class="switcher__legend"> Pinned </legend>
      <div class="switcher">
        <input type="radio" id="jform_sticky0" name="jform[sticky]" value="0" checked="checked" class="active">
        <label for="jform_sticky0">No</label>
        <input type="radio" id="jform_sticky1" name="jform[sticky]" value="1" >
        <label for="jform_sticky1">Yes</label>
        <span class="toggle-outside"><span class="toggle-inside"></span></span> </div>
    </fieldset>
  </div>
  <div id="jform[sticky]-desc"> <small class="form-text text-muted"> Whether or not the Banner is 'pinned'. If one or more Banners in a Category are pinned, they will take priority over Banners that are not pinned. For example, if two Banners in a Category are pinned and a third Banner is not pinned, the third Banner will not display if the module setting is 'Pinned, Randomise'. Only the two pinned Banners will display. </small> </div>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants