? Pending

User tests: Successful: Unsuccessful:

avatar dgrammatiko
dgrammatiko
23 Jul 2018

Pull Request for Issue joomla/40-backend-template#395 but without transforming it to web component

Summary of Changes

  • properly initialize the element only once
  • add/remove listeners correctly
  • various improvements
  • a11y redone

Testing Instructions

Edit an article,
Resize the window multiple times
Make sure that there is only one switcher for the field Featured
Make sure that the data is stored correctly
Audit for a11y

Expected result

On window resize no artefacts

Actual result

Multiple switchers on the page

Documentation Changes Required

avatar dgrammatiko dgrammatiko - open - 23 Jul 2018
avatar dgrammatiko dgrammatiko - change - 23 Jul 2018
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 23 Jul 2018
Category JavaScript Repository
avatar brianteeman
brianteeman - comment - 23 Jul 2018

Still no better. Is it working like a checkbox or a radio? If it's a checkbox then normally to make a label etc you would put the fields in a fieldset and set the label as the legend. That way you get the label and the value etc announced

avatar dgrammatiko
dgrammatiko - comment - 23 Jul 2018

Hmmm, that was also Stefan's idea but can a fieldset have a nested fieldset? (eg the switcher is included inside a form fieldset, quite common for J)
By the way it's a radio

avatar brianteeman
brianteeman - comment - 23 Jul 2018

Yes it can. I double checked that as we already have some nested fieldset. The only issue with nested fieldset is if you have some orphaned fields but that wouldn't be the case here afaict

avatar dgrammatiko dgrammatiko - change - 23 Jul 2018
Labels Added: ?
avatar dgrammatiko
dgrammatiko - comment - 23 Jul 2018

@brianteeman let me know if this is any better

avatar brianteeman
brianteeman - comment - 23 Jul 2018
avatar brianteeman
brianteeman - comment - 23 Jul 2018

Your code is no better

avatar dgrammatiko
dgrammatiko - comment - 23 Jul 2018

Ok, so what exactly fails here?

avatar brianteeman
brianteeman - comment - 23 Jul 2018

