a few tune-ups for the PDF contribution reports

[mrose17]

Did you search for similar issues before submitting this one? yes

Describe the issue you encountered:

1. Verified publishers not shown as verified
2. The OK button is not placed properly (see attachment)
3. the PDF file is generated in a new tab which then disappears and a file save dialog comes up. this is jarring. wouldn't it be better to open a new tab, generate the PDF and then let the user view it normally, and then let them decide to save it?

Expected behavior:

1. show verified publishers with icon
2. place OK button properly
3. when a contribution report is generated, just open a new tab and show it as a PDF

• Platform (Win7, 8, 10? macOS? Linux distro?): macOS

• Brave Version: 0.12.10 (master)

• Steps to reproduce:

  1. go to about:preferences#payments
  2. click on 'View Payment History"
  3. click on a contribution

• Screenshot if needed:
  

• Any related issues:
  none.
  

[luixxiul]

I'd like to take the button alignment issue.

[mrose17]

@luixxiul - thanks!

[willy-b]

Hey @luixxiul, can we split the Payment History modal OK button issue from the Contribution Statement PDF bugs @mrose17 brought up here?

That OK button regression is entirely separate and so I think if I fix the PDF Statement issues we won't step on each other's toes at all.

[luixxiul]

Agree, will do! @willy-b

It has been done by @bradleyrichter #6059

[willy-b]

That seems like yet another issue:
#6059 is about Payment History having correct past/future context, not the OK button position, and the screenshot is of a different version of Brave which is before the Contribution PDF was added.

Can the OK button be it's own issue? It doesn't exist in the version in the screenshot from @bradleyrichter in #6059.

[luixxiul]

@willy-b my bad, you're right!

[bsclifton]

great- those two other issues are now split out; let us know if there's anything else you need, @willy-b 😄 thanks for reviewing!

[mrose17]

@bsclifton - can we close this issue now?

[willy-b]

@mrose17, I don't think so. I'm working on this now. To my knowledge no one has pushed a fix for verified publishers or opening the generated contribution PDF in PDF.js instead of launching a download.

[mrose17]

@willy-b - thanks. do you think you can address this issue?

[willy-b]

yes, I'll let you know if I encounter problems.
working on verified publishers right now, will update here when that is finished and I begin work on opening with PDF.js.

do you know what release this is targeted for?

[mrose17]

i'm hoping end of next week? is that reasonable?

[willy-b]

sounds good, go ahead and set a milestone if you like

[mrose17]

will do, thanks!

[willy-b]

I pushed the verified publisher icon fix last week and the open-in-PDF.js fix is ready, though it depends on being able to open local PDFs which may have just been fixed as #6330 cc / @bbondy (previously was a bug as #2714)

[willy-b]

Just opened a PR for the one-liner verified publisher icon fix (or feel free to make change yourself as part of one of your commits, you'll see it's really minor).

[willy-b]

Hey @mrose17 , update:
The publisher icon fix is ready in #6375, but the PDF.js fix will not be ready today as changes in PDF.js have to be made and I am not familiar enough.

Also the issue for the original feature has been reopened (#4769) due to a regression on Windows and possibly elsewhere, so I need to work on that.

[mrose17]

@willy-b thanks for the update... many thanks for sticking with this!

cc: @bsclifton

[willy-b]

hey @mrose17, another status update:

The situation with the underlying feature (contrib stmt PDFs) being broken during Chromium54 upgrade is now mostly figured out (#4769 regression).

The remaining webview API regression that this depends on is webview.printToPDF.
It was broken during the Chromium54 upgrade (being fixed in #6450) and is needed (even with PDF.js change) because it generates the actual PDF doc in first place.

In chasing the printToPDF issue (@bbondy assigned himself last night in #6450 but I have been looking in parallel) I have been catching up on Electron/Muon standalone build being replaced by browser-laptop-bootstrap in last 2 weeks (brave/muon#132 , PR brave/muon#136). Still getting the new browser-laptop-bootstrap build going, issues I have with that are/will be tracked in https://github.com/brave/browser-laptop-bootstrap/issues/9.

thanks.

[mrose17]

@willy-b - thanks! my hope is that by this time tomorrow, it will all be resolved and tested...

[willy-b]

@mrose17, quick clarification:
by tomorrow you are expecting fixes for the printToPDF (#6450) and setTabIndex (#6442) muon changes that broke the original feature?

or two items you requested here?

• the verified publisher fix (done, fix verified publisher icon in Contribution Statement PDF #6375)
• opening the PDF in PDF.js after generating it (depends on pdfjs needs local file access #2714)

given the original feature is still pending #6450, #6442 we can have #6375 now (though can't test it without rewinding to old chromium) but PDF.js will not be here tomorrow :-/

[mrose17]

my expectation is whatever your comfortable with!

[bbondy]

print-to-pdf is working on mac and linux on this branch: print-to-pdf
It isn't tested yet on Windows, but it might need another fix there.

The setTabIndex stuff should be resolved now with Preview7 which you'll get if you npm install now. (Electron 2.0.7

[willy-b]

The print-to-pdf branch in brave/muon, not browser-laptop (this repo), right?

when will it hit browser-laptop?

[willy-b]

Fix in Muon doesn't mean I or many others can test it or work on it:
Muon can't be built standalone anymore (brave/muon#132) and the setup/build for browser-laptop-bootstrap is slow and tricky (e.g. https://github.com/brave/browser-laptop-bootstrap/issues/9 )

(it is awesome to have the fix, just saying cannot test in Brave until it is distributed to browser-laptop since the build scripts that used to allow pushing Electron/Muon to browser-laptop were stripped out 2 weeks ago, brave/muon#132).

[mrose17]

@bsclifton - would it be OK to ask @willy-b to submit a PR to get this into https://github.com/brave/browser-laptop/tree/0.13.1-branch ?

[bsclifton]

@mrose17 PRs definitely welcome 😄

Since this issue is kind of a "catch all", I'd suggest breaking out the items being fixed into discrete issues which are easy to test (and capture all the info about the issue). I'm not sure which issue his PR would be addressing?

[mrose17]

@willy-b - can you generate a PR ?

@bsclifton - the three things in the issue are kind of small. do we really need to split this issue into three?

[bsclifton]

@mrose17 After reading through the original post, it should be OK (sorry I had just glanced before) 😄

[mrose17]

no worries!

[willy-b]

there's a PR #6375 for the publisher icon.
the other part (opening in new tab) depends on #2714 (ability of Brave to open local PDFs) which is still outstanding.

[mrose17]

@willy-b - thanks! i'll see what we can do...

[bsclifton]

Fixed with #6375