-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Specify polymorphic reflection for elements. #3925
Conversation
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.
Very interesting. This seems to work OK; the biggest issue is that now web developers aren't really sure what el.htmlFor
will return. It could return an element, it could return a string. It seems like they'll have to always check which it is before they use it, at which point I'm not sure what we've gained over having two separate properties.
It's also a slight backward-compatibility concern. Not in the sense of breaking any existing code, but if you mix new code (which does el.htmlFor = someElement
) with old code (which assumes el.htmlFor
is always a string), things could get messy.
Also worth noting this doesn't seem to solve our out-of-sync problems mentioned in #3515 (comment), right?
So on balance I guess I find #3917 the better solution. What do you think?
then it has a relationship with the reflected content attribute as follows: | ||
|
||
<ul> | ||
<li><p>The element <var>hostElement</var> is said to have a(n) |
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.
Just "an" is fine
|
||
<ul> | ||
<li><p>The element <var>hostElement</var> is said to have a(n) | ||
<var>attr</var>-associated element, which may be implicit or explicit:</p> |
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 cross-referencing for this is tricky. I think you'll want to wrap this in <dfn>
, then reference it as
<span data-x="attr-associated element"><code data-x="">htmlFor</code>-associated element</span>
That should work...
set the content attribute to <var>newValue</var>.</p></li> | ||
|
||
<li><p>If <var>newValue</var> is an instance of <var>Interface</var>, | ||
run the following algorithm:</p> |
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.
s/run the following algorithm/then
<li><p>Set the <var>attr</var>-associated element of <var>hostElement</var> | ||
to null.</p></li> | ||
|
||
<li><p>Let <var>newElement</var> be <var>newValue</var>.</p></li> |
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 think this aliasing doesn't buy us anything; we can just use newValue, right?
<li><p>If <var>newElement</var> is not in the same tree as <var>hostElement</var>, | ||
nor the same tree as <var>hostElement</var>'s shadow-including root, | ||
then remove the content attribute and return. | ||
</p></li> |
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.
Cross-reference tree and shadow-including root. No newline/spaces before </p></li>
whose <span data-x="concept-ID">ID</span> is the value | ||
of that <code data-x="attr-id">id</code> attribute, | ||
then let <var>id</var> be the value of that <code data-x="attr-id">id</code> attribute. | ||
</p></li> |
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.
No newline/spaces before </p></li>
<code>label</code> element itself.</p> | ||
<code>label</code> element's <dfn>labeled control</dfn></span>, | ||
either using the <code data-x="attr-label-for">for</code> attribute, | ||
or by putting the form control inside the <code>label</code> element itself.</p> |
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.
This just changed the wrapping, right? Better to not do this if possible.
</li> | ||
|
||
<li><p>On setting the IDL attribute to a new value, <var>newValue</var>:</p> | ||
<ul> |
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.
ol, not ul.
<ul> | ||
<li><p>The element <var>hostElement</var> is said to have a(n) | ||
<var>attr</var>-associated element, which may be implicit or explicit:</p> | ||
<ul> |
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.
ol, not ul
then it has a relationship with the reflected content attribute as follows: | ||
|
||
<ul> | ||
<li><p>The element <var>hostElement</var> is said to have a(n) |
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.
1-space indents throughout, not 2-space
@domenic I think both solutions are workable and similar in complexity to implement and spec. However, I think this solution (the polymorphic one) is far simpler to explain in a concise, casual way for friendly documentation like MDN. 3925: The 'for' attribute reflects as htmlFor. However, you can also assign an Element directly to htmlFor, in which case it no longer reflects. Did I get that right? That seems really easy to explain, 99% of developers will intuitively figure out how it behaves based on that explanation. In comparison, I find it harder to explain 3917 in a concise way. It's consistent and logical once you understand it, but it just requires a lot more explanation. 3917: The 'for' attribute reflects as htmlFor, and the element it references is reflected in htmlForElement. Modifying htmlForElement sets the 'for' attribute to the id of the element if it has an unambiguous id, or to the empty string otherwise. |
@minorninth In this version, at least, it's not accurate that it no longer reflects. It reflects the same way that |
What if it always returned the However, then you'd still run into the backwards-compatibility issue. |
Hmm, my take would be:
The third alternative seems a simpler; the first two seem of similar complexity to me.
Right, and then we'd actually be breaking old web pages, not just breaking new-code/old-code mixes. (Because we'd instantly change htmlFor so that it never returns strings like it used to, even in old code.) If we were to go that route (not for htmlFor, but for new stuff like ARIA attributes) it might be worth not even accepting strings at all, so that the type is just |
I'd be fine with the third alternative. Simple is good. I think sometimes we're worrying too much about corner cases that are almost never going to matter. Developers who have a good need for htmlForElement will use it, there will rarely be an id on the target element, so the rest of the details don't matter. Same with the aria relationship attributes. |
i.e. this option? |
@domenic Forked from #3917 - let me know if I should pull these changes in there instead.
/common-dom-interfaces.html ( diff )
/forms.html ( diff )