no improvement on the label
the toggle cant be switched by the keyboard (until you have done it once with the mouse

Please install nvda!!

avatar dgrammatiko
dgrammatiko - comment - 23 Jul 2018

nvda on a mac? Nah

  • Label fixed
  • toggle should be ok for keyboard as well now
avatar dgrammatiko
dgrammatiko - comment - 23 Jul 2018

@brianteeman let's make this easier:
There are 2 states, so can you provide the accessible html markup for those, then I can do the needed js

avatar brianteeman
brianteeman - comment - 23 Jul 2018

sorry but i am not wasting my time on this anymore. its pointless you changing something and then me testing it. its a waste of both our times. just use nvda or voiceover and when you have it working then ask me or the a11y team to confirm.

right now i am seeing custom elements as a massive waste of time with zero benefits

avatar dgrammatiko
dgrammatiko - comment - 23 Jul 2018

Wow collaborating towards solving a problem is waste of time. Also expecting someone else to do all the research and implementation and then just come and say, "it works" will also work very nice for me. I am not an a11y expert, this isn't some code I introduced and quite frankly if you think you have a better solution as you're implying, let me close this and wait for your PR.

avatar brianteeman
brianteeman - comment - 23 Jul 2018

I am not an a11y expert either I just took the time to install nvda and use it

To be honest right now you are the main one who is pushing for custom elements everywhere. We can have an accessible interface without them

avatar dgrammatiko
dgrammatiko - comment - 23 Jul 2018

We can have an accessible interface without them

With the cost that:

  • fields will need to be altered to work with repeatable, so it's not so straightforward the usage of "off the shelf" solutions
  • fields will still need to be unified with the css framework
  • using plain html elements (divs etc) means that each element will need some very specific markup

But in particular, for the problem here, can you provide with the a11y markup, I'll do the rest

avatar brianteeman
brianteeman - comment - 23 Jul 2018
avatar fuzzbomb
fuzzbomb - comment - 24 Jul 2018

@brianteeman referred me here, and asked if I would take a look at this.

I can't understand the scope of the issue - it doesn't say what page, or UI component this is about. The issue mentions something called a switcher but I've no idea what that is or where to find it.

avatar brianteeman
brianteeman - comment - 24 Jul 2018

It's ok I will explain it to you over the next beer

avatar brianteeman
brianteeman - comment - 25 Jul 2018

Been doing some reading around and from what i can tell only certain native elements support label so if you want to use a label you have to use aria-labelledby

Can you try this

instead of doing

<label id="jform_show_page_heading-lbl" for="jform_show_page_heading">
	Show Page Heading</label>

followed by

<fieldset class="switcher success" tabindex="0" id="jform_show_page_heading" aria-checked="true" role="switch" aria-label="Hide"><input type="radio" id="jform_show_page_heading0" name="jform[show_page_heading]" value="0" checked="checked" class="active" tabindex="-1"><input type="radio" id="jform_show_page_heading1" name="jform[show_page_heading]" value="1" tabindex="-1"><span class="switch success"></span></fieldset>

Can you try the following

<label id="jform_show_page_heading-lbl">
	Show Page Heading</label>

Followed by

<fieldset class="switcher success" tabindex="0" id="jform_show_page_heading" aria-labelledby="jform_show_page_heading" aria-checked="true" role="switch"><input type="radio" id="jform_show_page_heading0" name="jform[show_page_heading]" value="0" checked="checked" class="active" tabindex="-1"><input type="radio" id="jform_show_page_heading1" name="jform[show_page_heading]" value="1" tabindex="-1"><span class="switch success"></span></fieldset>
avatar brianteeman
brianteeman - comment - 25 Jul 2018

@fuzzbomb in the administrator of joomla on most options pages you can find a switcher - usually a yes/no or show/hide

avatar brianteeman
brianteeman - comment - 8 Aug 2018

@dgrammatiko could you have a go at replicating the structure I posted above please. If thats correct then it will apply to all web component fields.

avatar dgrammatiko
dgrammatiko - comment - 8 Aug 2018

sure

avatar joomla-cms-bot joomla-cms-bot - change - 8 Aug 2018
Category JavaScript Repository JavaScript Repository Layout Libraries
avatar dgrammatiko
dgrammatiko - comment - 8 Aug 2018

@brianteeman let me know if this is any better

avatar brianteeman
brianteeman - comment - 8 Aug 2018

A job for tomorrow

avatar brianteeman
brianteeman - comment - 9 Aug 2018

@dgrammatiko I think this is getting close - although not matching my suggestion ;)

this.inputsContainer.setAttribute('aria-label',${this.id}-lbl); //this.spans[1].innerHTML);
In my tests this code produces
aria-label="-lbl" sometimes ie working bu not finding the id

but other times doesnt work at all.

See this output where it kind of worked on the second but not the first

<div class="control-group">
	<div class="control-label">
	</div>
	<div class="controls">
		<label id="jform_sef_ids-lbl">Remove IDs from URLs</label>
		<joomla-field-switcher off-text="No" on-text="Yes" type="success">
			<fieldset class="switcher success has-success" tabindex="0" id="jform_sef_ids" aria-checked="false" role="switch" aria-label=" No">
				<input type="radio" id="jform_sef_ids0" name="jform[sef_ids]" value="0" class="valid active form-control-success" tabindex="-1" aria-invalid="false" checked="">
				<input type="radio" id="jform_sef_ids1" name="jform[sef_ids]" value="1" tabindex="-1" class="valid form-control-success" aria-invalid="false">
				<span class="switch success"></span>
			</fieldset>
			<span class="switcher-labels">
				<span class="switcher-label-0 active">No</span>
				<span class="switcher-label-1">Yes</span>
			</span>
		</joomla-field-switcher>
	</div>
