emberjs / rfcs

RFCs for changes to Ember
https://rfcs.emberjs.com/
790 stars 408 forks source link

Pre-RFC: (guid-for) helper #612

Closed steveszc closed 3 years ago

steveszc commented 4 years ago

The goal:

Provide a guid-for template helper for ergonomically generating unique IDs.

<label for={{guid-for "toggle"}}>Toggle</label>
<input id={{guid-for "toggle"}} type="checkbox">

This idea originated from the Accessibility Strike Team's discussions of #595

Motivation

The primary use case for an guid-for helper is to programmatically associate labels and input elements using the label's for attribute and the input's id attribute. This pattern is required for accessibility in cases where it is not possible or convenient to nest input elements inside of their associated label (details here). In addition to that use-case, this helper would be useful anytime the html id attribute needs to added to a dom element.

The HTML spec requires that id attributes be unique in the whole document (https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/id). Ember components provide the elementId attribute which could be used to derive unique ids within components, but this is no longer the case with Glimmer components.

Since providing some faculty for generating unique IDs for dom elements can reasonably be considered a requirement for most web apps wishing to provide accessible form elements, it is reasonable for Ember to provide this at the framework-level. Ember already provides the guidFor util in javascript, so it is reasonable for Ember to expose this via a helper within templates.

Approach

This helper could be included directly in ember as a built-in helper, or it could exist as an add-on that would be included in the default blueprint for new Ember apps.

Ergonomics

Developers should be able to use the guid-for helper to generate the same guid value every time the helper is called with a given argument within a given template invocation. But in another template (or within another invocation of the same template) that same argument should be expected to return a different guid.

For example, in an app with a sign-in form and a password reset form, both existing in separate templates, I should be able to use (guid-for "email-address") in both templates and have a different guid generated. Additionally, I should be able to create a CustomInput component that renders a generic label/input pair using (guid-for "input"), and render that component multiple times on a page, and (guid-for "input") should generate a different guid in every instance of that component.

Since the current guidFor util function accepts a single argument (any object, string, number, Element, or primitive) and will always return the same guid for the same argument, the guid-for helper cannot be a simple alias of the guidFor util. The helper must maintain some knowledge of the context it was called from, and always return the same guid when called with the same value from within the same context.

Edit: We may want to require that the context be passes explicitly as the first arg to the helper. This would make the helper clearer and more flexible, though slightly more verbose for common use-case.

<label for={{guid-for this "toggle"}}>Toggle</label>
<input id={{guid-for this "toggle"}} type="checkbox">

Learning

Once this helper exists in the blueprint for new ember apps, the guides should be updated to include this helper anywhere an input element is used in examples. This will both teach accessible label/input associations by default, and teach that this helper exists.

pzuraq commented 4 years ago

I understand the goal, and I think the ergonomics are a strong selling point. I have concerns about the need for an implicit context though. It's something we've avoided for a variety of reasons, it's difficult to implement and definitely opens the door for a lot of possibly problematic code patterns. Even the {{action}} helper doesn't actually have this ability, it gets turned into {{action this this.someFunction}} at compile time, only for backwards compatibility reasons.

We typically also don't include new one-offs or built-in helpers that have special abilities as a design principle. In general, everything that a built in can do, userland helpers should be able to do too. So, I think we should either try to formalize that part of the proposal and see if it makes sense to add for all helpers/modifiers, or possibly explore alternatives.

