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 enableBackgroundClick experiment #3450

Merged
merged 12 commits into from
Mar 8, 2024
22 changes: 22 additions & 0 deletions src/components/dialog-manager-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,28 @@ describes.realWin('DialogManager', (env) => {
expect(dialogManager.dialog_).to.equal(dialog);
});

it('should pass experiment as false to Dialog', async () => {
await dialogManager.openDialog();

const experimentValue = await dialogManager.dialog_
.enableBackgroundClickExperiment_;
expect(experimentValue).to.be.false;
});

it('should pass experiment as true to Dialog', async () => {
// Open dialog twice.
dialogManager = new DialogManager(
new GlobalDoc(win),
Promise.resolve(true)
);

await dialogManager.openDialog();

const experimentValue = await dialogManager.dialog_
.enableBackgroundClickExperiment_;
expect(experimentValue).to.be.true;
});

it('should re-open the same dialog', async () => {
// Open dialog twice.
const dialog1 = await dialogManager.openDialog();
Expand Down
9 changes: 8 additions & 1 deletion src/components/dialog-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,18 @@ const POPUP_Z_INDEX = 2147483647;
*/
export class DialogManager {
private readonly doc_: Doc;
private readonly enableBackgroundClickExperiment_: Promise<boolean>;
private dialog_: Dialog | null;
private openPromise_: Promise<Dialog> | null;
private readonly popupGraypane_: Graypane;
private popupWin_: Window | null;

constructor(doc: Doc) {
constructor(
doc: Doc,
enableBackgroundClickExperiment = Promise.resolve(false)
) {
this.enableBackgroundClickExperiment_ = enableBackgroundClickExperiment;

this.doc_ = doc;

this.dialog_ = null;
Expand All @@ -58,6 +64,7 @@ export class DialogManager {
if (!this.openPromise_) {
this.dialog_ = new Dialog(
this.doc_,
this.enableBackgroundClickExperiment_,
/* importantStyles */ {},
/* styles */ {},
dialogConfig
Expand Down
34 changes: 30 additions & 4 deletions src/components/dialog-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,9 @@ describes.realWin('Dialog', (env) => {

describe('dialog', () => {
beforeEach(() => {
dialog = new Dialog(globalDoc, {height: `${documentHeight}px`});
dialog = new Dialog(globalDoc, Promise.resolve(false), {
height: `${documentHeight}px`,
});
graypaneStubs = sandbox.stub(dialog.graypane_);
});

Expand Down Expand Up @@ -443,6 +445,7 @@ describes.realWin('Dialog', (env) => {
};
dialog = new Dialog(
globalDoc,
Promise.resolve(false),
{},
{},
{closeOnBackgroundClick: true, shouldDisableBodyScrolling: true}
Expand All @@ -467,6 +470,7 @@ describes.realWin('Dialog', (env) => {
};
dialog = new Dialog(
globalDoc,
Promise.resolve(false),
{},
{},
{closeOnBackgroundClick: false, shouldDisableBodyScrolling: true}
Expand Down Expand Up @@ -497,7 +501,7 @@ describes.realWin('Dialog', (env) => {
const clickFun = function () {
wasClicked = true;
};
dialog = new Dialog(globalDoc);
dialog = new Dialog(globalDoc, Promise.resolve(true));
const el = dialog.graypane_.getElement();
const openedDialog = await dialog.open(NO_ANIMATE);
await openedDialog.openView(view);
Expand All @@ -512,7 +516,13 @@ describes.realWin('Dialog', (env) => {
const clickFun = function () {
wasClicked = true;
};
dialog = new Dialog(globalDoc, {}, {}, {closeOnBackgroundClick: false});
dialog = new Dialog(
globalDoc,
Promise.resolve(true),
{},
{},
{closeOnBackgroundClick: false}
);
const el = dialog.graypane_.getElement();
const openedDialog = await dialog.open(NO_ANIMATE);
await openedDialog.openView(view);
Expand All @@ -527,7 +537,13 @@ describes.realWin('Dialog', (env) => {
const clickFun = function () {
wasClicked = true;
};
dialog = new Dialog(globalDoc, {}, {}, {closeOnBackgroundClick: true});
dialog = new Dialog(
globalDoc,
Promise.resolve(true),
{},
{},
{closeOnBackgroundClick: true}
);
const el = dialog.graypane_.getElement();
const openedDialog = await dialog.open(NO_ANIMATE);
await openedDialog.openView(view);
Expand All @@ -540,6 +556,7 @@ describes.realWin('Dialog', (env) => {
it('respects not closable', async () => {
dialog = new Dialog(
globalDoc,
Promise.resolve(true),
{},
{},
{closeOnBackgroundClick: false, shouldDisableBodyScrolling: true}
Expand All @@ -558,6 +575,7 @@ describes.realWin('Dialog', (env) => {
it('respects closable', async () => {
dialog = new Dialog(
globalDoc,
Promise.resolve(true),
{},
{},
{closeOnBackgroundClick: true, shouldDisableBodyScrolling: true}
Expand All @@ -581,6 +599,7 @@ describes.realWin('Dialog', (env) => {
it('adds swg-disable-scroll class if config specifies scrolling should be disabled', async () => {
dialog = new Dialog(
globalDoc,
Promise.resolve(false),
/* importantStyles */ {},
/* styles */ {},
{shouldDisableBodyScrolling: true}
Expand All @@ -594,6 +613,7 @@ describes.realWin('Dialog', (env) => {
it('does not add swg-disable-scroll otherwise', async () => {
dialog = new Dialog(
globalDoc,
Promise.resolve(false),
/* importantStyles */ {},
/* styles */ {},
{shouldDisableBodyScrolling: false}
Expand All @@ -609,6 +629,7 @@ describes.realWin('Dialog', (env) => {
it('removes swg-disable-scroll class', async () => {
dialog = new Dialog(
globalDoc,
Promise.resolve(false),
/* importantStyles */ {},
/* styles */ {},
{shouldDisableBodyScrolling: true}
Expand All @@ -630,6 +651,7 @@ describes.realWin('Dialog', (env) => {
beforeEach(() => {
dialog = new Dialog(
globalDoc,
Promise.resolve(false),
{height: `${documentHeight}px`},
/* styles */ {},
{maxAllowedHeightRatio: MAX_ALLOWED_HEIGHT_RATIO}
Expand Down Expand Up @@ -659,6 +681,7 @@ describes.realWin('Dialog', (env) => {
beforeEach(() => {
dialog = new Dialog(
globalDoc,
Promise.resolve(false),
{height: `${documentHeight}px`},
/* styles */ {},
{desktopConfig: {supportsWideScreen: true}}
Expand All @@ -678,6 +701,7 @@ describes.realWin('Dialog', (env) => {
beforeEach(() => {
dialog = new Dialog(
globalDoc,
Promise.resolve(false),
{height: `${documentHeight}px`},
/* styles */ {},
{iframeCssClassOverride}
Expand Down Expand Up @@ -714,6 +738,7 @@ describes.realWin('Dialog', (env) => {

dialog = new Dialog(
globalDoc,
Promise.resolve(false),
{height: `${documentHeight}px`},
/* styles */ {},
{desktopConfig: {isCenterPositioned: true}}
Expand Down Expand Up @@ -782,6 +807,7 @@ describes.realWin('Dialog', (env) => {

dialog = new Dialog(
globalDoc,
Promise.resolve(false),
{height: `${documentHeight}px`},
/* styles */ {},
{desktopConfig: {isCenterPositioned: true}}
Expand Down
34 changes: 17 additions & 17 deletions src/components/dialog.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
* limitations under the License.
*/

import {ArticleExperimentFlags} from '../runtime/experiment-flags';
import {Doc, resolveDoc} from '../model/doc';
import {FriendlyIframe} from './friendly-iframe';
import {Graypane} from './graypane';
Expand All @@ -27,7 +26,6 @@ import {
removeChildren,
removeElement,
} from '../utils/dom';
import {isExperimentOn} from '../runtime/experiments';
import {setImportantStyles, setStyles} from '../utils/style';
import {transition} from '../utils/animation';

Expand Down Expand Up @@ -117,12 +115,13 @@ export class Dialog {
/** Helps identify stale animations. */
private animationNumber_: number;
private hidden_: boolean;
private closeOnBackgroundClick_: boolean;
private closeOnBackgroundClick_?: boolean;
private previousProgressView_: View | null;
private maxAllowedHeightRatio_: number;
private positionCenterOnDesktop_: boolean;
private shouldDisableBodyScrolling_: boolean;
private desktopMediaQuery_: MediaQueryList;
private enableBackgroundClickExperiment_: Promise<Boolean>;
/** Reference to the listener that acts on changes to desktopMediaQuery. */
private desktopMediaQueryListener_: (() => void) | null;

Expand All @@ -131,6 +130,7 @@ export class Dialog {
*/
constructor(
doc: Doc,
enableBackgroundClickExperiment = Promise.resolve(false),
importantStyles: {[key: string]: string} = {},
styles: {[key: string]: string} = {},
dialogConfig: DialogConfig = {}
Expand All @@ -152,20 +152,8 @@ export class Dialog {

this.graypane_ = new Graypane(doc, Z_INDEX - 1);

// Avoid modifying the behavior of existing callers by only registering
// the click event if isClosable is set and the experiment is active.
if (
dialogConfig.closeOnBackgroundClick !== undefined &&
isExperimentOn(
this.doc_.getWin(),
ArticleExperimentFlags.BACKGROUND_CLICK_BEHAVIOR_EXPERIMENT
)
) {
this.graypane_
.getElement()
.addEventListener('click', this.onGrayPaneClick_.bind(this));
}
this.closeOnBackgroundClick_ = !!dialogConfig.closeOnBackgroundClick;
this.closeOnBackgroundClick_ = dialogConfig.closeOnBackgroundClick;
this.enableBackgroundClickExperiment_ = enableBackgroundClickExperiment;

const modifiedImportantStyles = Object.assign(
{},
Expand Down Expand Up @@ -211,6 +199,18 @@ export class Dialog {
* Opens the dialog and builds the iframe container.
*/
async open(hidden = false): Promise<Dialog> {
const enableBackgroundClickExperiment = await this
.enableBackgroundClickExperiment_;

if (
mborof marked this conversation as resolved.
Show resolved Hide resolved
enableBackgroundClickExperiment &&
this.closeOnBackgroundClick_ !== undefined
) {
this.graypane_
.getElement()
.addEventListener('click', this.onGrayPaneClick_.bind(this));
}

const iframe = this.iframe_;
if (iframe.isConnected()) {
throw new Error('already opened');
Expand Down
16 changes: 14 additions & 2 deletions src/runtime/runtime.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import {
Subscriptions,
} from '../api/subscriptions';
import {AnalyticsService} from './analytics-service';
import {ArticleExperimentFlags} from './experiment-flags';
import {ButtonApi} from './button-api';
import {Callbacks} from './callbacks';
import {ClientConfigManager} from './client-config-manager';
Expand Down Expand Up @@ -660,8 +661,6 @@ export class ConfiguredRuntime implements Deps, SubscriptionsInterface {

this.storage_ = new Storage(this.win_);

this.dialogManager_ = new DialogManager(this.doc_);

this.callbacks_ = new Callbacks();

// Start listening to Google Analytics events, if applicable.
Expand Down Expand Up @@ -691,6 +690,19 @@ export class ConfiguredRuntime implements Deps, SubscriptionsInterface {
integr.enableDefaultMeteringHandler || false
);

const backgroundClickExp = this.entitlementsManager_
.getArticle()
.then(
(article) =>
!!this.entitlementsManager_
.parseArticleExperimentConfigFlags(article)
.includes(
ArticleExperimentFlags.BACKGROUND_CLICK_BEHAVIOR_EXPERIMENT
)
);

this.dialogManager_ = new DialogManager(this.doc_, backgroundClickExp);

this.clientConfigManager_ = new ClientConfigManager(
this, // See note about 'this' above
pageConfig.getPublicationId(),
Expand Down
Loading