Donate dialog #556

petemill · 2018-10-03T07:48:53Z

TODO

More of the features should be supported for brave/brave-browser#1221 although not all are required for this Donate dialog, which has a closer milestone than the Tipping dialog

Current state

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.
Request a security/privacy review as needed.
Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone

Test Plan:

clean profile
enable rewards
go to brave.com and click on BAT logo in the url
in the panel click on donate now
site banner should appear with default text
go to 3zsistemi.si and click on BAT logo in the url
in the panel click on donate now
site banner should have custom text

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

petemill · 2018-10-03T07:56:38Z

I suggest we work together on this branch @NejcZdovc. There's work to do on two fronts:

Get more of the actual frontend and API for chrome://donate implemented, and make sure we're passing the correct data in to the dialog
Improve the Dialog UX (position, size, some behavior), as well as see if it passes review.

Alternatively, we could get this PR merge-able by removing the code path to show the dialog, and then work in two subsequent PRs for the above issues.

cc @bbondy @bridiver (to review the correct BrowserContext is being used) and @bsclifton @simonhong for feedback

petemill · 2018-10-10T05:31:19Z

I've had no luck with some design ambitions here:

Having the web view be full-width of the parent tab, but have a dynamic height. The closest I could get this is to have auto-size (like an extension popup) but min/max the width of the tab and allow the height to auto expand. Problem is the height doesn't expand high enough (even if I change the dialog to use normal document flow instead of position: fixed. Other problem is that min/max cannot be updated, so window resize does not change the size of the popup.
Solution is for now to have a fixed size.
Having the popup be semi-transparent. No luck so far. Related to the first point I wanted to try setting a transparent background on the WebContents, but I didn't get far with that. A big re-architecture to get the webview inside a non-WebContent popup window with a transparent area may work, but that would take a lot of time. So it can be revisited in a separate issue.

For now I've set the popup to be a fixed [window width - 25px] x 500px, so it looks similar to other popups (Print Preview..., Cast...). Alternative would be to change to autosize and have the height manually set in the html.

petemill · 2018-10-10T05:40:52Z

Scratch that last point - I've got the auto-size working with some adjustments to the html content. Still doesn't resize when the window resizes, but I'm ok with that for now.

… instead of fixed position as per brave/brave-core#556

browser/extensions/api/brave_rewards_api.cc

jumde · 2018-10-10T22:18:56Z

browser/ui/webui/brave_donate_ui.cc

+  }
+  result.SetDictionary("social", std::move(social));
+
+  web_ui()->CallJavascriptFunctionUnsafe("brave_rewards_donate.publisherBanner", result);


Use CallJavascriptFunction instead?

I forgot why, but CallJavascriptFunction was not working. cc @bbondy if you can remember

we currently use CallJavascriptFunctionUnsafe across all our webui logic

jumde · 2018-10-11T01:18:52Z

components/definitions/rewardsDonate.ts

+    publisherKey: string
+    title: string
+    description: string
+    background: string


Is the publisher background and logo stored locally?

no, we get it from publisher API. Example of the url https://rewards-stg.bravesoftware.com/P5RxjtP44tJnVNeR9nNCLmgX. In this PR we have both of them disabled and we will fix this in the follow up, because you can't do requests outside from webui

bridiver · 2018-10-11T03:26:02Z

browser/ui/webui/brave_donate_ui.cc

