1340027 - Add annotation with the [CEReactions] attribute

Status: RESOLVED FIXED

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

Description

[John Dai[:jdai]]

When we add [CEReactions] in webidls, we need to add testcases in order to appropriately track and invoke custom element reactions.

Comment 1

[John Dai[:jdai]]

Attachment: (WIP) Bug 1340027 - Part 1: Add Dom CEReactions annotation.

Comment 2

[John Dai[:jdai]]

Attachment: (WIP) Bug 1340027 - Part 2: Add HTML CEReactions annotation.

Comment 3

[John Dai[:jdai]]

Attachment: (WIP) Bug 1340027 - Part 3: Add Selection CEReactions annotation.

Comment 4

[John Dai[:jdai]]

Attachment: (WIP) Bug 1340027 - Part 4: Add Dom parsing CEReactions annotation.

Comment 5

[John Dai[:jdai]]

Attachment: (WIP) Bug 1340027 - Part 5: Add XSLTProcessor CEReactions annotation.

Spec: https://wiki.whatwg.org/wiki/DOM_XSLTProcessor

Comment 6

[John Dai[:jdai]]

* Some attritubes aren't supported by currently implementation:
- Element.webidl
[CEReactions, Unscopable] attribute DOMString slot;

- HTMLElement.webidl
[CEReactions] attribute boolean translate;

- HTMLLinkElement.webidl
[CEReactions] attribute RequestDestination as; // (default "")
[CEReactions] attribute DOMString nonce;
[CEReactions] attribute USVString scope;
[CEReactions] attribute WorkerType workerType;
[CEReactions] attribute boolean useCache;

- HTMLStyleElement.webidl
[CEReactions] attribute DOMString nonce;

- HTMLIFrameElement.webidl
[CEReactions] attribute boolean allowPaymentRequest;
[CEReactions] attribute boolean allowUserMedia;

- HTMLVideoElement.webidl
[CEReactions] attribute boolean playsInline;

- HTMLInputElement.webidl
[CEReactions] attribute DOMString dirName;

- HTMLTextAreaElement.webidl
[CEReactions] attribute DOMString autocomplete;
[CEReactions] attribute DOMString dirName;
[CEReactions] attribute DOMString inputMode;

- HTMLScriptElement.webidl
[CEReactions] attribute boolean noModule;
[CEReactions] attribute DOMString nonce;

- ElementContentEditable.webidl
[CEReactions] attribute DOMString contentEditable;

* We don't have HTMLSlotElement.webidl and HTMLMarqueeElement.webidl now.

* TODO:
Add testcases to support all of CEReactions annotation.

Comment 7

[John Dai[:jdai]]

Hi bz,
I have some questions when I was adding [CEReactions] annotation, I list as following:

1) We have different extended attributes on a overload |open| function[1] and it causes a build error: "WebIDL.WebIDLError: error: Extended attributes differ on different overloads...". Can we loose this[2] constrain? 

2) The Element.webidl has a CEReactions annotation on |attribute DOMTokenList classList|[3]. It seems like an oversight bug, because [CEReactions] is no longer allowed on [PutForwards] readonly attributes[4].
 

[1] https://html.spec.whatwg.org/#the-document-object
[2] https://searchfox.org/mozilla-central/source/dom/bindings/parser/WebIDL.py#4745-4748
[3] https://dom.spec.whatwg.org/#interface-element
[4] https://github.com/whatwg/html/issues/2362

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Can we loose this[2] constrain? 

Not easily, without rearchitecting how we do overloads.  Right now the extended attributes end up on an object that all the overloads share, so they have to match; if they don't match there's no way to represent that state in the data model in our bindings code.

That said, (1) I don't quite understand why the spec has [CEReactions] on only one overload; proving that the other overload doesn't need it seems hard and (2) there is no problem with adding [CEReactions] to functions that don't need it; if it's not needed it's a bit of overhead but doesn't change behavior.  The three-arg overload of document.open is not going to notice the overhead, so just add [CEReactions] to it.

