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

web: Fix the config negotation between self-hosted and the extension #17346

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
57 changes: 37 additions & 20 deletions web/packages/core/src/internal/player/inner.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -164,8 +164,11 @@ export class InnerPlayer {
// not show the context menu.
private _suppressContextMenu = false;

// The effective config loaded upon `.load()`.
public loadedConfig?: URLLoadOptions | DataLoadOptions;
// The options loaded upon `.load()`.
private options?: URLLoadOptions | DataLoadOptions;

// Whether or not `.load()` is called as a result of a polyfill.
private isPolyfillElement?: boolean;

private swfUrl?: URL;
private instance: RuffleHandle | null;
Expand Down Expand Up @@ -764,7 +767,8 @@ export class InnerPlayer {
}

/**
* Reloads the player, as if you called {@link RufflePlayer.load} with the same config as the last time it was called.
* Reloads the player, as if you called {@link RufflePlayer.load} with a similar config as the last time it was called,
* but possibly changed if `window.RufflePlayer.config` or `window.RufflePlayer.extensionConfig` has since changed.
*
* If this player has never been loaded, this method will return an error.
*/
Expand Down Expand Up @@ -813,26 +817,13 @@ export class InnerPlayer {
}

try {
this.loadedConfig = {
...DEFAULT_CONFIG,
// The default allowScriptAccess value for polyfilled elements is samedomain.
...(isPolyfillElement && "url" in options
? {
allowScriptAccess: parseAllowScriptAccess(
"samedomain",
options.url,
)!,
}
: {}),
...(window.RufflePlayer?.config ?? {}),
...this.config,
...options,
};
this.options = options;
this.isPolyfillElement = isPolyfillElement;

// Pre-emptively set background color of container while Ruffle/SWF loads.
if (
this.loadedConfig.backgroundColor &&
this.loadedConfig.wmode !== WindowMode.Transparent
this.loadedConfig?.backgroundColor &&
this.loadedConfig?.wmode !== WindowMode.Transparent
Comment on lines +825 to +826
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the new getter.

) {
this.container.style.backgroundColor =
this.loadedConfig.backgroundColor;
Expand Down Expand Up @@ -865,6 +856,32 @@ export class InnerPlayer {
}
}

// The effective config loaded upon `load`, possibly changing if `window.RufflePlayer` changes later.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

loadedConfig needs to be able to change after load in the event that load is run either before the extension loads or before a self-hosted script that sets window.RufflePlayer.config runs.

get loadedConfig(): URLLoadOptions | DataLoadOptions | undefined {
if (this.options) {
return {
...DEFAULT_CONFIG,
// The default allowScriptAccess value for polyfilled elements is samedomain.
...(this.isPolyfillElement && "url" in this.options
? {
allowScriptAccess: parseAllowScriptAccess(
"samedomain",
this.options.url,
)!,
}
: {}),
...(window.RufflePlayer?.config ?? {}),
// This will make extension config take precedence, if I move this one line higher self-hosted config will take precedence
// Let the discussions begin
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment is here to facilitate discussion. Start discussing below. This has come up before. Site owners have come out against it, mainly arguing against showSwfDownload being able to be changed by end-users. However, it is true that end users can usually just check the network tab, making this just a convenience method.

Copy link
Member

Choose a reason for hiding this comment

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

The extension config taking precedence is fine with me 👍

...(window.RufflePlayer?.extensionConfig ?? {}),
...this.config,
...this.options,
};
} else {
return undefined;
}
}

/**
* Plays or resumes the movie.
*/
Expand Down
3 changes: 3 additions & 0 deletions web/packages/core/src/public-api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ declare global {
*/
export interface PublicAPILike {
config?: DataLoadOptions | URLLoadOptions | object;
extensionConfig?: DataLoadOptions | URLLoadOptions | object;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Two separate configs, one for the extension and the other for self-hosted, ensures the values of one can not override the values of the other. We can ensure the values in the extension don't override the values of self-hosted without a separate config, as we control the way the values in the extension config are set. However, we can not do the same in reverse, as we can't dictate how self-hosted config values are set.

sources?: Record<string, SourceAPI>;
invoked?: boolean;
newestName?: string | null;
Expand All @@ -46,6 +47,7 @@ export class PublicAPI implements PublicAPILike {
* The configuration object used when Ruffle is instantiated.
*/
config: DataLoadOptions | URLLoadOptions | object;
extensionConfig: DataLoadOptions | URLLoadOptions | object;
sources: Record<string, SourceAPI>;
invoked: boolean;
newestName: string | null;
Expand All @@ -68,6 +70,7 @@ export class PublicAPI implements PublicAPILike {
public constructor(prev?: PublicAPILike | null) {
this.sources = prev?.sources || {};
this.config = prev?.config || {};
this.extensionConfig = prev?.extensionConfig || {};
this.invoked = prev?.invoked || false;
this.newestName = prev?.newestName || null;

Expand Down
12 changes: 8 additions & 4 deletions web/packages/extension/src/content.ts
Original file line number Diff line number Diff line change
Expand Up @@ -198,14 +198,18 @@ function isXMLDocument(): boolean {
}
}
});

const autoStartOptions = options.autostart
? {
autoplay: "on",
unmuteOverlay: "hidden",
splashScreen: false,
}
: {};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea here, much like explicitOptions, is to only set these options if autostart is not the default (false), so if a self-hosted config sets autoplay to "on", for example, it's not ignored when the extension is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not important if the self-hosted config takes precedence over the extension toggles as is currently the case.

await sendMessageToPage({
type: "load",
config: {
...explicitOptions,
autoplay: options.autostart ? "on" : "auto",
unmuteOverlay: options.autostart ? "hidden" : "visible",
splashScreen: !options.autostart,
...autoStartOptions,
},
});
})();
6 changes: 1 addition & 5 deletions web/packages/extension/src/ruffle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,8 @@ function handleMessage(message: Message) {
if (window.RufflePlayer === undefined) {
window.RufflePlayer = {};
}
if (window.RufflePlayer.config === undefined) {
window.RufflePlayer.config = {};
}
window.RufflePlayer.config = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

window.RufflePlayer.config is now explicitly the self-hosted config.

window.RufflePlayer.extensionConfig = {
...message.config,
...window.RufflePlayer.config,
openInNewTab,
};
installRuffle("extension");
Expand Down