-
Notifications
You must be signed in to change notification settings - Fork 887
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
Element picker #8503
Element picker #8503
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks awesome! I'm probably not the best person to be reviewing the JS code or CSS selector generation logic, but I like what I see just from playing around with it for a bit 😄
components/brave_extension/extension/brave_extension/elementPicker.js
Outdated
Show resolved
Hide resolved
components/brave_extension/extension/brave_extension/background/events/cosmeticFilterEvents.ts
Show resolved
Hide resolved
<head> | ||
<meta charset="utf-8"> | ||
<title>Brave Element Picker</title> | ||
<style> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a design expert of course, but I'd suggest using our Poppins font for the buttons and maybe a monospace font for the selector, so it's at least consistent with the brave://adblock
page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay so i really tried to include that poppins.css
file but i was not able to pull it in. since it lives at chrome://
and the extension lives in chrome-extension://
it denies it because the extension is accessing an internal resource. i wasn't able to include it as a web-accessible-resource either. for now i just did font-family: "Noto Sans", Arial, sans-serif;
because i saw that style on the brave://adblock
page too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, sorry about that! If I knew it was that difficult to get working I wouldn't have even suggested it 😓
Already passed CI and i since it's just JS changes, it's a waste of money to keep running it. Added CI/skip |
components/brave_extension/extension/brave_extension/content_cosmetic.ts
Outdated
Show resolved
Hide resolved
components/brave_extension/extension/brave_extension/content_cosmetic.ts
Outdated
Show resolved
Hide resolved
This is the first pass of the element picker. Let's discuss how it operates. Overview -------- When the user activates the picker via the context menu, the element picker iframe is appended to the document.documentElement from the content script. We apply some CSS rules so the iframe overlays the entire page and handles all pointer events. The latter is important so that links and buttons don't compete with the element picker UI. The former is important so that points inside the iframe are the same as points on the parent page. Drawing element bounding boxes ------------------------------ When the user hovers around the page, bounding boxes are drawn around "target" elements. Since the iframe consumes all pointer events, it sends a message to the content script with the coordinates of the cursor. The element on the source page is fetched with document.elementFromPoint(..). The candidate element has its bounding box coordinates sent back to the iframe. The box is drawn by applying a <rect> masks on an otherwise transparent black background. Selecting an element -------------------- When a user clicks a bounding box, once again, the coordinates are used to fetch an element on the source page. We create a CSS selector from the element id, classes, and tree hierachy, applying the process recursively on parent nodes until we reach the body. We use this selector and fetch all matching elements; the bounding box coordinates of all matching elements are sent to the iframe. Drawing is done the same way as discussed in the previous section. Creating the filter ------------------- Once a user has selected an element, they will see orange bounding boxes on the screen, representing all elements that match the filter rule. Upon selecting "Create", the cosmetic filter is applied and saved for that site. Why go through the trouble of an iframe? ---------------------------------------- (1) It helps us isolate click events from the source page. (2) Popping up the filter selection on the source page would mean that the source page could run JavaScript and modify the cosmetic filter rules before the user clicked "Create". We set the src of the iframe to be a web accessible resource, since a nice property of these is the uri is chrome-extension://. The source page will always have a protocol of http://, it will never be able to modify the contents of the iframe due to the same-origin-policy. Future TODOs ------------ * Make the UI nicer (I'm not a gifted UI person and could use some help) * Add a "Preview" button to the picker UI that temporarily applies the cosmetic filter rule. * Implement the uBlock "specificity" algorithm. This runs heuristics to make selector less generic. Used for matching elements that seem similar to the target. Resolves brave/brave-browser#13808
We are going to get rid of the extension. In order to support it on mobile it has been redone and the content_cosmetic.ts that is going to be used was placed here https://github.com/brave/brave-core/blob/master/components/cosmetic_filters/resources/data/content_cosmetic.ts. The only thing that delays us is that we are checking why the new implementation doesn't behave the same way as expected(most likely scriptlets we inject are done for extensions calls only). I would propose to make the picker as a separate content file |
@SergeyZhukovsky even when we switched over to the native cosmetic filtering version we'd kept the "legacy" cosmetic filtering stuff in the extension (including all right-click cosmetic filtering functionality). I've since migrated that to use the custom filters in brave://adblock, but we're still using the extension to handle the right-click menu, and some other (e.g. rewards related) functionality. The extension isn't going away soon, but it's gonna be pretty difficult to get a reasonable element picker implementation on Android anyways, so cross-platform shouldn't be a big concern. |
content_cosmetic.ts will be deprecated when it moves to the native cosmetic filtering part of the codebase.
components/brave_extension/extension/brave_extension/background/events/cosmeticFilterEvents.ts
Show resolved
Hide resolved
components/brave_extension/extension/brave_extension/content_element_picker.ts
Show resolved
Hide resolved
components/brave_extension/extension/brave_extension/content_element_picker.ts
Outdated
Show resolved
Hide resolved
components/brave_extension/extension/brave_extension/content_element_picker.ts
Show resolved
Hide resolved
@pes10k one thing is the CSS escapes are actually necessary because some pages have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
NOTE: The sonarcloud analysis is flagging the xml namespace for svg being http, and post-init is failing because of audit deps (this introduces no deps). Both unrelated. |
Element Picker initial implementation
Resolves brave/brave-browser#13808
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
,npm run lint
,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan: