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.

Sign up for GitHub

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

Jump to bottom

fix: do not focus the field on helper element click #2232

Merged
merged 25 commits into from
Jul 19, 2021
Merged

fix: do not focus the field on helper element click #2232

merged 25 commits into from
Jul 19, 2021

Conversation

hdamr
Copy link
Contributor

@hdamr hdamr commented Jul 12, 2021 •
edited
Loading

Description

Some components that have helper-text capability, When their helper-text is set via slot and a component like vaadin-text-field is put inside of them, the moment the user clicks on the input in the helper slot, the focus jumps to the component that sits higher than our input that is in the slot. There is also another problem with vaadin-select, When we have a button as helper text, click on that button causes the vaadin-select input to be opened automatically which is not our intended behaviour.

This PR fixes this issue by removing on-click from various components that have such handler on helper-text element.
It also fixes the issue with vaadin-select when a button is used inside helper, click on the button causes the select element to be opened, it fixes it by preventing default behaviour when helper text is clicked.

Fixes vaadin/flow-components#955

Type of change

  • Bugfix
  • Feature

Checklist

  • I have read the contribution guide: https://vaadin.com/docs-beta/latest/guide/contributing/overview/
  • I have added a description following the guideline.
  • The issue is created in the corresponding repository and I have referenced it.
  • I have added tests to ensure my change is effective and works as intended.
  • New and existing tests are passing locally with my change.
  • I have performed self-review and corrected misspellings.

@hdamr hdamr requested a review from sosa-vaadin July 12, 2021 12:53
web-padawan
web-padawan reviewed Jul 12, 2021
View reviewed changes
Copy link
Member

@web-padawan web-padawan left a comment

Choose a reason for hiding this comment

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

I have checked the other implementations e.g. Material components:

https://material.io/components/text-fields
https://material-ui.com/components/text-fields/#form-props

In those cases, clicking the helper text does not focus the input.
So we probably can do the same and remove click listeners.

@hdamr hdamr requested a review from DiegoCardoso July 13, 2021 08:41
@hdamr hdamr changed the title Fix inconsistent focus 955 fix: inconsistent focus behaviour of nested helper components Jul 13, 2021
@hdamr hdamr marked this pull request as ready for review July 13, 2021 09:16
@sonarcloud
Copy link

sonarcloud bot commented Jul 15, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@web-padawan web-padawan merged commit 4e251b1 into master Jul 19, 2021
@web-padawan web-padawan deleted the fix-inconsistent-focus-955 branch July 19, 2021 07:25
vaadin-bot pushed a commit that referenced this pull request Jul 19, 2021
@hdamr @vaadin-bot
fix: do not focus the field on helper element click (#2232)
bc99672
web-padawan pushed a commit that referenced this pull request Jul 19, 2021
@vaadin-bot @hdamr
fix: do not focus the field on helper element click (#2232) (#2250)
ef7ed56 
Co-authored-by: Hadi Amiri <[email protected]>
@web-padawan web-padawan mentioned this pull request Jul 19, 2021
Merged
1 task
alvarezguille added a commit to vaadin/vaadin-checkbox that referenced this pull request Jul 21, 2021
@alvarezguille
fix: do not focus the field on helper element click
4a00f2b 
cherry pick of vaadin/web-components#2232 for v14
cherry pick of vaadin/web-components#2256 for v14

Fixes vaadin/flow-components#955
alvarezguille added a commit to vaadin/vaadin-custom-field that referenced this pull request Jul 21, 2021
@alvarezguille
fix: do not focus the field on helper element click
640c7ce 
cherry pick of vaadin/web-components#2232 for v14

Fixes vaadin/flow-components#955
alvarezguille added a commit to vaadin/vaadin-radio-button that referenced this pull request Jul 21, 2021
@alvarezguille
fix: do not focus the field on helper element click
b1f78f2 
cherry pick of vaadin/web-components#2232 for v14

add missing test

Fixes vaadin/flow-components#955
alvarezguille added a commit to vaadin/vaadin-select that referenced this pull request Jul 21, 2021
@alvarezguille
fix: do not open the field on helper element click
058c273 
cherry pick of vaadin/web-components#2232 for v14

Fixes vaadin/flow-components#955
alvarezguille added a commit to vaadin/vaadin-text-field that referenced this pull request Jul 21, 2021
@alvarezguille
fix: do not focus the field on helper element click
0653a36 
cherry pick of vaadin/web-components#2232 for v14

Fixes vaadin/flow-components#955
This was referenced Jul 21, 2021
Merged
Merged
Merged
Merged
Merged
alvarezguille added a commit to vaadin/vaadin-checkbox that referenced this pull request Jul 21, 2021
@alvarezguille
fix: do not focus the field on helper element click (#205)
94a4af5 
cherry pick of vaadin/web-components#2232 for v14
cherry pick of vaadin/web-components#2256 for v14

Fixes vaadin/flow-components#955
alvarezguille added a commit to vaadin/vaadin-radio-button that referenced this pull request Jul 21, 2021
@alvarezguille
fix: do not focus the field on helper element click (#197)
f0a010c 
cherry pick of vaadin/web-components#2232 for v14

add missing test

Fixes vaadin/flow-components#955
alvarezguille added a commit to vaadin/vaadin-select that referenced this pull request Jul 21, 2021
@alvarezguille
fix: do not open the field on helper element click (#269)
4e1ee8e 
cherry pick of vaadin/web-components#2232 for v14

Fixes vaadin/flow-components#955
alvarezguille added a commit to vaadin/vaadin-text-field that referenced this pull request Jul 21, 2021
@alvarezguille
fix: do not focus the field on helper element click (#579)
d3f1a2b 
* fix: do not focus the field on helper element click

cherry pick of vaadin/web-components#2232 for v14

Fixes vaadin/flow-components#955

* chore: move helper tests with own fixture into own suite

also pick vaadin/web-components#2251 for v14
also pick vaadin/web-components#2256 for v14
@web-padawan web-padawan mentioned this pull request Oct 11, 2021
Closed
@knoobie
Copy link
Contributor

knoobie commented Feb 5, 2022

@web-padawan I'm wondering.. did we miss the combo box? The issue is still there https://vaadin.com/docs/latest/example?path=component/combobox/combo-box-custom-entry-1.ts

@web-padawan web-padawan mentioned this pull request Feb 6, 2022
Closed
@web-padawan
Copy link
Member

@knoobie Thanks, submitted #3399 to fix this for combo-box.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knoobie knoobie knoobie left review comments

@web-padawan web-padawan web-padawan approved these changes

Assignees
No one assigned
Labels
cherry-picked-20.0 target/20.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Inconsistent focus behaviour of nested helper components
4 participants
@hdamr @knoobie @web-padawan @vaadin-bot