A few ideas for alternatives:

  1. Require the user to pass the context:

    <label for={{guid-for this "toggle"}}>Toggle</label>
    <input id={{guid-for this "toggle"}} type="checkbox">

    A bit less ergonomic, but also more clear about where the context comes from.

  2. Use the let helper:

    {{#let (guid-for toggle) as |toggleGuid|}}
      <label for={{toggleGuid}}>Toggle</label>
      <input id={{toggleGuid}} type="checkbox">
    {{/let}}

    Could have issues with flexibility in the template though.

  3. Ship some form of the {{let}} for the rest of the template proposal and use that:

    {{let (guid-for toggle) as |toggleGuid|}}
    
    <label for={{toggleGuid}}>Toggle</label>
    <input id={{toggleGuid}} type="checkbox">
steveszc commented 4 years ago

@pzuraq Personally I don't have any issue requiring the context to be passed as the first arg. That would make it more clear what the helper is actually doing and adds flexibility (ie passing something other than this as the first arg).

steveszc commented 4 years ago

Here's a bare-bones example of how this helper might be implemented in userland to support the following API.

<label for={{guid-for this "toggle"}}>Toggle</label>
<input id={{guid-for this "toggle"}} type="checkbox">
import { helper } from '@ember/component/helper';
import { guidFor as _guidFor } from '@ember/object/internals';

export default helper(function guidFor([context, specifier]) {
  if (typeof specifier === 'undefined') {
    return _guidFor(context);
  } else {
    return _guidFor(context) + _guidFor(specifier);
  }
});
jrjohnson commented 4 years ago

How does passing context work in a template only component? Do you need a backing class just for this?

rwjblue commented 4 years ago

Thanks for working this up @steveszc! Two main thoughts:

First, I think we should go with a name other than guid-for / guidFor. Specifically, guid for is "in the weeds" and is a thing we happen to understand (because it is something we essentially have used before), but it doesn't really describe the thing we care about. The thing we want to signify is that the helper will provide a unique identifier string. Maybe {{unique-id}} and {{unique-id "special-thing"}}?

Second, the solution for template only components (which works in both cases nicely) is effectively to leverage an AST transform that converts:

<label for="{{unique-id}}-toggle">Toggle</label>
<input id="{{unique-id}}-toggle" type="checkbox">

Into:

{{#let (unique-id) as |id|}}
  <label for="{{id}}-toggle">Toggle</label>
  <input id="{{id}}-toggle" type="checkbox">
{{/let}}
rwjblue commented 4 years ago

Whoops, I thought of another minor point (which I had accidentally leveraged in the examples for the second point just above) but didn't say:

We should probably not support any arguments to {{unique-id}}. If you want to add a suffix, it is really easy and "feels good" to leverage normal interpolation:

<label for="{{unique-id}}-toggle">Toggle</label>

Also, it is isn't 100% obvious to me that the "magical" AST transform expansion is needed. It doesn't seem terrible to just suggest folks author like this:

{{#let (unique-id) as |id|}}
  <label for="{{id}}-toggle">Toggle</label>
  <input id="{{id}}-toggle" type="checkbox">
{{/let}}
jrjohnson commented 4 years ago

I super ❤️ this syntax:

<label for="{{unique-id}}-toggle">Toggle</label>
<input id="{{unique-id}}-toggle" type="checkbox">

I do think it raises a question when reading - are those going to end up being two different unique ids?

<label for="123-toggle">Toggle</label>
<input id="456-toggle" type="checkbox">

I agree that uuid is not a great concept to expose, the for part gives a sense about scope.

Possible alternative template-id?

<label for="{{template-id}}-toggle">Toggle</label>
<input id="{{template-id}}-toggle" type="checkbox">
Gaurav0 commented 4 years ago

@rwjblue You're ok with an implicit context (this)?

jelhan commented 4 years ago

If I understand rwjblue's proposal right it's not an implicit context. It's just always returning the same ID per template invocation. I like the proposed name {{template-id}} as it makes this more clear. Maybe even {{component-id}}?

This would be straightforward and removes all complexity that comes with a context at all. The docs could be as simple as:

The {{template-id}} template helper returns a unique ID for the invocation of a component.

KamiKillertO commented 4 years ago

I like the proposed name {{template-id}} as it makes this more clear. Maybe even {{component-id}}?

I like {{template-id}} as it implies it generates a unique id for a template which could be a component template or a route template.

KamiKillertO commented 4 years ago

As discussed during the last meeting of the Accessibility Strike Team, I've created an addon ember-unique-id-helper for this RFC.

jelhan commented 4 years ago

We should probably not support any arguments to {{unique-id}}.

The strict mode templates RFC, which is in final comment period, treats helpers without arguments as an edge case and requires a more complex syntax if they are used to generate the value for a component's named argument. See https://github.com/emberjs/rfcs/blob/strict-mode/text/0496-handlebars-strict-mode.md#3-no-implicit-invocation-of-argument-less-helpers for details.

Gaurav0 commented 4 years ago

I'm a bit concerned about teaching this to new users. Explaining that the helper returns the same value when invoked multiple times in the same instance of a template while a different value each time the template is instantiated seems needlessly complex. {{unique-id this}} piggybacks on what users have already learned about this from object oriented programming.

I also think if we are encouraging people to use {{this.property}} instead of {{property}} and referring to the latter as an "implicit this", they are going to question why a helper named {{template-id}} should be permitted to avoid this rule.

chriskrycho commented 4 years ago

Actually, this helper has to be able to work without requiring this because it otherwise cannot be used in template-only Glimmer components, where this is always undefined. That is not an argument for an implicit context, only observing that having/requiring a context param is not as simple as passing this.

lifeart commented 4 years ago

one more related addon - https://emberobserver.com/addons/@ember-lux/id-helper

lougreenwood commented 4 years ago

Maybe I'm mis-understanding, but wasn't the original proposal to allow generating/retrieving a new unique id for a specific key, but the proposals have morphed into fetching the guid for the current context?

Seems that the guid for the current context wouldn't solve the initial use case where there is a need for multiple unique ids?

<div>
    <label for={{guid-for "toggle-one"}}>Toggle One</label>
    <input id={{guid-for "toggle-one"}} type="checkbox">

    <label for={{guid-for "toggle-two"}}>Toggle Two</label>
    <input id={{guid-for "toggle-two"}} type="checkbox">
</div>
ro0gr commented 4 years ago

@lougreenwood it solves the issue, you'd just need to manually concat, to get a specific ID:

<div>
    <label for="{{element-id}}-toggle-one">Toggle One</label>
    <input id="{{element-id}}-toggle-one" type="checkbox">

    <label for="{{element-id}}-toggle-two">Toggle Two</label>
    <input id="{{element-id}}-toggle-two" type="checkbox">
</div>
MelSumner commented 4 years ago

@pzuraq @rwjblue @steveszc to bring this back around, what needs to be figured out in order to move this forward?

jgwhite commented 4 years ago

For completeness, @hergaiety, @josephdsumner, and I put together a twiddle demonstrating @rwjblue’s proposal in https://github.com/emberjs/rfcs/issues/612#issuecomment-611183877.

chriskrycho commented 4 years ago

That's excellent! I'd suggest a tiny tweak to it, to allow it to be bound to a given context if users so desire:

import { helper } from '@ember/component/helper';
import { guidFor } from '@ember/object/internals';

export default helper((params) => guidFor(params[0] ?? {}));

Then users could invoke with or without and have it "just work":

{{#let (unique-id this) as |id|}}
  <fieldset>
      <legend>A label, input, and alert tied to together with {{id}}</legend>

    <label for="{{id}}-input">Email</label>

    <input
      type="email"
      id="{{id}}-input"
      aria-describedby="{{id}}-alert"
    >

    <span id="{{id}}-alert" role="alert">
      Please enter a valid email address.
    </span>
  </fieldset>
{{/let}}

The API design nerd in me wants it to be unique-id for=<context>, which might be implemented like so:

import { helper } from '@ember/component/helper';
import { guidFor } from '@ember/object/internals';

export default helper(({ "for": context = {} } = {}) => guidFor(context));

(That gnarly rename with defaults on the helper is really something. Might be better to write it out longer. :joy:)

{{#let (unique-id for=this) as |id|}}
  <fieldset>
    <legend>A label, input, and alert tied to together with {{id}}</legend>

    <label for="{{id}}-input">Email</label>

    <input
      type="email"
      id="{{id}}-input"
      aria-describedby="{{id}}-alert"
    >

    <span id="{{id}}-alert" role="alert">
      Please enter a valid email address.
    </span>
  </fieldset>
{{/let}}
jrjohnson commented 4 years ago

My concern with doing this in userland and at runtime is this guidFor(params[0] ?? {})) that anonymous object {} is going to be different for each invocation and so will the ID. So you MUST do this within a {{let}} block which is needlessly verbose from my point of view. A template transform that converts {{template-id}} during build time seems like it would get the job done more predictably. I'm hesitant to prove this out in an addon though because I don't understand yet how build time addons are going to work in the embroider era.

chriskrycho commented 4 years ago

FWIW, I think it's roughly fine (insert hand waving here) if we make a built-time transform available to eliminate the need for the let. I would personally prefer to use the let because it makes explicit what's happening, but I get why others might differ.

More importantly, exposing something like this as the base primitive but also supplying a transform for convenience on top of it might be a "best of both worlds." In any case, I think we could ship the primitive even before working out exactly what the sugar on top should look like.

chriskrycho commented 4 years ago

I realized I should add: "More predictably" is definitely in the eye of the beholder. Something like template-id has more "magic" in that you don't have to understand it, and you still need some way of making sure it's only as unique as the end user wants it to be—you definitely want a way to use multiple distinct IDs in the same template for forms with more than one input, for example. So the use of let gives you the control you need in a lot of cases. That said, the nesting is a bit annoying, and you do have to understand a (little) bit more to use it correctly.

To me, though, that starts to sound more like an argument for this API but with something like non-block {{let}} to avoid the nesting, which has been suggested a few different times, and would solve the problem orthogonally with a single syntax transform that would be useful to many scenarios, not just this one:

{{template-let (unique-id) as |nameId|}}
{{template-let (unique-id for=this) as |ageId|}}
{{template-let (unique-id for=(hash theThing="email")) as |emailId|}}

<form {{on "submit" (fn this.processForm nameId ageId emailId)}}>
  <label for={{nameId}}>Name</label>
  <input id={{nameId}} type="text" />

  <label for={{ageId}}>Age</label>
  <input id={{ageId}} type="number" min=18 />

  <label for={{emailId}}>Email</label>
  <input id={{emailId}} type="email" />
</form>
KamiKillertO commented 4 years ago

@chriskrycho @jgwhite I've published this addon that provide an helper generating unique id. By default it use this as context to generate the id but you can provide something else if you want + it work with glimmer template only.

ro0gr commented 4 years ago

@chriskrycho I think argument can lead to some confusion. Let's assume your example:

{{template-let (unique-id) as |nameId|}}
{{template-let (unique-id for=this) as |ageId|}}

Seems these two statements would lead to the same result string, since both would use the same this to produce a string.

In my opinion, it still may be a good idea, to prevent such situations by avoiding any args to the helper, and leverage string concatenation:

{{#let 
  (unique-id)
  (concat (unique-id) "-age") 
  as |id ageId|
}}

which should be less error prone due to its restrictive api.

chriskrycho commented 4 years ago

@ro0gr nope, they wouldn't! The sample that @jgwhite @hergaiety and @josephdsumner built, and the modified version I built, don't have any reference to this unless you pass it explicitly—unlike some of the other proposals up-thread which implicitly use the this of the backing context.

You can test in the twiddle that @jgwhite linked and see that you get a consistent and stable GUID per unique invocation, since each time it uses a new {} to construct the object. Where passing an argument as in my example code, you get either a unique ID for each distinct time you call without arguments (as in the original), or you get one that's specific to whatever object you pass to for=, which is exactly what you want here: it's stable on object identity.

I do like the multiple bindings in a single let as a nice workaround for not having a template level let.

{{#let
  (unique-id)
  (unique-id for=this)
  (unique-id) for=(hash theThing="email")
  as |nameId ageId emailId|
}}
  <form {{on "submit" (fn this.processForm nameId ageId emailId)}}>
    <label for={{nameId}}>Name</label>
    <input id={{nameId}} type="text" />

    <label for={{ageId}}>Age</label>
    <input id={{ageId}} type="number" min=18 />

    <label for={{emailId}}>Email</label>
    <input id={{emailId}} type="email" />
  </form>
{{/let}}

I also note that the string argument from @KamiKillertO's implementation could be nice here, and it's easy to support that as well as the for= approach!

elwayman02 commented 4 years ago

I can't say that I'm a huge fan of forcing the let syntax. I would much prefer that whatever solution we land on also support this:

<form {{on "submit" (fn this.processForm nameId ageId emailId)}}>
    <label for={{unique-id}}>Name</label>
    <input id={{unique-id}} type="text" />

    <label for={{unique-id for=this}}>Age</label>
    <input id={{unique-id for=this}} type="number" min=18 />

    <label for={{unique-id for=this.someHash}}>Email</label>
    <input id={{unique-id for=this.someHash}} type="email" />
  </form>

That gives developers the freedom to decide which syntax works best based on how they're using the helper and how many times the ID is being used. If it's only used twice (as in the above example), a let declaration seems overkill.

krisselden commented 4 years ago

The thing about the let syntax is it makes it clear what the scope of the unique id is and that it is the same for the label/input

https://glimmerjs.github.io/glimmer-experimental/?snippet=%0Aimport+Component%2C+%7B+hbs%2C+tracked+%7D+from+%27%40glimmerx%2Fcomponent%27%3B%0A%0Aconst+uniqueId+%3D+%28%28%29+%3D%3E+%7B%0A++let+id+%3D+0%3B%0A++return+function+makeId%28%29+%7B%0A++++return+id%2B%2B%3B%0A++%7D%0A%7D%29%28%29%3B%0A%0Aexport+default+class+MyForm+extends+Component+%7B%0A++static+template+%3D+hbs%60%0A++++%3Cform%3E%0A++++%7B%7B%23let+%28uniqueId%29+as+%7CidSuffix%7C%7D%7D%0A++++++%3Cp%3E%0A++++++++%3Clabel+for%3D%22beer-%7B%7BidSuffix%7D%7D%22%3EDo+you+like+beer%3F%3C%2Flabel%3E%0A++++++++%3Cinput+type%3D%22checkbox%22+name%3D%22beer%22+id%3D%22beer-%7B%7BidSuffix%7D%7D%22%3E%0A++++++%3C%2Fp%3E%0A%0A++++++%3Cp%3E%0A++++++++%3Clabel+for%3D%22coffee-%7B%7BidSuffix%7D%7D%22%3EDo+you+like+coffee%3F%3C%2Flabel%3E%0A++++++++%3Cinput+type%3D%22checkbox%22+name%3D%22coffee%22+id%3D%22coffee-%7B%7BidSuffix%7D%7D%22%3E%0A++++++%3C%2Fp%3E%0A++++%7B%7B%2Flet%7D%7D%0A++++%3C%2Fform%3E%0A++%60%3B%0A%7D%0A

elwayman02 commented 4 years ago

I disagree, actually. We should not think of these as singularly unique, we should think of these as uniquely-reproducible IDs. Given the same input, the helper should always have the same output. To that end, this is perfectly valid code:

<form {{on "submit" (fn this.processForm nameId ageId emailId)}}>
  {{#let (unique-id) as |nameId|}}
    <label for={{nameId}}>Name</label>
  {{/let}}
  {{#let (unique-id) as |nameId|}}
    <input id={{nameId}} type="text" />
  {{/let}}
</form>

Is it concise? No. Should you write code like this? Probably not. But it is CORRECT and VALID, and we should expect that the label and input have the same ID. Thus, I do not buy the justification that separate invocations of this helper cannot be easily understood to produce the same output when given the same input.

chriskrycho commented 4 years ago

The constraint that the same input should always produce the same output is one I'm sympathetic to in general, but here I think it fails to earn its tradeoffs. It constrains you to having a single UUID as the value anywhere you use that bare invocation in your app (unless you add magic that makes it per-template?), and requires you to do more work for the very common case of wanting a handful of separate IDs in a single template without generating a bunch of extra data for it via a backing class.

In other words, if I want to just get UUIDs for a bunch of different items in a form, but don't want a backing class for it (because template-only components are often awesome!), I end up having to write something like this, where fresh-obj is a helper that gives you a new {} every time (basically manually doing what the implementation @jgwhite, @hergaiety, and @josephdsumner does by default):

<form {{on "submit" @submit}}>
  {{#let (unique-id for=(fresh-obj)) as |nameId|}}
    <label for={{nameId}}>Name</label>
    <input id={{nameId}} type='text' />
  {{/let}}
  {{#let (unique-id for=(fresh-obj)) as |nameId|}}
    <label for={{nameId}}>Name</label>
    <input id={{nameId}} type='text' />
  {{/let}}
  {{#let (unique-id for=(fresh-obj)) as |ageId|}}
    <label for={{ageId}}>Age</label>
    <input id={{ageId}} type='number' />
  {{/let}}
  {{#let (unique-id for=(fresh-obj)) as |emailId|}}
    <label for={{emailId}}>Email</label>
    <input id={{emailId}} type='text' />
  {{/let}}
</form>

Philosophically, I think a bare unique-id here should be analogous to calling new UuidV4() in many other contexts, whereas with an argument it's more like doing a new UuidV3(someData). Both are useful! But requiring that you have to pass a seed to get a different value—while incredibly helpful in truly pure functional contexts—doesn't seem like a great fit for Glimmer template contexts, which already has a variety of side effects and doesn't have the other benefits of pure functional languages to make up for that additional cost.

elwayman02 commented 4 years ago

To solve that problem, I am a huge fan of the variation in the original proposal where you invoke it like this:

<form {{on "submit" @submit}}>
  <label for={{unique-id this 'name'}}>Name</label>
  <input id={{unique-id this 'name'}} type='text' />
  <label for={{unique-id this 'age'}}>Age</label>
  <input id={{unique-id this 'age'}} type='text' />
</form>

this provides the unique template context and name/age provides a logical string that differentiates it from other ids generated in the same template. There was some concern above that passing this is a bit verbose for the basic use-cases, but I don't have a strong opinion between that and making it implicit inside the helper. That is, I am amenable to just {{unique-id 'name'}} with the understanding that it's unique to that component and would not produce the same id as the same syntax in another component, but I personally have no problem with requiring this to be explicitly passed in.

pzuraq commented 4 years ago

@elwayman02 that won't work, unfortunately, because Template Only components do not have a this as @chriskrycho mentioned. I don't think it's likely that one will be added either, it's pretty fundamental to the system that they are this-less.

I think the idea that (unique-id) generates a unique ID each time is good. We can create a unique-id-for helper, or add a for= hash argument, if it turns out that we need to support that use case, but for TO components, the ability to generate an ID on it's own is going to be pretty important.

elwayman02 commented 4 years ago

@pzuraq then I would go back to @rwjblue pointing out that we can do the AST transform to let format on our end. That is,

<form {{on "submit" @submit}}>
  <label for={{unique-id 'name'}}>Name</label>
  <input id={{unique-id 'name'}} type='text' />
  <label for={{unique-id 'age'}}>Age</label>
  <input id={{unique-id 'age'}} type='text' />
</form>

transforms into a let invocation without the consumer having to think about it. The end-user syntax is far more ergonomic and feels like the way you would want to use a feature like this outside of the context of Ember-specific templating concerns. If we want people to actually adopt this and treat a11y as a first-class citizen, we have to care deeply about the developer experience and making it feel effortless to use. The let formats suggested above are decidedly not that.

jrjohnson commented 4 years ago

Allowing more than one context for this helper seems to substantially complicate the API and teachability. I'm not clear on what the purpose of being able to create more than one id in a template is so I see:

<label for="{{template-id}}-toggle">Toggle</label>
<input id="{{template-id}}-toggle" type="checkbox">

<label for="{{template-id}}-name">Name</label>
<input id="{{template-id}}-name">

As a much clearer concept which handles the case of ensuring that id is unique for all elements in the DOM.

pzuraq commented 4 years ago

@elwayman02 generally we try to avoid template transforms as part of the default stack, when possible. We haven't based a feature solely on them. I think if we were to add a contextual value of some sort, it would likely be added as a keyword, not a helper.

@jrjohnson a common use case I've run into is labels within an each loop, such as in a table:

<table>
  {{#each this.rows as |row|}}
    <tr>
      <td>
        {{#let (unique-id) as |id|}}
          <label for={{id}}>Selected</label>
          <input id={{id}} type="checkbox" />
        {{/let}}
      </td>
    </tr>
  {{/each}}
</table>
jrjohnson commented 4 years ago

@pzuraq that's a really interesting example, I'm going to pick at it 😁

As a teaching tool how would beginners know that id is going to be unique per-invocation of the loop? Abstractly even as an advanced user I will wonder if let might get evaluated once and tokenized and just referenced within the each. I'd have to run it and test to know for sure.

Isn't

<table>
  {{#each this.rows as |row i|}}
    <tr>
      <td>
        <label for="{{template-id}}-box-{{i}}">Selected</label>
        <input id="{{template-id}}-box-{{i}}"} type="checkbox" />
      </td>
    </tr>
  {{/each}}
</table>

clearer still? Plus it leverages a loop counter which is already documented and not a surprising construct as it's in the first few things you end up learning after you hello world in most languages.

pzuraq commented 4 years ago

@jrjohnson that is a fair point, and something we considered as well. I think the main thing that folks on the core team want to avoid is introducing a magic variable that comes from nowhere. Even if we teach it as a keyword, it is another unique, context shifting-like keyword that we will have to support indefinitely.

These constructs add weight to the system, and make it hard to maintain. They also are harder to teach, because they're one-offs. You aren't teaching a general construct anymore, you're teaching a specific special-case. There is definitely an argument that this is a special case worth considering.

FWIW, I originally had the perspective that {{template-id}} made sense and was vouching for it, but these concerns were raised and made me reconsider. At this point I feel like we really need a solution, and using the existing construct of {{let}} is much more likely to get consensus in a timely manner. It also does not prevent us from adding {{template-id}} in the future, if we decide it's worth the convenience. It can also be added by addons using template transforms, as was described earlier.

pzuraq commented 4 years ago

Also, sorry about closing, my finger slipped and I thought I prevented it by reloading, but GitHub just decided to show me the update 😅

ijlee2 commented 4 years ago

Is it fair to assume these 3 things?

If so, maybe we can consider using a modifier instead of a helper. I tried creating an {{id-for}} modifier, to be used like this:

{{!-- app/components/my-form/index.hbs --}}
<form>
  <div>
    <label {{id-for "confirm-policy"}}>
      I agree to the Privacy Policy.
    </label>
    <input type="checkbox" {{id-for "confirm-policy"}}>
  </div>

  <div>
    <label {{id-for "confirm-tnc"}}>
      I agree to Terms and Conditions.
    </label>
    {{!-- An extra div for fun --}}
    <div>
      <input type="checkbox" {{id-for "confirm-tnc"}}>
    </div>
  </div>
</form>
{{!-- app/templates/application.hbs --}}
<MyForm />
<MyForm />
// app/modifiers/id-for.js
import { guidFor } from '@ember/object/internals';
import { modifier } from 'ember-modifier';

export default modifier(
  function idFor(element, params) {
    const parentId = getParentId(element);
    const postfix = params[0]; // required (TODO: use `assert`)
    const id = `${parentId}-${postfix}`;

    switch (element.tagName) {
      case 'INPUT': {
        element.setAttribute('id', id);
        break;
      }

      case 'LABEL': {
        element.setAttribute('for', id);
        break;
      }
    }
  }
);

function getParentId(element) {
  const formElement = element.closest('form');
  // Perform some transformation of element into a unique string
  const path = getDomPath(formElement ?? element).join(', ');

  return guidFor(path);
}

/*
  Adapted from https://gist.github.com/karlgroves/7544592
*/
function getDomPath(el) {
  const stack = [];

  while (el.parentNode !== null) {
    let sibCount = 0;
    let sibIndex = 0;

    for (let i = 0; i < el.parentNode.childNodes.length; i++) {
      let sib = el.parentNode.childNodes[i];

      if (sib.nodeName === el.nodeName) {
        if (sib === el) {
          sibIndex = sibCount;
        }
        sibCount++;
      }
    }

    let name = el.nodeName.toLowerCase();

    if (el.hasAttribute('id') && el.id !== '') {
      name += `#${el.id}`;
    } else if (sibCount > 1) {
      name += `:eq(${sibIndex})`;
    }

    stack.unshift(name);
    el = el.parentNode;
  }

  return stack.slice(1); // removes the html element
}
modifier-approach
elwayman02 commented 4 years ago

Modifiers were the first thing I thought of for this problem, but everyone was going down the helper path so I didn't think about it too hard. :P

elwayman02 commented 4 years ago

@pzuraq to the point of unique magic variables that come from nowhere, doesn't that describe every helper/modifier in core today?

pzuraq commented 4 years ago

@ijlee2 that's an interesting approach! I hadn't considered it before, and it does seem like it would be fairly ergonomic. You could probably simplify it further by using the guidFor(formElement) rather than the string, that should be unique enough.

That said, I'm not sure every one of those constraints will hold:

We want to create IDs only for elements inside a <form> element.

As I understand it, forms are the most common use case, and it's best practice to place all inputs inside a form element, but I'm not sure we should design a system that would prevent using these APIs outside of them. I think we would need folks with more A11y experience to chime in here.

We know on which elements we can set the id attribute versus the for attribute.

I don't believe this is true, because we also need to consider elements that are using aria-labelledby and other A11y APIs.

I think this approach would be excellent to try out as an addon, but personally I still think the helper approach is more general and more solid based on our current models and past experiences, and more suitable for adding to the core Ember experience at the moment.

ijlee2 commented 4 years ago

Yeah, I think a problem with this pre-RFC issue (not to blame the author or others) was that the title limited discussing possible solutions to a helper.

Since the problem we want to solve is for Octane apps, I think it makes sense to try newer stuffs. 🙂

(My screenshot above shows the id and for attributes mixed. My bad. I fixed the code.)

pzuraq commented 4 years ago

@elwayman02 right, and we're moving away from that direction as quickly as possible with features such as Template Strict Mode and imports. All core helpers will be importable in that world, so it'll be much less magical as a whole 😄

ijlee2 commented 4 years ago

Yeah, I imagine there are cases for which the {{id-for}} modifier will fail. I only covered the simple case, heh.

chriskrycho commented 4 years ago

There are definitely other, non-accessibility-related places this would be useful, too! Any kind of library which needs to interact with third-party code which wants you to supply unique IDs in various places, for example, is a prime area where you may need it.

@jrjohnson I think it’s fair to raise the question of how the result does or doesn’t get reused across loops, but I think the question arises primarily because there has historically been a lot more magical behavior that requires you to figure out what kind of implicit context is being passed around. In today’s Glimmer components (and even more in tomorrow’s, with strict mode and template imports), that largely goes away, and the behavior of any given item becomes much more predictable.

In the case in question here, you can think of it as being basically equivalent to this—

let ids = rows.map((row) => uuidV3(row));

—where the template #each is like JS-side map. But that’s also suggestive: if you’re working with a robust modern UUID library, it also supports UUID v4, which is just a randomly generated library, and could just as well be this unless there’s a specific reason to provide a given hashable context for it (which there might or might not be):

let ids = rows.map((_) => uuidV4())

I’d be surprised if most devs coming newly to Glimmer templates would expect anything other than behavior akin to map here. And as regards teaching, I don’t think it’s particularly complicated here: if you pass it without an argument, you get a randomly generated ID; if you need to get the same value every time, you pass it a for= key and it’s stable on object identity. People can pick up UUID libraries and similar; I expect people can pick this up too. (Heck, people learned the Ember Component API and it’s massive!)

I’d also suggest that while a given ID per template is nice in some ways, it’s not necessarily the only thing someone would ever want—even if it’s the primary thing you might want personally. And that leads directly to my final note (and after that I’ll bow out unless someone has a specific question for me!): shipping a useful primitive lets others experiment and build on top of it easily, as suits their own needs. For example, if your team finds that you want a per-template ID, with the {{template-id}} syntax, that can straightforwardly be built in terms of an AST transform addon that sits on top of the underlying design proposed by @jgwhite, @hergaiety, and @josephdsumner—but the same is not true in reverse!

Over the last few years in Ember, we’ve increasingly come to prefer shipping a useful primitive and then layering abstractions on top of it as the utility of such abstractions becomes clear over time, rather than starting with the higher-level abstraction and then later struggling to decompose it. ({{action}} is probably the biggest offender here, but there are many other examples as well!) To me, this looks like a perfect place to do that again!

jgwhite commented 4 years ago
<table>
  {{#each this.rows as |row i|}}
    <tr>
      <td>
        <label for="{{template-id}}-box-{{i}}">Selected</label>
        <input id="{{template-id}}-box-{{i}}"} type="checkbox" />
      </td>
    </tr>
  {{/each}}
</table>

@jrjohnson this proposal is interesting. My one nitpick is that template-id suggests a value that is unique for the template, rather than for each invocation of the template.

What do you think about this?:

{{#let (unique-id) as |base-id|}}
  <table>
    {{#each this.rows as |row i|}}
      <tr>
        <td>
          <label for="{{base-id}}-box-{{i}}">Selected</label>
          <input id="{{base-id}}-box-{{i}}"} type="checkbox" />
        </td>
      </tr>
    {{/each}}
  </table>
{{/let}}

I'm biased, but I don't think the let block adds too much complexity in this context, and I don't think it's a hard concept to grasp.

I will concede, however, that argument-less (unique-id) doesn't look like a function call on first blush unless you're already familiar with lisp. It's also not obvious what id={{unique-id}} would do. One might suppose it would count as a helper invocation but my guess is that Glimmer treats it as a variable lookup, and that you'd have to use id={{(unique-id)}} which (let's face it) looks like a typo 😅.

hergaiety commented 4 years ago

I concur with @jgwhite that I don't think the let block adds much complexity.

Also wanted to say that we may not need to fix every potential case with this one helper, if we try to then this may live in limbo. I like @ijlee2's 3 assumptions to limit scope of the problem being solved:

We need to create IDs for Glimmer components (i.e. a solution that works for Octane and beyond). We want to create IDs only for elements inside a <form> element. We know on which elements we can set the id attribute versus the for attribute.

If a user needs to solution that doesn't fit well within this scope, they can continue to import guidFor in their JS and use it as they need. It's okay for this RFC to focus on solving for Accessibility instead of addressing every potential use-case, solving for A11y is a strong enough goal to stand on its own.

jelhan commented 4 years ago

I have the feeling that one main driver for the discussion here are issues with the let helper API. There seems to be strong concerns against any API that require usage of let helper. I'm wondering if there would be still the same objections after let helper API has been improved. E.g. by adding an inline form or by supporting multi-assignment. Maybe time would be better spend on improving that helper than on discussing how an API for guid-for needs to look like to not require let helper?

MelSumner commented 4 years ago

I don't mind the {{#let}} or any nested block solution. It makes a lot of sense to me as an Ember developer, and seems aligned with the other mental paradigms we teach, but I'd like the other @emberjs/learning-team-managers to weigh in here too if possible.

As with any syntax, of course developers can do weird things with it, but we minimize those risks by providing good documentation and sufficient linting rules to let them know what the well-lit path looks like.

Based on all of the conversations I've had on this particular subject, I'm endorsing a nested block solution for this issue.