germsvel / phoenix_test

MIT License
144 stars 20 forks source link

Currently no way to handle fill_in with multiple inputs with the same label #86

Open srcrip opened 2 months ago

srcrip commented 2 months ago

main problem

there's currently nothing you can really do I think if you have more than one input with the same label name. This is a common occurrence when using inputs_for in a has_many form.

fill_in should have the option of taking a selector instead of just a label

I think label is a great default, but you should be able to fill in based on an exact name as well. I have a nested embeds form I want to fill in where the name of the field is like resume[work_experiences][0][company], and it is a nested inputs_for where each company field shares the same label but has unique names.

I'm not sure the best API to suggest, but I think some kind of ability to complexly select the element to fill in would be key.

Alternatively maybe there could be something like within that can select any element (not just a form) and would scope any further fill_ins to that scope.

germsvel commented 2 months ago

Thanks for opening this issue @srcrip!

I've worked with nested inputs too, and something that I've come to realize by working on this library is that we probably should have labels for each nested input -- even if those labels aren't visible (they might be only accessible to screen readers).

If you're using tailwind, you could add a class sr-only and the label will not be visible but it will be accessible to screen readers. Now, your app may or may not need to be accessible to screen readers, but part of the idea of this library is for the library to make it easy to do the recommended thing. I don't think it's required by any HTML standard to associate a label to an input (so I get your point), but I do think it's recommended.

Also, if you're filling in by the name of the field (in your case resume[work_experiences][0][company]), that's typically what I might use for my id. I might do something like this:

<label class="sr-only" for="resume_work_experiences_0_company">Work Experience for Company (Line 0)</label>
<input id="resume_work_experiences_0_company" name="resume[work_experiences][0][company]" type="text" />

Does that make sense?

srcrip commented 2 months ago

The thing is though this is a case where the element does have a label, it's just the same label as every other one in the nested inputs for. It's a dynamic form where theres a list of inputs that are all the same thing. So I can't really change the label to something descriptive in this case because I don't want SR only on it, I want it to continue to display.

srcrip commented 2 months ago

I think selecting on label only is going to be too restrictive in the long run. For this and other reasons. What if someones using one of the other ways to set accessible names like aria-label or aria-labelledby? I think it has to be opened up to just any selector, with a preference towards label.

germsvel commented 2 months ago

What if someones using one of the other ways to set accessible names like aria-label or aria-labelledby?

Yeah, that's an excellent point. My intention would be to support finding by aria-label or aria-labelledby, in addition to finding by label. I'd really like to support that. Haven't gotten around to it though.

srcrip commented 1 month ago

honestly I think the cleanest thing would be to just use xpath style selectors ala Phoenix.LiveViewTest.

Given that Phoenix has an API that looks like this with this optional text filter:

assert view
  |> element("#term > :first-child", "Increment")

and phoenix_test currently does something like this:

session
  |> fill_in("Name", with: "Aragorn")

I would suggest it be changed to match this 'optional text filter' kind of thing, except its the xpath part thats optional and instead of a text filter its the label. It could look like this:

# continues to work the same
session
  |> fill_in("Name", with: "Aragorn")

# but you can also do this
session
  |> fill_in("input[data-whatever=foo]", "Name", with: "Aragorn")

So it would gain an extra optional parameter, which would be the second parameter, and it would take the same selectors as https://hexdocs.pm/phoenix_live_view/Phoenix.LiveViewTest.html#element/3.

This would have the benefit of solving this problem without making a breaking API change, cause all the current usage would still work.

If you are ok with breaking changes there may be other ways, I don't know.

What do you think about that kind of an idea?

germsvel commented 1 month ago

