Update all the Web IDL #2053

Zirro · 2017-11-17T21:22:11Z

With a combination of automated and manual work, I have gone through and updated all the Web IDL files in the nodes/ directory. The initial commit only includes the changed IDL, but there are a number of changes to the code that will need to be made before this is merged. I'll be looking closer at making those soon, as well as updating the remaining IDL in other directories.

In the meantime I would appreciate help in reviewing the updated IDL.

TimothyGu

Magnificent work! 👍

Zirro · 2017-11-18T21:36:35Z

As a way for me to keep track of the code that'll need to be changed, as well as make it easier to write the changelog later - here is a list of the changes resulting from this which might cause any noticeable difference for jsdom users:

Document

createElement() gained a second optional argument
createElementNS() gained a third optional argument
HTMLScriptElement changed to HTMLOrSVGScriptElement
URL, documentURI, origin, referrer and cookie changed from DOMString to USVString

GlobalEventHandlers

Removed onshow
Added onsecuritypolicyviolation

HTMLBaseElement

href changed from DOMString to USVString

HTMLCanvasElement

probablySupportsContext() and a stub for setContext() was removed

HTMLElement

focus() gained an optional argument

HTMLEmbedElement

src changed from DOMString to USVString

HTMLFormElement

Added OverrideBuiltins and LegacyUnenumerableNamedProperties
action changed from DOMString to USVString
length changed from long to unsigned long

HTMLFrameElement

src and longDesc changed from DOMString to USVString

HTMLIFrameElement

longDesc changed from DOMString to USVString

HTMLImageElement

src, srcset, currentSrc, lowsrc and longDesc changed from DOMString to USVString
hspace and vspace changed from long to unsigned long

HTMLInputElement

files is no longer readonly and should get a setter
src changed from DOMString to USVString

HTMLMediaElement

src and currentSrc changed from DOMString to USVString
~~play() returns a promise~~ - no need to change as it is notImplemented()

HTMLMenuElement

Removed the type and label attributes

HTMLModElement

cite changed from DOMString to USVString

HTMLObjectElement

data changed from DOMString to USVString

HTMLQuoteElement

cite changed from DOMString to USVString

HTMLSourceElement

src changed from DOMString to USVString

HTMLTrackElement

src changed from DOMString to USVString

HTMLVideoElement

Added the playsInline attribute
poster changed from DOMString to USVString

Node

baseURI changed from DOMString to USVString

WindowEventHandlers

Added onmessageerror

snuggs · 2017-11-19T09:19:42Z

🎉 🎉 BRAVO @Zirro @TimothyGu. This is 🦄 ✨

domenic

Some comments about things that got added that we don't actually support, but excited about the majority of changes.

domenic · 2017-11-19T20:03:42Z

lib/jsdom/living/nodes/Document.webidl

  HTMLCollection getElementsByTagNameNS(DOMString? namespace, DOMString localName);
  HTMLCollection getElementsByClassName(DOMString classNames);

-  [NewObject] Element createElement(DOMString localName);
-  [NewObject] Element createElementNS(DOMString? namespace, DOMString qualifiedName);
+  [CEReactions, NewObject] Element createElement(DOMString localName, optional ElementCreationOptions options);


I don't know if we should add this until we actually support custom elements.

@domenic I thought of this last night and concur 100%. Perhaps we can roll this into #1030? Seems to be a touch stale but great progress nonetheless. Can take the enhancements from here and push that PR forward with these changes if you recommend this to be a good strategy.

domenic · 2017-11-19T20:04:14Z

lib/jsdom/living/nodes/Document.webidl

+
+// https://html.spec.whatwg.org/#document
+enum DocumentReadyState { "loading", "interactive", "complete" };
+typedef (HTMLScriptElement or SVGScriptElement) HTMLOrSVGScriptElement;


How does this work given that we don't implement SVGScriptElement?

domenic · 2017-11-19T20:04:41Z

lib/jsdom/living/nodes/HTMLElement.webidl

-  attribute long tabIndex;
-  void focus();
+  [CEReactions] attribute long tabIndex;
+  void focus(optional FocusOptions options);


I'm not sure we should add this given that we don't implement it.

domenic · 2017-11-19T20:05:35Z

lib/jsdom/living/nodes/HTMLFormElement.webidl

