-
-
Notifications
You must be signed in to change notification settings - Fork 407
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
Dynamic tags in glimmer templates #389
Conversation
162452e
to
2303f8d
Compare
text/0000-dynamic-tag-names.md
Outdated
|
||
I think that this repetition makes templates harder to read, so I advocate that only simple bindings | ||
can be passed in. If the user wants to use something more complex than a binding, they could use the `let`/`with` helpers | ||
for that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking of how I used to use tagName
, I believe disallowing expressions would cause a lot of boilerplate. The old tagName
always defaulted to div
(or the tagName
that was set on the component class), which was quite handy when you only sometimes needed to explicitly set a tagName
from the invoking template.
Keeping this ability would be really nice. One can either easily do this by allowing an expression like you've shown.
The alternatives would either entail the {{#with}}
construct or a computed property on the component class. Both feel boilerplatey to me.
However, I do agree that, if the same expression would be repeated multiple times in the template, or if it is ridiculously complex, users should not inline it.
Instead of outright banning this, we could also add such a linting rule to ember-template-lint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the same expression would be repeated multiple times in the template
Oh, I forgot about the closing tag. Maybe we could allow some sort of "infering" for the closing tag?
<{{or @tagName "div"}}>
</>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered it, but it looks ugly to me.
I know that at this point glimmer templates don't have to respect html rules, but the expectation that opening and closing tag are identical is very engrained in my brain.
2303f8d
to
674921f
Compare
There's a potential risk of dynamic tag names. Changing the tag name from the outside might break HTML conventions or cause conflicts between default styles of the browser and styles defined by the component. I feel like the component being solely responsible for its semantics is good practice. Maybe these use cases can be solved by lifting the container element from the component's template?
|
I'm not sure I follow. Do you mean that changing from a span to a button can break styles? If so, I'd say that it's totally expected and not an issue with dynamic tag names, but with how browsers behave. But it's up to the user. Tbh in my use cases the tagName is never changed in runtime once set. Maybe that is acceptable too. I'll add that to the open questions |
We had actually considered a very similar syntax for component invocation in the past (as an alternative to the current angle bracket syntax). I agree that if we want to support something like this, this is the most immediately intuitive syntax. My biggest concern is how much complexity supporting dynamic tag names pushes into Glimmer VM. Unlike almost everything else, the DOM really really really does not want you to treat tag name as mutable. Most things that look dynamic in a template—like binding an attribute, for example—are relatively simple to implement because we can call Making How would you feel about the constraint that tag name could be dynamic, but immutable? That is, you could set tag name based on a runtime value, but if that property changed after the component was rendered, it would produce a runtime error. This would make the implementation much simpler, and would eliminate many of the edge cases I'm worried about. I also agree with @buschtoens on the concerns about arbitrary expressions and closing tags. For me, the rule is:
Unfortunately, this rule is difficult to apply in this scenario. The proposed syntax looks like an expression, so it would be very confusing why On the other hand, |
@tomdale That's exactly what I was thinking when I said:
I was updating the RFC to propose that tagName is dynamic but unbound. I was aware that tagName is not a property you can change on a DOM node, and changing it would mean creating a whole new element. It totally seems a good compromise. I'll update the RFC. About allowing expressions in tagNames, I was concerned because of ugliness, nothing else. What do think is that the opening and closing tags must have the exact same expression, not just one that resolves to the same value. For instance: <{{or @tagName "div"}}
The content
</{{if @tagName @tagName "div}}> Both expression are semantically equivalent, but they are not the ideantical. I propose that this would not even compile instead of waiting for runtime to see if expression result on the same value. |
a3b5824
to
f87795d
Compare
Have you considered modeling this as a dynamic component invocation instead? {{#let (element "li") as |Tag|}}
<Tag ...>...</Tag>
{{/let}} The |
@chancancode That is not super different to what I proposed in the alternatives: <DynamicElement @tagName={{@tagName}} class="some-class" ...attributes>
</DynamicElement> I still prefer to interpolate inside the just after the open markers. I believe it reads better and it's what I'd have naively tried if I was a newcomer to Ember. What I care more about is about having the feature. If there is technical reasons at the parser/VM level that make that syntax problematic and we should have a built-in component or helpers I can live with it. |
I don't think interpolation of expressions should be allowed in tagName. Sure, it could lead to boilerplate, but I'd hope with glimmer templates, changing the tagName in more the exception than the rule, so, using computeds should be ok |
I think there are several reasons that I think the “interpolation” syntax is problematic:
I published ember-element-helper based on my comment above. I encourage everyone to try it out and see if this works well for the use cases we have in mind in the real world. If it works out, we can implement this natively which would eliminate any performance concerns. I am open to adding an |
@chadhietala for what I can see, this approach requires relies on the tagged nature of
You've convinced me that having a dedicated helper or component is probably a better solution. What I'm not convinced yet is if this All things said is that this not a feature that is used all the time, so I could live with an inconvenient syntax if that makes things clearer or the implementation easier. |
Having just come to this specific proposal but having thought on this a lot b/c we do occasionally use dynamic tags… I'm inclined to say the explicitness of |
I really like @chancancode's suggestion here as it allows for addon authors to explore the usefulness of this pattern. I think that if this ends up being a very popular, perhaps we can consider adding syntax that basically does an expansion to what @chancancode's addon does but elides the component creation. |
@cibernox Correct. The "user land" implementation relies on I agree with you an others that this is sort of a power-user feature that should be used rarely, so at the end of the day ergonomics probably doesn't matter so much. To elaborate on my (weakly-held) position that the teardown behavior/expectation is more expected with this setup: {{#let (element this.tagName) as |Tag|}}
<Tag>hello world</Tag>
~~~ since this thing is dynamic, it makes sense to me that when this changes, everything goes away
{{/let}}
{{!-- whereas... --}}
<Element @tagName={{this.tagName}} as |Tag|>
~~~~~~~~ it is less expected that changing this will make the whole outer thing go away
hello world
</Element> I am still open to changing my mind on that one, since it is more of a heuristic than a rule. You can easily write a component that violates that mental model today, I just personally think it's uncommon and probably a bad pattern, so I'm less inclined to adopt that kind of API in core. |
I think you changed mine. I'll update my proposal. {{#let (element (unbound this.tagName)) as |Tag|}}
<Tag>hello world</Tag>
~~~ since this thing is dynamic, it makes sense to me that when this changes, everything goes away
{{/let}} |
We've written a fairly large-scale dynamic form implementation that simply would not be possible without dynamic tags.
And this could be comfortably handled with the |
@JonForest 👍 that's a pretty useful data-point! when you get a chance, I would love for you to try the add-on in your app to confirm that it works for your use cases. |
I'm pretty 👍 on the current rendition of this, thanks for iterating towards such a nice solution! |
Ideas, feel free to add to list or claim: - [ ] ```I've been getting a lot of questions about how tree-shaking is coming along. I would be willing to train anyone that wants to help on what's already done and what still needs to be done. Disclaimer: It's a lot of work! #emberjs #EmberFest``` https://twitter.com/kellyselden/status/1050717338595745792 - [ ] Include summary from pixelhandler (and cc him) #issue-triage team: https://discordapp.com/channels/480462759797063690/480523776845414412/500377829452546058 - [x] From jenweber on #support-ember-times: Another times topic: emberjs/ember.js#16910 I would summarize as, "ember.js has an addition to the test suite to help with API documentation! In the past, you might have noticed that a small number of methods had missing docs. When code gets moved around in ember.js, such as putting functions in their own modules, it's easy to make mistakes that impact the documentation parser. This test builds the docs and checks them for unintentional changes. Thanks <link to ed> for the test and to everyone who helped fix missing docs over the years." Todd Jordan did the majority of the fixes when things got dropped, I think. Hard to say for sure 🔏 @kennethlarsen - [x] emberjs/rfcs#384 (:lock: @jessica-jordan) - [x] emberjs/rfcs#386 (:lock: @jessica-jordan) - [ ] emberjs/rfcs#387 (:lock: @jessica-jordan) - [x] emberjs/rfcs#388 (:lock: @Alonski) - [ ] emberjs/rfcs#389 - [x] EmberConf brainstorming session date, if confirmed (:lock: @amyrlam) - [ ] Hacktoberfest roundup? - [ ] Check https://github.com/jessica-jordan/whats-new-in-emberland - [x] [ember-self-focused](https://engineering.linkedin.com/blog/2018/10/making-ember-applications--ui-transitions-screen-reader-friendly) (🔒 @chrisrng ) - [ ] ... Ideas we are waiting on: - [ ] EmberCamp talks, deep dive? http://embercamp.com/ and https://github.com/ember-chicago/ember-camp-2018-notes ... Maybe we wait for the talk videos to be published? Keep an eye on #ember-camp in Discord. not out yet - [ ] GraphQL with Ember? https://emberfest.eu/schedule/#rocky-neurock Or maybe we can reach out to them for a post-EmberFest writeup? See also the blog post: https://medium.com/kloeckner-i/ember-and-graphql-8aa15f7a2554
text/0000-dynamic-tag-names.md
Outdated
|
||
## How we teach this | ||
|
||
The new helpers must be documented with examples in the API and Guides. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing we are trying hard to do better is make sure that RFCs include enough information to the "how we teach this" section to be an MVP documentation for the guides. In order for this RFC to move forward, some more content would need to be added here.
Additionally, this RFC seems like it will give folks a lot more surface area to do detrimental things re: a11y, and it would be important to address this in this section, even if it's more of a warning to authors to make sure they keep that in mind. 👍
@hakubo today on the core team call we confirmed the modifier restriction for components is unintentional. I'll write up a quick amendment RFC to clarify that today and lift that restriction in Ember soon. |
Today we discussed this on the core team call, we felt that it was pretty close, but as @MelSumner mentioned we felt that we can flesh out the teaching section more. In addition to what @MelSumner said (examples, accessibility), @wycats and @krisselden also felt that there are probably some small XSS risk with this and we should at minimum add some warning to the teaching section. |
Thinking about it a little, I think that there is some correctness risk with accepting arbitrary tag names (for example I'd like to restrict the values to element names that look like simple HTML tags: lowercase tag names (including dashes); no colons, no sigils, etc. Given these risks, it is also probably good to find something to say in the documentation about the risks, but that doesn't need to hold up the RFC. |
@MelSumner @chancancode I've updated the RFC implementing the feedback! |
text/0000-dynamic-tag-names.md
Outdated
Example: | ||
|
||
```hbs | ||
{{#let (element "li") as |Tag|}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe (element this.tagName)
– using a literal string here is a little pointless in this example
text/0000-dynamic-tag-names.md
Outdated
``` | ||
Since the element name can be defined in runtime, in order to protect the user from XSS vulnerabilities, | ||
the passed tag name cannot contain anything but lowercase letters and dashes. Tag names containing | ||
other characters (p.e `xlink:`) will throw an error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e.g.
?
text/0000-dynamic-tag-names.md
Outdated
## How we teach this | ||
|
||
If this RFC is accepted, Ember would get a new built-in helper to be added to `component`, `array`, `each` and in | ||
[Ember.Templates.helpers](https://emberjs.com/api/ember/release/classes/Ember.Templates.helpers). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation will need a at least somewhat meaningful and self-contained example to illustrate the point of the helper (and how to use it). Could you provide one? I assume you have some examples from your addons that could be distilled down.
text/0000-dynamic-tag-names.md
Outdated
[Ember.Templates.helpers](https://emberjs.com/api/ember/release/classes/Ember.Templates.helpers). | ||
|
||
Since this feature is not very commonly used I think that updating the section of the API docs above is enough, | ||
no need to mention it in the guides. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since using something like dynamic tags could have a serious impact on accessibility, I think it's very important to mention this in the documentation.
I can see dynamic tags being used by developers who believe they are being very clever, but in fact are being quite careless due to their ignorance. Especially considering places that don't encourage developer education, such as enterprise environments - where the end product has an even higher chance of being used by a large number of users, and as such, a larger percentage of folks who need things to be accessible.
The more I think about this RFC the more I feel very uneasy about it- I think the very least we can do is ensure that we inform developers of the potential foot guns associated with dynamic tag use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MelSumner do you have some examples of the potential foot guns you have in mind that @cibernox can incorporate them into the text here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chancancode good idea. I can help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chancancode @cibernox here's the general idea of what I think should be included, at a minimum- whether or not this is in the guides or the API could perhaps be discussed via the learning core team (although I would probably recommend a reference in both). This is a pretty high risk area, imo.
How We Teach This
It's vitally important that developers have awareness the negative impact that misuse of dynamic tags can have on the accessibility of an Ember application.
While dynamic tags enable a great deal of flexibility in components, it's essential to recognize that these can also negatively impact accessibility. Developers should be keenly aware of the appropriate role that should be applied per HTML element (as specified in the WAI-ARIA specification), and ensure that the role is also updated, if necessary, as the tag name is changed. In some cases, the use of native, semantic HTML elements may eliminate the need to apply a role at all, so developers should consult the WAI-ARIA specification until they are certain of the correct use for their specific use cases.
text/0000-dynamic-tag-names.md
Outdated
If this RFC is accepted, Ember would get a new built-in helper to be added to `component`, `array`, `each` and in | ||
[Ember.Templates.helpers](https://emberjs.com/api/ember/release/classes/Ember.Templates.helpers). | ||
|
||
Since this feature is not very commonly used I think that updating the section of the API docs above is enough, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming the current structure of the guide, this should fit into the Components > Customizing a Component's Element
section. @jenweber can weigh in on where it will fit in the new guides (though I don't think it's necessary this RFC's job to hypothesis that).
While I agree that 1) the current coverage of "what goes into API docs vs guides" for template helpers is a bit ad-hoc and inconsistent, 2) it is not good to just basically duplicate the same information twice in both places, I think there are some good guiding principles of what goes where:
-
API docs is narrowly focused on a single helper. The objective is "so, you've figured out you want to use this helper, let me teach you how". It can cover things like the arguments it takes, return value, examples, (to an extent) use cases it solves, caveats/cautions and so on.
-
Guides should be more topical, focusing on the general use case as well as cross-cutting concerns. The objective is "so, you have a problem, you may not know how to solve it yet so let me present to you some options". It serves more of a discovery role, and can showcase different ways of doing the same thing, the pros and cons of each, examples, but can defer to the API docs for the specific details.
So, I think the point of including this in the guides is that, it is quite likely that you are aware of the problem but didn't doesn't yet know about the element
helper as the solution you are looking for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(That's only my personal take)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @chancancode. My own recommendation would be to briefly describe it in the guides, and link to the API docs from within the Components > Customizing a Component's Element
section. The bulk of the explanation and use case with example would then go in the API docs, to avoid duplication. We are trying to move towards using the Guides to show how things fit together in real apps, rather than covering the API surface.
text/0000-dynamic-tag-names.md
Outdated
|
||
For accessibility and semantic reasons, sometimes a `<div>` may not be the best kind of tag. | ||
We might want the panel to be a `<section>` element, or an `<aside>` when it's content is somewhat unrelated | ||
with the rest of the page. Or maybe a `<legend>` if it's and the end of a form. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add something like
With the
element
helper proposed in this RFC, this can be accomplished with something like this:{{#let (element @tagName) as |Tag|}} <Tag class="pt-10 pb-10 ps-20 box-shadow" ...attributes> {{yield}} </Tag> {{/let}}
text/0000-dynamic-tag-names.md
Outdated
We might want the panel to be a `<section>` element, or an `<aside>` when it's content is somewhat unrelated | ||
with the rest of the page. Or maybe a `<legend>` if it's and the end of a form. | ||
|
||
## Detailed design |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest a reordering of the paragraphs.
We propose to add a new
element
helper that takes a tag name and generates a contextual component that, when invoked, renders the selected element.Example:
...
Unlike ids, classes or other attributes, the tag name of DOM element cannot be changed in runtime. ...
To help the user understand that changing the tag of an element in runtime is an expensive operation, the syntax is chosen such that...
Since the element name can be defined in runtime, in order to protect the user from XSS vulnerabilities, ...
...
text/0000-dynamic-tag-names.md
Outdated
|
||
```hbs | ||
{{!-- sidebar.hbs --}} | ||
{{#let (element this.htmlTag) as |Tag|}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{{!-- sidebar.hbs, a template-only component --}}
{{#let (element @htmlTag) as |Tag|}}
This way, we can eliminate the classic component class, both to simplify the example, and to avoid the awkwardness of "couldn't you just use tagName
"?
I'm aware that we will loose the default aside
tag name here, but maybe that's okay for the purpose of this example. Otherwise, something like {{#let (element (if @htmlTag @htmlTag "aside")) as |Tag|}}
would be fine also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If ember-truth-helpers where part of core, the pattern I use the most is:
{{#let (element (or @htmlTag "aside") as |Tag|}}
<Tag ...attributes>...</Tag>
{{/let}}
That's why I resorted to having a JS file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I think for the sake of the example, either not caring about having a default, using the if
dance, or using a hypothetical or
is all fine. However, using tagName: ''
and essentially re-implementing the feature in user land is.... causing distraction at minimum. Classic component classes doesn't really have the problem that motivates the feature imo – my understanding is that we are designing this feature to backfill something we used to be able to do there, so using classic components to illustrate it doesn't quite work imo.
699341e
to
aff3d25
Compare
Another update. |
@cibernox sorry for the delay (due to EmberConf, Octane, etc). I'll nominate this for FCP as soon as we resume the regular meetings (next Friday, probably). |
Having another read through, I would like if this RFC had a full example of what it looks like for someone to update the ARIA roles, as the tag changes. We will have to write those docs and this goes best if the content is more detailed, so that people who are unfamiliar with the problem space can help out. |
We discussed this on the framework team meeting today and believe this is ready for FCP! |
@jenweber it kind of depends on the use case. 1. If, a component is internally doing the switch based on some argument, it would probably be something like this: {{#let (element this.tagName) as |Tag|}}
<Element role={{this.role}} ...attributes>...</Element>
{{/let}} class MyComponent extends GlimmerComponent {
get tagName() {
return this.args.simulated ? 'div' : 'a';
}
get role() {
// unsure if there are problems with just always adding the "link" role,
// even when using a native "a" element
return this.args.simulated ? 'link' : undefined;
}
} It's kind of contrived, but the intent of this example is to switch between using a native "a" tag or using making a different element and make "simulated link" instead. Generally, I think there are very few (if any) good use cases for this where using the {{#if @actAsButton}}
<button (extra button specific things here)>...</button>
{{else if @actAsSwitch}}
<button (extra switch specific things here)>...</button>
{{else}}
<a (extra link specific things here)>...</button>
{{/if}} To share the content implementation (the "...") here, extract this wrapper part into a separate component instead. 2. If, a component is intending to allow the caller to customize the element, it should be the caller's responsibility to provide the tag name as long as the aria roles and other appropriate aria attributes: {{#if @customTag}}
{{#let (element @customTag) as |Tag|}}
<Tag ...attributes>...</Tag>
{{/let}}
{{else}}
<aside ...attributes>...</aside>
{{/if}} Here the |
Rendered