</div>

<div class="control-group">
	<div class="control-label">
	</div>
	<div class="controls">
		<label id="jform_custom_fields_enable-lbl">Enable Custom Fields</label>
		<joomla-field-switcher off-text="No" on-text="Yes" type="success">
			<fieldset class="switcher success active" tabindex="0" id="jform_custom_fields_enable" aria-checked="true" role="switch" aria-label="-lbl">
				<input type="radio" id="jform_custom_fields_enable0" name="jform[custom_fields_enable]" value="0" tabindex="-1">
				<input type="radio" id="jform_custom_fields_enable1" name="jform[custom_fields_enable]" value="1" checked="checked" class="active" tabindex="-1">
				<span class="switch success"></span>
			</fieldset>
			<span class="switcher-labels">
				<span class="switcher-label-0">No</span>
				<span class="switcher-label-1 active">Yes</span>
			</span>
		</joomla-field-switcher>
	</div>
</div>
avatar dgrammatiko
dgrammatiko - comment - 9 Aug 2018

@brianteeman that was my bad, late night code I guess. Let me know if this is ok now

avatar brianteeman
brianteeman - comment - 9 Aug 2018

Got it - one small change
Your code is now producing this
<fieldset class="switcher success active has-success" tabindex="0" id="jform_show_title" aria-checked="true" role="switch" aria-label="jform_show_title-lbl"></fieldset>

If you change aria-label to aria-labelledby then it will work - which is what I wrote here #21227 (comment) :)

avatar dgrammatiko
dgrammatiko - comment - 9 Aug 2018

@brianteeman sorry I need to read more carefully I guess. Should be fine now (btw voice over was picking it correctly, I guess it's much more advanced than the nvda)

avatar rdeutz
rdeutz - comment - 10 Aug 2018

The javascript test needs a fix before merging

avatar brianteeman
brianteeman - comment - 10 Aug 2018

The screenreader is now correctly announcing the label of the button but it not reading out the options. I am sure someone on the a11y team will be better than me in suggesting the correct markup - sorry

avatar dgrammatiko
dgrammatiko - comment - 10 Aug 2018

@rdeutz PR: joomla/test-javascript#9

@wilsonge please merge joomla/test-javascript#9, update the base branch here and wait for tests pass before merging this one

avatar dgrammatiko
dgrammatiko - comment - 10 Aug 2018

I'm genuinely surprised that this actually worked ?

avatar wilsonge wilsonge - change - 10 Aug 2018
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2018-08-10 15:01:54
Closed_By wilsonge
avatar wilsonge wilsonge - close - 10 Aug 2018
avatar wilsonge wilsonge - merge - 10 Aug 2018
avatar wilsonge
wilsonge - comment - 10 Aug 2018

Well you did good ;)

avatar mbabker
mbabker - comment - 10 Aug 2018

I'm genuinely surprised that this actually worked ?

Our reaction to many of your pull requests ?

avatar brianteeman
brianteeman - comment - 10 Aug 2018

Why was this merged - it is not working correctly #21227 (comment)

avatar dgrammatiko
dgrammatiko - comment - 10 Aug 2018

@brianteeman, first of all, thank you for your help here. Yes, the PR didn't fix all the a11y problems but we need the fix for the wrong initialization. There will be more PRs for the remaining a11y issues (announcing the options if I got that right). Let's not wait till everything is ironed, small steps are easier

avatar brianteeman
brianteeman - comment - 10 Aug 2018

ok - I think I was just surprised and also wanted to be certain you had not misunderstood me and assumed it was completed

avatar brianteeman
brianteeman - comment - 10 Aug 2018

The good think is that we now know for the custom elements to work we need to add aria-labelled by to the field instead of adding aria-to the label

Add a Comment

Login with GitHub to post a comment