-
Notifications
You must be signed in to change notification settings - Fork 868
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
Add support for Twitter tips #2195
Conversation
717e4f3
to
cb6a746
Compare
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 reviewed the button-injection methodology and left some feedback. This is great!
"https://*.twitter.com/*" | ||
], | ||
"css": [ | ||
"content.css" |
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 suggest we don't add a css file to the content_scripts
since, unlike JS, I believe this is added to the global document styles. Instead, especially since these styles are minimal, perhaps we add them via JS, and ideally within a shadow DOM so that nothing is affected by the document's stylesheet.
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.
Great point, I didn't realize that. Will fix, thank you.
} | ||
], | ||
"web_accessible_resources": [ | ||
"img/bat-16.png" |
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.
Again, suggest we expose as little as possible to the site. Here, we can inline an SVG. Or, if it must be PNG (not ideal since not scalable), then that can be inlined via data: URI.
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.
SVG should work I think, I'll investigate.
|
||
const createBraveTipAction = (tweet: Element) => { | ||
// Create the tip action | ||
const braveTipAction = document.createElement('div') |
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.
Right now I believe this method of creating DOM elements has the following issues that were brought up in an older security discussion:
- The site can detect that the user is a Brave Rewards user via detecting the element
- The site can intercept the click and do something malicious
- The site's styles can unintentionally / intentionally interfere with the button.
These issues can be at least minimized by at least creating a single element, and then putting all the button content inside that element's shadow DOM.
And then, we could explore not having to add an element at all, but instead adding the button to the parent's closed shadow DOM. This is what I did on my prototype for the popup.
Let me know what you think @emerick
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.
Really good point, will get that in there!
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.
@petemill Do you happen to know: is there a clever way to access a CSS class defined in the parent document from the shadow DOM? The issue I'm running into is that we reference a handful of CSS classes defined in the Twitter stylesheet, but the shadow DOM doesn't have access to those styles. I'm reluctant to copy those underlying styles directly into the shadow DOM as then it becomes kind of brittle (e.g., if/when Twitter tweaks those styles).
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.
No, I guess that's the point of the shadow DOM - that the document styles cannot affect the shadow elements. So it's a trade-off between wanting to affect the styles intentionally and not wanting unintentional or malicious styles. Perhaps someone like @diracdeltas could weight in on the priority.
However, I would add that using the css class names could be just as brittle, since these could change anytime. One way to somewhat get around that (again not using shadow DOM) would be to find an existing button's node that is a sibling of our 'tip' button, and call .cloneNode()
on it, then add our own (inline) styles to override.
Are there many / any native document styles that we need to inherit anyway, since AFAIK our button is simply our icon?
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.
Right, I see what you're saying (and you make a good point about the class names). I think you're right that we don't end up needing very many of the inherited styles, just some text properties for the "Tip" label and sizing properties most likely. Probably not too much harm including them in the shadow DOM if that's the case. I'll experiment with that, thanks again!
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.
++ for encapsulating our injected element in a shadow DOM
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.
Added to shadow DOM.
93a3dc6
to
d0c84ea
Compare
7042fb6
to
96b43d4
Compare
Fixes brave/brave-browser#4346
Fixes brave/brave-browser#4393
Adds support for Twitter tips, as described in https://docs.google.com/document/d/1OlRWQ1Ksqyt6XT5N0_k503sRP-7clEthDFG1BXeZI5E
Submitter Checklist:
npm test brave_unit_tests && npm test brave_browser_tests && npm run test-security
) onnpm run lint
)git rebase master
(if needed).git rebase -i
to squash commits (if needed).Test Plan:
Note: You can create a verified Twitter publisher by visiting this special link and following the Twitter flow:
https://publishers-staging.basicattentiontoken.org/publishers/home?twitter=true
Enabling/disabling the Brave Tip icon
Tipping a Twitter user
Reviewer Checklist: