Document.currentScript does not work for SVGScriptElements
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.
This is manifest by the failing WPT test html/dom/documents/dom-tree-accessors/Document.currentScript.html
Created attachment 401695 [details] Patch
Created attachment 401696 [details] Patch
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
Created attachment 401702 [details] Patch
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?
Committed r262945: <https://trac.webkit.org/changeset/262945>
<rdar://problem/64296452>
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.
Sorry, didn’t notice my comments were post-commit.
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.
(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.
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.
(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!
(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());`
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 :)