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

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

Closed
steveszc opened this issue Apr 3, 2020 · 56 comments
Closed

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

steveszc opened this issue Apr 3, 2020 · 56 comments

Comments

@steveszc
Copy link

steveszc commented Apr 3, 2020

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
Copy link
Contributor

pzuraq commented Apr 3, 2020

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
Copy link
Author

steveszc commented Apr 3, 2020

@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
Copy link
Author

steveszc commented Apr 6, 2020

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
Copy link

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

@rwjblue
Copy link
Member

rwjblue commented Apr 8, 2020

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
Copy link
Member

rwjblue commented Apr 8, 2020

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
Copy link

jrjohnson commented Apr 8, 2020

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
Copy link
Contributor

Gaurav0 commented Apr 8, 2020

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

@jelhan
Copy link
Contributor

jelhan commented Apr 16, 2020

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
Copy link
Contributor

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
Copy link
Contributor

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

@jelhan
Copy link
Contributor

jelhan commented Apr 18, 2020

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
Copy link
Contributor

Gaurav0 commented Apr 19, 2020

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
Copy link
Contributor

chriskrycho commented Apr 19, 2020

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
Copy link

lifeart commented May 8, 2020

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

@lougreenwood
Copy link

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
Copy link

ro0gr commented May 14, 2020

@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
Copy link
Contributor

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

@jgwhite
Copy link
Contributor

jgwhite commented Jun 25, 2020

For completeness, @hergaiety, @josephdsumner, and I put together a twiddle demonstrating @rwjblue’s proposal in #612 (comment).

@chriskrycho
Copy link
Contributor

chriskrycho commented Jun 25, 2020

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. 😂)

{{#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
Copy link

jrjohnson commented Jun 25, 2020

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
Copy link
Contributor

chriskrycho commented Jun 25, 2020

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
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link

ro0gr commented Jun 25, 2020

@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
Copy link
Contributor

@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
Copy link
Contributor

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.

@elwayman02
Copy link
Contributor

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.

@elwayman02
Copy link
Contributor

@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
Copy link

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
Copy link
Contributor

pzuraq commented Jul 28, 2020

@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
Copy link

@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 pzuraq closed this as completed Jul 29, 2020
@pzuraq
Copy link
Contributor

pzuraq commented Jul 29, 2020

@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 pzuraq reopened this Jul 29, 2020
@pzuraq
Copy link
Contributor

pzuraq commented Jul 29, 2020

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
Copy link
Member

ijlee2 commented Jul 29, 2020

Is it fair to assume these 3 things?

  • 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 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
Copy link
Contributor

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
Copy link
Contributor

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

@pzuraq
Copy link
Contributor

pzuraq commented Jul 29, 2020

@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
Copy link
Member

ijlee2 commented Jul 29, 2020

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
Copy link
Contributor

pzuraq commented Jul 29, 2020

@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
Copy link
Member

ijlee2 commented Jul 29, 2020

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

@chriskrycho
Copy link
Contributor

chriskrycho commented Jul 29, 2020

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
Copy link
Contributor

jgwhite commented Jul 29, 2020

<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
Copy link
Contributor

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
Copy link
Contributor

jelhan commented Jul 29, 2020

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
Copy link
Contributor

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.

@ijlee2
Copy link
Member

ijlee2 commented Jul 29, 2020

I'm also okay with using {{let}} helper (having nested block) to create a scoped ID.

As for updating the learning materials, I think we'll want to update the code examples at these 2 places:

If the solution ends up being a helper, I think we can also mention it in:

@MelSumner
Copy link
Contributor

@ijlee2 thank you for doing some of the "how do we teach this" pre-work. @steveszc do you feel like you have a clear path forward for the RFC?

@steveszc
Copy link
Author

steveszc commented Aug 12, 2020

@MelSumner I need to give another pass through all the comments but I do think there is enough consensus here to get an RFC authored. I'll aim to get a first draft put together in the next week or two. My intent is to propose the helper-and-let-based solution that has been discussed.

I think the discussion in this issue has been super valuable, thanks to everyone that gave their input!

@steveszc
Copy link
Author

@rwjblue Hoping to briefly revisit this name discussion...

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"}}?

I definitely agree that guid-for is too in the weeds. I think unique-id works pretty well, and folks seemed to gravitate toward unique-id in the issue discussion here.

However, one other possible name I wanted to bring up for some discussion is the name id.

Although the name unique-id is obviously a more descriptive name and would be my personal preference for an addon or userland helper, there is quite a bit of precedent among the existing built-in helpers for favoring terse names over more descriptive names. Looking at the existing list of built-in helpers I think the name id will fit much more naturally in that list. It's terseness also has the benefit of being immediately obvious and easy to remember.

I think the biggest question for discussion is whether the concept of uniqueness is essential to the name of this helper.

@MelSumner
Copy link
Contributor

I'd prefer unique-id because I think it will decrease confusion. It's super frustrating to me when things are named the same but then don't work the same way.

@sandstrom
Copy link
Contributor

Can this be closed now that #659 is merged?

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

No branches or pull requests