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

#934 - throw an error if assertValue() is used with an element that does not support the value attribute #936

Merged
merged 1 commit into from
Oct 13, 2021

Conversation

u01jmg3
Copy link
Contributor

@u01jmg3 u01jmg3 commented Oct 12, 2021

Throw an error if assertValue() / assertValueIsNot() are used with an element that does not support the value attribute.

Note: a textarea is also permitted even though it doesn't have a value attribute. This is because its content is still retrieved using $textarea->getAttribute('value').

The list of elements that support the value attribute was sourced from: https://www.w3schools.com/tags/att_value.asp


(Previously closed PR: #935)

…sed with an element that does not support the `value` attribute
@taylorotwell taylorotwell merged commit 97cb71d into laravel:6.x Oct 13, 2021
@lk77
Copy link
Contributor

lk77 commented Nov 10, 2021

Why this change ?

There is a valid scenario for assertValue with vue2 for custom inputs :

$browser->assertValue('.component', $value)

which is not the same as assertVue, because it check the value that was emitted

$browser->assertValue('.color-picker', "#eee");//emitted value
$browser->assertVue('value', '.color-picker', "#eee");//internal value

if the component fail to emit it's value back to the parent for any reason, assertVue will pass and assertValue will not.

@u01jmg3
Copy link
Contributor Author

u01jmg3 commented Nov 10, 2021

if the component fail to emit it's value back to the parent for any reason, assertVue will pass and assertValue will not.

What prevents the value not being emitted back to the parent? Are you using assertValue as a fail safe if the DOM and your Vue component data become out of sync (because assertVue will pass regardless)?

This change was introduced to prevent assertValue being used on known HTML elements that don't support the value attribute. I hadn't considered custom Vue inputs which I assume can have any customised tag name.

If my PR is reverted it might be a good idea to add a test that includes applying assertValue on a custom Vue input.

@lk77
Copy link
Contributor

lk77 commented Nov 10, 2021

It's a fail safe, we could do without it, but with custom components, especially third party one, we are not sure that input events are properly triggered.

In our case it's a color picker from vue-color, with a pretty complex emitting logic, but i think there is other cases out there.
You could argue that testing that is a little bit out of scope, and that i should trust third party components, but i prefer to be on the safe side.

i don't really ask this to be reverted tho, i understand w3c compliance concerns

@u01jmg3
Copy link
Contributor Author

u01jmg3 commented Nov 10, 2021

Thanks for the further explanation

I guess it depends what is more important. Supporting the use case you've described or preventing developers mistakenly calling assertValue on an element that doesn't support the value attribute and will therefore always have a value of null (assertValue('') is always true). 🤷🏻‍♂️

@lk77
Copy link
Contributor

lk77 commented Nov 10, 2021

to be fair, this behaviour has changed in vue3, it's modelValue now, to distinguish between native value and component value.
i don't think it's worth fixing this specific use case, assertAttribute could do the trick for now

@u01jmg3
Copy link
Contributor Author

u01jmg3 commented Nov 10, 2021

That's a good change Vue made in order to distinguish

Glad you've found a middle ground ⛰️

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