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

Tweak markup so screen readers use correct context of ID and title #1840

Open
petecooper opened this issue Nov 7, 2022 · 18 comments
Open

Tweak markup so screen readers use correct context of ID and title #1840

petecooper opened this issue Nov 7, 2022 · 18 comments

Comments

@petecooper
Copy link
Member

https://forum.textpattern.com/viewtopic.php?id=51932

When in the admin article or category lists (I haven’t tested files or images), the title and ID of the article or category is not displayed in a way that my screen reader can announce it to me.

Follow up from @Bloke:

We should definitely tweak the anchors or markup so that screen readers etc can latch onto the context of the ID and title.

@phiw13
Copy link

phiw13 commented Nov 8, 2022

You should widen the scope of this and improve all uses of aria-label across all panes. For instance on the Article list panel, under the status column: the word (live or draft or …) is a link with aria-label=View which then actually hide the actual status of the article to AT. Some parts of the Sections panel do slightly less poorly (e.g On default page column: aria-label=Toggle yes/no) although that still does not broadcast the actual state.

@Bloke
Copy link
Member

Bloke commented Nov 8, 2022

This is important and will be a lot easier now we have the UI library. As we gradually migrate all controls over to the library, we'll ensure that aria labels are consistently applied.

So, best practice for those horrible clickable toggle state controls is actually to label them as the state they are in, and not the action that will happen if activated? Or both? E.g. aria-label=Yes (togglable) or something like that?

@philwareham philwareham self-assigned this Nov 8, 2022
@philwareham
Copy link
Member

I'll take ownership of this when I can get back onto Textpattern work. I'll also read up on the latest accessibility stuff as I've been away from web design work mostly this year, so a little rusty.

@phiw13
Copy link

phiw13 commented Nov 9, 2022

Or both? E.g. aria-label=Yes (togglable) or something like that?

Yes in the case of those toggle controls, that would be a better option.

In general, always first ponder - is there a textnode in the HTML ? If yes, then, do I really need to add an aria-label? For example, on the Articles list panel, Title column: what the user want to know is the title of the article. That is there in the HTML, no need for a aria-label. The screenreader will additionally tell the user, there is a link with target that-and-that.html.

Thought: what could be an improvement for all users for that particular case: add the “Edit” word in the HTML and use that as link-name.

<td class="">title of article (<a href="write-panel.html">Edit</a>)</td>

@Bloke
Copy link
Member

Bloke commented Nov 9, 2022

Nice idea and scope, thanks @phiw13

@philwareham
Copy link
Member

I have now removed any aria-label attributes from the control panel where title already exists. Hope that helps.

Any other accessibility updates I'd rather leave until the new UI system is being used. Any issues can then be raised and fixed as individual items at that time. Thanks.

@petecooper petecooper added this to the v4.9 milestone Jan 19, 2023
@philwareham
Copy link
Member

philwareham commented Dec 12, 2023

For yes/no and on/off combinations (such as on the prefs panel), I guess we need to add aria-labelledby attributes (or aria-describedby, because the radio elements already have labels) to the yes/on and the no/off radios, linking back to the parent label, for example:

<div class="txp-form-field" id="prefs-enable_dev_preview">
    <div class="txp-form-field-label" id="prefs-enable_dev_preview-label">Enable development theme preview</div>
    <div class="txp-form-field-value">
        <input class="radio" id="enable_dev_preview-0" aria-describedby="prefs-enable_dev_preview-label" name="enable_dev_preview" type="radio" value="0">
        <label for="enable_dev_preview-0">No</label>
        <input class="radio active" id="enable_dev_preview-1" aria-describedby="prefs-enable_dev_preview-label" name="enable_dev_preview" type="radio" checked="checked" value="1">
        <label for="enable_dev_preview-1">Yes</label>
    </div>
</div>

What do you think?

@phiw13
Copy link

phiw13 commented Dec 15, 2023

For the radio combo sets on the Preferences panel (e.g) I don’t think you need to add any aria-describedby attribute as by default the<label> does that job anyway. Another story is that horrible yes/no toggle for the active state on the plugin panel – or those columns on the Sections panel (on default page ? etc). There the proposed aria-label text will do a decent job to clarify (assuming that toggle is kept “as is”…).

@philwareham
Copy link
Member

philwareham commented Dec 15, 2023

On those prefs radios, doesn't the label just state "yes"/"no" - not the overarching summary of what the preference is?

So, if I tab to the radio combo, wouldn't the screen reader just read out "yes" for example, without the parent summary being read out? If so, that is bad.

@phiw13
Copy link

phiw13 commented Dec 15, 2023

On those prefs radios, doesn't the label just state "yes"/"no" - not the overarching summary of what the preference is?

You are right, the labels actually don’t say much about the actual question for each field. Ignore me. t

The aria-describedby would indeed help.

@philwareham
Copy link
Member

OK, I'll attempt to action this later today.

@philwareham
Copy link
Member

philwareham commented Dec 15, 2023

