Add the support for nomodule attribute on script element

• Component: DOM
• Status: RESOLVED FIXED

[Ryosuke Niwa]

See https://github.com/whatwg/html/pull/2261

We should add the support for nomodule attribute which prevents scripts from running now that we have the support for type=module.

[Radar WebKit Bug Importer]

<rdar://problem/30000539>

[Yusuke Suzuki]

Created attachment 298755 [details]
Patch

[Sam Weinig]

Comment on attachment 298755 [details]
Patch

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

> Source/WebCore/html/HTMLScriptElement.idl:32
> +    attribute boolean noModule;

Can this be a [Reflect] attribute, and avoid the need for the explicit implementation?

[Yusuke Suzuki]

Comment on attachment 298755 [details]
Patch

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

>> Source/WebCore/html/HTMLScriptElement.idl:32
>> +    attribute boolean noModule;
> 
> Can this be a [Reflect] attribute, and avoid the need for the explicit implementation?

Ah, I thought we need the implementation because of the name `noModule` JS property v.s. `nomodule` attribute name.
But it seems that we can use `Reflect` and `ImplementedAs`. Fixed.

[Yusuke Suzuki]

Comment on attachment 298755 [details]
Patch

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

>>> Source/WebCore/html/HTMLScriptElement.idl:32
>>> +    attribute boolean noModule;
>> 
>> Can this be a [Reflect] attribute, and avoid the need for the explicit implementation?
> 
> Ah, I thought we need the implementation because of the name `noModule` JS property v.s. `nomodule` attribute name.
> But it seems that we can use `Reflect` and `ImplementedAs`. Fixed.

Oops, no, we can just use [Reflect] according to the CodeGenerator.pm.

[Yusuke Suzuki]

Created attachment 298757 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 298757 [details]
Patch

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

> Source/WebCore/dom/ScriptElement.h:112
> +    virtual bool noModuleAttributeValue() const = 0;

I think it’s better to call this hasNoModuleAttribute instead of noModuleAttributeValue.

[Yusuke Suzuki]

Comment on attachment 298757 [details]
Patch

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

>> Source/WebCore/dom/ScriptElement.h:112
>> +    virtual bool noModuleAttributeValue() const = 0;
> 
> I think it’s better to call this hasNoModuleAttribute instead of noModuleAttributeValue.

If we change this to `hasNoModuleAttribute`, I think we should change deferAttributeValue, asyncAttributeValue.
Changed.

[Yusuke Suzuki]

Created attachment 298893 [details]
Patch for landing

[Ryosuke Niwa]

Did you ever land this patch? I can't find it on trac.

[Yusuke Suzuki]

(In reply to comment #10)
> Did you ever land this patch? I can't find it on trac.

yeah, I need to land it.

[Yusuke Suzuki]

Committed r211078: <http://trac.webkit.org/changeset/211078>

[Serg Hospodarets]

Just want to make sure, will the first stable version of Safari with ES modules support also contain the "nomodule" addition?
Otherwise, the whole point may be missed: https://blog.hospodarets.com/native-ecmascript-modules-nomodule#comment-3218775686