vaadin / web-components

A set of high-quality standards based web components for enterprise web applications. Part of Vaadin 20+
https://vaadin.com/docs/latest/components
464 stars 83 forks source link

[radio-button] Group should ignore keydown and click events handled by components in radio-buttons #1112

Open NickdeK opened 6 years ago

NickdeK commented 6 years ago

When you have a vaadin-radio-group with multiple radio buttons that show aditional fields below the selected radio button the arrow navigation breaks cursor navigation in (for example) vaadin-text-field.

Example (not working):

<vaadin-radio-group value="{{selectedRadio}}" theme="vertical">
  <vaadin-radio-button>Radio 1</vaadin-radio-button>
  <template is="dom-if" if="{{selectedRadio}}">
    <vaadin-text-field></vaadin-text-field>
    <vaadin-text-field></vaadin-text-field>
  </template>
  <vaadin-radio-button>Radio 2</vaadin-radio-button>
</vaadin-radio-group>

Above example shows what we are trying to accomplish. When a user selects radio 1 a few extra text-fields show. The users sets it's cursor in the text-field, but when the user uses the arrow keys on it's keyboard the selected radio changes and the fields disappear.

This listener events should be edited to only do something when the key event comes from the radio button. The key events bubble up from the text field and get's fired on the radio-group. https://github.com/vaadin/vaadin-radio-button/blob/master/src/vaadin-radio-group.html#L131

NickdeK commented 6 years ago

On further debugging I found out that this occurs if you place the vaadin-radio-group in the shadow dom. e.target in this case returns the root element that is placed in the light-dom.

This breaks the navigation on radio buttons with the keyboard in shadow dom, not only in the original case with text fields. The navigation doesn't work because it selectes the next or previous radio button based on the event.target here: https://github.com/vaadin/vaadin-radio-button/blob/master/src/vaadin-radio-group.html#L133

I'm not sure how to solve this. You might think about using event.path or event.composed; both contain a list of elements that it bubbles trough, however both are not supported and in this case it always bubbles trough vaadin-radio-group so it might not fix everything...

For now I've added a option to disable keyboard navigation on the radio-group to prevent the problem from happening. I know it's not great, but filling in text fields and not being able to use your arrow keys is worse. See the PR here: https://github.com/vaadin/vaadin-radio-button/pull/57

web-padawan commented 5 years ago

One way could be adding a check in _addEctiveListeners e.g. like this one:

// if keydown is not on a vaadin-radio-button element, skip it
if (!this._radioButtons.some(radio => target === radio || radio.shadowRoot.contains(target))) {
  return;
}

Please not that at this point we don't have any recommendations on pleacing e.g. vaadin-text-field inside of vaadin-radio-group as that might make keyboard navigation even more complex.

This needs investigation and in general seems like a feature request rather than a bug fix to me.

web-padawan commented 3 years ago

One possible solution would be to use event.stopPropagation in all the interactive components placed in the group.

web-padawan commented 3 years ago

This is still an issue in Vaadin 22, for example the following group will work weirdly:

<vaadin-radio-group label="Job title">
  <vaadin-radio-button value="junior" label="Junior"></vaadin-radio-button>
  <vaadin-radio-button value="middle" label="Middle"></vaadin-radio-button>
  <vaadin-radio-button value="senior" label="Senior"></vaadin-radio-button>
  <vaadin-radio-button value="custom">
    <label slot="label">
      <span>Write your own:</span>
      <vaadin-text-field placeholder="Specialist"></vaadin-text-field>
    </label>
  </vaadin-radio-button>
</vaadin-radio-group>
anm-cb commented 2 years ago

https://github.com/vaadin/flow-components/issues/1697

Probably have to make our own RadioButtonGroup ;)

TatuLund commented 1 year ago

This is not the bug, see also: https://github.com/vaadin/flow-components/issues/1697#issuecomment-847111472

Also referring to guidelines of label tag usage here https://developer.mozilla.org/en-US/docs/Web/HTML/Element/label