Bug 134721 – Move dir attribute from HTMLDocument to Document to match the spec

• Component: DOM
• Status: RESOLVED CONFIGURATION CHANGED

[Prashant Hiremath]

Move dir attribute from HTMLDocument to Document to match the spec

http://www.w3.org/html/wg/drafts/html/master/dom.html#dom-dir 

We should consider merging blink merge https://src.chromium.org/viewvc/blink?revision=171529&view=revision

[Prashant Hiremath]

Created attachment 234556 [details]
Patch

proposed patch

[Prashant Hiremath]

Created attachment 234557 [details]
Patch

proposed patch

[Prashant Hiremath]

Created attachment 234615 [details]
Patch

Fixed Mac build failure

[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 234615 [details]
Patch

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

New failing tests:
fast/dom/assign-to-prototype-accessor-on-prototype-should-be-silent.html

[Build Bot]

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

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

[Ryosuke Niwa]

Comment on attachment 234615 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

You need a blank line after this.

> Source/WebCore/ChangeLog:8
> +        http://www.w3.org/html/wg/drafts/html/master/dom.html#dom-dir

Please refer to a specific version of the spec since WG could be updated in the future.

> Source/WebCore/ChangeLog:9
> +        This is a blinkMerge from https://src.chromium.org/viewvc/blink?revision=171529&view=revision

should read "a blink merge".

> Source/WebCore/bindings/objc/PublicDOMInterfaces.h:112
>  - (DOMElement *)createElement:(NSString *)tagName;

This attribute is certainly not available in OS X 10.5. r- because of this.

> Source/WebCore/bindings/objc/PublicDOMInterfaces.h:-456
> -@property (copy) NSString *dir WEBKIT_AVAILABLE_MAC(10_5);

I don't think we can make this change safely.

> LayoutTests/fast/dom/dir-non-html-document.html:12
> +var xmlDocument = document.implementation.createDocument("", "", null);
> +shouldBe("xmlDocument.__proto__", "Document.prototype");

Please test a SVG document as well as a MathML document to ensure
assigning a value doesn't modify SVG and/or MathML document.

You also need to fix fast/dom/assign-to-prototype-accessor-on-prototype-should-be-silent.html
as it assumes that Document.prototype.dir doesn't exist but the change set http://trac.webkit.org/r168385
makes me think that this patch may not be Web compatible to begin with.

[Daniel Bates]

Comment on attachment 234615 [details]
Patch

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

>> Source/WebCore/bindings/objc/PublicDOMInterfaces.h:112
>>  - (DOMElement *)createElement:(NSString *)tagName;
> 
> This attribute is certainly not available in OS X 10.5. r- because of this.

Yes, this is a bit disingenuous as it documents that this API was available on the base class since OS X 10.5. What is the preferred way is to resolve this issue? I mean, we're moving an API that was made available in OS X 10.5 from a derived class to a base class. I guess you could move this property to the base class mark it as available in a later release and then add a custom setter and getter for dir in DOMHTMLDocument with the WEBKIT_AVAILABLE_MAC(10_5) decoration that forwards to the corresponding setter and getter for the base class property :\

[Daniel Bates]

(In reply to comment #8)
> (From update of attachment 234615 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234615&action=review
> 
> >> Source/WebCore/bindings/objc/PublicDOMInterfaces.h:112
> >>  - (DOMElement *)createElement:(NSString *)tagName;
> > 
> > This attribute is certainly not available in OS X 10.5. r- because of this.
> 
> [...] What is the preferred way is to resolve this issue?
> [...]

For completeness, a similar approach to moving a public API from a derived interface to its base interface was done in the patch for <https://bugs.webkit.org/show_bug.cgi?id=131079> and <https://bugs.webkit.org/show_bug.cgi?id=67651>.

[Prashant Hiremath]

Created attachment 235048 [details]
Patch

Modified as per review comment

[Antonio Gomes]

If Prashant is not available to land the patch, I believe one should be able to carry over his initial work.

Matching the spec is always good.

[Ahmad Saleem]

I took test case from the attached patch but from Google Chrome source and then changed into this JSFiddle:

Link - https://jsfiddle.net/jdxf9u4v/show

All browsers (Firefox Nightly 105, Chrome Canary 106 and Safari 15.6 on macOS 12.5) match with each other. I am marking this as "RESOLVED CONFIGURATION CHANGED". Please reopen, if I am wrong. Thanks!

[Radar WebKit Bug Importer]

<rdar://problem/98197914>