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

Add switch example using HTML checkbox input #1895

Merged
merged 64 commits into from
Nov 10, 2021
Merged

Add switch example using HTML checkbox input #1895

merged 64 commits into from
Nov 10, 2021

Conversation

jongund
Copy link
Contributor

@jongund jongund commented May 7, 2021

@jongund
Copy link
Contributor Author

jongund commented May 7, 2021

@mcking65 @jnurthen
If you are using an input[type=checkbox] with the switch role, do you need to use aria-checked?
The W3C validator is requiring aria-checked.

@jnurthen
Copy link
Member

jnurthen commented May 7, 2021

https://www.w3.org/TR/html-aria/#el-input-checkbox states NOTE
The HTML checked attribute can be used instead of the aria-checked attribute for menuitemcheckbox, option or switch roles when used on type=checkbox.

The validator is wrong

See validator/validator#1122

@jongund
Copy link
Contributor Author

jongund commented May 7, 2021

@jnurthen
Thank you for the quick response, does this issue have any implication for the ARIA spec requiring the use of aria-checked for switch?

@jnurthen
Copy link
Member

jnurthen commented May 7, 2021

No - see the note at https://w3c.github.io/aria/#requiredState

@jongund
Copy link
Contributor Author

jongund commented May 13, 2021

@mcking65
For some reason this branch is failing the menu-button link test for the ENTER key, even though none of the menu button example files were touched in this pull request.

@shirsha
Copy link

shirsha commented May 25, 2021

I would suggest not to use CSS generated content as it is not supported screen readers on older versions, it is difficult to localize, difficult to change style by the user.

Suggest to have _for_ attribute for to match with _id_ attributes for input field even though we are implicit labelling method to increase the screenreader support.

@carmacleod
Copy link
Contributor

Functional review:

Chrome, Firefox, and Safari:

Safari:

  • If I click and hold the switch, the label text turns white. (Safari only)

@shirsha
Copy link

shirsha commented May 25, 2021

If you use user-select: none; then VO/safari ignores the control.
https://bugs.webkit.org/show_bug.cgi?id=82692

@jongund
Copy link
Contributor Author

jongund commented May 27, 2021

@mcking65
Made updates to:

  1. Move on/off labels from CSS to content, may need aria-hidden.
  2. Added no selection, so text is selected when activating with a mouse.

Copy link
Member

@zcorpan zcorpan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review: 1 comment, otherwise LGTM
Test review: LGTM

examples/switch/switch-checkbox.html Outdated Show resolved Hide resolved
@mcking65 mcking65 changed the title Switch Role: Initial switch example using input[type=checkbox] Add switch example using HTML checkbox input Oct 29, 2021
@mcking65
Copy link
Contributor

mcking65 commented Nov 1, 2021

@zcorpan, @howard-e, this has changed a fair amount since last review of code and test. Could one of you please have another look?

CC @richnoah

@zcorpan
Copy link
Member

zcorpan commented Nov 2, 2021

Run regression tests CI check failed for menubar_menubar-navigation, though that test was not changed in this PR. Maybe the test is flaky. I'll try rerunning.

Edit: now it's green.

Log output
menubar_menubar-navigation › after.always hook
not ok 84 - StaleElementReferenceError: The element reference of <a href="#academics"> is stale; either the element is no longer attached to the DOM, it is not in the current frame context, or the document has been refreshed
  ---
    remoteStacktrace: >
      WebDriverError@chrome://remote/content/shared/webdriver/Errors.jsm:181:5

      StaleElementReferenceError@chrome://remote/content/shared/webdriver/Errors.jsm:442:5

      element.resolveElement@chrome://remote/content/marionette/element.js:686:11

      evaluate.fromJSON@chrome://remote/content/marionette/evaluate.js:253:26

      evaluate.fromJSON@chrome://remote/content/marionette/evaluate.js:261:29

      receiveMessage@chrome://remote/content/marionette/actors/MarionetteCommandsChild.jsm:79:29
    name: StaleElementReferenceError
    message: >-
      The element reference of <a href="#academics"> is stale; either the element is
      no longer attached to the DOM, it is not in the current frame context, or the
      document has been refreshed
    at: >-
      Object.throwDecodedError (node_modules/selenium-webdriver/lib/error.js:517:15)

      parseHttpResponse (node_modules/selenium-webdriver/lib/http.js:671:13)

      Executor.execute (node_modules/selenium-webdriver/lib/http.js:597:28)

      thenableWebDriverProxy.execute
      (node_modules/selenium-webdriver/lib/webdriver.js:729:17)
  ...

examples/switch/switch-checkbox.html Outdated Show resolved Hide resolved
examples/switch/switch-checkbox.html Outdated Show resolved Hide resolved
examples/switch/switch-checkbox.html Outdated Show resolved Hide resolved
examples/switch/switch-checkbox.html Outdated Show resolved Hide resolved
Comment on lines 33 to 36
-webkit-user-select: none;
-moz-user-select: none;
-ms-user-select: none;
user-select: none;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't follow https://github.com/w3c/aria-practices/wiki/Code-Guide#prefixed-properties

Should we remove that section of the code guide?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zcorpan based on our Nov 2 discussion,I think we still need this for webkit. I think @jongund is looking into whether the others can be removed from this example without issue. Do we need a separate issue for conversation about what to do for prefix guidance in the code guide?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can track it in #2110

@zcorpan
Copy link
Member

zcorpan commented Nov 2, 2021

Test review: LGTM

mcking65 and others added 4 commits November 7, 2021 14:46
Co-authored-by: Simon Pieters <zcorpan@gmail.com>
Co-authored-by: Simon Pieters <zcorpan@gmail.com>
Co-authored-by: Simon Pieters <zcorpan@gmail.com>
Co-authored-by: Simon Pieters <zcorpan@gmail.com>
@mcking65
Copy link
Contributor

mcking65 commented Nov 7, 2021

@jongund, If I recall correctly, you were going to look into removeing browser prefixes from the css if possible? I think we found out it is necessary for webkit but may not the others? Not sure if I am remembering this correctly.

@jongund
Copy link
Contributor Author

jongund commented Nov 8, 2021

@mcking65 Updated CSS to remove -moz-user-select and -ms-user-select properties.

@richnoah richnoah requested a review from zcorpan November 9, 2021 15:44
@mcking65
Copy link
Contributor

@zcorpan
I think we have resolved all your feedback.
If you agree, pls change your to approve status.

@mcking65 mcking65 removed the request for review from jesdaigle November 10, 2021 05:24
@zcorpan
Copy link
Member

zcorpan commented Nov 10, 2021

Code review: LGTM

@howard-e howard-e merged commit fcaaba2 into main Nov 10, 2021
@howard-e howard-e deleted the switch-checkbox branch November 10, 2021 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Develop example of switch design pattern
8 participants