Document.currentScript does not work for SVGScriptElements

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

Document.currentScript does not work for SVGScriptElements

[Sam Weinig]

Document.currentScript is specified to work with both HTMLScriptElements and SVGScriptElements (https://html.spec.whatwg.org/multipage/dom.html#dom-document-currentscript) but out implementation only works with HTMLScriptElements.

[Sam Weinig]

This is manifest by the failing WPT test html/dom/documents/dom-tree-accessors/Document.currentScript.html

[Sam Weinig]

Created attachment 401695 [details]
Patch

[Sam Weinig]

Created attachment 401696 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Sam Weinig]

Created attachment 401702 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 401702 [details]
Patch

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

r=me

> Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMDocumentGtk.cpp:1913
> +        return NULL;

nullptr?

> Source/WebKitLegacy/mac/DOM/DOMDocument.mm:409
> +        return NULL;

nil?

[Sam Weinig]

Committed r262945: <https://trac.webkit.org/changeset/262945>

[Radar WebKit Bug Importer]

<rdar://problem/64296452>

[Darin Adler]

Comment on attachment 401702 [details]
Patch

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

> Source/WebCore/dom/Document.h:1040
> +    void pushCurrentScript(Element*);

This should take Element&.

> Source/WebCore/dom/Document.h:1792
> +    Vector<RefPtr<Element>> m_currentScriptStack;

This should be Vector<Ref<Element>>.

> Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMDocumentGtk.cpp:1911
> +    WebCore::Element* element = item->currentScript();

auto would make this better.

> Source/WebKitLegacy/mac/DOM/DOMDocument.mm:407
> +    WebCore::Element* element = IMPL->currentScript();

auto would make this better.

[Darin Adler]

Sorry, didn’t notice my comments were post-commit.

[Yusuke Suzuki]

Comment on attachment 401702 [details]
Patch

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

>> Source/WebCore/dom/Document.h:1040
>> +    void pushCurrentScript(Element*);
> 
> This should take Element&.

This needs to be Element* since this can take `nullptr` in meaningful way.
If the module script is executed (<script type="module">), currentScript needs to be `null` JS value.
This is represented by `nullptr` here.

[Sam Weinig]

(In reply to Darin Adler from comment #10)
> Comment on attachment 401702 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401702&action=review
> 
> > Source/WebCore/dom/Document.h:1040
> > +    void pushCurrentScript(Element*);
> 
> This should take Element&.
> 
> > Source/WebCore/dom/Document.h:1792
> > +    Vector<RefPtr<Element>> m_currentScriptStack;
> 
> This should be Vector<Ref<Element>>.

Unfortunately, null is used in a meaningful way in this stack, so it can't use references here.

[Darin Adler]

Comment on attachment 401702 [details]
Patch

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

>>>> Source/WebCore/dom/Document.h:1040
>>>> +    void pushCurrentScript(Element*);
>>> 
>>> This should take Element&.
>> 
>> This needs to be Element* since this can take `nullptr` in meaningful way.
>> If the module script is executed (<script type="module">), currentScript needs to be `null` JS value.
>> This is represented by `nullptr` here.
> 
> Unfortunately, null is used in a meaningful way in this stack, so it can't use references here.

So you’re saying that when executing a module script we explicitly push a nullptr? OK, got it.

[Sam Weinig]

(In reply to Darin Adler from comment #11)
> Sorry, didn’t notice my comments were post-commit.

Oh, please don't ever worry about that. I think some of my best comments are post-commit!

[Sam Weinig]

(In reply to Darin Adler from comment #14)
> Comment on attachment 401702 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401702&action=review
> 
> >>>> Source/WebCore/dom/Document.h:1040
> >>>> +    void pushCurrentScript(Element*);
> >>> 
> >>> This should take Element&.
> >> 
> >> This needs to be Element* since this can take `nullptr` in meaningful way.
> >> If the module script is executed (<script type="module">), currentScript needs to be `null` JS value.
> >> This is represented by `nullptr` here.
> > 
> > Unfortunately, null is used in a meaningful way in this stack, so it can't use references here.
> 
> So you’re saying that when executing a module script we explicitly push a
> nullptr? OK, got it.

Yeah. The code is:

`m_document.pushCurrentScript(shouldPushNullForCurrentScript ? nullptr : &scriptElement.element());`

[Yusuke Suzuki]

Comment on attachment 401702 [details]
Patch

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

>>>>>> Source/WebCore/dom/Document.h:1040
>>>>>> +    void pushCurrentScript(Element*);
>>>>> 
>>>>> This should take Element&.
>>>> 
>>>> This needs to be Element* since this can take `nullptr` in meaningful way.
>>>> If the module script is executed (<script type="module">), currentScript needs to be `null` JS value.
>>>> This is represented by `nullptr` here.
>>> 
>>> Unfortunately, null is used in a meaningful way in this stack, so it can't use references here.
>> 
>> So you’re saying that when executing a module script we explicitly push a nullptr? OK, got it.
> 
> Yeah. The code is:
> 
> `m_document.pushCurrentScript(shouldPushNullForCurrentScript ? nullptr : &scriptElement.element());`

Yes. And this is specified behavior :)