@srcrip I think this solution you mention could be interesting (though I'm not 100% sure yet on how it works):

# but you can also do this
session
  |> fill_in("input[data-whatever=foo]", "Name", with: "Aragorn")

What's the relation between "Name" and the CSS selector input[data-whatever=foo]?

Is "Name" the label of input[data-whatever=foo]? What would we gain with the CSS selector at that point? Or is "Name" something else?

srcrip commented 1 month ago

Name would be the label text yes. In this proposal parameter 3 would be the same as what parameter 2 is now.

srcrip commented 1 month ago

Alternatively we could think about some kind of 'locator' object similar to what's done in Playwright. It would be more complex but it may help the API stability if any endpoints that selected elements had a common API that was just a struct that could change in the future or something.

totaltrash commented 1 month ago

I wonder if filling in nested forms (with inputs that have the same labels) could be better implemented using within (or something similar)?

session
|> within("#form[nested_form][0]", fn session ->
  session
  |> fill_in("Name", with: "Aragorn")
  |> check("Ranger")
end)
|> within("#form[nested_form][1]", fn session ->
  session
  |> fill_in("Name", with: "Legolas")
  |> check("Legend")
end)
srcrip commented 1 month ago

I thought about that but I think more options on the selectors themselves would be cleaner

benswift commented 3 weeks ago

Any update on this? I came up against the same issue today - with the added bonus that the labels are auto-generated by a library (ash_authentication_phoenix) so I can't even easily change the labels to work around the issue.

germsvel commented 3 weeks ago

I haven't had much time to dig into this. But I don't quite understand the scenarios where we don't want labels on inputs (or use the same label across several inputs). As far as I understand, each input should have its own label. I promise I'm not trying to be obtuse. I just don't see the same code you're seeing. Can people share the code they have so we can see what kinds of forms we're dealing with?

That being said, maybe the library shouldn't be so prescriptive. I'd like for the library to make it easy to write good HTML, but it shouldn't hinder you either.

In my mind, the idea is to move more towards the idea of locators (or a similar concept) like Andrew said:

Alternatively we could think about some kind of 'locator' object similar to what's done in Playwright.

I've been studying Playwright a bit more (hadn't used it in the past) to see (a) what patterns it has, and (b) to see if we can integrate into PhoenixTest (for JS bits). But that's a separate conversation.

Until we figure this out, for now, you can drop down to LiveViewTest with #unwrap.

totaltrash commented 2 weeks ago

Can people share the code they have so we can see what kinds of forms we're dealing with?

For me, it's as the OP said, where I have a form with a has_many relationship, and the many nested forms are rendered using inputs_for. The nested forms have inputs with labels, but the same label text (not using the same label, just using the same label text)...

<!-- Row 1 -->
<div>
  <div>
    <label for="form_involved_0_contact_0_first_name">New Contact First Name</label>
    <input type="text" name="form[involved][0][contact][first_name]" id="form_involved_0_contact_0_first_name" value="">
  </div>
  <div>
    <label for="form_involved_0_contact_0_last_name">New Contact Last Name</label>
    <input type="text" name="form[involved][0][contact][last_name]" id="form_involved_0_contact_0_last_name" value="">
  </div>
  <div>
    <label for="form_involved_0_involved_how">How Involved</label>
    <select name="form[involved][0][involved_how]" id="form_involved_0_involved_how"></select>
  </div>
</div>
<!-- Row 2 -->
<div>
  <div>
    <label for="form_involved_1_contact_1_first_name">New Contact First Name</label>
    <input type="text" name="form[involved][1][contact][first_name]" id="form_involved_1_contact_1_first_name" value="">
  </div>
  <div>
    <label for="form_involved_1_contact_1_last_name">New Contact Last Name</label>
    <input type="text" name="form[involved][1][contact][last_name]" id="form_involved_1_contact_1_last_name" value="">
  </div>
  <div>
    <label for="form_involved_1_involved_how">How Involved</label>
    <select name="form[involved][1][involved_how]" id="form_involved_1_involved_how"></select>
  </div>
</div>

image

benswift commented 2 weeks ago

I'm not OP, but using unwrap will be fine for me. Honestly, I think that the API for PhoenixTest is pretty good.