Bug 119706 – Move id attribute to parent Element interface

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

Merge https://chromium.googlesource.com/chromium/blink/+/048383b5f7c71faffb20202c64e389ac23d2e955

Move id attribute from SVGElement / HTMLElement to their Element parent
interface to match the latest DOM specification and avoid duplication:
http://dom.spec.whatwg.org/#dom-element-id

This behavior is consistent with Firefox.

There is no web-exposed behavior change because we haven't moved
properties from instances to their prototype, as per the Web IDL
specification (http://dev.w3.org/2006/webapi/WebIDL/#es-attributes).

[Chris Dumez]

Created attachment 208602 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 208602 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208602&action=review

> Source/WebCore/dom/Element.idl:67
> +    // iht.com relies on id returning the empty string when no id is present.
> +    // Other browsers do this as well. So we don't convert null to JS null.

I don't think this comment belongs in the code since it can get outdated.
It's better to mention it in the change log since this statement is (supposedly) true as of today.

[Chris Dumez]

Comment on attachment 208602 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208602&action=review

>> Source/WebCore/dom/Element.idl:67
>> +    // Other browsers do this as well. So we don't convert null to JS null.
> 
> I don't think this comment belongs in the code since it can get outdated.
> It's better to mention it in the change log since this statement is (supposedly) true as of today.

I merely moved the comment over from HTMLElement. Since it is not new, I don't think I should mention this in my Changelog.
Do you propose we remove the comment altogether?

[Build Bot]

Comment on attachment 208602 [details]
Patch

Attachment 208602 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1381429

[Chris Dumez]

Created attachment 208604 [details]
Attempt to fix mac build

[Ryosuke Niwa]

Comment on attachment 208602 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208602&action=review

>>> Source/WebCore/dom/Element.idl:67
>>> +    // Other browsers do this as well. So we don't convert null to JS null.
>> 
>> I don't think this comment belongs in the code since it can get outdated.
>> It's better to mention it in the change log since this statement is (supposedly) true as of today.
> 
> I merely moved the comment over from HTMLElement. Since it is not new, I don't think I should mention this in my Changelog.
> Do you propose we remove the comment altogether?

This comment was added in http://trac.webkit.org/changeset/4417 10 years ago.
The compatibility with ich.com, which now redirects to http://global.nytimes.com/?iht, is hardly relevant at this point.
My preference will be to remove this comment since the current behavior is well specified in http://www.whatwg.org/specs/web-apps/current-work/multipage/common-dom-interfaces.html#reflecting-content-attributes-in-idl-attributes.

[Chris Dumez]

Created attachment 208606 [details]
Update Changelog.

[WebKit Commit Bot]

Please wait for approval from [e-mail] (or another member of the Apple Safari Team) before submitting because this patch contains changes to the Apple Mac WebKit.framework public API.

[Build Bot]

Comment on attachment 208606 [details]
Update Changelog.

Attachment 208606 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1451463

New failing tests:
platform/mac/accessibility/role-subrole-roledescription.html
fast/js/dom-static-property-for-in-iteration.html

[Build Bot]

Created attachment 208631 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.4

[Chris Dumez]

Created attachment 208633 [details]
Patch

[Chris Dumez]

Requesting review again due to the ObjC bindings change.

[Darin Adler]

Comment on attachment 208633 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208633&action=review

I really want to say review+, but the Objective-C aspect of this is wrong. We’ll probably have to leave something ugly around for Objective-C, at least for a while.

> Source/WebCore/bindings/objc/PublicDOMInterfaces.h:175
> +@property(copy) NSString *idName;

This is not correct. This means that the Objective-C header file will have interface that claims you can call idName on a DOMElement in older versions of WebKit. But if a programmer tries it they will get method-not-found. Tim Hatcher should give us some advice on what to do about this.

The problem, of course, is not in this header file alone, but generally in the generated code too.

[Darin Adler]

The easiest way to make a land-able patch is to leave the old attribute in place in HTMLElement for Objective-C only, and have the new attribute in Element for all other languages. Then someone can follow up by doing something better for Objective-C. Too bad the Objective-C bindings are getting in the way.

[Chris Dumez]

(In reply to comment #14)
> The easiest way to make a land-able patch is to leave the old attribute in place in HTMLElement for Objective-C only, and have the new attribute in Element for all other languages. Then someone can follow up by doing something better for Objective-C. Too bad the Objective-C bindings are getting in the way.

Ok, this sounds like a plan, thanks. I'll do that and reupload soon.

[Chris Dumez]

Created attachment 208646 [details]
Patch

[Build Bot]

Comment on attachment 208646 [details]
Patch

Attachment 208646 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1445785

New failing tests:
platform/mac/accessibility/role-subrole-roledescription.html

[Build Bot]

Created attachment 208652 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.4

[Chris Dumez]

(In reply to comment #17)
> (From update of attachment 208646 [details])
> Attachment 208646 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.appspot.com/results/1445785
> 
> New failing tests:
> platform/mac/accessibility/role-subrole-roledescription.html

Chris Fleizach, can this be a false positive? If not, do you have an idea of why it would fail?

The diff looks like:
-PASS: math
-      AXRole: 
-      AXSubrole: 
-      AXRoleDescription: 
-      
+FAIL: math
+      AXRole: AXGroup
+      AXSubrole: AXDocumentMath
+      AXRoleDescription: math
+      Expected: //
+

I looked at the test but I don't understand it much and I don't have mac to run it.

[chris fleizach]

(In reply to comment #19)
> (In reply to comment #17)
> > (From update of attachment 208646 [details] [details])
> > Attachment 208646 [details] [details] did not pass mac-ews (mac):
> > Output: http://webkit-queues.appspot.com/results/1445785
> > 
> > New failing tests:
> > platform/mac/accessibility/role-subrole-roledescription.html
> 
> Chris Fleizach, can this be a false positive? If not, do you have an idea of why it would fail?
> 
> The diff looks like:
> -PASS: math
> -      AXRole: 
> -      AXSubrole: 
> -      AXRoleDescription: 
> -      
> +FAIL: math
> +      AXRole: AXGroup
> +      AXSubrole: AXDocumentMath
> +      AXRoleDescription: math
> +      Expected: //
> +
> 
> I looked at the test but I don't understand it much and I don't have mac to run it.

Must be a false positive. That's a strange one to fail out of the blue. It's hard for me to believe that moving the id interface around would have an effect like this. I'd be OK even if this was a real failure with your patch to fix it up after you land because clearly something is amiss if it persists.

[Chris Dumez]

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #17)
> > > (From update of attachment 208646 [details] [details] [details])
> > > Attachment 208646 [details] [details] [details] did not pass mac-ews (mac):
> > > Output: http://webkit-queues.appspot.com/results/1445785
> > > 
> > > New failing tests:
> > > platform/mac/accessibility/role-subrole-roledescription.html
> > 
> > Chris Fleizach, can this be a false positive? If not, do you have an idea of why it would fail?
> > 
> > The diff looks like:
> > -PASS: math
> > -      AXRole: 
> > -      AXSubrole: 
> > -      AXRoleDescription: 
> > -      
> > +FAIL: math
> > +      AXRole: AXGroup
> > +      AXSubrole: AXDocumentMath
> > +      AXRoleDescription: math
> > +      Expected: //
> > +
> > 
> > I looked at the test but I don't understand it much and I don't have mac to run it.
> 
> Must be a false positive. That's a strange one to fail out of the blue. It's hard for me to believe that moving the id interface around would have an effect like this. I'd be OK even if this was a real failure with your patch to fix it up after you land because clearly something is amiss if it persists.

Ok, thanks for checking.

[Darin Adler]

Comment on attachment 208646 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208646&action=review

> Source/WebCore/svg/SVGElement.idl:-27
> -    [Reflect] attribute DOMString id;

May need to keep compiling this for Objective-C, otherwise apps calling the id method on SVGElement might fail, even though this is not technically a public ObJC DOM interface.

[Chris Dumez]

(In reply to comment #22)
> (From update of attachment 208646 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208646&action=review
> 
> > Source/WebCore/svg/SVGElement.idl:-27
> > -    [Reflect] attribute DOMString id;
> 
> May need to keep compiling this for Objective-C, otherwise apps calling the id method on SVGElement might fail, even though this is not technically a public ObJC DOM interface.

I remember Xenon telling me you were not generating ObjC bindings for SVG anymore so I thought it was safe to remove. Please let me know if I misunderstood and if I should add it back.

[Darin Adler]

(In reply to comment #23)
> I remember Xenon telling me you were not generating ObjC bindings for SVG anymore so I thought it was safe to remove. Please let me know if I misunderstood and if I should add it back.

Sounds right.

[Build Bot]

Comment on attachment 208646 [details]
Patch

Attachment 208646 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1451730

New failing tests:
platform/mac/accessibility/role-subrole-roledescription.html

[Build Bot]

Created attachment 208701 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[WebKit Commit Bot]

Comment on attachment 208646 [details]
Patch

Rejecting attachment 208646 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
0EC4.sh"
(1 failure)

Failed to run "['Tools/Scripts/build-webkit', '--release']" exit_code: 65
e: *** wait: No child processes.  Stop.
make: *** Waiting for unfinished jobs....
make: *** wait: No child processes.  Stop.
Command /bin/sh failed with exit code 2


** BUILD FAILED **


The following build commands failed:
	PhaseScriptExecution "Generate Derived Sources" "/Volumes/Data/EWS/WebKit/WebKitBuild/WebKit2.build/Release/Derived Sources.build/Script-C0CE72841247E66800BC0EC4.sh"
(1 failure)

Full output: http://webkit-queues.appspot.com/results/1462099

[WebKit Commit Bot]

Comment on attachment 208646 [details]
Patch

Rejecting attachment 208646 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
sh"
(1 failure)

Failed to run "['Tools/Scripts/build-webkit', '--release']" exit_code: 65
b
make: *** wait: No child processes.  Stop.
make: *** Waiting for unfinished jobs....
make: *** wait: No child processes.  Stop.
Command /bin/sh failed with exit code 2


** BUILD FAILED **


The following build commands failed:
	PhaseScriptExecution "Generate Derived Sources" "/Volumes/Data/EWS/WebKit/WebKitBuild/WebKit2.build/Release/Derived Sources.build/Script-C0CE72841247E66800BC0EC4.sh"
(1 failure)

Full output: http://webkit-queues.appspot.com/results/1458776

[Chris Dumez]

Committed r154057: <http://trac.webkit.org/changeset/154057>

[Alexey Proskuryakov]

> New failing tests:
> platform/mac/accessibility/role-subrole-roledescription.html

Looks like this was accurate, this is failing now all over the place.

[chris fleizach]

(In reply to comment #30)
> > New failing tests:
> > platform/mac/accessibility/role-subrole-roledescription.html
> 
> Looks like this was accurate, this is failing now all over the place.

Filed

https://bugs.webkit.org/show_bug.cgi?id=119821

[Alexey Proskuryakov]

This is failing on math, and this patch adds id to MathML elements, so it's not totally unrelated. Are they even supposed to have an id exposed?

[Chris Dumez]

(In reply to comment #32)
> This is failing on math, and this patch adds id to MathML elements, so it's not totally unrelated. Are they even supposed to have an id exposed?

According to http://www.w3.org/TR/mathml-for-css/#commatt, yes they are supposed to have an id attribute.

In MathML2, they had an id as well:
http://www.w3.org/TR/MathML2/appendixd.html#id.D.1.2