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

q-btn in q-field is incorrectly triggering click event #17005

Open
magnww opened this issue Mar 15, 2024 · 5 comments
Open

q-btn in q-field is incorrectly triggering click event #17005

magnww opened this issue Mar 15, 2024 · 5 comments
Labels
area/components bug/1-repro-available A reproduction is available and needs to be confirmed. flavour/quasar-cli-vite kind/bug 🐞 Qv2 🔝 Quasar v2 issues

Comments

@magnww
Copy link

magnww commented Mar 15, 2024

What happened?

q-btn in q-field is incorrectly triggering click event. Clicking on an area outside q-btn will also trigger the click event of q-btn.
Version 2.13.0 works fine, this problem occurs with version 2.13.1 and above.

What did you expect to happen?

The click event should only be fired when q-btn is clicked.

Reproduction URL

https://codepen.io/gefan/pen/PogGxvG

How to reproduce?

  1. Go to reproduction link.
  2. Open the console at the bottom of the page.
  3. Click on the white area of the q-field, you can see that the click event of q-btn is triggered.

The link below is the same example in version 2.13.0, clicking on the white area will not trigger the click event of q-btn.
https://codepen.io/gefan/pen/PogGxMG

Flavour

Quasar CLI with Vite (@quasar/cli | @quasar/app-vite)

Areas

Components (quasar)

Platforms/Browsers

Chrome

Quasar info output

No response

Relevant log output

No response

Additional context

No response

@magnww magnww added kind/bug 🐞 Qv2 🔝 Quasar v2 issues labels Mar 15, 2024
@github-actions github-actions bot added area/components bug/1-repro-available A reproduction is available and needs to be confirmed. flavour/quasar-cli-vite labels Mar 15, 2024
@rajithaeyee
Copy link

rajithaeyee commented Mar 15, 2024

@magnww this happens due to event bubbling if you use @click.stop it will prevent the issue that you are facing.
I believe this helps!

@magnww
Copy link
Author

magnww commented Mar 16, 2024

@magnww this happens due to event bubbling if you use @click.stop it will prevent the issue that you are facing. I believe this helps!

Thanks for the reply, but nothing changed after adding @click.stop.

@ahmedjas1m
Copy link
Contributor

@magnww as you mentioned, version 2.13.1 and above has this issue but it can be solved, I've updated your example

@ahmedjas1m
Copy link
Contributor

ahmedjas1m commented Mar 23, 2024

Most likely what caused this is getting rid of the for attr for the label tag which is a commit due to fix this issue.

after diving into the codebase I've found out that targetUid is null, meaning for attr is not set when there is no for property that is passed as a prop despite the default tag which is a label. which might have been added intentionally! because of mentioned issue.

we had a browser a11y and autofilling violation which is fixed by now in (v2.13.1 +) but we are facing an event capturing issue which is caused by the solution of the first issue (not setting for attr!).

chrome or browser yellings about a11y violations with an example:

First:
after inspecting the generated HTML we see a label like this in V2.13.0 and below:
image
but chrome yells at us:
image

Second:
after inspecting the generated HTML we see a label like this in V2.13.1 and above:
image
but again chrome warns us:
image

As you can see, Chromes warning is quiet logical !! when the value of for attr (targetUid) was generated no form control field was binded to it and also when for attr is not setted chrome says label is not associated with a form field !

I was thinking of a label tag with a for attr to be added dynamically based on the provided tag property, but that is also Incorrect, once more we see this in the log:
image
But this is a solution for this current issue (event capturing) as one might say or at least me but of course not the right approach since it incurs our previous issue! in another word, for is added with a value of targetUid but no element is bound to it therefore it does not trigger! and if we bind it to a form control field event capturing occurs. (generated for or id is passed via control slot).

Why Event capturing with label tag only ?

div and label tags both work as containers but when it comes to event propagation, they behave differently. not to be misunderstood both propagate according to the standard DOM event flow (bubbling and capturing).
but clicking on the label will focus or activate the associated form control. this behavior can affect how events are perceived to propagate, as clicking the label effectively triggers an event on another element !!

Why we did not have the event capturing issue before ?

we had a11y browser violation but we did not have event capturing since the label was only triggering the event based on the passed for attr which was a unique id f_uid and it did not exist in dom so to fire the event !

that means when we use label tag we should declare a specific for attr like for="username" then the button or input inside the control slot which must be a direct child to it, it should have an id specified with the same value in this case id="username" then it'll work like a clock !

in another word, there are two possible ways:

  1. QField should act as form control wrapper (somehow with providing both tag as label and for property binding it to the form field). which incurrs event capturing if we bind it to the form control field. which should be solved !
  2. QField act as a custom component wrapper (without form control fields i.e. input or button) more like a dummy input wrapper.

Since the QField component is created to work in both cases seamlessly, Its kinda a bit tricky to decide for a concise solution!
So I'd like to suggest, giving a warning note in the documentation, describing the edge case briely is gonna be pretty handy !

I hope, it helps to clarify the core concept, Thanks <3

@ahmedjas1m
Copy link
Contributor

Based on my understanding of this and the codebase, I've come up with an idea. so far, i think it is the right approach and I've created this Pull Request. which closes [ #17005, #16589, #16671, #16883 ]

Remembering the cause of the issue, which is getting rid of for attr, requiredForAttr: false while the tag is a label (up above i mentioned why). so i got to solve it like this:
requiredForAttr: props.tag === 'label'. meaning when the tag is label generate a unique for attr value. luckily it solves this issue, mission accomplished, but wait up !! that violates a11y, right ?
image
well, yes. that's why i did not stop there !

to prevent this violation, we have to know what QField is for ! which is a form wrapper right ? but a custom component form wrapper or a form control field wrapper ?

let's catch the first and say, a custom component form wrapper, then it is pretty easy we'd pass the tag property as div and now vola the a11y violation vanishes like socks in the dryer!

and for the second we change the tag property to label, which i don't because it is the default value. (before merging this PR).

well not quiet yet, you know there are many softwares using Quasar, without providing the property and they encounter this a11y violation (without even noticing).

even quasar QField examples/usages, we should go and change every one of them and provide the tag property as div when needed (majority cases). so that chrome stops screaming about autofilling !

thus, i wanted the least change, i got another magic touch. and changed the default value of tag property from label to div. with that concept in my mind QField was exposed to be a custom form component wrapper, i think it has more usages in this case, nevertheless for form control field it works but with an additional change providing the magic tag property again but with a label because i have a form control field !

then, about the usage of QField in other components ? does this change breaks anything ?
three components inherit QField, those are (QInput, QFile, QSelect). and the problem is that they actually need a label tag!
So i change QField usages in thpse components ? I guess not, Instead that passed flag argument inside QField tagProp: true handles this for me, look:
tag: tagProp === true ? computed(() => props.tag) : { value: 'label' }
and that helps not to change anything ! becaue tagProp flag is only true within QField.

to conclude this, the bug (event triggering) should have more priority than a11y violation which can be fixed after providing the tag property therefore i have updated the documenation, i found it handy to mention the importance of tag property.

Thanks for your time <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/components bug/1-repro-available A reproduction is available and needs to be confirmed. flavour/quasar-cli-vite kind/bug 🐞 Qv2 🔝 Quasar v2 issues
Projects
None yet
Development

No branches or pull requests

3 participants