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

Ratcheting #124

Merged
merged 4 commits into from
Oct 9, 2017
Merged

Ratcheting #124

merged 4 commits into from
Oct 9, 2017

Conversation

robdodson
Copy link
Contributor

@surma FYI

Just tidying up some stuff. I did a little trick to make howto-label available in demos.

@slauriat I think this should address #123, can you verify I'm using fieldset correctly here:
https://github.com/GoogleChromeLabs/howto-components/compare/ratcheting?expand=1#diff-44055033e9c75ba1de2117d595729201R13

@robdodson
Copy link
Contributor Author

robdodson commented Oct 7, 2017

Oh I should also mention, I'm moving away from directly styling ARIA states, and instead treating ARIA as an implementation detail. Since setting .checked or [checked] on a howto-checkbox will also set aria-checked as a side effect, we can make it so the consumer never needs to know that ARIA is operating under the hood. Ultimately this will be good when something like AOM lands because Custom Elements will no longer sprout ARIA attributes. Instead they'll do: this.acessibleNode.checked = true.

- No longer style against ARIA states. ARIA is an implementation detail.
- Change keyboard handers to use keyup to prevent rapid toggling while holding the key down.
- Update checkbox README to mention that tabindex is removed when the element is disabled.
- Add howto-label to howto-checkbox demo
Copy link
Contributor

@surma surma left a comment

Choose a reason for hiding this comment

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

One minor Q, but LGTM :)

@@ -106,7 +106,7 @@
this._upgradeProperty('checked');
this._upgradeProperty('disabled');

this.addEventListener('keydown', this._onKeyDown);
this.addEventListener('keyup', this._onKeyUp);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this intentional? I mean, for checkbox it makes sense to not rapidly change the .checked state when holding space, but for example in howto-menu should holding “arrow down” not cycle through the list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah that's why I limited it to just this component. I looked at what <input type=checkbox> does here and it doesn't rapidly toggle when you hold space. It does show an active state which is something we could add as an extra layer of polish someday.

Copy link

@nathanhammond nathanhammond Oct 10, 2017

Choose a reason for hiding this comment

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

It is also possible to trigger the down state by pressing on the associated label element so that a user can visually confirm the connection between label and checkbox. That is why that state exists and isn't something that I consider to be polish. I'd love for this to actually be tackled since this library is intended to be a reference point.

A few additional notes while I'm here:

  • It should disable dragging on the custom element.
  • Global mouseup and (possibly) keyup listeners are required to reset the down state in case they don't occur on the same element.
  • To get identical reading output to native and work around reading checkbox state backward in Chrome you'll need to use aria-labelledby to relate checkbox to label. Noticed that this is handled by the label element.
  • Form submissions can be made to work by mirroring to a display: none input type=checkbox.
  • Nested usage is a thing and is an especially good pattern for braille keyboards: <label><checkbox> blah blah</label>. Also, should work without having to specify identifiers.

/me disappears to my hole again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is also possible to trigger the down state by pressing on the associated label element so that a user can visually confirm the connection between label and checkbox. That is why that state exists and isn't something that I consider to be polish. I'd love for this to actually be tackled since this library is intended to be a reference point.

Interesting point about the label. The howto-label that was added in this PR does allow you to click on the label to toggle the control but neither support an :active state. I'll have to look into that as a future addition.

It should disable dragging on the custom element.

Sorry for the dumb question, but I'm not sure I understand what you mean? Which CSS property are you referring to here? I don't think we allow any dragging but I'm probably missing something :)

Global mouseup and (possibly) keyup listeners are required to reset the down state in case they don't occur on the same element.

I just checked the native <input type=checkbox> and it doesn't toggle state if you mousedown on the checkbox and then mouseup on a different node. It's as if you never clicked on it. That's the behavior we're emulating here. Again, maybe I'm missing something?

Form submissions can be made to work by mirroring to a display: none input type=checkbox.

Yeah we mention this in the docs. I guess this is kind of an editorial decision where we didn't want to pick a specific solution because there's a few different ways folks could skin this cat. At TPAC we're going to see if we can get some folks to discuss an onSubmitCallback. I need to think about this one a bit more... I'm not opposed to injecting a hidden input because others have asked how to make a custom element work in a form. We would just need to call out that there are other ways to solve the problem (like creating a custom form element or submitting the form yourself using AJAX).

Nested usage is a thing and is an especially good pattern for braille keyboards: blah blah.

howto-label supports this.

Also, should work without having to specify identifiers.

The only scenario where you need special identifiers for nested content is if you nest multiple elements within the label. That's mainly to avoid having to hard code a list of supported elements in our custom label and letting it support whatever custom element you want. I guess an alternative approach would be to have it find the first child with something like a .value property and consider that its target?

Choose a reason for hiding this comment

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

It should disable dragging on the custom element.

You can't click and drag a input type=checkbox. You can (in certain browsers, not sure which off the top of my head) drag a div. This is important for users with low motor accuracy. Click and accidentally drag and it shouldn't trigger this behavior. This is simply, ondragstart event.preventDefault();

Global mouseup and (possibly) keyup listeners are required to reset the down state in case they don't occur on the same element.

This comment was regarding the down state only. If you "down" on the element and "up" on another node (or outside of the browser) you need to remove the down state. Attaching a listener to document (not document.body) handles this.

Find the first child with something like a .value property and consider that its target

It's the set of aria roles in the taxonomy for input plus the default form elements.

(Kinda doing this drive by, sorry for terseness.)

@robdodson robdodson merged commit 0a8bf67 into master Oct 9, 2017
@robdodson robdodson deleted the ratcheting branch October 9, 2017 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants