Skip to content

Replace "instanceof" due to DOM realm mixin when Blazor is IFRAME-d #56367

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Replace "instanceof" due to DOM realm mixin when Blazor is IFRAME-d #56367

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Components/Web.JS/src/Rendering/BrowserRenderer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,10 @@ export class BrowserRenderer {
if (elementsToClearOnRootComponentRender.delete(element)) {
emptyLogicalElement(element);

if (element instanceof Comment) {
if (Object.prototype.toString.call(element) === "[object Comment]") {
// We sanitize start comments by removing all the information from it now that we don't need it anymore
// as it adds noise to the DOM.
element.textContent = '!';
(element as unknown as Comment).textContent = '!';
Comment on lines +68 to +71
Copy link
Member

Choose a reason for hiding this comment

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

This check is incorrect, the right check should be element instanceof element.ownerDocument.defaultView.Comment.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/instanceof#instanceof_and_multiple_realms

Copy link
Member

Choose a reason for hiding this comment

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

Also, rather than spread this logic all over, we should put it on a helper method isComment to ensure the check is the same across all the callsites.

Copy link
Author

Choose a reason for hiding this comment

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

Tested using element instanceof element.ownerDocument.defaultView.Comment. Result is similar to the original Microsoft implementation - the Comment element is NOT detected as such in all cases.

Here is the code and the console output when the error is present:

if (e instanceof Comment) {
  return e.parentNode;
} else if (Object.prototype.toString.call(e) === "[object Comment]") {
  console.warn("---> This *was NOT* detected as a Comment, but *is* one. element instanceof element.ownerDocument.defaultView.Comment is ", e instanceof e.ownerDocument.defaultView.Comment)
}

throw new Error("Not a valid logical element ", e)

When the error occurs, this is what is printed in the console - notice the false:
---> This *was NOT* detected as a Comment, but *is* one. Caller is reaching out for the parent node here. Btw element instanceof element.ownerDocument.defaultView.Comment is false

Copy link
Author

Choose a reason for hiding this comment

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

More console output while stopped at a breakpoint during console.warn above:
image

Copy link
Member

Choose a reason for hiding this comment

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

@GStoynev why can't we do (element instance of Comment || (window.frameElement && element instanceof window.frameElement.ownerDocument.defaultView.Comment))

Can you put up a repro as a public github repository to make sure we are looking at the same thing? I would expect element.ownerDocument to be the same as window.frameElement.ownerDocument when element is inside the iframe.

}
}

Expand Down
10 changes: 5 additions & 5 deletions src/Components/Web.JS/src/Rendering/LogicalElements.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ export function insertLogicalChild(child: Node, parent: LogicalElement, childInd
// If the child is a component comment with logical children, its children
// need to be inserted into the parent node
let nodeToInsert = child;
if (child instanceof Comment) {
if (Object.prototype.toString.call(child) === "[object Comment]") {
const existingGranchildren = getLogicalChildrenArray(childAsLogicalElement);
if (existingGranchildren?.length > 0) {
const lastNodeToInsert = findLastDomNodeInRange(childAsLogicalElement);
Expand Down Expand Up @@ -196,7 +196,7 @@ export function removeLogicalChild(parent: LogicalElement, childIndex: number):
const childToRemove = childrenArray.splice(childIndex, 1)[0];

// If it's a logical container, also remove its descendants
if (childToRemove instanceof Comment) {
if (Object.prototype.toString.call(childToRemove) === "[object Comment]") {
const grandchildrenArray = getLogicalChildrenArray(childToRemove);
if (grandchildrenArray) {
while (grandchildrenArray.length > 0) {
Expand Down Expand Up @@ -307,8 +307,8 @@ export function permuteLogicalChildren(parent: LogicalElement, permutationList:
export function getClosestDomElement(logicalElement: LogicalElement): Element | (LogicalElement & DocumentFragment) {
if (logicalElement instanceof Element || logicalElement instanceof DocumentFragment) {
return logicalElement;
} else if (logicalElement instanceof Comment) {
return logicalElement.parentNode! as Element;
} else if (Object.prototype.toString.call(logicalElement) === "[object Comment]") {
return (logicalElement as unknown as Comment).parentNode! as Element;
} else {
throw new Error('Not a valid logical element');
}
Expand All @@ -331,7 +331,7 @@ function appendDomNode(child: Node, parent: LogicalElement) {
// It does not update the logical children array of anything
if (parent instanceof Element || parent instanceof DocumentFragment) {
parent.appendChild(child);
} else if (parent instanceof Comment) {
} else if (Object.prototype.toString.call(parent) === "[object Comment]") {
const parentLogicalNextSibling = getLogicalNextSibling(parent) as any as Node;
if (parentLogicalNextSibling) {
// Since the parent has a logical next-sibling, its appended child goes right before that
Expand Down
Loading