Skip to content
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

Rewards Tip Floating Dialog #1221

Closed
simonhong opened this issue Sep 21, 2018 · 14 comments
Closed

Rewards Tip Floating Dialog #1221

simonhong opened this issue Sep 21, 2018 · 14 comments

Comments

@simonhong
Copy link
Member

Web-ui dialog

  • Similar to the certificate white popup when you click on Secure next to an https site.
  • Can be fully customized with a webui content (html/js).
  • Can specify location dynamically and can change it dynamically
  • Support an api to dismiss it
  • Can specify size
  • Dismissed if the user clicks outside of it

screen shot 2018-09-05 at 1 26 06 pm

screen shot 2018-09-05 at 1 26 10 pm

@simonhong simonhong self-assigned this Sep 21, 2018
@simonhong simonhong added this to the Releasable builds 0.55.x milestone Sep 21, 2018
@simonhong simonhong added this to Native UI in 0.55.x - Release Sep 21, 2018
@petemill petemill self-assigned this Sep 24, 2018
@petemill
Copy link
Member

petemill commented Oct 1, 2018

Looks like there's two levels of API needed:

  1. Open Popup for a tab.
    This is to enable direct donation to a site that is open in a tab.
    Url: chrome://donate
    Params needed: site origin
    Invocation source: c++ (backend of an extension popup, or a menu)

  2. Open Popup for a portion of WebContents.
    This is to enable direct donation to a user of a site that is open in a tab.
    Url: chrome://tip
    Params needed: site origin (twitter), content creator (e.g. twitter handle), DOM element (for position anchor)
    Invocation source: Javascript, inside a content script

Therefore we can implement two levels of API:

Static c++ API (Donate and Tip)

static DonationDialogController::Donate(WebContents* initiator)
static TipDialogController::Tip(WebContents* initiator, Rect element_bounds)

Javascript API (Tip)

js: brave.rewards.showContentCreatorTipDialog(creatorName, element)

  • Instantiated from content script which modifies behavior of site's buttons (twitter like, facebook like, youtube ThumbsUp)
  • Works loosely similar to Autofill - render receives command and calculates coordinates which are passed via IPC to browser client
  • Browser client calls static TipDialogController::Tip

@NejcZdovc
Copy link
Contributor

@petemill for donate we need a lot more data. Will this data be fetched inside this dialog and we can use origin as to what data we need in the webui?

@petemill
Copy link
Member

petemill commented Oct 1, 2018

@NejcZdovc lots of data can be fetched from the WebContents, including origin. The WebUI can then have it's own API for actions and to fetch data from Rewards service. So I imagine everything you need unique to that invocation will be covered in the WebContents - do you agree with that?

@NejcZdovc
Copy link
Contributor

yeah to get data from rewards service I only need origin. Based on that I can get all other data. Would it be possible to send in instead of origin publisher key? This way we don't need to calculate it from origin again?

@petemill
Copy link
Member

petemill commented Oct 1, 2018

@NejcZdovc that sounds possible if all triggers for the dialog know the Publisher Key. It is only the Rewards extension popup, and does that know it?

Edit: I'm just wondering if now or in the future we would have a unknowledgable trigger - such as a menu item that simply says 'Donate to this site'

@NejcZdovc
Copy link
Contributor

NejcZdovc commented Oct 1, 2018

@petemill yeah they know that key. For other options you could always call rewards service that will return publisher key based on origin and then you trigger donate

@petemill
Copy link
Member

petemill commented Oct 1, 2018

Sure we can support both 👍. What about tipping @NejcZdovc? Will it be enough to have origin / publisherId and the site-specific creator name. E.g. twitter.com and @twitterusername ?

@NejcZdovc
Copy link
Contributor

NejcZdovc commented Oct 1, 2018

yup as long as we know to who like/heart belong (twitter handler) we should be good

