Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Avoid IE problems by using nodeName instead of tagName #1219

Merged
merged 3 commits into from
Nov 12, 2018

Conversation

jeeyyy
Copy link
Contributor

@jeeyyy jeeyyy commented Nov 2, 2018

This is a maintenance PR, to update node.tagName usages to node.nodeName.

Did not update the usages in:

  • lib/core/utils/css-parser.js - as this is a copy/pasted (imported) module.
  • lib/core/utils/qsa.js - as this seems to break a lot of tests.

Closes issue:

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

  • Follows the commit message policy, appropriate for next version
  • Has documentation updated, a DU ticket, or requires no documentation change
  • Includes new tests, or was unnecessary
  • Code is reviewed for security by: << Marcy Sutton >>

@jeeyyy jeeyyy requested a review from a team as a code owner November 2, 2018 15:51
@marcysutton
Copy link
Contributor

I'm seeing additional usage in commons/color/get-background-color, should that be updated as well?

@@ -5,7 +5,7 @@ if (ariaHeadingLevel !== null) {
return true;
}

var headingLevel = node.tagName.match(/H(\d)/);
var headingLevel = node.nodeName.match(/H(\d)/);
Copy link
Contributor

Choose a reason for hiding this comment

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

do you need toUpperCase() here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am going to leave this on as is (no need to uppercase).

Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree. In xhtml nodeName is lower case, which means this won't match.

@jeeyyy
Copy link
Contributor Author

jeeyyy commented Nov 5, 2018

@marcysutton - the usage in commons/color/get-background-color, seems to me like an object with property tagName rather than an HTMLElement itself. Hence did not change it (as doing so broke a lot of tests).

@jeeyyy jeeyyy requested review from marcysutton and a team November 5, 2018 09:59
Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

@WilcoFiers WilcoFiers changed the title fix: replace node.tagName usage with node.nodeName fix: Avoid IE problems by using nodeName instead of tagName Nov 12, 2018
@WilcoFiers WilcoFiers merged commit cf86ff5 into develop Nov 12, 2018
@WilcoFiers WilcoFiers deleted the replace-tagname branch November 12, 2018 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants