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

VPN-4687: Add DNS settings help sheet #8864

Merged
merged 14 commits into from
Jan 18, 2024
19 changes: 13 additions & 6 deletions nebula/ui/components/MZHelpSheet.qml
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import Mozilla.Shared 1.0
/*
MZHelpSheet {
title: "MZHelpSheet"
iconSource: "qrc:/ui/resources/connection-info-dark.svg"

model: [
{type: MZHelpSheet.BlockType.Title, text: "title"},
Expand All @@ -39,7 +38,6 @@ import Mozilla.Shared 1.0
MZBottomSheet {
id: bottomSheet

property alias iconSource: icon.source
property alias title: title.text
required property var model

Expand All @@ -65,6 +63,7 @@ MZBottomSheet {
id: headerLayout

Layout.topMargin: 8
Layout.preferredWidth: parent.width

spacing: 0

Expand All @@ -82,7 +81,8 @@ MZBottomSheet {
id: icon
anchors.centerIn: parent

sourceSize.width: MZTheme.theme.iconSize
source: "qrc:/nebula/resources/tip-filled.svg"
Copy link
Contributor

Choose a reason for hiding this comment

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

Was the iconSource property alias removed because MZHelpSheet is expected to always use tip-filled.svg?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, exactly.

sourceSize.width: MZTheme.theme.iconSize * 1.5

mirror: MZLocalizer.isRightToLeft
fillMode: Image.PreserveAspectFit
Expand All @@ -96,24 +96,31 @@ MZBottomSheet {
Layout.topMargin: MZTheme.theme.windowMargin / 2
Layout.leftMargin: 8
Layout.alignment: Qt.AlignTop
Layout.fillWidth: true

verticalAlignment: Text.AlignVCenter
lineHeightMode: Text.FixedHeight
lineHeight: 24
elide: Text.ElideRight
}

Item {
Layout.fillWidth: true
}

MZIconButton {
id: iconButton

Layout.rightMargin: MZTheme.theme.windowMargin / 2

Layout.preferredHeight: MZTheme.theme.rowHeight
Layout.preferredWidth: MZTheme.theme.rowHeight

//Hacky workaround because for some reason, when opening a sheet via
//the right menu button in MZMenu, this close button is in a
//"Hovered" state, even though it wasn't hovered
Copy link
Contributor

@vinocher vinocher Jan 2, 2024

Choose a reason for hiding this comment

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

Could this be because the mouseup event that ends the hovered state is sent to the help sheet, so the right menu button's screen doesn't receive it and assumes that it is still hovered?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is the "X" button on the help sheet that is in the hovered state when the right menu button on the source screen is clicked.

Am I misunderstanding your comment?

Copy link
Member

Choose a reason for hiding this comment

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

@vinocher are you asking if the help sheet is "stealing" the mouseup event? Seems plausible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I was wondering it the mouse down and up are going to two different targets, causing this issue. I don't think anything needs to be addressed in this comment.

//Likely something to do with both icon buttons being in the same position
//Relative to their parent (top right corner)
mouseArea.hoverEnabled: bottomSheet.opened

onClicked: bottomSheet.close()

accessibleName: MZI18n.GlobalClose
Expand All @@ -133,7 +140,7 @@ MZBottomSheet {
Rectangle {
Layout.topMargin: 8
Layout.preferredHeight: MZTheme.theme.dividerHeight
Layout.preferredWidth: parent.width
Layout.fillWidth: true

color: MZTheme.colors.grey10
}
Expand Down
1 change: 1 addition & 0 deletions nebula/ui/components/MZIconButton.qml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ MZButtonBase {
property bool skipEnsureVisible: false
property var accessibleName
property var buttonColorScheme: MZTheme.theme.iconButtonLightBackground
property alias mouseArea: mouseArea
property alias backgroundRadius: uiStates.radius
property alias uiStatesVisible: uiStates.visible

Expand Down
5 changes: 3 additions & 2 deletions nebula/ui/components/MZMenu.qml
Original file line number Diff line number Diff line change
Expand Up @@ -103,10 +103,11 @@ Item {

sourceComponent: menuBar.rightButtonComponent
onItemChanged: {
if (item instanceof Text) {
//item.item is for right button components that might be inside a loder itself
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo

if (item instanceof Text || item.item instanceof Text) {
Copy link
Contributor

Choose a reason for hiding this comment

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

For the item.item condition, should the first item be checked if it is an instance of Loader? Also, are any null checks required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, null checks for what? item? If item is null, item instanceof Text will evaluate to false

Copy link
Contributor

Choose a reason for hiding this comment

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

Please ignore the null check in the comment.

anchors.rightMargin = MZTheme.theme.windowMargin
}
else if(item instanceof MZIconButton) {
else if(item instanceof MZIconButton || item.item instanceof MZIconButton) {
anchors.rightMargin = MZTheme.theme.windowMargin / 2
}
}
Expand Down
2 changes: 2 additions & 0 deletions nebula/ui/resources/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -71,5 +71,7 @@ qt_add_qml_module(resources
info.svg
lock.svg
startup.svg
question.svg
tip-filled.svg

)
3 changes: 3 additions & 0 deletions nebula/ui/resources/question.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
11 changes: 11 additions & 0 deletions nebula/ui/resources/tip-filled.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
7 changes: 7 additions & 0 deletions src/feature/featurelist.h
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,13 @@ FEATURE(gleanRust, // Feature ID
QStringList(), // feature dependencies
FeatureCallback_true)

FEATURE(helpSheets, // Feature ID
"Help sheets", // Feature name
FeatureCallback_true, // Can be flipped on
FeatureCallback_true, // Can be flipped off
QStringList(), // feature dependencies
FeatureCallback_false)

FEATURE(inAppAccountCreate, // Feature ID
"In-app Account Creation", // Feature name
FeatureCallback_true, // Can be flipped on
Expand Down
5 changes: 5 additions & 0 deletions src/mozillavpn.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1381,6 +1381,11 @@ void MozillaVPN::registerUrlOpenerLabels() {
uo->registerUrlLabel("upgradeToAnnualUrl", []() -> QString {
return Constants::upgradeToAnnualUrl();
});

uo->registerUrlLabel("sumoDns", []() -> QString {
return "https://support.mozilla.org/en-US/kb/"
"how-do-i-change-my-dns-settings";
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these strings be moved into Constants, like the other UrlLabels?

Copy link
Member

Choose a reason for hiding this comment

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

And we just need to drop the "en-US" so that moz.org can route to whichever locale is appropriate.

});
}

void MozillaVPN::errorHandled() {
Expand Down
20 changes: 20 additions & 0 deletions src/translations/strings.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -928,6 +928,20 @@ devices:
value: My devices
comment: Title for the menu bar when viewing the devices or device limit pages

helpSheets:
dnsTitle:
value: Custom DNS settings
comment: Title label for the custom dns help sheet
dnsHeader:
value: What is custom DNS?
comment: Header label for the custom dns help sheet
dnsBody1:
value: Whenever you connect to a website, a DNS (domain name system) first turns the domain name (e.g. www.mozilla.org) into an IP address that allows your internet traffic to reach that destination.
Copy link
Contributor

@vinocher vinocher Jan 2, 2024

Choose a reason for hiding this comment

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

Just checking if this content has been approved by UX? The help text doesn't seem to explain what 'resolve websites' means in the screen UI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

+1 @vinocher . We don't seem to be answering the question from dnsHeader 'What is a custom DNS?').

Choose a reason for hiding this comment

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

@MattLichtenstein I agree that updating the title makes sense (per Flod's suggestion).

I didn't work on this content (and don't know much about the subject matter), so I checked in with Chris, but he didn't have any additional updates. Chris is returning to content lead on VPN, so I guess we can leave the body content as-is.

comment: Body text for the custom dns help sheet
dnsBody2:
value: Mozilla VPN allows you to choose a custom DNS server if you prefer. If you use one, you won’t be able to use other privacy features in the VPN like tracker blocking.
comment: Body text for the custom dns help sheet

global:
expand:
value: Expand
Expand Down Expand Up @@ -977,6 +991,12 @@ global:
getStarted:
value: Get started
comment: Label for a button used to begin something / proceed
help:
value: Help
comment: Action taken that will help the user
learnMore:
value: Learn more
comment: Label for link to allow the user to learn more about a topic

getHelp:
helpCenter: Help Center
Expand Down
62 changes: 51 additions & 11 deletions src/ui/screens/settings/ViewDNSSettings.qml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,25 @@ MZViewBase {
property bool privacyDialogNeeded: true
property bool dnsSelectionChanged: false
readonly property string telemetryScreenId : "dns_settings"
property Component rightMenuButton: Component {
Loader {
active: MZFeatureList.get("helpSheets").isSupported
sourceComponent: MZIconButton {
onClicked: helpSheetLoader.active = true

accessibleName: MZI18n.GlobalHelp
Accessible.ignored: !visible
Copy link
Contributor

Choose a reason for hiding this comment

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

MZButtonBase has Accessible.ignored: !visible, so this should not be needed.


Image {
anchors.centerIn: parent

source: "qrc:/nebula/resources/question.svg"
fillMode: Image.PreserveAspectFit
}
}
}
}


function applyFrontendChanges(settingValue) {
if (settingValue === MZSettings.Gateway) {
Expand Down Expand Up @@ -73,7 +92,7 @@ MZViewBase {
function reset() {
root.customDNS = MZSettings.dnsProviderFlags === MZSettings.Custom;
root.privacyDialogNeeded = MZSettings.dnsProviderFlags !== MZSettings.Custom &&
MZSettings.dnsProviderFlags !== MZSettings.Gateway;
MZSettings.dnsProviderFlags !== MZSettings.Gateway;
ipInput.text = MZSettings.userDNS;
}

Expand Down Expand Up @@ -224,9 +243,9 @@ MZViewBase {
}

onActiveFocusChanged: {
if (!activeFocus && !ipInput.focusReasonA11y) {
maybeSaveChange();
}
if (!activeFocus && !ipInput.focusReasonA11y) {
maybeSaveChange();
}
}
}

Expand Down Expand Up @@ -258,8 +277,8 @@ MZViewBase {

Component.onCompleted: {
Glean.impression.dnsSettingsScreen.record({
screen: telemetryScreenId,
});
screen: telemetryScreenId,
});
reset();
}

Expand All @@ -268,11 +287,11 @@ MZViewBase {
}

onVisibleChanged: {
if (!visible) {
maybeSaveChange();
} else {
reset();
}
if (!visible) {
maybeSaveChange();
} else {
reset();
}
}

Loader {
Expand Down Expand Up @@ -315,5 +334,26 @@ MZViewBase {

onActiveChanged: if (active) { item.open() }
}

Loader {
id: helpSheetLoader

active: false

onActiveChanged: if (active) item.open()

sourceComponent: MZHelpSheet {
title: MZI18n.HelpSheetsDnsTitle

model: [
{type: MZHelpSheet.BlockType.Title, text: MZI18n.HelpSheetsDnsHeader},
{type: MZHelpSheet.BlockType.Text, text: MZI18n.HelpSheetsDnsBody1, margin: 8},
{type: MZHelpSheet.BlockType.Text, text:MZI18n.HelpSheetsDnsBody2, margin: 16},
{type: MZHelpSheet.BlockType.LinkButton, text: MZI18n.GlobalLearnMore, margin: 16, action: () => { MZUrlOpener.openUrlLabel("sumoDns") } },
]

onClosed: helpSheetLoader.active = false
}
}
}