1275832 - Implement custom element name validation for custom element

Status: RESOLVED FIXED

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

Description

[John Dai[:jdai]]

According to session 4.13.3 core concepts [1], we need to implement a function to validate custom element name.


[1] https://html.spec.whatwg.org/multipage/scripting.html#valid-custom-element-name

Comment 1

[Andrew Overholt [:overholt]]

(I'm marking all the Custom Elements bugs as "backlog" but that's just to indicate they're not something we're fixing urgently and no comment on priority or anything.)

Comment 2

[John Dai[:jdai]]

Attachment: wip, v1

Comment 3

[John Dai[:jdai]]

Attachment: wip, v2

Comment 4

[John Dai[:jdai]]

Attachment: Bug 1275832 - Implement custom element name validation for custom elements. v1

Comment 5

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Comment on attachment 8788848 [details] [diff] [review]
Bug 1275832 - Implement custom element name validation for custom elements. v1

Review of attachment 8788848 [details] [diff] [review]:
-----------------------------------------------------------------

Spec says name must not be any of the following: annotation-xml,

::: dom/base/nsContentUtils.cpp
@@ +2829,5 @@
> +  // A valid custom element name is a sequence of characters name which
> +  // must match the PotentialCustomElementName production:
> +  // PotentialCustomElementName ::= [a-z] (PCENChar)* '-' (PCENChar)*
> +  const char16_t* name = aName->GetUTF16String();
> +  if (name[0] < 'a' || name[0] > 'z') {

We still run this check for empty string, it is not right.
I think we should do some checks for empty string.

@@ +2837,5 @@
> +  int i = 1;
> +  int len = aName->GetLength();
> +  bool hasDash = false;
> +
> +  do {

The name contains only one character, e.g. "a", will still run into this do-while, it is not right and necessary. Could we use while-loop or for-loop here?

@@ +2838,5 @@
> +  int len = aName->GetLength();
> +  bool hasDash = false;
> +
> +  do {
> +    if (name[i] >= 0xD800 && name[i] <= 0xDBFF) {

Add some comments about this block is for UTF16 surrogate pairs.

@@ +2839,5 @@
> +  bool hasDash = false;
> +
> +  do {
> +    if (name[i] >= 0xD800 && name[i] <= 0xDBFF) {
> +      char16_t high_surrogates = name[i];

Nit: camelCase for variable names and remove 's', e.g. highSurrogate. here and elsewhere.

@@ +2840,5 @@
> +
> +  do {
> +    if (name[i] >= 0xD800 && name[i] <= 0xDBFF) {
> +      char16_t high_surrogates = name[i];
> +      if ((i+1 < len) && name[i+1] >= 0xDC00 && name[i+1] <= 0xDFFF) {

1. Nit: Space around operators, here and elsewhere.
2. Bail out as early as possible.

   e.g.
   if ((i + 1) > len || name[i + 1] < 0xC00 || name[i + 1] > 0xDFFF) {
     return false;
   }

@@ +2841,5 @@
> +  do {
> +    if (name[i] >= 0xD800 && name[i] <= 0xDBFF) {
> +      char16_t high_surrogates = name[i];
> +      if ((i+1 < len) && name[i+1] >= 0xDC00 && name[i+1] <= 0xDFFF) {
> +        char16_t low_surrogates = name[i+1];

Nit: s/low_surrogates/lowSurrogate/

@@ +2870,5 @@
> +         (name[i] >= 0x2070 && name[i] <= 0x218F) ||
> +         (name[i] >= 0x2C00 && name[i] <= 0x2FEF) ||
> +         (name[i] >= 0x3001 && name[i] <= 0xD7FF) ||
> +         (name[i] >= 0xF900 && name[i] <= 0xFDCF) ||
> +         (name[i] >= 0xFDF0 && name[i] <= 0xFFFD)) {

Use reverse logic and bail out early.

Comment 6

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

(In reply to Edgar Chen [:edgar][:echen] from comment #5)
> Comment on attachment 8788848 [details] [diff] [review]
> Bug 1275832 - Implement custom element name validation for custom elements.
> v1
> 
> Review of attachment 8788848 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Spec says name must not be any of the following: annotation-xml,
Please ignore this, I missed removing it.

Comment 7

[John Dai[:jdai]]

Attachment: Bug 1275832 - Implement custom element name validation for custom elements. v2

Addressed comment #2.

Comment 8

[William Chen [:wchen]]

Comment on attachment 8789317 [details] [diff] [review]
Bug 1275832 - Implement custom element name validation for custom elements. v2

Review of attachment 8789317 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsContentUtils.cpp
@@ +2849,5 @@
> +      char16_t highSurrogate = name[i];
> +      char16_t lowSurrogate = name[i+1];
> +      // Merged two 16-bit surrogate pairs into code point.
> +      char32_t code = 0x10000 + ((highSurrogate - 0xD800) << 10) +
> +                      (lowSurrogate - 0xDC00);

There are some helper macros in nsCharTraits.h that will do the high/low surrogate check and combine into a code point.

Comment 9

[John Dai[:jdai]]

Attachment: Bug 1275832 - Implement custom element name validation for custom elements. (Final) r=wchen

Addressed comment #8. Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4db12862a9b473b45a193dbfeaed74c243e7f0dc&filter-tier=1

Comment 10

[John Dai[:jdai]]

Attachment: Bug 1275832 - Implement custom element name validation for custom elements. (Final) r=wchen

The previous patch hit testing/web-platform/meta/html/semantics/interfaces.html web-platform-test, and updated a new patch to address them.
Try looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5629778c1a775cb26a55cd38afabc1f1c58c5c3&filter-tier=1&selectedJob=27763482

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b695086b9861
Implement custom element name validation for custom elements. r=wchen

Comment 12

[Carsten Book [:Tomcat]]

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