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] Switcher Field #24463

Merged
merged 24 commits into from
May 4, 2019
Merged

[4.0] Switcher Field #24463

merged 24 commits into from
May 4, 2019

Conversation

brianteeman
Copy link
Contributor

@brianteeman brianteeman commented Apr 3, 2019

Pull Request for Issue #20986

Summary of Changes

replace the custom element for the switcher field with pure css and accessible
it uses a legend to describe the fieldset and the label for the values as normal

Testing Instructions

Requires npm i

  • Component options
  • Module such as breadcrumbs
  • featured switch on article edit

Expected result

Visual

bread

Notes

  1. The scss file isnt really scss - needs an expert eye to rewrite it
  2. No RTL support - hopefully whoever fixes 1. will do that

@ghost
Copy link

ghost commented Apr 3, 2019

I have tested this item ✅ successfully on 6f96df0 using Module.

System information

Setting Value
PHP Built On Linux lamp122.cloudaccess.net 3.10.0-962.3.2.lve1.5.24.7.el6h.x86_64 nr.1 SMP Mon Dec 17 12:02:35 EST 2018 x86_64
Database Type mysql
Database Version 5.7.24-cll-lve
Database Collation utf8_general_ci
Database Connection Collation utf8_general_ci
PHP Version 7.1.26
Web Server Apache
WebServer to PHP Interface cgi-fcgi
Joomla! Version Latest Nightly Build Update from Joomla! 4.0.0-alpha8-dev Development [ Amani ] 9-March-2019 13:41 GMT
User Agent Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:66.0) Gecko/20100101 Firefox/66.0

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

@Bakual
Copy link
Contributor

Bakual commented Apr 3, 2019

As for the final location for the CSS file, I'd put it into https://github.com/joomla/joomla-cms/tree/4.0-dev/build/media_source/system/css/fields which ends up in /media/system/css/fields after building.
This way it can be overriden by the template if they choose so.

@brianteeman
Copy link
Contributor Author

@Bakual thanks that is what I was thinking too OR as scss to be compiled into the templates

@zwiastunsw
Copy link
Contributor

zwiastunsw commented Apr 4, 2019

  1. Keyboard support OK
  2. This is incomprehensible to me. In the context of the tested page on Joomla Backend, the screen reader (NVDA inc Chrome)
  • does not announce a switch label. Announces only states (Show, Hide),
  • treats all buttons as if it were one group of buttons and announces their status, e.g.

Show radio button checked 1 of 8
Hide radio button checked 4 od 8

instead:

Show radio button checked 1 of 2
Hide radio button checked 2 od 2

When I copy the source code of a page and read it outside the context of the backend (as a separate document), the screen reader correctly reads the label of each switch and its status. (Do not load any js then.)

Firefox test: switches do not receive keyboard focus at all
Can anyone confirm this?

@brianteeman
Copy link
Contributor Author

Which exact page. Maybe there is a markup bug somewhere

@brianteeman
Copy link
Contributor Author

I "think" I know what the problem is. It's not the code here. But could you please tell me the page you tested so I can confirm

@brianteeman
Copy link
Contributor Author

@zwiastunsw I think it could be because the fieldsets are nested inside other fieldsets https://test-cases.tink.uk/nested-fieldsets/index.html

@brianteeman
Copy link
Contributor Author

@zwiastunsw if that is the problem then we can use an alternative method instead of fieldset which is to use role=group

@zwiastunsw
Copy link
Contributor

I tested Site Modules > Breadcrumbs.
Windows 10, Chrome v. 73.0.3683.86 (64 bit).

@ghost ghost added J4 Issue and removed J4 Issue labels Apr 4, 2019
@brianteeman
Copy link
Contributor Author

@zwiastunsw I am going to have to reach out to some external a11y experts on this. As you discovered the code works perfectly on a plain html page but not within joomla. I cant see anything in joomla that would break it.

@ghost ghost added a11y Accessibility and removed J4 Issue labels Apr 13, 2019
@ghost ghost changed the title [4.0] [a11y] Switcher Field Proof of Concept [4.0] Switcher Field Proof of Concept Apr 19, 2019
@wilsonge wilsonge added this to In progress in [4.0] Administration via automation Apr 23, 2019
@brianteeman
Copy link
Contributor Author

@zwiastunsw @wilsonge I found the problem. It is NOT in the code here. The problem is in the tabs code. When you open a module for the first time ALL the sections are set with aria-hidden=true. The first tab should not be

image

@joomla-cms-bot joomla-cms-bot added the NPM Resource Changed This Pull Request can't be tested by Patchtester label Apr 29, 2019
@brianteeman brianteeman changed the title [4.0] Switcher Field Proof of Concept [4.0] Switcher Field [a11y] Apr 29, 2019
@brianteeman
Copy link
Contributor Author

Updated code and original post with test instructions

@brianteeman
Copy link
Contributor Author

The js tests are failing as expected as the js is no longer present. Should I remove the tests in this pr?

@ghost ghost changed the title [4.0] Switcher Field [a11y] [4.0] Switcher Field Apr 30, 2019
@brianteeman
Copy link
Contributor Author

tests removed

@wilsonge wilsonge merged commit 8507bfa into joomla:4.0-dev May 4, 2019
[4.0] Administration automation moved this from In progress to Done May 4, 2019
@wilsonge
Copy link
Contributor

wilsonge commented May 4, 2019

Thanks!

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

Woohoo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a11y Accessibility NPM Resource Changed This Pull Request can't be tested by Patchtester Unit/System Tests
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants