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

Specify polymorphic reflection for elements. #3925

Closed
wants to merge 5 commits into from

Conversation

alice
Copy link
Contributor

@alice alice commented Aug 15, 2018

@domenic Forked from #3917 - let me know if I should pull these changes in there instead.


/common-dom-interfaces.html ( diff )
/forms.html ( diff )

Copy link
Member

@domenic domenic left a 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)
Copy link
Member

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

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

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

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

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

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

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

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

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

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

@minorninth
Copy link

@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.

@alice
Copy link
Contributor Author

alice commented Aug 15, 2018

@minorninth In this version, at least, it's not accurate that it no longer reflects. It reflects the same way that htmlForElement reflects in #3917 reflects (ID if possible, empty string if not).

@alice
Copy link
Contributor Author

alice commented Aug 15, 2018

@domenic

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.

What if it always returned the htmlFor-associated element, whether explicit or implicit?

However, then you'd still run into the backwards-compatibility issue.

@domenic
Copy link
Member

domenic commented Aug 15, 2018

Hmm, my take would be:

  • 3925: the htmlFor IDL attribute will give the value of the for="" content atttribute if the for="" content attribute is set and nobody has ever set htmlFor to an element; otherwise, it will give the element it was last set to, assuming that element was in the right tree.
  • 3917: the htmlFor IDL attribute will give the value of the for="" content attribute, unless the htmlForElement IDL attribute has been set in which case it will give that element's ID if it has one. The htmlForElement attribute will give back what it was set to, assuming that element was in the right tree.
  • Third alternative I've mentioned a few times: The htmlFor IDL attribute will give the value of the for="" content attribute. The htmlForElement IDL attribute will give back what it was set to, assuming that element was in the right tree. If set, htmlForElement takes precedence over htmlFor.

The third alternative seems a simpler; the first two seem of similar complexity to me.

What if it always returned the htmlFor-associated element, whether explicit or implicit?

However, then you'd still run into the backwards-compatibility issue.

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 HTMLElement?.

@minorninth
Copy link

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.

@alice
Copy link
Contributor Author

alice commented Aug 15, 2018

i.e. this option?

@alice alice closed this Oct 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants