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

Design questions about IDREF/Element attribute reflection #4925

Closed
alice opened this issue Sep 21, 2019 · 31 comments
Closed

Design questions about IDREF/Element attribute reflection #4925

alice opened this issue Sep 21, 2019 · 31 comments
Labels
accessibility Affects accessibility

Comments

@alice
Copy link
Contributor

alice commented Sep 21, 2019

Relating to #3515 and #3917.

Relevant spec language from the current PR:
https://whatpr.org/html/3917/common-dom-interfaces.html#reflecting-content-attributes-in-idl-attributes:element

We discussed this during the Web Components meeting at TPAC (minutes).

@zcorpan summarised some requirements for the design of the API:

  1. Removing and re-adding an element which is the subject of an attr-association should ideally not cause the association to be lost.
    • @rniwa does not agree with this.
    • The developer cost if this is not true would be needing to add a secondary mechanism for tracking these relationships, so that they can be re-added after removing and re-adding the element.
  2. The design should not compromise the encapsulation provided by shadow roots.
  3. The design should not allow inadvertent creation of circular references.
    • @rniwa, @annevk: Neither Simon nor I could recall the details of this concern; would either of you mind fleshing out an example to illustrate this risk?
  4. The web developer should be able to specify associations for the constituent elements within a component before inserting the elements in the DOM, and the associations should survive regardless of the relative order of insertion and the relative order of the elements in the tree
    • for example:
    let container = document.createElement('div');
    let a = document.createElement('span');
    container.appendChild(a);
    let b = document.createElement('input');
    container.appendChild(b);
    b.ariaLabelledByElements = [a];  // a precedes b in the tree
    document.body.appendChild(container);
  5. ??? - @annevk, @rniwa: I thought there were seven enumerated requirements, but there seem to only be 5 in the minutes. Do either of you remember what the others were?
  6. ???
  7. The design (probably obviously) needs to be implementable.

Here are what I think are the open questions:

  1. In general, when and how should an attr-association have distinct behaviour from a regular (expando) JavaScript property on an Element DOM object? i.e.
    a.foo = b;
    // vs
    a.ariaActiveDescendantElement = b;
  2. Should an attr-association be allowed to be created, or persist under circumstances when it can't have an effect? For example, a <label> for relationship where the <label> has been removed from the tree (or is yet to be added) and thus can't act as a label.
    • This potentially goes to (1) and (4) in the developer needs noted above.
  3. Should an attr-association be treated like a regular javascript reference for the purposes of garbage collection? i.e. if the attr-association is the last remaining reference to an Element object which has been removed from the tree, should that Element object be considered garbage?
    • This is moot if attr-associations do not persist when the Element is removed from the tree.
    • If the Element should be garbage collected, then should the attribute and the attr-association be removed when it is? Does that constitute revealing gc timing?
    • How would we detect this occurring?
  4. How should attr associations behave across Shadow DOM boundaries?
    • Current spec allows creating an association to any element which is a descendant of a shadow-including ancestor, i.e. in the same shadow scope or any of the enclosing ones.
    • Allowing an association to go into an unrelated shadow scope would potentially leak implementation details.
      • This is also the case for a.foo = b.
    • In particular, how can we avoid a leak in the case where an author does something like:
    a.ariaActiveDesendantElement = b;
    deeperShadowRoot.appendChild(b);
    • If we want to remove the attribute at that point, how do we detect the moment when it needs to be removed?
    • If not, how do we avoid leaking encapsulated details?
      • Do we conceal the association, presumably by special-casing the getter method?
      // continued from above
      console.log(a.ariaActiveDescendantElement);   // logs null
      document.body.appendChild(b);
      console.log(a.ariaActiveDescendantElement);   // logs b
      • How should this work for associations with lists of elements, like ariaOwnsElements?
      • Does the association still have an effect (for example, on the accessibility tree) if it is concealed?
@annevk annevk added the accessibility Affects accessibility label Sep 23, 2019
@annevk
Copy link
Member

annevk commented Sep 23, 2019