-[OverrideBuiltins]
+[Exposed=Window,
+ OverrideBuiltins,
+ LegacyUnenumerableNamedProperties,


Given that we haven't added the named/indexed properties for the form element, I'm not sure we should add these.

Zirro · 2017-11-19T20:56:55Z

Thanks for the review! I'm still in the process of going over the updated IDL to determine what needs to change on the code side, what can be implemented quickly and what needs to be reverted. Indeed, for the four lines you highlighted I think I'll end up placing them behind a comment and adding a new line with the old IDL.

I've already completed updates for the rest of the IDL files outside the nodes/ directory, but I think I might save them for a new pull request as this is large enough of a project already.

Before this is merged, I'll need to add a USVString reflector to webidl2js. I'm using this in a local branch now:

module.exports.USVString = {
  get(objName, attrName) {
    return `
      const value = conversions["USVString"](this.getAttribute("${attrName}"));
      return value === null ? "" : value;
    `;
  },
  set(objName, attrName) {
    return `this.setAttribute("${attrName}", V);`;
  }
};

...but that doesn't cover the URL reflection portion of the spec, which will probably require a new extended attribute. Implementing that might not happen until the next weekend though as I'll be pretty busy.

domenic · 2017-11-19T21:04:39Z

Awesome! Let me assign this to you for now so I remember whose plate it is on.

snuggs · 2017-11-19T09:21:12Z

lib/jsdom/living/nodes/HTMLInputElement-impl.js

@@ -205,6 +206,12 @@ class HTMLInputElementImpl extends HTMLElementImpl {
    }
    return this[filesSymbol];
  }
