Element Picker on Android #26725

vadimstruts · 2024-11-25T14:47:05Z

Resolves brave/brave-browser#33241

Enabled Element picker for Android
Enhanced Element Picker's UI
Added Feature flag which is enabled by default for Desktop and disabled by default for Android

2024-12-04.13-44-36.mp4

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 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:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

Open Android Brave browser
Open any webpage you want to block any HTML element
Open Brave Shields menu -> Advanced controls
Make sure that Block elements item is absent (It means that brave://flags/#block-element-feature flag is disabled)
If it was disabled, (It was not enabled with Griffin), enable it and make sure that Block elements item is present in the Brave Shields menu -> Advanced controls menu
Click Block elements and make sure that Element Picker appears
Click any HTML element you want to block and press button Block Element, and make sure element blocked
Change Android Theme (Dark/Light) and make sure that Element picker has the same color

brave-builds · 2024-11-25T15:50:49Z

A Storybook has been deployed to preview UI for the latest push

github-actions · 2024-12-03T00:04:46Z

Chromium major version is behind target branch (131.0.6778.69 vs 132.0.6834.15). Please rebase.

AlexeyBarabash · 2024-12-04T12:50:26Z

android/java/res/layout/brave_shields_secondary_layout.xml

+        android:id="@+id/brave_shields_block_element_text"
+		android:layout_width="wrap_content"
+		android:layout_height="48dp"
+		android:paddingStart="16dp"


as paddingStart and paddingEnd are the same, so it is possible to use a single android:paddingHorizontal

AlexeyBarabash · 2024-12-04T12:57:44Z

components/cosmetic_filters/resources/data/element_picker.html

-  </div>
-</section>
+  </section>
+  


probably the last line should be empty

AlexeyBarabash

Android and native changes lgtm

deeppandya

Lgtm

antonok-edm · 2024-12-05T22:00:39Z

android/java/org/chromium/chrome/browser/app/BraveActivity.java

@@ -1620,6 +1622,20 @@ public void openBraveContentFilteringSettings() {
        settingsLauncher.startSettings(this, ContentFilteringFragment.class);
    }

+    public boolean isNightlyModeEnabled() {


nit: NightlyMode sounds like it has to do with the release channel, maybe NightMode instead?

antonok-edm · 2024-12-05T22:01:12Z

android/java/org/chromium/chrome/browser/cosmetic_filters/BraveCosmeticFiltersUtils.java

+    }
+
+    @CalledByNative
+    public static boolean isNightlyModeEnabled() {


antonok-edm · 2024-12-05T22:05:43Z

components/cosmetic_filters/resources/data/element_picker.ts

@@ -6,23 +6,18 @@ const NSSVG = 'http://www.w3.org/2000/svg'

 let pickerDiv: HTMLDivElement | null
 let shadowRoot: ShadowRoot | null
+const isAndroid = /(android)/i.test(navigator.userAgent)


is this best practice? Not a big deal, just a bit suprising, imo

Changed to use initialization call which returns the platform

antonok-edm · 2024-12-05T22:10:21Z

components/cosmetic_filters/resources/data/element_picker.ts

+      if (isAndroid) {
+        return
+      }


if it's as easy as removing early returns, I'd leave this kind of mouse-specific logic in on Android since some folks would e.g. use a keyboard/mouse combo on a tablet

Perhaps it's worth using pointer/hover media queries rather than UA inspection? ref

atuchin-m · 2024-12-06T08:34:09Z

components/cosmetic_filters/renderer/cosmetic_filters_js_handler.cc

+  blink::WebLocalFrame* web_frame = render_frame_->GetWebFrame();
+  std::string new_selectors_script = base::StringPrintf(
+      kSePickerThemeInfoScript, is_dark_mode_enabled, background_color);
+  web_frame->ExecuteScriptInIsolatedWorld(


It is possible to pass the result via getElementPickerThemeInfo(callback) instead of using ExecuteScriptInIsolatedWorld()?

atuchin-m · 2024-12-06T08:36:38Z

components/cosmetic_filters/renderer/cosmetic_filters_js_handler.cc

+
+mojo::AssociatedRemote<cosmetic_filters::mojom::CosmeticFiltersHandler>&
+CosmeticFiltersJSHandler::GetElementPickerRemoteHandler() {
+  if (!element_picker_actions_handler_ ||


Looks like we should just check is_connected():
https://source.chromium.org/chromium/chromium/src/+/main:mojo/public/cpp/bindings/associated_remote.h;l=94-114

atuchin-m · 2024-12-06T08:38:34Z

components/cosmetic_filters/renderer/cosmetic_filters_js_handler.h

  int OnEventBegin(const std::string& event_name);
  void OnEventEnd(const std::string& event_name, int);

  void InjectStylesheet(const std::string& stylesheet);

  bool generichide_ = false;

+  mojo::AssociatedRemote<cosmetic_filters::mojom::CosmeticFiltersHandler>&
+  GetElementPickerRemoteHandler();
+  void OnRemoteHandlerDisconnect();


nit: move the methods above.

mkarolin

strings++

components/cosmetic_filters/renderer/cosmetic_filters_js_handler.cc

goodov · 2024-12-11T11:48:51Z

browser/cosmetic_filters/cosmetic_filters_tab_helper.cc

+#if !BUILDFLAG(IS_ANDROID)
+      mojom::RunningPlatform::kDesktop
+#else   // !BUILDFLAG(IS_ANDROID)
+      mojom::RunningPlatform::kAndroid


this looks a bit unusual. You should know at compile time that you're on Android. I don't think mojom::RunningPlatform is necessary. Just check BUILDFLAG(IS_ANDROID) where you need to.

goodov · 2024-12-11T11:52:53Z

components/brave_shields/core/common/buildflags/buildflags.gni

+declare_args() {
+  enable_block_elements_feature =
+      (brave_channel == "development" || brave_channel == "nightly" ||
+       brave_channel == "beta" || (is_official_build && !is_android)) && !is_ios


pls don't introduce a buildflag that's configured like this (a combination of channel, platform and official_build). This may lead to a broken build when the branch migration happens, because the flag in false state is never checked on android builds until we start building release builds in the branch with these changes.

Instead pls make the feature DISABLED by default on Android and enable it via Griffin on beta/nightly. Everything will be simpler this way. You can add it into brave://flags so anyone can enable it manually in any branch.

goodov · 2024-12-11T11:55:39Z

browser/android/cosmetic_filters/cosmetic_filters_utils.cc

+
+bool IsDarkModeEnabled() {
+  JNIEnv* env = base::android::AttachCurrentThread();
+  return Java_BraveCosmeticFiltersUtils_isNightModeEnabled(env);


is there really no existing way to check if dark mode is enabled on Android from C++ code?

what about GetDarkModeState() from chrome/browser/flags/android/chrome_session_state.h?

if you really need these values from BraveActivity, please consider making it as browser-wide util, not bound to cosmetic_filters.

goodov · 2024-12-11T12:13:22Z

components/brave_shields/core/common/features.cc

@@ -128,6 +129,14 @@ BASE_FEATURE(kCosmeticFilteringSyncLoad,
 BASE_FEATURE(kBlockAllCookiesToggle,
             "BlockAllCookiesToggle",
             base::FEATURE_DISABLED_BY_DEFAULT);
+// when enabled, allow to select and block HTML elements
+BASE_FEATURE(kBlockElementFeature,


maybe BraveShieldsElementPicker?

goodov · 2024-12-11T12:19:02Z

components/cosmetic_filters/resources/data/element_picker.ts

 }

 const active = document.getElementById('brave-element-picker')
 if (!active) {
-  launchElementPicker(attachElementPicker())
+  api.initElementPicker((platform: number) => {


I'd go something like platform: 'desktop' | 'android'

components/cosmetic_filters/renderer/cosmetic_filters_js_handler.cc

goodov · 2024-12-12T09:34:10Z

components/cosmetic_filters/common/cosmetic_filters.mojom

@@ -24,6 +24,9 @@ interface CosmeticFiltersHandler {

  // Opens the custom filter section in Shields settings .
  ManageCustomFilters();
+
+  GetElementPickerThemeInfo() => (
+      bool is_nightly_mode_enabled, int32 background_color);


is_nightly_mode_enabled -> is_dark_mode_enabled

goodov · 2024-12-12T09:42:55Z

browser/cosmetic_filters/cosmetic_filters_tab_helper.cc

+      color_provider.GetColor(kColorSidePanelBadgeBackground));
+#else   // !BUILDFLAG(IS_ANDROID)
+  std::move(callback).Run(chrome::android::GetDarkModeState() ==
+                              chrome::android::DarkModeState::kDarkModeSystem,


I think we should check both kDarkModeSystem and kDarkModeApp.

Signed-off-by: Vadym Struts <[email protected]>

github-actions · 2024-12-16T16:48:01Z

[puLL-Merge] - brave/brave-core@26725

Description

This PR introduces a new feature for element picking and blocking in the Brave browser, primarily focusing on the Android platform. It adds functionality to select and block specific HTML elements on web pages, enhancing the user's ability to customize their browsing experience.

Changes

Changes

android/brave_java_sources.gni:
- Added a new Java source file for cosmetic filters utilities.
android/java/org/chromium/base/BraveFeatureList.java:
- Introduced a new feature flag BRAVE_SHIELDS_ELEMENT_PICKER.
android/java/org/chromium/chrome/browser/app/BraveActivity.java:
- Added methods to support the new element picker feature, including opening custom filter settings and getting theme background color.
android/java/org/chromium/chrome/browser/cosmetic_filters/BraveCosmeticFiltersUtils.java:
- New file implementing utility functions for cosmetic filters, including launching the content picker and handling theme-related information.
android/java/org/chromium/chrome/browser/shields/BraveShieldsHandler.java:
- Updated to include the new element picker feature in the Brave Shields menu.
android/java/res/layout/brave_shields_secondary_layout.xml:
- Added a new text view for the "Block element" option.
browser/about_flags.cc:
- Added a new flag for the block element feature.
browser/android/cosmetic_filters/cosmetic_filters_utils.cc and .h:
- New files implementing the native side of cosmetic filters utilities.
browser/cosmetic_filters/cosmetic_filters_tab_helper.cc and .h:
- Updated to support the new element picker feature, including theme handling.
browser/ui/android/strings/android_brave_strings.grd:
- Added a new string for the "Block element" text.
components/cosmetic_filters/resources/data/element_picker.html and .ts:
- Significantly updated the element picker UI and functionality, including support for Android.
Various build and configuration files were updated to include the new components.

sequenceDiagram
    participant User
    participant BraveShieldsUI
    participant CosmeticFiltersUtils
    participant ElementPicker
    participant WebContent

    User->>BraveShieldsUI: Tap "Block element"
    BraveShieldsUI->>CosmeticFiltersUtils: launchContentPickerForWebContent()
    CosmeticFiltersUtils->>ElementPicker: Inject and initialize
    ElementPicker->>WebContent: Attach to DOM
    User->>ElementPicker: Select element
    ElementPicker->>CosmeticFiltersUtils: Create filter
    CosmeticFiltersUtils->>WebContent: Apply filter
    ElementPicker->>User: Show confirmation
    User->>ElementPicker: Confirm or cancel
    ElementPicker->>CosmeticFiltersUtils: Save or discard filter
    CosmeticFiltersUtils->>BraveShieldsUI: Update UI

Possible Issues

The element picker functionality might not work correctly on all websites, especially those with complex DOM structures or dynamic content.
Performance impact on lower-end Android devices when using the element picker feature.

Security Hotspots

The element picker injects JavaScript into web pages, which could potentially be exploited if not properly sandboxed.
The use of XPath selectors in Android might lead to overly broad selections if not carefully implemented.

brave-builds · 2024-12-17T00:05:00Z

Released in v1.75.104

vadimstruts linked an issue Nov 25, 2024 that may be closed by this pull request

Element Picker on Android brave/brave-browser#33241

Closed

github-actions bot assigned vadimstruts Nov 25, 2024

github-actions bot added the CI/storybook-url Deploy storybook and provide a unique URL for each build label Nov 25, 2024

github-actions bot added the chromium-version-mismatch The Chromium version on the PR branch does not match the version on the target branch label Dec 3, 2024

vadimstruts force-pushed the 33241-element-picker-on-android branch from 0b29960 to 50b3c88 Compare December 3, 2024 00:07

github-actions bot removed the chromium-version-mismatch The Chromium version on the PR branch does not match the version on the target branch label Dec 3, 2024

vadimstruts requested a review from AlexeyBarabash December 4, 2024 12:36

vadimstruts marked this pull request as ready for review December 4, 2024 12:42

vadimstruts requested review from deeppandya and a team as code owners December 4, 2024 12:42

AlexeyBarabash reviewed Dec 4, 2024

View reviewed changes

AlexeyBarabash approved these changes Dec 4, 2024

View reviewed changes

deeppandya approved these changes Dec 5, 2024

View reviewed changes

vadimstruts requested review from atuchin-m and antonok-edm December 5, 2024 13:22

antonok-edm approved these changes Dec 5, 2024

View reviewed changes

atuchin-m reviewed Dec 6, 2024

View reviewed changes

mkarolin approved these changes Dec 9, 2024

View reviewed changes

vadimstruts requested a review from a team as a code owner December 10, 2024 14:39

github-actions bot added CI/run-upstream-tests Run upstream unit and browser tests on Linux and Windows (otherwise only on Linux) puLL-Merge labels Dec 10, 2024

github-actions bot reviewed Dec 10, 2024

View reviewed changes

components/cosmetic_filters/renderer/cosmetic_filters_js_handler.cc Show resolved Hide resolved

components/cosmetic_filters/renderer/cosmetic_filters_js_handler.cc Show resolved Hide resolved

github-actions bot added the needs-security-review label Dec 10, 2024

github-actions bot assigned goodov, iefremov and thypon Dec 10, 2024

vadimstruts force-pushed the 33241-element-picker-on-android branch from d83dbdf to 064dbbf Compare December 10, 2024 18:50

goodov reviewed Dec 11, 2024

View reviewed changes

github-actions bot reviewed Dec 11, 2024

View reviewed changes

components/cosmetic_filters/renderer/cosmetic_filters_js_handler.cc Show resolved Hide resolved

components/cosmetic_filters/renderer/cosmetic_filters_js_handler.cc Show resolved Hide resolved

goodov approved these changes Dec 12, 2024

View reviewed changes

vadimstruts added 8 commits December 16, 2024 17:44

Implemented element picker, android

716f0c6

Signed-off-by: Vadym Struts <[email protected]>

format

41cc03a

Signed-off-by: Vadym Struts <[email protected]>

element picker, android + desktop v2

48f0865

Signed-off-by: Vadym Struts <[email protected]>

element picker, android + desktop, modified styles

575a2c3

Signed-off-by: Vadym Struts <[email protected]>

review changes + feature flag

27b4c0c

Signed-off-by: Vadym Struts <[email protected]>

review changes

a0d1ed1

Signed-off-by: Vadym Struts <[email protected]>

review changes

5e79e18

Signed-off-by: Vadym Struts <[email protected]>

review changes

828afc3

Signed-off-by: Vadym Struts <[email protected]>

vadimstruts force-pushed the 33241-element-picker-on-android branch from 382878a to 828afc3 Compare December 16, 2024 16:47

vadimstruts merged commit 4be896e into master Dec 16, 2024
18 checks passed

vadimstruts deleted the 33241-element-picker-on-android branch December 16, 2024 21:54

github-actions bot added this to the 1.75.x - Nightly milestone Dec 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Element Picker on Android #26725

Element Picker on Android #26725

vadimstruts commented Nov 25, 2024 •

edited

Loading

brave-builds commented Nov 25, 2024

github-actions bot commented Dec 3, 2024

AlexeyBarabash Dec 4, 2024

AlexeyBarabash Dec 4, 2024

AlexeyBarabash left a comment

deeppandya left a comment

antonok-edm Dec 5, 2024

antonok-edm Dec 5, 2024

antonok-edm Dec 5, 2024

vadimstruts Dec 9, 2024

antonok-edm Dec 5, 2024

atuchin-m Dec 6, 2024

atuchin-m Dec 6, 2024 •

edited

Loading

atuchin-m Dec 6, 2024

mkarolin left a comment

goodov Dec 11, 2024

goodov Dec 11, 2024

goodov Dec 11, 2024

goodov Dec 11, 2024

goodov Dec 11, 2024

goodov Dec 12, 2024

goodov Dec 12, 2024

github-actions bot commented Dec 16, 2024

Changes

brave-builds commented Dec 17, 2024

Element Picker on Android #26725

Element Picker on Android #26725

Conversation

vadimstruts commented Nov 25, 2024 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

brave-builds commented Nov 25, 2024

github-actions bot commented Dec 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexeyBarabash left a comment

Choose a reason for hiding this comment

deeppandya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atuchin-m Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkarolin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 16, 2024

Description

Changes

Possible Issues

Security Hotspots

brave-builds commented Dec 17, 2024

vadimstruts commented Nov 25, 2024 •

edited

Loading

atuchin-m Dec 6, 2024 •

edited

Loading