selectedcontent post-connection steps did not break out of the loop

[annevk]

And also has an unused ancestor variable.

---

/form-elements.html ( diff )

[josepharhar]

I think we should move the below break into this "otherwise" condition. If we always break when the ancestor is a select element, then we can't detect the nested select elements case.

[josepharhar]

This is also what the chromium implementation does

[annevk]

Good point, I think this is lacking test coverage.

[annevk]

Although if we end up finding ourselves in a nested situation, why do we still run update select's selectedcontents on the innermost select?

[josepharhar]

Good point, we should not run that in this case. Can we add a check for selectedcontent-disabled in the line which returns early when nearestAncestorSelect is null or has the multiple attribute?

Good point, I think this is lacking test coverage.

I am hoping that this is exhaustive enough: https://chromium-review.googlesource.com/c/chromium/src/+/7125591

[dbaron]

If I'm understanding things correctly, I think there's an undesirable side-effect of this break. (I got here from reviewing @josepharhar's CL that implements this behavior.)

In particular (and hopefully I'm rememembering how all this stuff works correctly, since I'm not checking all the pieces with the spec as I go along), if we start with:

<select id="s">
  <button>
    <selectedcontent></selectedcontent>
  </button>
  <option selected>One</option>
</select>

then the <selectedcontent> will initially be filled with "One".

Now, suppose that JS comes along and does:

  const sc = document.createElement("selectedcontent");
  const o = document.createElement("option");
  o.append(sc);
  document.getElementById("s").prepend(o);

At this point, I think we're going to:

1. invoke these post-connection steps being modified in this PR
2. break at this break statement
3. Because of that break, leave the existing <selectedcontent> element with the "One" contents inside of it.

However, after this change, the "get a select's enabled selectedcontent" algorithm will now return null, because the <selectedcontent> added by the JS above is disabled. This means that as a result of this break, we'll be leaving the previously-working <selectedcontent> element with stale content, permanently. That seems pretty weird to me (though I admit that every case where a <selectedcontent> is disabled is effectively an error case).

[dbaron]

That said, now that I understand @josepharhar's comment on the rationale here, maybe we do need to break if we hit an ancestor selectedcontent, because otherwise the algorithm will go into an infinite loop. So maybe we want to change this so this particular break is only when the ancestor is a selectedcontent element?

[dbaron]

Also... I think something is wrong here too.

In particular, this "Break" now means that the "If nearestSelectAncestor is null" two steps earlier in this (most nested) list is meaningless because it will always be null, because we're going to break out of the loop once we make it non-null.

I think something different is intended here, but I'm not entirely sure what.

I do see Chromium has a code comment saying "If there are multiple ancestor selects, then cloning can lead to infinite loops, so disable this element." which might be the underlying rationale for not breaking here?