+  set files(value) {
+    if (this.type === "file" && value !== null) {
+      this[filesSymbol] = value;


Great catch here @Zirro ! Actually ran into this myself earlier this year. Could explain failing test. Or perhaps EBKAC error. ;-)

snuggs · 2017-11-19T09:24:48Z

lib/jsdom/living/nodes/ChildNode.webidl

+//  [CEReactions, Unscopable] void before((Node or DOMString)... nodes);
+//  [CEReactions, Unscopable] void after((Node or DOMString)... nodes);
+//  [CEReactions, Unscopable] void replaceWith((Node or DOMString)... nodes);
+  [CEReactions, Unscopable] void remove();


My favorite diff @Zirro. Been attempting to wrangle up a list of CEReactions myself. This will help with the custom element iteration for jsdom. Still a long way to go tho but there is progress in one of the branches.

Zirro · 2017-11-24T21:11:39Z

This is ready for a full review now 🎉

snuggs

Overall great work! Added some thoughts. I am currently updating MDN documentation and this helped. Also cleared up a couple impl conventions I was trying to wrap my head around.

snuggs · 2017-11-24T23:51:08Z

lib/jsdom/living/nodes/ChildNode.webidl

+  [CEReactions, Unscopable] void before((Node or DOMString)... nodes);
+  [CEReactions, Unscopable] void after((Node or DOMString)... nodes);
+  [CEReactions, Unscopable] void replaceWith((Node or DOMString)... nodes);
+  [CEReactions, Unscopable] void remove();


snuggs · 2017-11-24T23:52:45Z

lib/jsdom/living/nodes/Document.webidl

-  readonly attribute DOMString origin;
+  readonly attribute USVString URL;
+  readonly attribute USVString documentURI;
+  readonly attribute USVString origin;


I must not have gotten that memo (aka never knew) about USVString "replacing" DOMString. Where can I catch up on this conversion @Zirro? Thanks in advance.

This looks like a good place to start: whatwg/html#1282

snuggs · 2017-11-25T00:30:34Z

lib/jsdom/living/nodes/Document.webidl

@@ -13,20 +13,23 @@ interface Document : Node {

  readonly attribute DocumentType? doctype;
  readonly attribute Element? documentElement;
-  HTMLCollection getElementsByTagName(DOMString localName);
+  HTMLCollection getElementsByTagName(DOMString qualifiedName);


Nice catch on the qualifiedName. Allowed me to read up a bit in the spec on the algorithm. Am interested because traditionally this is fast(er) internally.

The irony I just realized is methinks the API should have been .getElementsByQualifiedName or .getElementsByLocalName. IIRC The .tagName of an element is an uppercased version of the qualified name.

document.getElementsByTagName('SECTION')[0].tagName // SECTION // We are actually getting by qualifiedName // document.getElementsByQualifiedName('section')[0].tagName // SECTION

Taken from Step 2.a in the getElementsByTagName algorithm

Otherwise, if root’s node document is an HTML document, return a HTMLCollection rooted at root, whose filter matches the following descendant elements:

Whose namespace is the HTML namespace and whose qualified name is qualifiedName, in ASCII lowercase.

Which is the reason why .getElementsByTagName('SECTION') would return a collection of <section>s but not <SECTION>.

Although the algo is correct. Seems user land has developed an antipattern.

// Common convention is getting by qualified name document .getElementsByTagName ('section')[0] // <section>...</section> .tagName // SECTION

@TimothyGu am I mistaken in this API misnomer? Am a little rusty on HTML namespace exclusion which removes forced casing.

Just some thoughts not suggestions as they would go over to the dom spec.

snuggs · 2017-11-25T00:46:29Z

lib/jsdom/living/nodes/Document.webidl

+//  [CEReactions, NewObject] Element createElement(DOMString localName, optional ElementCreationOptions options);
+//  [CEReactions, NewObject] Element createElementNS(DOMString? namespace, DOMString qualifiedName, optional ElementCreationOptions options);
+  [CEReactions, NewObject] Element createElement(DOMString localName);
+  [CEReactions, NewObject] Element createElementNS(DOMString? namespace, DOMString qualifiedName);


Curious. Why does .createElementNS use qualifiedName however .createElement uses localName? The Namespaces validate and extract algorithm are a little vague after reading thrice. I clicked on Name link and brought me to a W3C page. @domenic is this correct navigation outside the spec?

https://dom.spec.whatwg.org/#namespaces

snuggs · 2017-11-25T00:49:16Z

lib/jsdom/living/nodes/Document.webidl

@@ -113,7 +127,7 @@ partial interface Document {
  [SameObject] readonly attribute StyleSheetList styleSheets;
 };

-// http://w3c.github.io/page-visibility/
+// https://w3c.github.io/page-visibility/#extensions-to-the-document-interface


snuggs · 2017-11-25T01:06:38Z

lib/jsdom/living/nodes/HTMLDialogElement.webidl

-//  void close(optional DOMString returnValue);
+//  [CEReactions] void show();
+//  [CEReactions] void showModal();
+//  [CEReactions] void close(optional DOMString returnValue);


Great comment refactor 🏆 @Zirro

snuggs · 2017-11-25T01:07:52Z

lib/jsdom/living/nodes/HTMLFormElement.webidl


  [SameObject] readonly attribute HTMLFormControlsCollection elements;
-  readonly attribute long length;
+  readonly attribute unsigned long length;
 //  getter Element (unsigned long index);
 //  getter (RadioNodeList or Element) (DOMString name);

  void submit();


Any idea why .submit has no [CEReaction] ?

snuggs · 2017-11-25T01:10:58Z

lib/jsdom/living/nodes/HTMLTableElement.webidl

-  void deleteCaption();
-  attribute HTMLTableSectionElement? tHead;
+  [CEReactions] void deleteCaption();
+


@domenic do we need extra spacing here? I know you've mentioned this to me before.

This is the way it looks in the spec. We would prefer to make as few changes to the original formatting as possible to make updates like this one easier in the future.

snuggs · 2017-11-25T01:11:30Z

lib/jsdom/living/nodes/WindowEventHandlers.webidl

 interface WindowEventHandlers {
  attribute EventHandler onafterprint;
  attribute EventHandler onbeforeprint;
  attribute OnBeforeUnloadEventHandler onbeforeunload;
  attribute EventHandler onhashchange;
  attribute EventHandler onlanguagechange;
  attribute EventHandler onmessage;
+  attribute EventHandler onmessageerror;


snuggs · 2017-11-25T01:12:22Z

test/web-platform-tests/to-run.yaml

@@ -521,7 +521,6 @@ date.html: [fail, Unknown]
 datetime-local.html: [fail, Unknown]
 datetime.html: [fail, Unknown]
 email.html: [fail, Unknown]
-files.html: [fail, Unknown]


Why are we removing this file?

to-run.yaml lists all tests that are known to be failing. When I added the setter for HTMLInputElement.files this test began to pass and it could be removed from the list.

Zirro · 2017-11-25T14:56:20Z

Thanks for the review @snuggs. I've answered your questions where I'm able to, but several of them are more related to the specification than this pull request in particular, so it might be more appropriate if you try to locate this information on your own or ask the people who write the spec instead.

Zirro · 2017-11-25T15:34:36Z

jsdom/webidl2js#95 will need to be merged ahead of this since [Unscopable] is used in ChildNode.webidl and ParentNode.webidl by recently implemented methods.

snuggs · 2017-11-26T04:23:45Z

Copy that @Zirro. Also 🎉 @TimothyGu for the quick merge of jsdom/webidl2js#95

domenic · 2017-12-10T18:42:55Z

I've rebased and incorporated the webidl2js update, but I don't understand ea733c6#diff-9f9ee474930d621ac4fe8685e273b452R2 . From what I understand we do support LegacyUnenumerableNamedProperties and OverrideBuiltins. @Zirro, do you know why you commented those out?

Zirro · 2017-12-10T18:45:46Z

@domenic I thought that's what you were saying here: #2053 (comment) - Is that not the case?

domenic · 2017-12-10T18:48:46Z

Ah, right, sorry about that; my bad for not reviewing the whole file, but instead just the snippet I saw there. Indeed, we should only enable those when we've uncommented and implemented the getter lines.

probablySupportsContext() and a setContext() stub

Uses USVString conversion or reflectURLAttribute() depending on whether the attribute is specified as containing a URL.

The longDesc attribute is defined as containing a URL, and thus uses reflectURLAttribute() which produces the whole path.

domenic

Rebased. I noticed a couple things that are not showstoppers. Will probably end up merging anyway if CI comes through.

domenic · 2017-12-16T20:06:53Z

lib/jsdom/living/nodes/Element.webidl

 dictionary ScrollIntoViewOptions : ScrollOptions {
  ScrollLogicalPosition block = "start";
  ScrollLogicalPosition inline = "nearest";
 };

 partial interface Element {
-  [NewObject] sequence<DOMRect> getClientRects();
+  DOMRectList getClientRects();


This change seems bad because we don't implement DOMRectList.

domenic · 2017-12-16T20:07:10Z

lib/jsdom/living/nodes/Element.webidl

 };

+dictionary ShadowRootInit {


This should probably be commented out until we actually use it somewhere.

Zirro force-pushed the update-webidl branch 2 times, most recently from 03f0002 to 75da2e3 Compare November 17, 2017 21:38

TimothyGu approved these changes Nov 17, 2017

View reviewed changes

Zirro force-pushed the update-webidl branch from 75da2e3 to 61d17f2 Compare November 18, 2017 19:31

domenic reviewed Nov 19, 2017

View reviewed changes

domenic assigned Zirro Nov 19, 2017

snuggs reviewed Nov 20, 2017

View reviewed changes

Zirro force-pushed the update-webidl branch 6 times, most recently from 917a9a0 to deb266f Compare November 24, 2017 20:54

snuggs reviewed Nov 25, 2017

View reviewed changes

Zirro mentioned this pull request Nov 25, 2017

[Unscopeable] is now [Unscopable] jsdom/webidl2js#95

Merged

Zirro mentioned this pull request Dec 2, 2017

Maintain writer.js or not? w3c/webidl2.js#109

Closed

domenic force-pushed the update-webidl branch 2 times, most recently from 3333d12 to 67806cc Compare December 10, 2017 18:42

domenic force-pushed the update-webidl branch from 67806cc to ba3acc3 Compare December 10, 2017 18:50

domenic assigned domenic and unassigned Zirro Dec 10, 2017

Zirro mentioned this pull request Dec 12, 2017

Update all the Web IDL - Part Two #2081

Merged

Zirro added 6 commits December 16, 2017 15:03

Update all Web IDL in nodes/

cde987f

Remove obsolete <canvas> methods

95458bc

probablySupportsContext() and a setContext() stub

Add a setter for inputEl.files

2be7713

Update EventHandlers according to their IDL

ef180a7

Handle USVString reflection

27b2583

Uses USVString conversion or reflectURLAttribute() depending on whether the attribute is specified as containing a URL.

Remove old longDesc test which no longer seems correct

5e875e2

The longDesc attribute is defined as containing a URL, and thus uses reflectURLAttribute() which produces the whole path.

domenic force-pushed the update-webidl branch from ba3acc3 to 5e875e2 Compare December 16, 2017 20:06

domenic reviewed Dec 16, 2017

View reviewed changes

domenic merged commit 4c7698f into jsdom:master Dec 16, 2017

Update all the Web IDL #2053

Update all the Web IDL #2053

Conversation

Zirro commented Nov 17, 2017 • edited Loading

TimothyGu left a comment

Choose a reason for hiding this comment

Zirro commented Nov 18, 2017 • edited Loading

snuggs commented Nov 19, 2017

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zirro commented Nov 19, 2017

domenic commented Nov 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zirro commented Nov 24, 2017

snuggs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snuggs Nov 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zirro commented Nov 25, 2017

Zirro commented Nov 25, 2017

snuggs commented Nov 26, 2017

domenic commented Dec 10, 2017

Zirro commented Dec 10, 2017

domenic commented Dec 10, 2017

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zirro commented Nov 17, 2017 •

edited

Loading

Zirro commented Nov 18, 2017 •

edited

Loading

snuggs Nov 25, 2017 •

edited

Loading