1560744 - customElements.define() throw unclear `NotSupportedError` error message for already defined element

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Clément]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0

Steps to reproduce:

customElements.define('cus-tom', class a1{})
customElements.define('cus-tom', class a2{})

Actual results:

I get this error message:

NotSupportedError: Operation is not supported

Expected results:

A clearer message.
example (from chrome):

Uncaught DOMException: Failed to execute 'define' on 'CustomElementRegistry': this name has already been used with this registry

Comment 1

[Virginia (please send need info request to Brindusa Tot)]

Hi Clément

Could you share a test page and further steps to reproduce this issue?

Please download the latest Firefox Nightly from here: https://nightly.mozilla.org/ and test if it's reproducible.

If you still have the issue please create a new profile, you have the steps here:https://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-firefox-profiles?redirectlocale=en-US&redirectslug=Managing-profiles#w_starting-the-profile-manager

Also, please verify if the issue is reproducible in safe mode, here is a link that can help you:
https://support.mozilla.org/en-US/kb/troubleshoot-firefox-issues-using-safe-mode

If there is any other info you can provide to help us figure out how we can can reproduce the issue please share them.

Thanks for your contribution.

Comment 2

[Clément]

Hi Virginia

Yes, I confirm this bug on Nighty@latest with a new profile in safe mode.

You can test here: https://jsbin.com/dekewajura/edit?js,console

or simply by copying and pasting this into the console:

customElements.define('cus-tom', class a1{})
customElements.define('cus-tom', class a2{})

Comment 3

[Virginia (please send need info request to Brindusa Tot)]

Hi Clément,

Thanks for your feedback. I was able to reproduce the error on my end by testing on the page you provided on the latest Nightly 69.0a1 (2019-07-01) (64-bit).
I'll add a product and component so the devs can take a look at it. If this is not the right component feel free to change it.

Regards,

Comment 4

[Hsin-Yi Tsai (she/her)[:hsinyi]]

John, do you have ideas how to fix this? Could you help here?

Comment 5

[John Dai[:jdai]]

Sure, I'll take this.

Comment 6

[John Dai[:jdai]]

It seems that spec didn't define error message[1] for DOM exception, maybe we can allow customizing DOM exception error message? However, it seems more general question for DOM exception, I'd invite :bz, :anne to give more thought for this.

[1] https://heycam.github.io/webidl/#idl-exceptions

Comment 7

[Anne (:annevk)]

I think it's fine to do what OP asks for.

There's been an issue in the past with sites parsing specific exception messages and expecting that to work. This suggests we should perhaps standardize on the exact messages for each exception we throw. And to the extent we want to provide detailed contextual information there that would reveal more about the user, we should limit that to the console somehow by giving sites a simpler version that does not have such information. This is largely a separate effort and I don't think we need to block this on that. (I'm not sure if we're tracking this anywhere, it's mostly came up in conversation in various places.)

Comment 8

[Boris Zbarsky [:bzbarsky]]

It seems that spec didn't define error message[1] for DOM exception

Yes, error messages are not specified. It's fine, and often desirable, to make them more informative as needed.

maybe we can allow customizing DOM exception error message

We already allow that. This code in CustomElementRegistry:

  if (!nsContentUtils::IsCustomElementName(nameAtom, nameSpaceID)) {
    aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);

Can (and should) instead do:

  if (!nsContentUtils::IsCustomElementName(nameAtom, nameSpaceID)) {
    aRv.ThrowDOMException(NS_ERROR_DOM_SYNTAX_ERR, whatever-message-string-you-want-here);

Comment 9

[Boris Zbarsky [:bzbarsky]]

In general, it's worth auditing uses of ErrorResult::Throw and replacing the ones that are implementing spec-required exceptions with something that has a sane error message...

Comment 10

[John Dai[:jdai]]

Attachment: Bug 1560744 - Part 1: Add clearer error message for custom elements;

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aaf0807dcb4d
Add clearer message for custom elements; r=smaug

Comment 12

[John Dai[:jdai]]

Attachment: Bug 1560744 - Part 2: Add clearer error message for duplicate custom element name;

Comment 13

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/aaf0807dcb4d

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Is there a user impact which justifies backport consideration here or can this fix ride the trains?

Comment 15

[John Dai[:jdai]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #14)

Is there a user impact which justifies backport consideration here or can this fix ride the trains?

IMHO, we can fix ride the trains for better error messages.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/94a7077ef06e
Part 2: Add clearer error message for duplicate custom element name;

Comment 17

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/94a7077ef06e