In the discussion we also had the constraint that the elements have the same node document. It would be hard to implement without cycle collector (https://blog.kylehuey.com/post/27564411715/cycle-collection has a reasonable introduction on the concept) otherwise. WebKit does not have a cycle collector. It's not entirely clear to me if same-document is good with HTML modules however. If you always cloned those elements into your document it's probably fine, but it's hard to predict usage patterns in advance.

@alice
Copy link
Contributor Author

alice commented Sep 24, 2019

In the discussion we also had the constraint that the elements have the same node document. It would be hard to implement without cycle collector otherwise.

Could you help me understand the connection there? What would the failure case look like?

@annevk
Copy link
Member

annevk commented Oct 21, 2019

Trying to address some of the open questions:

  1. attr-association attributes are different from JavaScript expandos in that they are typed and cause manipulation of a similarly/identically-named content attribute. And they are also part of the platform API and therefore need to follow the rules set out for those. (Hope this is what you meant.)
  2. I think this depends and it's hard to give a general rule. For your example, the label element will still have a for attribute as well. And we typically do not adjust content attributes.
  3. Not sure.
  4. Perhaps a model whereby we only enforce upon insertion of the second node or removal of any node could work? It's worse than IDs in some aspects, but also better in some others. Perhaps this also argues that ID references and Element references should be decoupled somewhat, if they are not already.

@alice
Copy link
Contributor Author

alice commented Oct 30, 2019

Thanks for the answers, and sorry for being so slow to respond.

(Hope this is what you meant.)

Yes, thank you!

[Unlike JavaScript expandos] they are also part of the platform API and therefore need to follow the rules set out for those.

What rules are you referring to?

the label element will still have a for attribute as well.

Sorry, I should have been clearer here. I was referring to a potential API for for which uses element references, e.g.

<label id="label">Address:</label>
<input id="input" type="text">
label.for = input;

This would cause the label element to "sprout" a for attribute, like this:

<label id="label" for="input">Address:</label>
<input id="input" type="text">

If we then removed input from the DOM, would calling label.for return input, or null? (Presumably the content attribute would not be affected.)

Perhaps this also argues that ID references and Element references should be decoupled somewhat, if they are not already.

Currently they are reasonably closely coupled, cf. the example above, and also the reverse direction e.g. (starting from the original html example above)

label.setAttribute("for", "input");
console.log(label.for);   // logs a reference to the input element

However, there are some caveats:

  1. The content attribute is set once when the IDL attribute is set, but does not change as a result of any changes to the element. For example:
    label.for = input;
    console.log(label.getAttribute("for"));  // "input"
    input.removeAttribute("id");
    console.log(label.getAttribute("for"));  // still "input"
    console.log(label.for);   // still the input element
  2. If the content attribute is set directly, the IDL attribute getter effectively does the same as getElementById() each time. For example:
    label.setAttribute("for", "input");
    console.log(label.for);   // logs the input element
    input.removeAttribute("id");
    console.log(label.for);   // null

Perhaps a model whereby we only enforce upon insertion of the second node or removal of any node could work?

I need to think about this some more.

@annevk
Copy link
Member

annevk commented Oct 30, 2019

What rules are you referring to?

https://w3ctag.github.io/design-principles/ for instance.

This would cause the label element to "sprout" a for attribute, like this:

This seems problematic if there's also an element with ID input, no? I think it would have to be a magical value or no value (or would have to add an ID to the target and use that).

@alice
Copy link
Contributor Author

alice commented Oct 30, 2019

I’m not clear on what is problematic, could you clarify? (Sorry for not quoting properly, replying from phone)

@annevk
Copy link
Member

annevk commented Oct 30, 2019

Well, the for attribute is supposed to take an ID. Tools/scripts operating on the node tree might reasonably have that assumption. A new API that aside from an internal relation also puts the target's local name in the for attribute would confuse any such tools/scripts.

@alice
Copy link
Contributor Author

alice commented Oct 30, 2019

Hm, I think my example was confusing - the input had an ID of input. It’s not using the local name in general.

@annevk
Copy link
Member

annevk commented Oct 30, 2019

Ah, my bad. I don't really have a good sense for how to best overload the for attribute setter. What would happen if the input element did not have an ID? Would it get a generated one? (That seems somewhat interesting to me, but it doesn't work well for elements inside a shadow tree pointing to the light tree.)

@alice
Copy link
Contributor Author

alice commented Oct 30, 2019

for is just an example - just wanted to get away from assuming this is all about ARIA for a moment.

That being said, in answer to your question: if the ID can't be used (e.g. if an element doesn't have one, or it's in a different ID scope), the for attribute (or whatever attribute we're talking about) would be set to the empty string:

input.removeAttribute("id");
label.for = input;
console.log(label.getAttribute("for"));   // ""

If you remove the attribute, it removes the association as well:

label.removeAttribute("for");
console.log(label.for);   // null

