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] Modal select fields #24775

Merged
merged 3 commits into from
May 4, 2019
Merged

[4.0] Modal select fields #24775

merged 3 commits into from
May 4, 2019

Conversation

brianteeman
Copy link
Contributor

@brianteeman brianteeman commented May 2, 2019

The following fields were identified by @zwiastunsw as not having labels associated with the element

There were TWO different problems that this PR fixes

  1. the label was being associated with the hidden input and not the visible input
  2. The visible input was set to disabled - it should have been readonly because

When applied to a form field, the disabled attribute means that the field does not receive focus. resulting in the screen reader ignoring the field and not announcing the value of the field

Basically the fields that look like
image

Menus: Edit Item page, Single Article menu item type
Details tab: Select Article field

Menus: Edit Item page, Category blog menu item type
Details tab: Choose a Category field

Menus: Edit Item page, Single Contact menu item type
Details tab: Select Contact field

Menus: Edit Item page, Contact Single Category menu item type
Details tab: Select a Category field

Menus: Edit Item page, Single News Feed menu item type
Details tab: Feed field

@brianteeman brianteeman changed the title Disabled [4.0] Modal select fields [a11y] May 2, 2019
@brianteeman brianteeman marked this pull request as ready for review May 2, 2019 18:46
…eld.php


oops

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

wilsonge commented May 2, 2019

This is more complicated. There’s a technical difference too. Disabled means the field isn’t submitting. So the readonly attribute means it is submitting which is undesired. We only want the hidden field to be submitting the data

@brianteeman
Copy link
Contributor Author

Disabled also means the field should be skipped in the tab order which is why the screen reader never sees it. It might be submitted but what is it submitting? It can only be submitting exactly the same as the hidden field. As both fields have different id then surely we are already only storing the submitted values of the hidden field???

Have to ask though why we have a hidden field in the first place?

@ghost ghost added the a11y Accessibility label May 3, 2019
@ghost ghost changed the title [4.0] Modal select fields [a11y] [4.0] Modal select fields May 3, 2019
@SharkyKZ
Copy link
Contributor

SharkyKZ commented May 3, 2019

Hidden field holds the value/ID that is sent to the server. Text field just displays the name of selected item to the user.

@wilsonge True, but that shouldn't be an issue if input has no name attribute?

@wilsonge
Copy link
Contributor

wilsonge commented May 4, 2019

I can't find any docs on that either way. The HTML5 spec says that name is optional but doesn't tell browsers how to handle it when that is missing. I think practically speaking it's true it won't be submitted when it's missing but it's definitely not a given either.

As for why it's hidden it's as @SharkyKZ says. The value being submitted isn't the name of the article it's the id. The disabled field is sending the title of the newsfeed/article etc

@SniperSister
Copy link
Contributor

As discussed with @wilsonge: securitywise I have no concerns with the PR

@wilsonge
Copy link
Contributor

wilsonge commented May 4, 2019

Good to continue then @brianteeman

@brianteeman
Copy link
Contributor Author

After that small diversion its all good to go then. Just needs tests. Nothing else from me

@sanderpotjer
Copy link
Member

I have tested this item ✅ successfully on df3755c

There are some JS formvalidator issues when creating a new article via the Modal select fields, but that does not seem to be related to the changes of this PR.


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

@euismod2336
Copy link
Contributor

I have tested this item ✅ successfully on df3755c


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

@wilsonge wilsonge merged commit d97fbea into joomla:4.0-dev May 4, 2019
@wilsonge
Copy link
Contributor

wilsonge commented May 4, 2019

Thanks guys!

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

Thanks

@brianteeman brianteeman deleted the disabled branch May 4, 2019 12:09
@infograf768
Copy link
Member

This has changed drastically the id of the label, using _name instead of _id and therefore breaking some js as associations-edit.js
It took me some hours to find out what was broken and I can solve the js easily with a PR but I wonder if this is B/C.

Could someone explain in simple terms why this specific change ?
For example I get:
<label id="jform_associations_de_DE_name-lbl"
instead of
<label id="jform_associations_de_DE_id-lbl"

If it IS absolutely necessary then we should have some documentation about it.

@brianteeman
Copy link
Contributor Author

Before the markup was incorrect as it said label for= something that didnt exist
image

Now the markup is correct and correctly associates the label with the input
image

@infograf768
Copy link
Member

See #25060
I still think that this change should be documented.

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

Successfully merging this pull request may close these issues.

None yet

9 participants