+}
+
+void RewardsDonateDOMHandler::Init() {
+  Profile* profile = Profile::FromWebUI(web_ui());


as @jumde mentioned the UI should be disabled if RewardsServiceFactory::GetForProfile is null. Any menu items should similarly be disabled

Didn't see an obvious quick way to disable after the DomHandler::Init phase, so I disabled via the webui constructor, checking IsOffTheRecord. Shows chrome://network-error for now

bridiver · 2018-10-11T03:26:38Z

components/brave_rewards/browser/rewards_service_impl.cc

-const base::FilePath::StringType kPublisher_state("publisher_state");
-const base::FilePath::StringType kPublisher_info_db("publisher_info_db");
-const base::FilePath::StringType kPublishers_list("publishers_list");
+  const base::FilePath::StringType kPublisher_state("publisher_state");


why are these indented here?

jumde · 2018-10-11T04:04:32Z

common/webui_url_constants.cc

 const char kBraveNewTabJS[] = "brave_new_tab.js";
 const char kBraveUIWelcomeURL[] = "chrome://welcome/";
 const char kBraveUIRewardsURL[] = "chrome://rewards/";
 const char kBraveUIAdblockURL[] = "chrome://adblock/";
+const char kBraveUIDonateURL[] = "chrome://donate/";


is direct navigation to chrome://donate expected?

Is there an easy way to block this? Right now it errors because it doesn't get any JSON input args, and displays fallback copy. Either way, I talked with @NejcZdovc and we'll make sure the webui doesn't display anything if navigated to in a tab.

cc: @bridiver ^

For now, it's only possible in non-OffTheRecord profiles and is non-functional

i.e. I blocked it

Use ConstrainedWebDialogUI instead of regular WebUI in order to gain message passing for incoming arguments and dialog-closing function. Stub content for demonstration - to be replaced with content from brave-ui.

Expose `donateToSite` to `chrome.braveRewards` which accepts tabId and publisherKey Pass publisherKey to chrome://donate WebUI page Read publisher key in donate JS and write to html output

Includes tests

NejcZdovc · 2018-10-12T05:40:48Z

I see that security review was closed and all issues where addressed. If we missed anything @bridiver @jumde please ping me and I will create follow ups.

NejcZdovc · 2018-10-12T06:02:06Z

master a16f674
0.56.x 8405e0b
0.55.x 7d209e9

Donate dialog

…eight with normal DOM layout flow instead of fixed position as per #556

petemill self-assigned this Oct 3, 2018

petemill added the pr/work-in-progress label Oct 3, 2018

NejcZdovc force-pushed the donation-dialog branch 3 times, most recently from 5c19e6b to 0ef702d Compare October 4, 2018 05:55

NejcZdovc self-assigned this Oct 4, 2018

petemill force-pushed the donation-dialog branch 2 times, most recently from 6a87038 to 3c3a635 Compare October 5, 2018 21:38

NejcZdovc mentioned this pull request Oct 8, 2018

Adds support for site banner brave-intl/bat-native-ledger#131

Merged

NejcZdovc force-pushed the donation-dialog branch 2 times, most recently from 7608b93 to 2a62cb5 Compare October 8, 2018 18:25

petemill added a commit to brave/brave-ui that referenced this pull request Oct 10, 2018

Allow donation dialog to auto-size height with normal DOM layout flow…

87687f3

… instead of fixed position as per brave/brave-core#556

petemill mentioned this pull request Oct 10, 2018

Allow donation dialog to auto-size height with normal DOM layout flow brave/brave-ui#193

Merged

NejcZdovc pushed a commit to brave/brave-ui that referenced this pull request Oct 10, 2018

Allow donation dialog to auto-size height with normal DOM layout flow…

77ff55c

… instead of fixed position as per brave/brave-core#556

NejcZdovc force-pushed the donation-dialog branch from b4704ed to 9508bb5 Compare October 10, 2018 20:57

jumde reviewed Oct 10, 2018

View reviewed changes

browser/extensions/api/brave_rewards_api.cc Show resolved Hide resolved

jumde reviewed Oct 10, 2018

View reviewed changes

jumde reviewed Oct 11, 2018

View reviewed changes

bridiver reviewed Oct 11, 2018

View reviewed changes

jumde reviewed Oct 11, 2018

View reviewed changes

petemill force-pushed the donation-dialog branch from 7964203 to e2a65f4 Compare October 11, 2018 18:20

petemill added 3 commits October 11, 2018 20:48

Add chrome://donate dialog

073b948

Use ConstrainedWebDialogUI instead of regular WebUI in order to gain message passing for incoming arguments and dialog-closing function. Stub content for demonstration - to be replaced with content from brave-ui.

API for showing donations dialog constrained to a Tab

6fa3586

Connect Rewards extension popup to Donations popup

78ebfe0

Expose `donateToSite` to `chrome.braveRewards` which accepts tabId and publisherKey Pass publisherKey to chrome://donate WebUI page Read publisher key in donate JS and write to html output

NejcZdovc force-pushed the donation-dialog branch from 11cc346 to 699f6c3 Compare October 11, 2018 18:50

NejcZdovc changed the title ~~[WIP] Donate dialog~~ Donate dialog Oct 11, 2018

NejcZdovc force-pushed the donation-dialog branch from 252b660 to 2955b14 Compare October 11, 2018 20:02

petemill force-pushed the donation-dialog branch from 2955b14 to 3151f55 Compare October 11, 2018 20:13

petemill removed the pr/work-in-progress label Oct 11, 2018

NejcZdovc and others added 8 commits October 11, 2018 13:25

Adds ui for rewards donate dialog from brave-ui

433b074

Includes tests

donation dialog gets fixed width and automatic height

45bcdbf

OpenDonationDialog moves from donations namespace to brave_rewards

c2b1476

rewards donateToSite: sanity check context is not private

dc6ad35

Updates DEPS

4e7d378

rewards extension popup closes when ask for donation UI

92e7fee

Adds missing translations

6b294a2

chrome://donate shows error if loaded in private or tor context

f190808

petemill force-pushed the donation-dialog branch from 3151f55 to f190808 Compare October 11, 2018 20:25

NejcZdovc approved these changes Oct 12, 2018

View reviewed changes

NejcZdovc merged commit a16f674 into master Oct 12, 2018

NejcZdovc deleted the donation-dialog branch October 12, 2018 05:51

NejcZdovc added a commit that referenced this pull request Oct 12, 2018

Merge pull request #556 from brave/donation-dialog

7d209e9

Donate dialog

NejcZdovc added a commit that referenced this pull request Oct 12, 2018

Merge pull request #556 from brave/donation-dialog

8405e0b

Donate dialog

NejcZdovc added merged/0.55.x feature/rewards labels Oct 12, 2018

bbondy added this to the 0.55.x - Release milestone Jan 14, 2019

NejcZdovc pushed a commit that referenced this pull request Sep 19, 2019

Rewards: Imported from brave-ui: Allow donation dialog to auto-size h…

18602ba

…eight with normal DOM layout flow instead of fixed position as per #556

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Donate dialog #556

Donate dialog #556

petemill commented Oct 3, 2018 •

edited by NejcZdovc

Loading

petemill commented Oct 3, 2018

petemill commented Oct 10, 2018 •

edited

Loading

petemill commented Oct 10, 2018

jumde Oct 10, 2018

NejcZdovc Oct 10, 2018

jumde Oct 11, 2018

NejcZdovc Oct 11, 2018

bridiver Oct 11, 2018

petemill Oct 11, 2018

bridiver Oct 11, 2018

NejcZdovc Oct 11, 2018

jumde Oct 11, 2018

petemill Oct 11, 2018

jumde Oct 11, 2018

petemill Oct 11, 2018

petemill Oct 11, 2018

NejcZdovc commented Oct 12, 2018

NejcZdovc commented Oct 12, 2018 •

edited

Loading

Donate dialog #556

Donate dialog #556

Conversation

petemill commented Oct 3, 2018 • edited by NejcZdovc Loading

TODO

Current state

Submitter Checklist:

Test Plan:

Reviewer Checklist:

petemill commented Oct 3, 2018

petemill commented Oct 10, 2018 • edited Loading

petemill commented Oct 10, 2018

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

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

NejcZdovc commented Oct 12, 2018

NejcZdovc commented Oct 12, 2018 • edited Loading

petemill commented Oct 3, 2018 •

edited by NejcZdovc

Loading

petemill commented Oct 10, 2018 •

edited

Loading

NejcZdovc commented Oct 12, 2018 •

edited

Loading