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
11 changes: 9 additions & 2 deletions 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 Down Expand Up @@ -60,7 +66,8 @@ export class DialogManager {
this.doc_,
/* importantStyles */ {},
/* styles */ {},
dialogConfig
dialogConfig,
this.enableBackgroundClickExperiment_
);
this.openPromise_ = this.dialog_.open(hidden);
}
Expand Down
33 changes: 28 additions & 5 deletions src/components/dialog-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -497,7 +497,13 @@ describes.realWin('Dialog', (env) => {
const clickFun = function () {
wasClicked = true;
};
dialog = new Dialog(globalDoc);
dialog = new Dialog(
globalDoc,
undefined,
undefined,
undefined,
Promise.resolve(true)
);
const el = dialog.graypane_.getElement();
const openedDialog = await dialog.open(NO_ANIMATE);
await openedDialog.openView(view);
Expand All @@ -512,7 +518,13 @@ describes.realWin('Dialog', (env) => {
const clickFun = function () {
wasClicked = true;
};
dialog = new Dialog(globalDoc, {}, {}, {closeOnBackgroundClick: false});
dialog = new Dialog(
globalDoc,
{},
{},
{closeOnBackgroundClick: false},
Promise.resolve(true)
);
const el = dialog.graypane_.getElement();
const openedDialog = await dialog.open(NO_ANIMATE);
await openedDialog.openView(view);
Expand All @@ -527,7 +539,14 @@ describes.realWin('Dialog', (env) => {
const clickFun = function () {
wasClicked = true;
};
dialog = new Dialog(globalDoc, {}, {}, {closeOnBackgroundClick: true});
dialog = new Dialog(
globalDoc,

{},
{},
{closeOnBackgroundClick: true},
Promise.resolve(true)
);
const el = dialog.graypane_.getElement();
const openedDialog = await dialog.open(NO_ANIMATE);
await openedDialog.openView(view);
Expand All @@ -540,9 +559,11 @@ describes.realWin('Dialog', (env) => {
it('respects not closable', async () => {
dialog = new Dialog(
globalDoc,

{},
{},
{closeOnBackgroundClick: false, shouldDisableBodyScrolling: true}
{closeOnBackgroundClick: false, shouldDisableBodyScrolling: true},
Promise.resolve(true)
);
const el = dialog.graypane_.getElement();
const openedDialog = await dialog.open(NO_ANIMATE);
Expand All @@ -558,9 +579,11 @@ describes.realWin('Dialog', (env) => {
it('respects closable', async () => {
dialog = new Dialog(
globalDoc,

{},
{},
{closeOnBackgroundClick: true, shouldDisableBodyScrolling: true}
{closeOnBackgroundClick: true, shouldDisableBodyScrolling: true},
Promise.resolve(true)
);

const el = dialog.graypane_.getElement();
Expand Down
40 changes: 22 additions & 18 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 @@ -133,7 +132,8 @@ export class Dialog {
doc: Doc,
importantStyles: {[key: string]: string} = {},
styles: {[key: string]: string} = {},
dialogConfig: DialogConfig = {}
dialogConfig: DialogConfig = {},
enableBackgroundClickExperiment = Promise.resolve(false)
) {
this.doc_ = doc;

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,22 @@ export class Dialog {
* Opens the dialog and builds the iframe container.
*/
async open(hidden = false): Promise<Dialog> {
// If this experiment is active, the behavior of the grey background
// changes. If closable, clicking the background closes the dialog. If not
// closable, clicking the background now prevents you from clicking links
// on the main page.
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