search

aforemny / elm-mdc

Elm port of the Material Components for the Web CSS/JS library
https://aforemny.github.io/elm-mdc
Apache License 2.0
354 stars 43 forks source link

Use the new string id to set the id on the native control #102

Closed berenddeboer closed 6 years ago

berenddeboer commented 6 years ago

It should emit the id to the "id" attribute of the native control, and to the "for" attribute for the label IMO. I need an id on the native control to make BDD work, i.e. without id it's hard for external programs to interact with elm-mdc programs. You can't use label for example as the for attribute isn't set.

Can I submit a patch?

aforemny commented 6 years ago

Hi! I think so, but I am not sure if I fully grasp this. What is BDD? Can you give a concrete example please?

berenddeboer commented 6 years ago

BDD: Behaviour Driven Development.

Scripts look like this:

  Given I login for the first time
  And I press "Hi Mrs, let's measure your impact"
  Then I should see the measure home impact screen
  When I fill in "electricity-bill" with "160"
  And I select "Piped" from "use-gas"
  And I fill in "gas-piped-usage-kwh" with "5000"
  And I press "Save and continue"

My particular issue is that not all controls have a label (due to this particular design). And given we now have a string id, we can use that to give the actual control an id, which allows it to be easily manipulated by external software that drives the browser (selenium).

Have a look at the proposed commit. Works great for me :-)

aforemny commented 6 years ago

Hi @berenddeboer, thank you for clarifying. I do understand the use-case now and I do like the change that you are proposing. I would like to offer a different rationale for implementing that change, though.

This is mainly, because I do not agree that Behavior Driven Development should influence the library in this substantial way: this rationale is nowhere in the MDC Web implementation that this library is a re-implementation of, and it has been possible to do what you are asking before by setting

Textfield.nativeControl (Options.attribute (Html.id "my-id"))

I am however interested in this change because the MDC Web implementation recommends setting the for attribute on labels which refer to a nativeControl. It seems that the following components should set the for attribute on labels and hence should set an id attribute on the component's native control.

The following components do mention the for attributes on labels explicitly:

The select component does not mention that, but I do think this is an oversight, because previously the select component did not build upon a native control input element.

It is not perfectly clear how this should look like for the mdc-checkbox, mdc-radio, mdc-switch and mdc-form-field components because their DOM does not include labels necessarily. I think we would have to look at examples here to evaluate.

The upstream documentation also mentions that you can nest the native control inside the label like so:

<label>My label <input></input></label>

This practice seems to be discouraged by W3C which seems to favor the pattern for + id for accessibility reasons.

So I would like to continue this discussion in this sense, which I hope would be fine by you @berenddeboer, because the implementation is the same as far as I can see. Feel free to point out any differences, as I would like to easily support your use-case.

I would suggest to implement that change and then look at how we like examples involving the aforementioned components which are not quite clear.

berenddeboer commented 6 years ago

@fbessou great points. My response:

  1. If we have multiple Elm models on a page, automatically adding ids won't make things worse. I.e. it won't produce failures we would not have.

But yes, this points out something my implementation does not yet have: the ability to override the automatic id.

  1. I agree we need to document the behaviour, but this can be done best by making "id" an explicit property like "aria". That way we can document that the id will be set on the native control.
berenddeboer commented 6 years ago

@aforemny:

The upstream documentation also mentions that you can nest the native control inside the label like so

Note that this is exactly what my change to Textfield does, as we didn't really have an id at that time, and even in cases with a label I couldn't target the control. We can now, in the absence of id/for the label text can be used.

I suggest we leave that in place, but still add id/for.

I suggest we do that by making "id" like "aria" a first class citizen of the relevant controls. If not set, it should default to the id of the control. I think that should be straight-forward to implement.

And agree the pull request should contain this for all relevant controls.

aforemny commented 6 years ago

Hi @berenddeboer, I agree. Is that something that you would like to attempt? If so, take a look at Internal.Options.NativeControl. I think the pattern used there would fit this as well.

Coming back to your original use-case, on what other components do you usually require Html.ids? The choice which components use Material.Index is quite arbitrary since it depends on whether or not they are maintaining state in Material.Model only.

berenddeboer commented 6 years ago

Users can always fallback to manually providing an id, this is just to provide an initial good experience.

Yes, will expand my pull request in this direction.

aforemny commented 6 years ago

In a first iteration, I ended up implementing setting Html.id on the following component's native control element (07b4b3cbf9c74179f4a35c8d9d7fcccb011a0390):

Those made sense to me because they are used with Html.for in Html.labels. I would be happy to discuss implementing the same thing for more components (all components that use render?, all interactive? components?).

Please open an issue if you want to discuss about that. I will close here for now. Thanks for everyone's feedback on this here and in #103!