@alice
Copy link
Contributor Author

alice commented Nov 12, 2019

I wrote up some options for moving forward: https://gist.github.com/alice/174ae481dacdae9c934e3ecb2f752ccb

@rniwa
Copy link

rniwa commented Nov 22, 2019

Just to keep you all posted, I've been thinking about this problem more & consulting with more WebKit engineers about the potential implementation challenges. We're entering Thanksgiving break soon but we hope to give back a feedback soon.

@alice
Copy link
Contributor Author

alice commented Dec 9, 2019

Here's an idea, from a discussion with @justinfagnani.

We could introduce a concept of an opaque Element (or Node) reference. This would be a handle to an Element/Node which could be used in place of an Element/Node for this type of API.

You could have a Document-level factory method, something like:

document.getOpaqueReference(el);

which would return a "weak" reference to the element, say OpaqueReference.

Like uniqueID, this would return a consistent unique reference each time it is called on the same Element/Node.

Then, if something has access to something inside of a Shadow root, it can use this method to avoid leaking a reference when setting up a relationship.

class XContainer extends HTMLElement {
  constructor() {
    super();
    let shadow = this.attachShadow({mode: 'open'});
    let input = document.createElement('input');
    shadow.append(input);

    const inputRef = document.getOpaqueRef(input); 
    this.ariaActiveDescendant = inputRef;
  }
  
  clear() {
    // inputRef is weak, so this doesn't cause a memory leak:
    this.shadowRoot.innerHTML = '';
  }
}

