Closed Bug 1738221 Opened 4 years ago Closed 4 years ago

HTMLSlotElement.assign(...NodeList) should be HTMLSlotElement.assign(NodeList)

Categories

(Core :: DOM: Core & HTML, defect)

Firefox 93
defect

Tracking

()

RESOLVED INVALID

People

(Reporter: orstavik77, Unassigned)

Details

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:93.0) Gecko/20100101 Firefox/93.0

Steps to reproduce:

Both mdn and whatwg spec says that the HTMLSlotElement.prototype.assign() method should take a sequence of Nodes as its first argument. Currently, the HTMLSlotElement.prototype.assign() must use the rest operator and destructering to list the arguments.

https://jsfiddle.net/oq7vs0xy/

<shopping-list>
  <li>bread</li>
  <li>butter</li>
</shopping-list>


<script>
  const template = `
<ol>
  <li>diapers</li>
  <li>beer</li>
  <li>milk</li>
  <li>more beer</li>
  <li>passifiers</li>
  <li>don't forget beer</li>
  <slot></slot>
</ol>
`;

  class ShoppingList extends HTMLElement{
    constructor(){
      super();
      this.attachShadow({mode: 'open', slotAssignment: 'manual'});
      this.shadowRoot.innerHTML = template;
    }
    
    connectedCallback(){
      //this works
      this.shadowRoot.querySelector('slot').assign(...this.childNodes);
      //this throws an Error
      this.shadowRoot.querySelector('slot').assign(this.childNodes);
    }
  }

  customElements.define('shopping-list', ShoppingList);
</script>

Actual results:

The first call to slot.assign illustrates how the HTMLSlotElement.assign(...this.childNodes) currently works.

The second call to slot.assign(this.childNodes) throws a bug.
Uncaught TypeError: HTMLSlotElement.assign: Argument 1 could not be converted to any of: Element, Text.

The second call is in line with MDN and the spec.

The behavior in FF is the same as in Chrome Ubuntu Version 94.0.4606.81 (Official Build) (64-bit). But older versions of Chrome worked as the spec illustrated.

Expected results:

The slot.assign(this.childNodes) should work.

I think the real issue here is if the slot.assign method should use
a) nodes as individual arguments (...this.childNodes) or (el1, el2),
b) send in an iterable list of nodes as the first argument (this.childNodes) or ([el1, el2]) or
c) do both: if the first argument is iterable, then do b), otherwise do a). To make it work like c), one can add the following monkeypatch.

(function () {
//monkeypatch to fix the slot.assign destructor destruction.
const desc = Object.getOwnPropertyDescriptor(HTMLSlotElement.prototype, "assign");
const og = desc.value;
desc.value = function assign(...args) {
typeof args[0][Symbol.iterator] === 'function' && (args = args[0]);
return og.call(this, ...args);
}
Object.defineProperty(HTMLSlotElement.prototype, 'assign', desc);
})();

I think it is not good to only do b). I think that either a) or c) should be chosen. Why?

  1. slot.assign is a library method. I do not think that library methods should demand "the understanding of the rest operator for arguments" of its users. That will make life harder for beginners than it strictly needs to be. A beginner should be able to use the whole library without knowing the rest operator syntax. I like rest operator, but I didn't as a beginner.
  2. maybe the need for a second option to slot.assign might appear in the future? like addEventListener(type, cb, suddenlyOneNeededMoreArguments). I think finding a new name for that function is just as good as having options available, but the issue is there.
  • Personally, I like the destructering b). It is a nice feature when you only need a couple of nodes by reference. But it was a puzzle to uncover the api worked like that. I attempted to fix the issue in 5-6 other ways first, before I stumbled upon the "trick". It surprised me. yeah, console.log() does it, but slot.assign? So, I like c).

I can reproduce the issue using the latest Nightly 96.0a1 on macOS Big Sur 11.6.1.
Moving this to DOM: Core & HTML component so that our dev team could take a look over it (if this is not the right component, please move it to a more suitable one).
Thanks!

Component: Untriaged → DOM: Core & HTML
Product: Firefox → Core

Seems like this should be raised as a spec issue? The spec matches our implementation: https://html.spec.whatwg.org/#dom-slot-assign

Flags: needinfo?(orstavik77)

The old Chrome had non-specified behavior.
https://github.com/whatwg/html/pull/6561#issuecomment-826391595
If you think the spec should change, please file a spec bug. If the spec doesn't change, Gecko's behavior shouldn't change either.

Status: UNCONFIRMED → RESOLVED
Closed: 4 years ago
Resolution: --- → INVALID
Flags: needinfo?(orstavik77)

Hi there!
I'm trying to reach the dragons that are guarding the treasure, the web? I'm just visiting your castle, but I happened to stumble upon a little situation... You see, I saw this janitor down the hall with what seemed like a golden ring in his pocket. I don't think he is a thief or anything, it actually looked like the ring most likely had fallen into his pocket by accident. But when I told him about it, he didn't seem to understand what I was talking about. And just said "it's fine, it's nothing, it's just a coke bottle cap". But to me, that actually looks like a golden ring, one of those special ones.

Could you maybe take a second look? Last I saw the janitor was about to walk out of the castle, ring in pocket.

PS: Just ask if you needinfo on why/how the principle "you can always both call&apply every JS function" is important treasure. Happy to contribute :)
PPS: Thanks for bugzilla!

Flags: needinfo?(emilio)
Flags: needinfo?(bugs)
Flags: needinfo?(annevk)

There are clearly people with stronger opinions on this than me :)

You don't seem to mean call as in Function.prototype.call, right? You mean something more abstract like "can I achieve the same result via non-spread function calls"?

Anyways as I said, I don't have much of an opinion on this. Maybe we can change the idl to be able to take sequences or what not, but again that's something to be discussed on GitHub, and there seems there's been active discussion in that issue.

Flags: needinfo?(emilio)

yeah, changing this is a spec level thing. Once the spec has been changed the implementations will follow. That is the time to file implementation bugs.

Flags: needinfo?(bugs)
Flags: needinfo?(annevk)
You need to log in before you can comment on or make changes to this bug.