@Bloke the advanced options on/off combo seems to have an erroneous extra <label> tag within (could be all on/off prefs but that is the only one I can see). The yes/no combos are correct (without that extra <label>). I've looked at the code but it's too complex for me to understand. Can you please fix?

Example HTML, where the <div class="txp-form-field-label"> on second block has the unwanted <label> tag within it:

<div class="txp-form-field" id="prefs-enable_xmlrpc_server">
    <div class="txp-form-field-label">Enable XML-RPC server?&nbsp;<a rel="help" title="Help" role="button" class="pophelp" href="#"><span class="ui-icon ui-icon-help">Help</span></a></div>
    <div class="txp-form-field-value">
        <input class="radio active" id="enable_xmlrpc_server-0" name="enable_xmlrpc_server" type="radio" checked="checked" value="0">
        <label for="enable_xmlrpc_server-0">No</label>
        <input class="radio" id="enable_xmlrpc_server-1" name="enable_xmlrpc_server" type="radio" value="1">
        <label for="enable_xmlrpc_server-1">Yes</label>
    </div>
</div>

<div class="txp-form-field" id="prefs-advanced_options">
    <div class="txp-form-field-label">
        <label for="advanced_options">Advanced options&nbsp;<a rel="help" title="Help" role="button" class="pophelp" href="#"><span class="ui-icon ui-icon-help">Help</span></a></label>
    </div>
    <div class="txp-form-field-value">
        <input class="radio active" id="advanced_options-0" name="advanced_options" type="radio" checked="checked" value="0">
        <label for="advanced_options-0">Off</label>
        <input class="radio" id="advanced_options-1" name="advanced_options" type="radio" value="1">
        <label for="advanced_options-1">On</label>
    </div>
</div>

@Bloke
Copy link
Member

Bloke commented Dec 15, 2023

Hmmmmm. I must have messed up. I'll try and investigate tonight

@Bloke
Copy link
Member

Bloke commented Dec 15, 2023

Spurious. It may be because the advanced_email pref goes through the UI library, whereas things like xmlrpc_server uses the old calls.

I think there's a reason for having the label inside the div, but I'd have to comb the commits to find out what it is. Unless @phiw13 can remember offhand?

@phiw13
Copy link

phiw13 commented Dec 16, 2023

I think there's a reason for having the label inside the div, but I'd have to comb the commits to find out what it is.

We had some discussion at the time you did the rebuilding of the plugin edit panel, where those labels were already inserted (UI Library?). I still see absolutely no need for that construction.

Not sure where that discussion was, in the context of a commit maybe? Or was it in the forum ? my GH is never good, and a quick search on the forum turns out nothing ATM.

@phiw13
Copy link

phiw13 commented Dec 16, 2023

I found the previous discussion, in this forum thread.

@Bloke
Copy link
Member

Bloke commented Jan 21, 2024

Turns out we weren't telling the prefs panel to remove the automatic labelling on RadioSet-based and CheckboxSet-based elements. The patch above (eaa4cdc) rectifies that in a roundabout way by looking for events of those types explicitly and blanking out the label before the UI control is called.

It does, however, smell hacky to me. Part of me thinks that the UI library itself should just not output label tags for those combinatorial elements. Is that something you think we should do? So by default it's not possible to output a label tag around the label text if you make any of our RadioSet or CheckboxSetUI elements.

Put another way: are there any situations where rendering a <label> for a connected group of elements makes sense, to allow you to click the label text and select, well, what exactly? Given that the group itself doesn't have an id handle, only the individual radios or checkboxes in the set have them.

P.S. it would still be possible to add a label to such a control set, but you'd have to construct it yourself by building a set of radio buttons and a label tag. You wouldn't be able to do it from the convenience wrappers.

EDIT: No, hang on. I've just analysed it further. Scratch the above. The UI controls don't render a label by default. They only render individual labels for each radio/checkbox element in the set.

What does create the label is calling inputLabel() which needs to be told not to render the tag for certain elements passed into it. So that begs the question: now that inputLabel() is effectively deprecated - even though we still use it in core during the transition phase to complete UI library usage - should our InputLabel class check the type of stuff it's been given to render and not output a label tag for combinatorial elements? Or is that too presumptuous?

@philwareham
Copy link
Member

That seems fine to presume to me - the combination children themselves have the labels on them. The summary title for that pref should not have a label. So, the example that was mentioned few posts above (noting the use of an aria-describedby attribute and corresponding id:

<div class="txp-form-field" id="prefs-enable_dev_preview">
    <div class="txp-form-field-label" id="prefs-enable_dev_preview-label">Enable development theme preview</div>
    <div class="txp-form-field-value">
        <input class="radio" id="enable_dev_preview-0" aria-describedby="prefs-enable_dev_preview-label" name="enable_dev_preview" type="radio" value="0">
        <label for="enable_dev_preview-0">No</label>
        <input class="radio active" id="enable_dev_preview-1" aria-describedby="prefs-enable_dev_preview-label" name="enable_dev_preview" type="radio" checked="checked" value="1">
        <label for="enable_dev_preview-1">Yes</label>
    </div>
</div>

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

No branches or pull requests

4 participants