(In this instance, setting the relationship on XContainer's ElementInternals object would also work, but this demonstrates that even without that there is no leak of Shadow DOM internals.)


Justin points out that, assuming an OpaqueReference is serializable, this could also be used to pass element references to worker contexts.

@rniwa
Copy link

rniwa commented Dec 9, 2019

So OpaqueReference is a weak reference to an element? How does one get the element out of OpaqueReference? Call some method on Document?

@alice
Copy link
Contributor Author

alice commented Dec 9, 2019

How does one get the element out of OpaqueReference? Call some method on Document?

I would imagine it would be a one-way street: if you call getOpaqueReference() again you'll get the same (===) reference, but you can't use the OpaqueReference to get back a reference to a Node/Element.

The User Agent, however, could use it to look up the Node/Element internally.

@justinfagnani
Copy link

+1

If there's a use case for it, maybe there can be an API on ShadowRoot or ElementInternals to dereference an OpaqueReference from that element's shadow root. This would allow an element to send out and receive back a reference and do something valuable with it.

@rniwa
Copy link

rniwa commented Dec 23, 2019

Sorry about the delayed response.

We've carefully reviewed the plausibility of implementing option (1) in https://gist.github.com/alice/174ae481dacdae9c934e3ecb2f752ccb.

Implementing option (1) in WebKit would require a significant and complex rearchitecture of the way DOM objects are managed with our garbage collector. Since this is the only feature which requires such a rearchitecting, we’re not interested in pursuing this approach.

We're also concerned that allowing cross-document referencing would result in accidental whole document leaks. Using nodes from another global object has other kinds of hazards like instanceof not working and prototype object being different.

Given this, we don't think we should proceed with option (1).

@alice
Copy link
Contributor Author

alice commented Jan 22, 2020

We met today to discuss the implementation issues mentioned in the above comment.

It seems like some (but not all) of the issues are resolved if we disallow cross-document references.

I added a discussion of the options we discussed to the gist - @rniwa could you possibly take a look and check I've accurately represented what we talked about?

I also added a brief comment to the Option 1 "Cons" list to try and capture the gist of the issue with cross-document references:

Cross-document references may cause memory leaks in practice, due to a Node effectively having two owner Documents (its actual owner Document, and the owner Document of the node which has the reference)

Does that roughly capture the issue we discussed?

@asurkov
Copy link

asurkov commented Feb 6, 2020

It seems like some (but not all) of the issues are resolved if we disallow cross-document references.

I'm supportive to no cross-document references since 1) this is a simple design: easy to understand and easy to implement and 2) it matches aria-activedescendant behavior. Indeed, if aria-activedescendant element is moved to another document then all its references are recomputed I believe: if ID exists in a new document, then reference is updated to new element, if ID doesn't exist, then value is considered invalid.

However I'm not clear on what happens/should happen with references when aria-acivedescendant element is moved to another document. Do the references are simply dropped or does behavior match aria-activedescendant, i.e. if a new document has an element with ID matching an old referred element ID, then use that element. The latter behavior perhaps is obscure, so I suppose I'd vote to simply drop references.

It seems like some (but not all) of the issues are resolved

Which issues are left?

@alice
Copy link
Contributor Author

alice commented Feb 25, 2020

@asurkov Nice to hear from you!

I'm not clear on what happens/should happen with references when aria-acivedescendant element is moved to another document. ... Do the references are simply dropped or does behavior match aria-activedescendant, i.e. if a new document has an element with ID matching an old referred element ID, then use that element. The latter behavior perhaps is obscure, so I suppose I'd vote to simply drop references.

I agree, I would expect references to be dropped... unless there is an element in the new document which has a "classic" style IDREF attribute (i.e. set via content/setAttribute()).

e.g.

el.setAttribute('aria-activedescendant', 'one');
el.appendChild(document.adoptNode(iframe.getElementById('one')));

Which issues are left?

I can't remember, unfortunately.

@asurkov
Copy link

asurkov commented Feb 26, 2020

@asurkov Nice to hear from you!

thank you!

I'm not clear on what happens/should happen with references when aria-acivedescendant element is moved to another document. ... Do the references are simply dropped or does behavior match aria-activedescendant, i.e. if a new document has an element with ID matching an old referred element ID, then use that element. The latter behavior perhaps is obscure, so I suppose I'd vote to simply drop references.

I agree, I would expect references to be dropped... unless there is an element in the new document which has a "classic" style IDREF attribute (i.e. set via content/setAttribute()).

e.g.

el.setAttribute('aria-activedescendant', 'one');
el.appendChild(document.adoptNode(iframe.getElementById('one')));

I suppose it's aligned with ARIA spec, so agreed no point to change the behaviour. Not sure though I can see a valid use case for this.

Which issues are left?

I can't remember, unfortunately.

My understanding is this is the only issue left to sort out to resolve all concerns for the spec, and get the green light for implementations. Thus curious, how this issue can be resolved. What is the process?

@rniwa
Copy link

rniwa commented Mar 20, 2020

Sorry for the delayed response.

We met today to discuss the implementation issues mentioned in the above comment.

It seems like some (but not all) of the issues are resolved if we disallow cross-document references.

I added a discussion of the options we discussed to the gist - @rniwa could you possibly take a look and check I've accurately represented what we talked about?

I agree those are two good options to pursue.

However, the example for WeakReference version is not right. WeakReference doesn't become null just because it got moved to a different document. Instead, it depends on GC and can be null in the future if the element is no longer kept alive by anyone. So e.g.

const iframe = document.body.appendChild(document.createElement('iframe'));
const doc = iframe.contentDocument;
let el = doc.body.appendChild(document.createElement('div'));
// not keeping a JS reference to the newly created element
el.ariaActiveDescendantElement = doc.body.appendChild(doc.createElement('div'));  
console.log(el.ariaActiveDescendantElement);  // definitely logs newly created element from above

iframe.remove();
setTimeout(() => {
    // may or may not return null depending on whether the finalization has happed or not.
    console.log(el.ariaActiveDescendantElement);
}, 1000);

@alice
Copy link
Contributor Author

alice commented Mar 26, 2020

I created a new gist to capture the "1.1" proposal from the old gist, which we all seem to agree on.

I filed a new issue #5401 to track the discussion around shadow roots.

One remaining issue is the issue of garbage collection:

  • Should an attr-association be treated like a regular javascript reference for the purposes of garbage collection? i.e. if the attr-association is the last remaining reference to an Element object which has been removed from the tree, should that Element object be considered garbage?
    • This is moot if attr-associations do not persist when the Element is removed from the tree.
    • If the Element should be garbage collected, then should the attribute and the attr-association be removed when it is? Does that constitute revealing gc timing?
    • How would we detect this occurring?

@dot-miniscule
Copy link

dot-miniscule commented Sep 15, 2020

To sum up discussion points for the F2F meeting:

We have a core use case for allowing attr-associated Elements to be a descendant of a "host" Element's shadow including ancestor (i.e relationships can cross into a "lighter" shadow DOM, but cannot penetrate a "darker" shadow DOM).

The common motivating use case for this is an autocomplete widget that has a HTMLInputElement in shadow DOM, with the specific autocomplete options given in light DOM, and the author wants to set the active descendant of the input element to be one of the options in light DOM.

<custom-combobox> 
 #shadow-root (open)
  |  <!-- this doesn't work! -->
  |  <input aria-activedescendant="opt1"></input>
  |  <slot></slot>  
<custom-optionlist>
    <x-option id="opt1">Option 1</x-option>
    <x-option id="opt2">Option 2</x-option>
    <x-option id='opt3'>Option 3</x-option>
 </custom-optionlist>
</custom-combobox>

Note that for this use case, IDREFS are insufficient in order to set the relationship.

There has so far been consensus that an Element should only be allowed to be an attr-associated Element  if it belongs to the same document as the "host" Element (the one that has the relationship set). Element Reflection was discussed at TPAC last year, and we have a few open questions to address:

  1. What is the attr-associated element when the scope differs?
    For the above example, where el is the input element inside the shadow-root
console.log(el.ariaActiveDescendantElement); // ???
console.log(el.getAttribute('aria-activedescendant')); // ?
  1. If the only reference to an Element is via the IDL attribute, should it be a candidate for garbage collection? Does removing the content attribute when an attr-associated element is garbage collected reveal GC timing?
let el = document.body.appendChild(document.createElement('div'));
el.ariaActiveDescendantElement = document.body.appendChild(document.createElement('div'));
el.ariaActiveDescendantElement.remove(); // do we gc now?
console.log(el.ariaActiveDescendantElement);  // ???
console.log(el.getAttribute('aria-activedescendant'));  // ???
  1. What do we expose to AT? Should this be in sync with JS?
let el = document.body.appendChild(document.createElement('div'));
el.ariaActiveDescendantElement = document.createElement('div');
console.log(el.ariaActiveDescendantElement);  // ???

@JanMiksovsky
Copy link

We discussed this as a group yesterday, and captured notes in the agenda document.

In the discussion, we considered the scenario A→B, where node A references node B through element reflection, and B does not reference A. The conclusions of the discussion were:

  • If A→B, and B is not in in the document, then A’s reference returns null until B is added to the document. The corresponding getAttribute will return the empty string. Even if A holds the only reference to node B, B will be kept alive and not garbage collected. In this state, if A’s reference to B is explicitly set to null, the reference is cleared.
  • If A→B, and B is moved to another document, that’s an implicit signal that the author intends to clear A's reference to B. The content attribute is removed. A’s reference returns null.
  • If the page author sets A.ariaFooElement = null, then A’s ref is set to null, and the content attribute is removed.
  • Likewise, if the page author calls A.removeAttribute("aria-foo"), then A’s reference is set to null.
  • If the page author calls `A.setAttribute("aria-foo", ""), i.e., sets to empty string, that also clear's A's reference.

If anyone who attended the meeting feels I haven't captured things correctly, please chime in.

@alice
Copy link
Contributor Author

alice commented Sep 16, 2020

  • If A→B, and B is not in in the document, then A’s reference returns null until B is added to the document. The corresponding getAttribute will return the empty string. Even if A holds the only reference to node B, B will be kept alive and not garbage collected. In this state, if A’s reference to B is explicitly set to null, the reference is cleared.

What was the reasoning for this decision, as opposed to returning B?

@rniwa
Copy link

rniwa commented Sep 16, 2020

  • If A→B, and B is not in in the document, then A’s reference returns null until B is added to the document. The corresponding getAttribute will return the empty string. Even if A holds the only reference to node B, B will be kept alive and not garbage collected. In this state, if A’s reference to B is explicitly set to null, the reference is cleared.

What was the reasoning for this decision, as opposed to returning B?

The idea was that when B is not in a document, AT won't see / can't use the fact B being referenced by A so we'd like to match what AT sees in this case and return null. This was mostly based on the preference of web developers working on various aspects of accessibility.

@alice
Copy link
Contributor Author

alice commented Sep 16, 2020

This was mostly based on the preference of web developers working on various aspects of accessibility.

Was that discussion minuted? It would be helpful to understand the context and trade-offs here.

@alice
Copy link
Contributor Author

alice commented Sep 16, 2020

Also, was there any consensus reached on how element references will interact with Shadow DOM?

@JanMiksovsky
Copy link

Also, was there any consensus reached on how element references will interact with Shadow DOM?

Just made a comment on Issue 5401 with the results of that conversation.

@zcorpan
Copy link
Member

zcorpan commented Jun 30, 2022

#7934 was merged; closing.

@zcorpan zcorpan closed this as completed Jun 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility Affects accessibility
Development

No branches or pull requests

8 participants