> The Element.webidl has a CEReactions annotation on |attribute DOMTokenList classList|[3].

Clearly a bug in the DOM spec.  Please file it.

Comment 9

[John Dai[:jdai]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #8)
> > Can we loose this[2] constrain? 
> 
> Not easily, without rearchitecting how we do overloads.  Right now the
> extended attributes end up on an object that all the overloads share, so
> they have to match; if they don't match there's no way to represent that
> state in the data model in our bindings code.
> 
> That said, (1) I don't quite understand why the spec has [CEReactions] on
> only one overload; proving that the other overload doesn't need it seems
> hard and (2) there is no problem with adding [CEReactions] to functions that
> don't need it; if it's not needed it's a bit of overhead but doesn't change
> behavior.  The three-arg overload of document.open is not going to notice
> the overhead, so just add [CEReactions] to it.
> 
Will do. Should it be filed a spec bug in order to make other browsers alignment? 

> > The Element.webidl has a CEReactions annotation on |attribute DOMTokenList classList|[3].
> 
> Clearly a bug in the DOM spec.  Please file it.

Filed https://github.com/whatwg/dom/issues/439

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Should it be filed a spec bug in order to make other browsers alignment? 

No need for that, if the three-arg version indeed can't trigger custom element stuff; at that point we're just working around a limitation in our data model, and that shouldn't affect the spec.

Comment 11

[John Dai[:jdai]]

This bug is too big, it contains 81 interfaces which need to write testcases. It should be split to small pieces and landed it one by one.

Comment 12

[John Dai[:jdai]]

Attachment: (WIP) Bug 1340027 - Part 3: Add CSSStyleDeclaration CEReactions annotation.

- Selection.webidl has been added [CEReactions] annotation at bug 1359371, removed it from wip patches. 
- Add CSSStyleDeclaration [CEReactions] annotation.

Comment 13

[John Dai[:jdai]]

It remains 59 interfaces need to cover. Filed a web-platform-tests issue to track: https://github.com/w3c/web-platform-tests/issues/5727

Comment 14

[John Dai[:jdai]]

Currently, each browsers haven't implemented customized built-in element with custom element constructor [1]. I'll suspend implementing this bug.

[1] https://software.hixie.ch/utilities/js/live-dom-viewer/?saved=5076

Comment 15

[John Dai[:jdai]]

Split adding tests to bug 1374912.

Comment 16

[John Dai[:jdai]]

Attachment: Bug 1340027 - Part 1: Add Dom CEReactions annotation. v1

Hi Edgar,
Could you help to feedback this patch? This patch follows dom spec[1]. Thank you.

[1]https://dom.spec.whatwg.org/

Comment 17

[John Dai[:jdai]]

Attachment: Bug 1340027 - Part 2: Add HTML CEReactions annotation. v1

This patch adds [CEReactions] which follows HTML spec[1]. 

[1] https://html.spec.whatwg.org/

Comment 18

[John Dai[:jdai]]

Attachment: Bug 1340027 - Part 3: Add CSSStyleDeclaration CEReactions annotation. v1

This patch adds [CEReactions] which follows CSSOM spec[1]. 

[1] https://drafts.csswg.org/cssom/

Comment 19

[John Dai[:jdai]]

Attachment: Bug 1340027 - Part 4: Add Dom parsing CEReactions annotation. v1

This patch adds [CEReactions] which follows DOM parsing spec[1]. 

[1] https://w3c.github.io/DOM-Parsing/

Comment 20

[John Dai[:jdai]]

Attachment: Bug 1340027 - Part 5: Add XSLTProcessor CEReactions annotation. v1

This patch adds [CEReactions] which follows DOM XSLTProcessor spec[1]. 

[1] https://wiki.whatwg.org/wiki/DOM_XSLTProcessor

Comment 21

[John Dai[:jdai]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d24704f6a313be4e9fd6de94e67160ef53e213a9&filter-tier=1&group_state=expanded

Comment 22

[Edgar Chen [:edgar]]

Comment on attachment 8879881 [details] [diff] [review]
Bug 1340027 - Part 1: Add Dom CEReactions annotation. v1

Review of attachment 8879881 [details] [diff] [review]:
-----------------------------------------------------------------

> Bug 1340027 - Part 1: Add Dom CEReactions annotation.
s/Dom/DOM/

::: dom/webidl/Attr.webidl
@@ +12,5 @@
>  
>  interface Attr : Node {
>    readonly attribute DOMString localName;
> +  [CEReactions, SetterThrows]
> +  attribute DOMString value;

Nit: keep original alignment for this line (align two "attribute"). i.e.

readonly attribute DOMString localName;
[CEReactions, SetterThrows]
         attribute DOMString value;

Comment 23

[John Dai[:jdai]]

Attachment: Bug 1340027 - Part 1: Add Dom CEReactions annotation. v2

Address comment #22, and carry f+.

Comment 24

[Edgar Chen [:edgar]]

Comment on attachment 8879886 [details] [diff] [review]
Bug 1340027 - Part 4: Add Dom parsing CEReactions annotation. v1

Review of attachment 8879886 [details] [diff] [review]:
-----------------------------------------------------------------

> Bug 1340027 - Part 4: Add dom parsing CEReactions annotation.
s/dom/DOM/

Comment 25

[John Dai[:jdai]]

Attachment: Bug 1340027 - Part 4: Add DOM parsing CEReactions annotation. v2

Address comment #24 and carry f+.

Comment 26

[Edgar Chen [:edgar]]

Comment on attachment 8879883 [details] [diff] [review]
Bug 1340027 - Part 2: Add HTML CEReactions annotation. v1

Review of attachment 8879883 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/webidl/HTMLIFrameElement.webidl
@@ +46,3 @@
>             attribute DOMString longDesc;
>  
> +  [CEReactions, TreatNullAs=EmptyString,SetterThrows,Pure]

Nit: add space after `,`.

@@ +46,5 @@
>             attribute DOMString longDesc;
>  
> +  [CEReactions, TreatNullAs=EmptyString,SetterThrows,Pure]
> +           attribute DOMString marginHeight;
> +  [CEReactions, TreatNullAs=EmptyString,SetterThrows,Pure]

Ditto

::: dom/webidl/HTMLMenuElement.webidl
@@ +23,1 @@
>             attribute DOMString label;

I didn't find HTMLMenuElement has type and label attribute defined in whatwg sepc, but adding CEReactions to them looks good to me.

::: dom/webidl/HTMLMenuItemElement.webidl
@@ +18,1 @@
>             attribute DOMString type;

I didn't find HTMLMenuItemElement defined in spec, but adding CEReactions looks good to me.

::: dom/webidl/HTMLOptionsCollection.webidl
@@ +11,5 @@
>   */
>  
>  interface HTMLOptionsCollection : HTMLCollection {
> +  [CEReactions]
> +  attribute unsigned long length;

Nit: keep original alignment.

::: dom/webidl/HTMLTableColElement.webidl
@@ +22,1 @@
>             attribute DOMString align;

I didn't find HTMLTableColElement has these attributes defined in whatwg spec, but it makes sense to have CEReations annotated since the same attribute defined in other HTMLTable*Element has CEReations annotation.

::: dom/webidl/HTMLTableElement.webidl
@@ -13,5 @@
>  
>  [HTMLConstructor]
>  interface HTMLTableElement : HTMLElement {
> -           [SetterThrows]
> -           attribute HTMLTableCaptionElement? caption;

Nit: keep original alignment.

@@ -19,3 @@
>    void deleteCaption();
> -           [SetterThrows]
> -           attribute HTMLTableSectionElement? tHead;

Ditto

@@ -23,3 @@
>    void deleteTHead();
> -           [SetterThrows]
> -           attribute HTMLTableSectionElement? tFoot;

Ditto

Comment 27

[Boris Zbarsky [:bzbarsky]]

> I didn't find HTMLTableColElement has these attributes defined in whatwg spec

It's in https://html.spec.whatwg.org/multipage/obsolete.html#other-elements,-attributes-and-apis

Comment 28

[John Dai[:jdai]]

(In reply to Edgar Chen [:edgar] from comment #26)
> Comment on attachment 8879883 [details] [diff] [review]
> Bug 1340027 - Part 2: Add HTML CEReactions annotation. v1
> 
> Review of attachment 8879883 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/webidl/HTMLMenuElement.webidl
> @@ +23,1 @@
> >             attribute DOMString label;
> 
> I didn't find HTMLMenuElement has type and label attribute defined in whatwg
> sepc, but adding CEReactions to them looks good to me.
I didn't find HTMLMenuElement has type and label attribute defined in whatwg sepc either, I guess it has been removed.

> 
> ::: dom/webidl/HTMLMenuItemElement.webidl
> @@ +18,1 @@
> >             attribute DOMString type;
> 
> I didn't find HTMLMenuItemElement defined in spec, but adding CEReactions
> looks good to me.
> 
I didn't find HTMLMenuItemElement defined in whatwg spec either, I guess it has been removed.

Comment 29

[John Dai[:jdai]]

Attachment: Bug 1340027 - Part 2: Add HTML CEReactions annotation. v2

Address comment #26 and carry f+. I didn't remove CEReactions annotation in HTMLMenuElement and HTMLMenuItemElement, because we need to keep these attributes with CEReactions annotation before we removed them in our codebase.

Comment 30

[John Dai[:jdai]]

Comment on attachment 8881837 [details] [diff] [review]
Bug 1340027 - Part 2: Add HTML CEReactions annotation. v2

Hi Olli, 
It changes webidl files which needs DOM peer approval this patch, could you help to review it? Thank you.

Comment 31

[Boris Zbarsky [:bzbarsky]]

menu/menuitem bits went away in https://github.com/whatwg/html/issues/2730 I believe.  See bug 1372276.

Comment 32

[John Dai[:jdai]]

Attachment: Bug 1340027 - Part 1: Add DOM CEReactions annotation. v3

Add reviewer name, and carry r+.

Comment 33

[John Dai[:jdai]]

Attachment: Bug 1340027 - Part 2: Add HTML CEReactions annotation. v3

Add reviewer name, and carry r+.

Comment 34

[John Dai[:jdai]]

Attachment: Bug 1340027 - Part 3: Add CSSStyleDeclaration CEReactions annotation. v3

Add reviewer name, and carry r+.

Comment 35

[John Dai[:jdai]]

Attachment: Bug 1340027 - Part 4: Add DOM parsing CEReactions annotation. v3

Add reviewer name, and carry r+.

Comment 36

[John Dai[:jdai]]

Attachment: Bug 1340027 - Part 5: Add XSLTProcessor CEReactions annotation. v3

Add reviewer name, and carry r+.

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0ebadb854d0
Part 1: Add DOM CEReactions annotation. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/68a06c85c290
Part 2: Add HTML CEReactions annotation. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/18e7e7fa05d5
Part 3: Add CSSStyleDeclaration CEReactions annotation. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5f5c66b9e16
Part 4: Add DOM parsing CEReactions annotation. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c9b8d85935e
Part 5: Add XSLTProcessor CEReactions annotation. r=smaug

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e0ebadb854d0
https://hg.mozilla.org/mozilla-central/rev/68a06c85c290
https://hg.mozilla.org/mozilla-central/rev/18e7e7fa05d5
https://hg.mozilla.org/mozilla-central/rev/b5f5c66b9e16
https://hg.mozilla.org/mozilla-central/rev/1c9b8d85935e