Add annotation with the [CEReactions] attribute

RESOLVED FIXED in Firefox 56

Status

()

defect
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jdai, Assigned: jdai)

Tracking

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: dom-ce-m3, URL)

Attachments

(5 attachments, 14 obsolete attachments)

10.23 KB, patch
jdai
: review+
Details | Diff | Splinter Review
86.24 KB, patch
jdai
: review+
Details | Diff | Splinter Review
1.38 KB, patch
jdai
: review+
Details | Diff | Splinter Review
1.87 KB, patch
jdai
: review+
Details | Diff | Splinter Review
1.48 KB, patch
jdai
: review+
Details | Diff | Splinter Review
(Assignee)

Description

2 years ago
When we add [CEReactions] in webidls, we need to add testcases in order to appropriately track and invoke custom element reactions.
(Assignee)

Updated

2 years ago
Summary: Add annotated and testcase with the [CEReactions] attribute → Add annotation and testcase with the [CEReactions] attribute
Priority: -- → P3
Whiteboard: dom-ce-m3
(Assignee)

Comment 6

2 years ago
* 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.
(Assignee)

Comment 7

2 years ago
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
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 9

2 years ago
(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
> 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.
(Assignee)

Comment 11

2 years ago
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.
Depends on: 1293921
(Assignee)

Comment 12

2 years ago
- Selection.webidl has been added [CEReactions] annotation at bug 1359371, removed it from wip patches. 
- Add CSSStyleDeclaration [CEReactions] annotation.
Attachment #8856947 - Attachment is obsolete: true
(Assignee)

Comment 13

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

Comment 14

2 years ago
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
(Assignee)

Comment 15

2 years ago
Split adding tests to bug 1374912.
Summary: Add annotation and testcase with the [CEReactions] attribute → Add annotation with the [CEReactions] attribute
(Assignee)

Comment 16

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

[1]https://dom.spec.whatwg.org/
Attachment #8856945 - Attachment is obsolete: true
Attachment #8856946 - Attachment is obsolete: true
Attachment #8856948 - Attachment is obsolete: true
Attachment #8856949 - Attachment is obsolete: true
Attachment #8862290 - Attachment is obsolete: true
Attachment #8879881 - Flags: feedback?
(Assignee)

Comment 17

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

[1] https://html.spec.whatwg.org/
Attachment #8879883 - Flags: feedback?(echen)
(Assignee)

Updated

2 years ago
Attachment #8879881 - Attachment description: Bug 1340027 - Part 1: Add Dom CEReactions annotation. → Bug 1340027 - Part 1: Add Dom CEReactions annotation. v1
(Assignee)

Comment 18

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

[1] https://drafts.csswg.org/cssom/
Attachment #8879885 - Flags: feedback?(echen)
(Assignee)

Comment 19

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

[1] https://w3c.github.io/DOM-Parsing/
Attachment #8879886 - Flags: feedback?(echen)
(Assignee)

Comment 20

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

[1] https://wiki.whatwg.org/wiki/DOM_XSLTProcessor
Attachment #8879887 - Flags: feedback?(echen)
(Assignee)

Updated

2 years ago
Attachment #8879881 - Flags: feedback? → feedback?(echen)
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;
Attachment #8879881 - Flags: feedback?(echen) → feedback+
(Assignee)

Comment 23

2 years ago
Address comment #22, and carry f+.
Attachment #8879881 - Attachment is obsolete: true
Attachment #8881440 - Flags: feedback+

Updated

2 years ago
Attachment #8879885 - Flags: feedback?(echen) → feedback+
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/
Attachment #8879886 - Flags: feedback?(echen) → feedback+

Updated

2 years ago
Attachment #8879887 - Flags: feedback?(echen) → feedback+
(Assignee)

Comment 25

2 years ago
Address comment #24 and carry f+.
Attachment #8879886 - Attachment is obsolete: true
Attachment #8881455 - Flags: feedback+
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
Attachment #8879883 - Flags: feedback?(echen) → feedback+
> 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
(Assignee)

Comment 28

2 years ago
(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.
(Assignee)

Comment 29

2 years ago
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.
Attachment #8879883 - Attachment is obsolete: true
Attachment #8881837 - Flags: feedback+
(Assignee)

Updated

2 years ago
Attachment #8879885 - Flags: review?(bugs)
(Assignee)

Updated

2 years ago
Attachment #8879887 - Flags: review?(bugs)
(Assignee)

Updated

2 years ago
Attachment #8881440 - Flags: review?(bugs)
(Assignee)

Updated

2 years ago
Attachment #8881455 - Flags: review?(bugs)
(Assignee)

Comment 30

2 years ago
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.
Attachment #8881837 - Flags: review?(bugs)
menu/menuitem bits went away in https://github.com/whatwg/html/issues/2730 I believe.  See bug 1372276.
Attachment #8879885 - Flags: review?(bugs) → review+
Attachment #8879887 - Flags: review?(bugs) → review+
Attachment #8881455 - Flags: review?(bugs) → review+
Attachment #8881440 - Flags: review?(bugs) → review+
Attachment #8881837 - Flags: review?(bugs) → review+
(Assignee)

Comment 32

2 years ago
Add reviewer name, and carry r+.
Attachment #8879885 - Attachment is obsolete: true
Attachment #8879887 - Attachment is obsolete: true
Attachment #8881440 - Attachment is obsolete: true
Attachment #8881455 - Attachment is obsolete: true
Attachment #8881837 - Attachment is obsolete: true
Attachment #8882213 - Flags: review+
(Assignee)

Comment 33

2 years ago
Add reviewer name, and carry r+.
Attachment #8882216 - Flags: review+
(Assignee)

Comment 34

2 years ago
Add reviewer name, and carry r+.
Attachment #8882217 - Flags: review+
(Assignee)

Comment 35

2 years ago
Add reviewer name, and carry r+.
Attachment #8882220 - Flags: review+
(Assignee)

Comment 36

2 years ago
Add reviewer name, and carry r+.
Attachment #8882221 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 37

2 years ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.