Skip to content

Conversation

calebcordry
Copy link
Member

Part of #27189

// certain elements from the detached DOM.
const nextElement = element.nextElementSibling;

switch (element.tagName) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is tagName guaranteed to be upperCase across browsers?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From https://developer.mozilla.org/en-US/docs/Web/API/Element/tagName

  • For DOM trees which represent HTML documents, the returned tag name is always in the canonical upper-case form. For example, tagName called on a
    element returns "DIV".
  • The tag names of elements in an XML DOM tree are returned in the same case in which they're written in the original XML file. If an XML document includes a tag "", then the tagName property's value is "SomeTag".

I don't think we need to worry about XML case here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

asked because I saw many usages of tagName.toUpperCase() in our code base.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense, you think I should add it just to be safe?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would do so, just to be safer. (browsers can be weird)

@lgtm-com
Copy link

lgtm-com bot commented Jul 9, 2020

This pull request introduces 4 alerts when merging 73c49f2 into 8e03ac2 - view on LGTM.com

new alerts:

  • 2 for Inefficient regular expression
  • 1 for Incomplete regular expression for hostnames
  • 1 for Incomplete string escaping or encoding

@lgtm-com
Copy link

lgtm-com bot commented Jul 9, 2020

This pull request introduces 4 alerts when merging a7e9176 into 8e03ac2 - view on LGTM.com

new alerts:

  • 2 for Inefficient regular expression
  • 1 for Incomplete regular expression for hostnames
  • 1 for Incomplete string escaping or encoding

// certain elements from the detached DOM.
const nextElement = element.nextElementSibling;

switch (element.tagName) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

asked because I saw many usages of tagName.toUpperCase() in our code base.

@lgtm-com
Copy link

lgtm-com bot commented Jul 9, 2020

This pull request introduces 4 alerts when merging 54d4884 into 81f88ce - view on LGTM.com

new alerts:

  • 2 for Inefficient regular expression
  • 1 for Incomplete regular expression for hostnames
  • 1 for Incomplete string escaping or encoding

@calebcordry
Copy link
Member Author

Adding @rsimha as owners bot suggestion for build-system/tasks/presubmit-checks.js

@calebcordry calebcordry requested a review from rsimha July 10, 2020 19:33
@calebcordry calebcordry merged commit 12cf5af into ampproject:master Jul 10, 2020
@calebcordry calebcordry deleted the validate-head branch July 10, 2020 20:05
mszylkowski pushed a commit to mszylkowski/amphtml that referenced this pull request Jul 22, 2020
* head validation

* rename

* add tests

* docs and types

* allow preloadExtension

* comments

* rename func

* dep check

* dep check again

* toUpperCase
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants