Informs to users about system location service #19529

simonhong · 2023-08-02T03:54:12Z

If system location service is enabled, browser can get more accurate geolocation data.
Launch dialog about this when site asks geolocation permission if system location service is not yet enabled.

NOTE: focus ring color should be handled separately.

Resolves

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

GeolocationAccuracyBrowserTest.DialogLaunchTest

Disable Location services from Privacy & security settings
Load https://browserleaks.com/geo
Check permission bubble and above dialog are launched
When click Ok, Privacy & Secury > Location setting page is opened
Load https://browserleaks.com/geo and check Don't show again checkbox
Hide dialog and load again
Check dialog is not shown

bsclifton · 2023-08-07T22:01:00Z

I know it's a WIP - but just pulled the code 🙂 This is looking nice!

Site I tested location was https://www.gps-coordinates.net/my-location
Modal shows up - looks good 😄👍
Help link in modal goes to https://support.microsoft.com/en-us/windows/windows-location-service-and-privacy-3a8eee0a-5b0b-dc07-eede-2a5ca1c49088 👍
After clicking OK on the modal, the location panel opened up in my OS (Windows 11, x64)
When allowing access (I picked Forever), I was getting a Position unavailable error
When I enabled Location Services (via settings) and refreshed page, actual location shown 🎉

simonhong · 2023-08-16T03:17:29Z

I know it's a WIP - but just pulled the code 🙂 This is looking nice!

Site I tested location was https://www.gps-coordinates.net/my-location

Modal shows up - looks good 😄👍

Help link in modal goes to https://support.microsoft.com/en-us/windows/windows-location-service-and-privacy-3a8eee0a-5b0b-dc07-eede-2a5ca1c49088 👍

After clicking OK on the modal, the location panel opened up in my OS (Windows 11, x64)

When allowing access (I picked Forever), I was getting a Position unavailable error

When I enabled Location Services (via settings) and refreshed page, actual location shown 🎉

@bsclifton Thanks for testing! I'm updating dialog UI (https://www.figma.com/file/tU9zrk9Ite8B06JSjHAHhn/General-dialogs-and-popups?type=design&node-id=113-17205&mode=design&t=ReAGaEUCCkOSD3OC-0)

simonhong · 2023-08-16T08:54:37Z

Updating UI like below. Ok button has proper color in normal state(blue) but shows orange color on hovered.
Checking which part affects hover color. GetButtonThemes() in our md_text_button.cc defines proper hover color
for kPrimary kind. but that color is not applied. => Fixed.

simonhong · 2023-08-17T03:55:44Z

Found another issue with current PR. After closing helper dialog, permission request bubble is shown.
When allowing from permission bubble,
original behavior - page renders map
changed behavior - page shows Off - Allow location access instead of renders map. Proper map is shown after reloading.

simonhong · 2023-08-17T04:25:21Z

@goodov it's not ready yet to ask review as this PR introduced regression . but can you give early reivew about this PR's approach for launching another dialog before showing geolocation permission bubble? I used nested loop to block permission bubble launching.

goodov · 2023-08-17T09:52:30Z

@goodov it's not ready yet to ask review as this PR introduced regression . but can you give early reivew about this PR's approach for launching another dialog before showing geolocation permission bubble? I used nested loop to block permission bubble launching.

having RunLoop::QuitClosure() in production code is wrong, I don't think we should do that.

do we really want to add a separate nagging window before the permission popup? This doesn't look as a great UX tbh. cc @pes10k ?

simonhong · 2023-08-18T03:05:59Z

@goodov it's not ready yet to ask review as this PR introduced regression . but can you give early reivew about this PR's approach for launching another dialog before showing geolocation permission bubble? I used nested loop to block permission bubble launching.

having RunLoop::QuitClosure() in production code is wrong, I don't think we should do that.

Yup, I think this causes some regressions. Trying to find another way to avoid it.

fix brave/brave-browser#16897 If system location service is enabled, browser can get more accurate geolocation data. Launch dialog about this when site asks geolocation permission if system location service is not yet enabled.

sangwoo108

LGTM 👍

sangwoo108 · 2023-08-21T12:13:03Z

browser/ui/geolocation/geolocation_accuracy_tab_helper.cc

+  dialog_asked_in_current_navigation_ = true;
+  is_dialog_running_ = true;
+  brave::ShowGeolocationAccuracyHelperDialog(
+      web_contents(),
+      base::BindOnce(&GeolocationAccuracyTabHelper::OnDialogClosed,
+                     weak_ptr_factory_.GetWeakPtr()));


Would it better to move this into IS_WIN guard together? I know this tab helper won't be populated on other platforms but just in case.

sangwoo108 · 2023-08-21T12:16:30Z

browser/ui/geolocation/geolocation_accuracy_utils_win.h

+bool IsSystemLocationSettingEnabled();
+void LaunchLocationServiceSettings();


I think we need namespace here as these plain functions are global namespace.

sangwoo108 · 2023-08-21T12:17:05Z

browser/ui/geolocation/pref_names.h

+constexpr char kShowGeolocationAccuracyHelperDialog[] =
+    "brave.show_geolocation_accuracy_helper_dialog";


sangwoo108 · 2023-08-21T12:24:13Z

browser/ui/views/geolocation_accuracy_helper_dialog_view.h

+class Checkbox;
+}  // namespace views
+
+class GeolocationAccuracyHelperDialogView : public views::DialogDelegateView {


optional: From my experience, when having modal window, using constrained_window could be a good choice as it provides more functionality to make it sure that the modal dialog is visible to users, such as restoring window when it's minimized.

mkarolin · 2023-08-21T15:04:01Z

app/brave_generated_resources.grd

+        <message name="IDS_GEOLOCATION_ACCURACY_HELPER_DLG_CONTENTS_LABEL" desc="The text for content part in the dialog">
+          The Brave browser is attempting to access your location. You may not get accurate results unless you enable your operating system's <ph name="PART_ONE">$1<ex>Location services</ex></ph> setting and also enable <ph name="PART_TWO">$2<ex>Let apps access your location</ex></ph>.
+        </message>
+        <message name="IDS_GEOLOCATION_ACCURACY_HELPER_DLG_CONTENTS_PART_ONE_LABEL" desc="The text for content part one in the dialog">
+          Location services
+        </message>
+        <message name="IDS_GEOLOCATION_ACCURACY_HELPER_DLG_CONTENTS_PART_TWO_LABEL" desc="The text for content part two in the dialog">
+          Let apps access your location
+        </message>


Doing this is not great for localization. Not sure if this is doable, but would it be possible to use two placeholders for each fragment just to get offsets? For example:

<message name="IDS_GEOLOCATION_ACCURACY_HELPER_DLG_CONTENTS_LABEL" desc="The text for content part in the dialog"> The Brave browser is attempting to access your location. You may not get accurate results unless you enable your operating system's <ph name="STYLE_ONE_BEGIN">$1</ph>Location services<ph name="STYLE_ONE_END">$2</ph> setting and also enable <ph name="STYLE_TWO_BEING">$3</ph>Let apps access your location<ph name="STYLE_TWO_END">$4</ph>. ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Extra line breaks for visibility in GitHub

Then use l10n_util::GetStringFUTF16 to get all the offsets and apply styling to ranges in between _BEGIN and _END offsets. I guess we can replace placeholders themselves with empty strings, if necessary. WDYT?

simonhong · 2023-09-01T09:11:08Z

Changed to draft as we could change UI. cc @rebron

simonhong · 2024-04-24T08:11:31Z

Closed as we'll add more information to geolocation permission bubble instead of another dialog.

simonhong self-assigned this Aug 2, 2023

github-actions bot added the potential-layer-violation-fixes This PR touches a BUILD.gn file with check_includes=false label Aug 2, 2023

simonhong added the CI/skip Do not run CI builds (except noplatform) label Aug 2, 2023

simonhong force-pushed the geolocation_accuracy branch 3 times, most recently from 2173a24 to 7ed867a Compare August 2, 2023 09:08

simonhong removed the CI/skip Do not run CI builds (except noplatform) label Aug 3, 2023

simonhong force-pushed the geolocation_accuracy branch from 7ed867a to e2d2ecd Compare August 3, 2023 06:59

simonhong added CI/skip Do not run CI builds (except noplatform) and removed CI/skip Do not run CI builds (except noplatform) labels Aug 4, 2023

simonhong force-pushed the geolocation_accuracy branch 3 times, most recently from 6bcab7e to 738e19e Compare August 4, 2023 06:40

simonhong force-pushed the geolocation_accuracy branch 2 times, most recently from 187bbdb to 778df10 Compare August 16, 2023 08:49

simonhong force-pushed the geolocation_accuracy branch 2 times, most recently from 7c23e41 to 44e28c6 Compare August 17, 2023 02:59

simonhong added the CI/skip Do not run CI builds (except noplatform) label Aug 18, 2023

simonhong force-pushed the geolocation_accuracy branch from 44e28c6 to 78cad9d Compare August 18, 2023 02:46

simonhong removed the CI/skip Do not run CI builds (except noplatform) label Aug 21, 2023

simonhong force-pushed the geolocation_accuracy branch 3 times, most recently from adf4401 to a2dce0c Compare August 21, 2023 08:07

simonhong marked this pull request as ready for review August 21, 2023 09:21

simonhong requested review from goodov and a team as code owners August 21, 2023 09:21

Informs to users about system location service

22d1ccc

fix brave/brave-browser#16897 If system location service is enabled, browser can get more accurate geolocation data. Launch dialog about this when site asks geolocation permission if system location service is not yet enabled.

simonhong force-pushed the geolocation_accuracy branch from a2dce0c to 22d1ccc Compare August 21, 2023 10:58

simonhong requested a review from sangwoo108 August 21, 2023 10:59

sangwoo108 approved these changes Aug 21, 2023

View reviewed changes

mkarolin reviewed Aug 21, 2023

View reviewed changes

simonhong marked this pull request as draft September 1, 2023 09:11

simonhong closed this Apr 24, 2024

simonhong mentioned this pull request May 7, 2024

Update Geolocation permission bubble #23278

Merged

24 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Informs to users about system location service #19529

Informs to users about system location service #19529

simonhong commented Aug 2, 2023 •

edited

Loading

bsclifton commented Aug 7, 2023

simonhong commented Aug 16, 2023

simonhong commented Aug 16, 2023 •

edited

Loading

simonhong commented Aug 17, 2023

simonhong commented Aug 17, 2023 •

edited

Loading

goodov commented Aug 17, 2023

simonhong commented Aug 18, 2023 •

edited

Loading

sangwoo108 left a comment

sangwoo108 Aug 21, 2023

sangwoo108 Aug 21, 2023

sangwoo108 Aug 21, 2023

sangwoo108 Aug 21, 2023

mkarolin Aug 21, 2023 •

edited

Loading

simonhong commented Sep 1, 2023

simonhong commented Apr 24, 2024

		bool IsSystemLocationSettingEnabled();
		void LaunchLocationServiceSettings();

		constexpr char kShowGeolocationAccuracyHelperDialog[] =
		"brave.show_geolocation_accuracy_helper_dialog";

Informs to users about system location service #19529

Informs to users about system location service #19529

Conversation

simonhong commented Aug 2, 2023 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

bsclifton commented Aug 7, 2023

simonhong commented Aug 16, 2023

simonhong commented Aug 16, 2023 • edited Loading

simonhong commented Aug 17, 2023

simonhong commented Aug 17, 2023 • edited Loading

goodov commented Aug 17, 2023

simonhong commented Aug 18, 2023 • edited Loading

sangwoo108 left a comment

Choose a reason for hiding this comment

sangwoo108 Aug 21, 2023

Choose a reason for hiding this comment

sangwoo108 Aug 21, 2023

Choose a reason for hiding this comment

sangwoo108 Aug 21, 2023

Choose a reason for hiding this comment

sangwoo108 Aug 21, 2023

Choose a reason for hiding this comment

mkarolin Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

simonhong commented Sep 1, 2023

simonhong commented Apr 24, 2024

simonhong commented Aug 2, 2023 •

edited

Loading

simonhong commented Aug 16, 2023 •

edited

Loading

simonhong commented Aug 17, 2023 •

edited

Loading

simonhong commented Aug 18, 2023 •

edited

Loading

mkarolin Aug 21, 2023 •

edited

Loading