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

customElements.define should throw on registration of definitions that shadow built-in properties / methods. #583

Closed
bicknellr opened this issue Oct 7, 2016 · 15 comments

Comments

@bicknellr
Copy link

bicknellr commented Oct 7, 2016

At some point in the future, it would be nice to allow custom elements to override methods and properties used as part of APIs that are currently defined natively. However, it is currently possible to override these methods on a custom element, but these methods aren't actually be used by the native code when performing those operations. I think the custom elements registry should throw when custom element authors register definitions that shadow these APIs so that they could one day be opened up as real extension points without worrying about breaking existing code.

For example, take Element's setAttribute method. A custom element without the above restriction can override this method and provide their own behavior. Typical non-native code will automatically use the custom element author's version of setAttribute. However, without the internals also calling this extension for every setAttribute operation, the internal implementation has to be rationalized by the custom elements author as calling something like Element.prototype.setAttribute.apply(node, [name, value]) rather than node.setAttribute(name, value), which is really odd.

I'm under the impression that making these properties real extension points is very challenging from an architectural standpoint in any given browser, but I think it's the ideal behavior. Enforcing this restriction would at least prevent custom element authors and consumers from starting to use non-native extensions to these methods before the browser implementors could use them and, in particular, prevents authors from relying on the browser not using them.

@bicknellr
Copy link
Author

Possibly a better twist: the native constructors themselves could throw when called if that constructor detects that the object being created has shadowed a banned property. This would allow each individual constructor to check for its specific properties and prevents the custom elements registry from taking on this responsibility.

@annevk
Copy link
Collaborator

annevk commented Oct 8, 2016

Why would you need to change setAttribute? It's behavior is identical everywhere.

@bicknellr
Copy link
Author

After giving it a few days, I think I've mostly talked myself out of this. The concern was less about why it would be useful but how the implementation has to be justified given what's observable from the outside. Custom elements are going to take something that I thought might have been obscure enough to be changed for the better and put a spotlight on it. Maybe there are lots of people out there that patch over these properties on built-in elements? I suppose this is why the lifecycle callbacks are permanently stored during definition.

@annevk
Copy link
Collaborator

annevk commented Oct 10, 2016

I still don't understand.

@bicknellr
Copy link
Author

So, the custom elements spec allows authors to extend HTMLElement and I can put a setAttribute function on my custom element's prototype. However, when HTML containing my custom element and a few attributes gets parsed, my definition of setAttribute isn't called. So, there must be another unexposed thing for setting attributes that the browser uses when parsing because it's not just making a call like element.setAttribute(x, y). The intent of this issue was to prevent people from expecting a particular behavior so that it might be possible in the future to do things like define parsing in terms of the public API of the elements being created. However, I get the feeling that this ship might have already sailed if people already regularly do things like put their own setAttribute on native elements.

This:

<my-element a="b" c="d"></my-element>

doesn't have the same result as this:

let e = document.createElement('my-element');
e.setAttribute('a', 'b');
e.setAttribute('c', 'd');

if my-element is defined as:

customElements.define('my-element', class extends HTMLElement {
  setAttribute() {
    // Do nothing.
  }
});

@annevk
Copy link
Collaborator

annevk commented Oct 10, 2016

There's a low-level operation for setting an attribute on an element, true, but it's not used for parsing the value necessarily.

@bicknellr
Copy link
Author

Sure, setAttribute might not have anything to do with responding to the new value. I just wanted to point out that the difference in the two is strange from the perspective of someone who thinks they're just extending HTMLElement.

The parser can't be explained as

let e = document.createElement('my-element');
e.setAttribute('a', 'b');

instead, it has to be closer to

let e = document.createElement('my-element');
Element.prototype.setAttribute.apply(e, ['a', 'b']);

and custom elements make that much more obvious.

@trusktr
Copy link

trusktr commented Nov 23, 2016

After giving it a few days, I think I've mostly talked myself out of this.

No, you're right, don't talk yourself out of this. I think it is important that we honor The Extensible Web Manifesto.

  • Expose low-level capabilities that explain existing features, such as HTML and CSS, allowing authors to understand and replicate them.
customElements.define('my-element', class extends HTMLElement {
  setAttribute() {
    // Do nothing.
  }
});

^ That should in fact do nothing! If it did nothing, it would explain the web better, which is important. We can achieve this by (re)writing the native interfaces so that they behave as if they were written in JavaScript.

@bicknellr bicknellr reopened this Jun 5, 2017
@bicknellr
Copy link
Author

I hope it's not too late to bring this up again but I think its worth revisiting. The point is the same, this is about allowing the things the browser does internally to be described using the same APIs as those which are exposed to users. Particularly, by allowing custom element authors to shadow built in API, it becomes implicitly true that the internal implementation of the browser (the parser's tree construction, stuff that elements defined in the HTML spec do, etc.) can't be described in these terms and makes rationalizing the observable results more complicated.

@bicknellr
Copy link
Author

No, I don't expect that browsers would want to literally implement their internals in JavaScript; preparing to oust native implementation of the DOM in C++ (for example) is absolutely a non-goal here. However, if tree building during parsing (for example) was defined in terms of DOM operations through the JavaScript API it would be much cleaner but allowing custom element authors to shadow these APIs while the browser doesn't actually call them for these operations means that this cleaner description will never be possible in the future.

@rniwa
Copy link
Collaborator

rniwa commented Jun 5, 2017

Given that Chrome and Safari have already shipped custom elements v1 API, I don't think we can make such a backwards incompatible change now.

@bicknellr
Copy link
Author

I think its worth considering if its possible but who knows if people have already started relying on this behavior - I hope not. :/

@trusktr
Copy link

trusktr commented Jun 7, 2017

who knows if people have already started relying on this behavior

Relying on which behavior?

@bicknellr
Copy link
Author

@trusktr

If we let authors override setAttribute, for example, in a custom element definition but the parser doesn't call their element's setAttribute while constructing trees, then we can't ever define tree construction that way because authors will already have written code assuming that the parser doesn't do this. I don't want authors to be able to rely on that assumption and prevent that redefinition.


It's important to note that this behavior currently is observable without being able to define custom elements but it's fairly unusual. For example:

<div>
  <script>
    const parentNode = document.currentScript.parentNode;
    const overriddenMethod = parentNode.insertBefore;
    parentNode.insertBefore = function(...args) {
      console.log("This will never be logged.");
      return overriddenMethod.call(this, ...args);
    };
  </script>
  <div>The `insertBefore` above is never called with this child.</div>
</div>

or

const elt = document.createElement('div');
const overriddenMethod = elt.setAttribute;
elt.setAttribute = function(...args) {
  console.log("This will never be logged.");
  return overriddenMethod.call(this, ...args);
};
elt.id = "abc"; // The `setAttribute` above isn't called when `id` is set.

However, I suspect that authors don't do this kind of thing particularly often and this behavior might be breakable without much consequence. Maybe this assumption isn't correct?

@annevk
Copy link
Collaborator

annevk commented Sep 4, 2017

I'm going to close this since no user agent would want to roundtrip all treebuilding through JavaScript. That would incur massive performance penalties. And I seriously doubt it would be web-compatible.

@annevk annevk closed this as completed Sep 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants