-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
core(page-functions): don't try to clone a ShadowRoot #9079
Changes from 1 commit
a5f9343
5de9be9
bf80f17
f34fd58
51449a0
a1528b6
13d4753
29b209d
cbd0e48
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -113,18 +113,20 @@ function getElementsInDocument(selector) { | |||||
*/ | ||||||
/* istanbul ignore next */ | ||||||
function getOuterHTMLSnippet(element, ignoreAttrs = []) { | ||||||
const clone = element.cloneNode(); | ||||||
|
||||||
ignoreAttrs.forEach(attribute =>{ | ||||||
clone.removeAttribute(attribute); | ||||||
}); | ||||||
|
||||||
const reOpeningTag = /^[\s\S]*?>/; | ||||||
const match = clone.outerHTML.match(reOpeningTag); | ||||||
|
||||||
return (match && match[0]) || ''; | ||||||
try { | ||||||
const clone = element.cloneNode(); | ||||||
ignoreAttrs.forEach(attribute =>{ | ||||||
clone.removeAttribute(attribute); | ||||||
}); | ||||||
const reOpeningTag = /^[\s\S]*?>/; | ||||||
const match = clone.outerHTML.match(reOpeningTag); | ||||||
return (match && match[0]) || ''; | ||||||
} catch (error) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
we name our unused variables |
||||||
return element.localName; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just to match the expected output of this function under normal conditions There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks a lot for the feedback @patrickhulce . There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's expected :) Steps:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you mean something like this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. meh either way, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I meant something like it('should handle dom nodes that cannot be inspected', () => {
const element = dom.createElement('div');
element.cloneNode = () => { throw new Erorr('oops!') };
assert.equal(pageFunctions.getOuterHTMLSnippet(element), '<div>');
}); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @patrickhulce I commited the test case :) |
||||||
} | ||||||
} | ||||||
|
||||||
|
||||||
/** | ||||||
* Computes a memory/CPU performance benchmark index to determine rough device class. | ||||||
* @see https://docs.google.com/spreadsheets/d/1E0gZwKsxegudkjJl8Fki_sOwHKpqgXwt8aBAfuUaB8A/edit?usp=sharing | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the types on this whole function get kind of broken if we aren't really accepting just
Element
.removeAttribute
andouterHTML
(and more or lesslocalName
) exist only onElement
.Maybe we should type it as
@param {Element|ShadowRoot} element
and (if we don't need thatlocalName
check) have the conditional be justif (element instanceof ShadowRoot) element = element.host;
(with a comment somewhere explaining that this function handles the ShadowRoot case).It's still playing a little fast and loose with types (it doesn't help that the tsc built in types don't recognize
ShadowRoot
as a global value), but it simplifies things a bit.