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

extension: Fix some minor issues #12917

Merged
merged 2 commits into from
Aug 26, 2023
Merged

Conversation

n0samu
Copy link
Member

@n0samu n0samu commented Aug 23, 2023

  • Fix improper spacing around the "Open in a new tab" button due to wrongly structured HTML:
    image
  • Don't show a CORS error on the internal player page, which is not subject to CORS restrictions.

} else if (
window.location.origin === this.swfUrl!.origin ||
// The extension's internal player page is not restricted by CORS
window.location.protocol.includes("extension")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this part work for Edge?

Copy link
Member Author

@n0samu n0samu Aug 23, 2023

Choose a reason for hiding this comment

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

Yes, Edge's extension protocol is called extension:. I've never seen a Chromium-based browser that used an extension protocol not including the word "extension" in its name.

EDIT: Oh that's funny, Edge actually uses chrome-extension: internally but displays it in the address bar as extension:.

const div = document.createElement("div");
div.id = "message_overlay";
const innerDiv = document.createElement("div");
innerDiv.className = "message";
innerDiv.innerHTML = textAsParagraphs("message-cant-embed");
Copy link
Contributor

@danielhjacobs danielhjacobs Aug 24, 2023

Choose a reason for hiding this comment

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

This can be innerText or textContent now, right? Or is message-cant-embed an HTML element? We should be moving away from innerHTML.

Copy link
Contributor

@danielhjacobs danielhjacobs Aug 24, 2023

Choose a reason for hiding this comment

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

Oh no, I see textAsParagraphs returns an HTML string. It should return an HTMLDivElement though ideally, so you can just appendChild the result.

Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC this should be fixed in #12937, right? Let's merge this as-is for now.

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.

5 participants