Skip to content

Commit

Permalink
Improvement: Allow disconnect from 'Connecting', with limits. (#147)
Browse files Browse the repository at this point in the history
This PR: 
- Disables the toggle when the extension is connecting. 
   **Why:** 
- Button mashing on the toggle when the extension is 'connecting'
(meaning the VPN client is transitioning from StateOn to StateOnPartial)
results in wonky/unexpected behavior.
- The toggle already appears disabled when connecting. You wouldn't
expect clicking on it to do anything.
- Adds a 15 second timeout, after which the extension toggle is
re-enabled (actually and visibly)
**Why:** 
- If the toggle remains disabled, and the client never successfully
transitions to 'StateOnPartial', the extension is stuck in the
connecting state.
  • Loading branch information
lesleyjanenorton authored Dec 16, 2024
1 parent 1ffa427 commit cd17527
Show file tree
Hide file tree
Showing 4 changed files with 49 additions and 9 deletions.
20 changes: 20 additions & 0 deletions src/background/extensionController/extensionController.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export class ExtensionController extends Component {
static properties = {
state: PropertyType.Bindable,
toggleConnectivity: PropertyType.Function,
allowDisconnect: PropertyType.Bindable,
};

/**
Expand All @@ -51,6 +52,14 @@ export class ExtensionController extends Component {
async init() {}

toggleConnectivity() {
if (
this.#mState.value.state === "Connecting" &&
this.clientState.state === "Disabled"
) {
this.#mState.set(new StateFirefoxVPNDisabled(true));
this.vpnController.postToApp("deactivate");
return;
}
if (this.#mState.value.enabled) {
// We are turning off the extension

Expand All @@ -73,6 +82,12 @@ export class ExtensionController extends Component {
}

this.#mState.set(new StateFirefoxVPNConnecting());

this.#mAllowDisconnect.value = false;
setTimeout(() => {
this.#mAllowDisconnect.value = true;
}, 10000);

// Send activation to client and wait for response
this.vpnController.postToApp("activate");
}
Expand All @@ -81,6 +96,10 @@ export class ExtensionController extends Component {
return this.#mState.readOnly;
}

get allowDisconnect() {
return this.#mAllowDisconnect.readOnly;
}

/**
*
* @param {VPNState} newClientState
Expand Down Expand Up @@ -137,5 +156,6 @@ export class ExtensionController extends Component {
}

#mState = property(new FirefoxVPNState());
#mAllowDisconnect = property(false);
mKeepAliveConnection = false;
}
21 changes: 12 additions & 9 deletions src/components/prefab-screens.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@ const sumoLink =
"https://support.mozilla.org/products/firefox-private-network-vpn";

const closeAfter = (f) => {
if(f){
f();
}
window.close();
}
if (f) {
f();
}
window.close();
};

const defineMessageScreen = (
tag,
Expand All @@ -31,7 +31,7 @@ const defineMessageScreen = (
primaryAction,
onPrimaryAction,
secondaryAction = tr("getHelp"),
onSecondaryAction = () => closeAfter (()=>open(sumoLink)),
onSecondaryAction = () => closeAfter(() => open(sumoLink)),
totalPages = 0,
currentPage = 0
) => {
Expand Down Expand Up @@ -73,7 +73,8 @@ defineMessageScreen(
tr("bodySubscribeNow"),
tr("btnSubscribeNow"),
() => {
() => closeAfter (()=>open("https://www.mozilla.org/products/vpn#pricing"));
() =>
closeAfter(() => open("https://www.mozilla.org/products/vpn#pricing"));
}
);

Expand All @@ -84,7 +85,8 @@ defineMessageScreen(
tr("bodyNeedsUpdate2"),
tr("btnDownloadNow"),
() => {
() => closeAfter (()=>open("https://www.mozilla.org/products/vpn/download/"));
() =>
closeAfter(() => open("https://www.mozilla.org/products/vpn/download/"));
}
);

Expand All @@ -105,7 +107,8 @@ defineMessageScreen(
`,
tr("btnDownloadNow"),
() => {
() => closeAfter (()=>open("https://www.mozilla.org/products/vpn/download/"));
() =>
closeAfter(() => open("https://www.mozilla.org/products/vpn/download/"));
}
);

Expand Down
12 changes: 12 additions & 0 deletions src/components/vpncard.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export class VPNCard extends LitElement {
stability: { type: String },
hasContext: { type: Boolean },
connecting: { type: Boolean },
allowDisconnect: { type: Boolean },
};

constructor() {
Expand All @@ -43,6 +44,7 @@ export class VPNCard extends LitElement {
this.connectedSince = 0;
this.stability = VPNState.Stable;
this.connecting = false;
this.allowDisconnect = false;
}
#intervalHandle = null;
#shieldElement = createRef();
Expand Down Expand Up @@ -81,6 +83,7 @@ export class VPNCard extends LitElement {
box: true,
on: this.enabled,
connecting: this.connecting,
allowDisconnect: this.allowDisconnect,
unstable: this.enabled && this.stability === VPNState.Unstable,
noSignal: this.enabled && this.stability === VPNState.NoSignal,
stable:
Expand Down Expand Up @@ -240,6 +243,15 @@ export class VPNCard extends LitElement {
background: var(--main-card-background);
box-shadow: var(--box-shadow-on);
}
.box.connecting .pill {
pointer-events: none;
}
.box.connecting.allowDisconnect .pill {
pointer-events: all;
opacity: 1;
}
main,
footer {
display: flex;
Expand Down
5 changes: 5 additions & 0 deletions src/ui/browserAction/popupPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ export class BrowserActionPopup extends LitElement {
_siteContext: { type: Object },
hasSiteContext: { type: Boolean },
_siteContexts: { type: Array },
allowDisconnect: { type: Boolean },
alerts: { type: Array },
};

Expand All @@ -90,6 +91,9 @@ export class BrowserActionPopup extends LitElement {
this.updatePage();
});
this.updatePage();
extController.allowDisconnect.subscribe((s) => {
this.allowDisconnect = s;
});
}
updatePage() {
Utils.getCurrentTab().then(async (tab) => {
Expand Down Expand Up @@ -215,6 +219,7 @@ export class BrowserActionPopup extends LitElement {
.stability=${this.vpnState?.connectionHealth}
.hasContext=${this._siteContext}
.connecting=${this.extState?.connecting}
.allowDisconnect=${this.allowDisconnect}
></vpn-card>
${this.locationSettings()}
</main>
Expand Down

0 comments on commit cd17527

Please sign in to comment.