@bbondy bbondy added the QA/Yes label Oct 10, 2018
@petemill petemill assigned simonhong and unassigned simonhong Oct 12, 2018
@petemill petemill changed the title Implement Dialog for webui Rewards Tip Floating Dialog Oct 12, 2018
@petemill petemill modified the milestones: 0.55.x, 0.56.x Oct 12, 2018
@bbondy bbondy added this to Rewards in 1.0 Tasks by Category Oct 14, 2018
@petemill petemill removed this from Native UI in 0.55.x - Release Oct 15, 2018
@petemill
Copy link
Member

@rebron @mandar-brave please can we confirm which milestone we're currently targeting floating user tips, so I can prioritize work - is it still '1.0'?

@NejcZdovc
Copy link
Contributor

@petemill it's targeted for 57 build

@petemill petemill modified the milestones: 1.0, 0.57.x - Dev Oct 23, 2018
@NejcZdovc NejcZdovc removed this from Rewards in 1.0 Tasks by Category Oct 23, 2018
@NejcZdovc NejcZdovc added this to Backlog in Rewards Oct 30, 2018
@bbondy bbondy removed this from the 0.57.x - Dev milestone Oct 30, 2018
@bbondy bbondy added this to the 1.x Backlog milestone Oct 30, 2018
@NejcZdovc NejcZdovc added the priority/P3 The next thing for us to work on. It'll ride the trains. label Oct 31, 2018
@NejcZdovc NejcZdovc moved this from Untriaged Backlog to P3, P4, & P5 Backlog in Rewards Oct 31, 2018
@petemill
Copy link
Member

Just noting that we still don't have:

  • A dialog which can:
    • Auto-size whilst being located in a specific position (though ConstrainedWebDialog is very close)
    • Have rounded borders via clipping the webview or via a transparent background, or a mix of native UI controls as well as the webview to achieve a similar look
    • Have custom shape (arrow pointing at popup anchor) via native UI controls
  • An easy means of querying the DOM from the browser UI layer in order to get the dialog position, though this could be provided via JS content script.

@petemill
Copy link
Member

For Donate dialog I did use ShowConstrainedWebDialogWithAutoResize. It's problems were:

  • Could not overlay the rest of the content with a semi-transparent background (not a big priority though)
  • Could not have it take 100% width of parent with auto-sized height
    • Can only give an autoresizing dialog initial min and max for both width and height, whereas we want to have a min and max for height, and a fixed width that changes on browser window resize. Whereas a non-auto-resize ShowConstrainedWebDialog can have its width and height changed at any time, but will not auto-resize. This fixed logic for both width and height seems to go down to the render layer.
      So for the Donate dialog, the main bug with this compromise only presents itself when resizing the browser window.

For the Tip dialog, we are considering using DOM injection for now, e.g. brave/brave-core#1040

@NejcZdovc NejcZdovc added this to Others in Rewards Team Jan 4, 2019
@petemill
Copy link
Member

Just found an interesting precedent in blink for displaying an overlay on the webcontent which is native (and hence not part of the host DOM) but contains web content with a custom js api, as well as exactly the custom popup shape we're looking for: InspectorOverlayPage.

image

InspectorOverlayAgent.cc
InspectorOverlayPage.html

cc @simonhong @pilgrim-brave @davidtemkin

(UX considerations aside)

@rebron rebron modified the milestone: 1.x Backlog Feb 7, 2019
@NejcZdovc
Copy link
Contributor

closing as we implemented this in a different way

Rewards automation moved this from Icebox to Done May 27, 2019
Rewards Team automation moved this from Others to Closed May 27, 2019
@NejcZdovc NejcZdovc removed this from Done in Rewards May 27, 2019
@NejcZdovc NejcZdovc removed this from Closed in Rewards Team May 27, 2019
@NejcZdovc NejcZdovc added closed/not-actionable and removed QA/Yes priority/P3 The next thing for us to work on. It'll ride the trains. labels May 27, 2019
@NejcZdovc NejcZdovc added this to the Dupe / Invalid / Not actionable milestone Jun 3, 2019
@bbondy bbondy removed this from the Dupe / Invalid / Not actionable milestone May 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants