Update Settings Dialog to allow for better scaling #984

MortimerGoro · 2019-03-05T18:55:00Z

bluemarvin · 2019-03-05T19:45:38Z

Now that the pref dialog is larger, it feels too close. We might want to consider moving it back a bit.
The external storage preference seems to be broken:

The switch shows Allow but it is still red. Also there is this dialog behind the preference dialog which can be seen if you close the preference dialog:

If this dialog is accepted, then you get the system prompt. This might be a bug in GeckoView.

The environment dialog has the wrong text for the reset button:

The privacy dialog has the wrong text for the reset button as well.

I also really dislike using toggle buttons for this functionality. My expectation as a user is that clicking on it will change the preference. I find this dialog confusing and unexpected:

bluemarvin · 2019-03-05T19:51:33Z

Looks like the external storage is a GeckoView issue: https://bugzilla.mozilla.org/show_bug.cgi?id=1527074

bluemarvin · 2019-03-05T20:52:01Z

One other thing that might be useful is to add a Use Current Page button to the Homepage option.

bluemarvin · 2019-03-05T20:57:26Z

Got this crash while testing noapi https://crash-stats.mozilla.com/report/index/bp-bb896c7a-043d-4243-940e-1cb560190305

Unfortunately stack trace isn't helpful. I was in the Display prefs dialog and had pressed the back button. Haven't been abler to reproduce the issue yet.

philip-lamb

I haven't done a full test of the build yet but I concur with Randall's observations. As regards the permissions dialog. while the UIS showed toggle switches, these are inappropriate given that we can't disable the system setting. So I would recommend a two-element approach, where if the permission is disabled, we show a button with the label "Enable", and if the permission is enabled, the button is removed and replaced with a label "Enabled".

In the environments panel, the "Allow environment override" doesn't really explain who or what would override. Might be better as "Allow sites to override environment".

MortimerGoro · 2019-03-06T14:37:35Z

PR updated:

Use button and label instead of the switch
Fix settings distance due to larger size
Fix wrong text in reset buttons
Fix external storage read permission
Improve text on honeybomb buttons (sans-serif-light doesn't look good in VR)

@philip-lamb regarding "Allow environment override" I agree that it's confusing. It's used to load enviroments from a hardcoded folder in sdcard. We need to find a better text, and maybe add a tooltip/more info button because there is not enough space to explain it. We can address that as a follow-up issue

bluemarvin · 2019-03-06T15:24:59Z

Wiki page explaining external environments:
https://github.com/MozillaReality/FirefoxReality/wiki/Environments

MortimerGoro · 2019-03-06T15:34:42Z

I can add a more info button below the env override switch that opens that URL, what do you think?

bluemarvin · 2019-03-06T17:58:24Z

I got this crash when I hit the reset button in Display options dialog:

https://crash-stats.mozilla.com/report/index/d7b7d268-2f35-42da-8008-1dd570190306
https://crash-stats.mozilla.com/report/index/bp-9854595e-bf55-4655-b780-e42e40190306

It was reproducible. This was with the latest version of the PR.

bluemarvin · 2019-03-06T18:24:36Z

It looks like the tracking protection toggle doesn't actually work.
If you go here: https://mozilla.github.io/tracking-test/
If TP is disabled you should see:

But I always see regardless of the setting:

bluemarvin · 2019-03-06T18:28:12Z

app/src/common/shared/org/mozilla/vrbrowser/browser/SessionStore.java

+    public void setTrackingEnabled(final boolean aEnabled) {
+        State state = getCurrentState();
+        if (state != null && state.mSettings.trackingProtection != aEnabled) {
+            state.mSettings.multiprocess = aEnabled;


This seems to be the problem, looks like a CCP error. multiprocess should be trackingProtection

MortimerGoro · 2019-03-06T19:17:06Z

@bluemarvin Pushed a new commit with fixes for tracking and the crash. The crash also happened on master, was not related to this PR.

MortimerGoro · 2019-03-07T12:38:02Z

I have added a help icon in the environment override setting that opens the wiki page

philip-lamb · 2019-03-07T22:31:28Z

Looks good, except for one minor UI issue... if you click "deny" on any permissions dialog, and later trigger the dialog again, the "don't ask again" box is really cramped. Is this our dialog or a system one?

bluemarvin · 2019-03-07T22:41:34Z

Looks good, except for one minor UI issue... if you click "deny" on any permissions dialog, and later trigger the dialog again, the "don't ask again" box is really cramped. Is this our dialog or a system one?

That's the system's UI

philip-lamb

Tested one additional item; scroll direction. The "natural" vs "reversed" should apply to both X and Y axes... this is basically the difference between touch-surface scrolling and joystick scrolling, so it should be consistent in both axes. In #705 there was a request to separate X and Y scrolling axes which I think we should consider in another issue (including design).

MortimerGoro · 2019-03-11T18:49:59Z

@philip-lamb good catch. I updated the PR to fix the horizontal reversed scrolling

cvan

really solid work 👍 this adds some outstanding improvements to the Settings UX.

apologies in advance for having to read all my nitpicky comments. I'll leave it up to your discretion of what to punt on for now.

the items I feel strongly about:

improving UX of Privacy & Security > Permissions: Enabled vs. Enable buttons.
adding the missing Notifications item to the list of Permissions.
adding titles at the top of the Settings dialog windows.
changing Privacy Policy's Show button -> Learn More (in Privacy & Security > Permissions)
increasing the click targets of the Environments radio buttons so they don't miss user clicks. (it'd be great if we can at least increase the height so the area above/below the text is clickable in the row, and ideally we can make the entire row clickable, but that's fine if it's not handled in this PR.)

many thanks for reading all my (mostly nitpicky) comments and taking the time to iterate on this. and thank you for patiently waiting for my review on this. 👍

cvan · 2019-03-12T10:40:54Z

app/src/common/shared/org/mozilla/vrbrowser/VRBrowserActivity.java

-                permission,
-                aCallback);
+    public void requestPermission(String uri, @NonNull String permission, GeckoSession.PermissionDelegate.Callback aCallback) {
+        if (uri != null && uri.length() > 0) {


can we replace .length > 0 with .isEmpty(), to be consistent with usage mostly elsewhere.

cvan · 2019-03-12T10:41:39Z

app/src/common/shared/org/mozilla/vrbrowser/VRBrowserActivity.java

+        if (uri != null && uri.length() > 0) {
+            mPermissionDelegate.onAppPermissionRequest(SessionStore.get().getCurrentSession(), uri, permission, aCallback);
+        } else {
+            mPermissionDelegate.onAndroidPermissionsRequest(SessionStore.get().getCurrentSession(), new String[]{permission}, aCallback);


nit: can new String[]{permission} be permission.toString()?

app/src/common/shared/org/mozilla/vrbrowser/VRBrowserActivity.java

app/src/common/shared/org/mozilla/vrbrowser/browser/SessionStore.java

cvan · 2019-03-12T10:44:24Z

app/src/common/shared/org/mozilla/vrbrowser/browser/SessionStore.java

+        }
+    }
+
+    public void setTrackingEnabled(final boolean aEnabled) {


nit: should this be called setTrackingProtection instead of setTrackingEnabled (for consistency with the names of this class' methods and their respective setting properties)?

cvan · 2019-03-12T13:11:55Z

app/src/common/shared/org/mozilla/vrbrowser/ui/widgets/dialogs/EnvironmentOptionsWidget.java

+            if (mAudio != null) {
+                mAudio.playSound(AudioEngine.Sound.CLICK);
+            }
+            SessionStore.get().loadUri(getContext().getString(R.string.environment_override_help_url));


this is helpful. thanks for adding 👍

P.S. UX nit request: to make it easier for developers who will likely be reading documentation from their desktop PC screens, it might be nice to have @philip-lamb submit a bit.ly request for a short URL (à la https://mzl.la/fxr or https://mzl.la/reality), and the tiny URL could possibly be shown in a tooltip upon hover.

cvan · 2019-03-12T13:16:01Z

app/src/common/shared/org/mozilla/vrbrowser/ui/widgets/dialogs/EnvironmentOptionsWidget.java

+
+        String env = SettingsStore.getInstance(getContext()).getEnvironment();
+        mEnvironmentsRadio = findViewById(R.id.environmentRadio);
+        mEnvironmentsRadio.setOnCheckedChangeListener(mEnvsListener);


UX request: would it be possible to increase the radius of the click target for the radio such that clicking anywhere on the row (the thumbnail, the radio button, or anywhere around but not precisely on the text line) would trigger a checked-change event?

I found myself thinking the UI was broken, until I was more precise with my clicks.

app/src/common/shared/org/mozilla/vrbrowser/ui/widgets/dialogs/EnvironmentOptionsWidget.java

app/src/common/shared/org/mozilla/vrbrowser/ui/widgets/options/ControllerOptionsWidget.java

MortimerGoro · 2019-03-13T13:18:56Z

Good catch @cvan on the radio button click area. I refactored the environment layout to make it work clicking on any area of the row.

ghost assigned MortimerGoro Mar 5, 2019

ghost added the in progress label Mar 5, 2019

MortimerGoro requested review from cvan, philip-lamb and bluemarvin March 5, 2019 18:55

philip-lamb suggested changes Mar 6, 2019

View reviewed changes

MortimerGoro force-pushed the settings_scale branch from c6911a9 to d9c7aec Compare March 6, 2019 13:33

MortimerGoro mentioned this pull request Mar 6, 2019

Allow swappable Y-Axis option for touch pad scrolling #705

Closed

bluemarvin reviewed Mar 6, 2019

View reviewed changes

MortimerGoro force-pushed the settings_scale branch from d9c7aec to c6ac211 Compare March 6, 2019 19:13

bluemarvin approved these changes Mar 7, 2019

View reviewed changes

philip-lamb approved these changes Mar 7, 2019

View reviewed changes

philip-lamb self-requested a review March 9, 2019 03:18

philip-lamb suggested changes Mar 9, 2019

View reviewed changes

cvan suggested changes Mar 12, 2019

View reviewed changes

philip-lamb approved these changes Mar 13, 2019

View reviewed changes

Update Settings Dialog to allow for better scaling

675218e

MortimerGoro force-pushed the settings_scale branch from 061de9b to 675218e Compare March 13, 2019 13:17

MortimerGoro merged commit a626f1d into master Mar 13, 2019

ghost removed the in progress label Mar 13, 2019

MortimerGoro mentioned this pull request Mar 13, 2019

UR Finding: Users would like to choose between different 3D environments #214

Closed

bluemarvin deleted the settings_scale branch March 13, 2019 17:11

cvan mentioned this pull request May 1, 2019

Settings layout broken due to pixel-density scaling on WaveVR #1137

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Settings Dialog to allow for better scaling #984

Update Settings Dialog to allow for better scaling #984

MortimerGoro commented Mar 5, 2019 •

edited

Loading

bluemarvin commented Mar 5, 2019

bluemarvin commented Mar 5, 2019

bluemarvin commented Mar 5, 2019

bluemarvin commented Mar 5, 2019

philip-lamb left a comment

MortimerGoro commented Mar 6, 2019 •

edited

Loading

bluemarvin commented Mar 6, 2019

MortimerGoro commented Mar 6, 2019

bluemarvin commented Mar 6, 2019 •

edited

Loading

bluemarvin commented Mar 6, 2019

bluemarvin Mar 6, 2019

MortimerGoro commented Mar 6, 2019

MortimerGoro commented Mar 7, 2019

philip-lamb commented Mar 7, 2019

bluemarvin commented Mar 7, 2019

philip-lamb left a comment

MortimerGoro commented Mar 11, 2019

cvan left a comment •

edited

Loading

cvan Mar 12, 2019

cvan Mar 12, 2019

cvan Mar 12, 2019

cvan Mar 12, 2019

cvan Mar 12, 2019

MortimerGoro commented Mar 13, 2019

Update Settings Dialog to allow for better scaling #984

Update Settings Dialog to allow for better scaling #984

Conversation

MortimerGoro commented Mar 5, 2019 • edited Loading

bluemarvin commented Mar 5, 2019

bluemarvin commented Mar 5, 2019

bluemarvin commented Mar 5, 2019

bluemarvin commented Mar 5, 2019

philip-lamb left a comment

Choose a reason for hiding this comment

MortimerGoro commented Mar 6, 2019 • edited Loading

bluemarvin commented Mar 6, 2019

MortimerGoro commented Mar 6, 2019

bluemarvin commented Mar 6, 2019 • edited Loading

bluemarvin commented Mar 6, 2019

bluemarvin Mar 6, 2019

Choose a reason for hiding this comment

MortimerGoro commented Mar 6, 2019

MortimerGoro commented Mar 7, 2019

philip-lamb commented Mar 7, 2019

bluemarvin commented Mar 7, 2019

philip-lamb left a comment

Choose a reason for hiding this comment

MortimerGoro commented Mar 11, 2019

cvan left a comment • edited Loading

Choose a reason for hiding this comment

cvan Mar 12, 2019

Choose a reason for hiding this comment

cvan Mar 12, 2019

Choose a reason for hiding this comment

cvan Mar 12, 2019

Choose a reason for hiding this comment

cvan Mar 12, 2019

Choose a reason for hiding this comment

cvan Mar 12, 2019

Choose a reason for hiding this comment

MortimerGoro commented Mar 13, 2019

MortimerGoro commented Mar 5, 2019 •

edited

Loading

MortimerGoro commented Mar 6, 2019 •

edited

Loading

bluemarvin commented Mar 6, 2019 •

edited

Loading

cvan left a comment •

edited

Loading