Add feature window.prompt

[AlexRobinson-]

Fixes #94

What it looks like

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. (Ask a Brave employee to help if you cannot access this document.)

Test Plan:

See #94

Automated test plan

npm run unittest -- --grep="onWindowPrompt"
npm run unittest -- --grep="MessageBox component unit tests"

Reviewer Checklist:

• Request a security/privacy review as needed if one was not already requested.

Tests

• Adequate test coverage exists to prevent regressions
• Tests should be independent and work correctly when run individually or as a suite ref
• New files have MPL2 license header

[codecov-io]

Codecov Report

Merging #13563 into master will increase coverage by 0.03%.
The diff coverage is 73.33%.

@@            Coverage Diff             @@
##           master   #13563      +/-   ##
==========================================
+ Coverage   56.45%   56.49%   +0.03%     
==========================================
  Files         284      284              
  Lines       29361    29380      +19     
  Branches     4877     4879       +2     
==========================================
+ Hits        16576    16597      +21     
+ Misses      12785    12783       -2

Flag	Coverage Δ
#unittest	56.49% <73.33%> (+0.03%)	⬆️

Impacted Files	Coverage Δ
app/locale.js	35.77% <ø> (ø)	⬆️
app/renderer/components/common/textbox.js	46.47% <40%> (+4.22%)	⬆️
app/renderer/components/common/messageBox.js	86.11% <72.72%> (-1.65%)	⬇️
app/browser/tabMessageBox.js	73.14% <85.71%> (+4.46%)	⬆️

[ryanml]

@AlexRobinson- what a small world... I started on this implementation last night and our approaches are very close! Really solid job! I'm excited to see this get implemented :D

[AlexRobinson-]

Oh no, I'm sorry for stealing this from you @ryanml ! Are there any parts of your implementation you prefer over mine you would like to see go into the PR?

[ryanml]

Oh no not at all @AlexRobinson- ! I didn't call it any way, just was a coincidence :) And honestly aside from naming we did just about the same thing

[bradleyrichter]

@AlexRobinson- Do you happen to have any example pages where this is used in Chrome? I'm wondering if there are any standards to guide the layout of the dialog.

Of not, then please just enlarge the dialog enough to allow for a larger input field, input text size (min 13px) and some margin around the text.

thanks!

[NejcZdovc]

Don't forget to add translation for this two buttons

[AlexRobinson-]

Can do! Did you want me to replace the other locations this text appears as well?

E.g.
https://github.com/AlexRobinson-/browser-laptop/blob/ef1cb6caf42f1a79c71715e20bb2f450e58d74b6/app/browser/tabMessageBox.js#L24

https://github.com/AlexRobinson-/browser-laptop/blob/ef1cb6caf42f1a79c71715e20bb2f450e58d74b6/app/browser/tabMessageBox.js#L38

[bsclifton]

@AlexRobinson- that would be great 😄 👍

[AlexRobinson-]

@bradleyrichter An example page I have been using that was mentioned in the original issue #94 is https://www.w3schools.com/js/tryit.asp?filename=tryjs_prompt

Chrome has the prompt as a full width input which is what I am planning to do. However I'll also make sure to increase the size of the text field as you said

[AlexRobinson-]

@bradleyrichter What are your thoughts on this?

[bradleyrichter]

Perfection! Cc @bclifton

…

On Mar 23, 2018, at 5:50 PM, Alex Robinson ***@***.***> wrote: @bradleyrichter What are your thoughts on this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[bsclifton]

@AlexRobinson- anything else required (is this ready for review)? 😄 Wanted to check as the initial checkboxes in top post haven't been updated

[AlexRobinson-]

Hey @bsclifton

Only thing left to do is add some tests :)

I struggled with them a bit on my first attempt and unfortunately I have been a bit busy lately, so haven't quite had time to finish these up.

Hopefully I'll have some time this weekend to have another go, but I may need some help with the tests.

Am on mobile so having a hard time linking files, but I tried to copy the tests for other functions (window.confirm) that use HTML files to load up and test the whole flow.

Any advice/help with writing the window.prompt tests are appreciated!

[immanuelfodor]

Please somebody help @AlexRobinson- to write those tests to finally merge these changes, the missing window.prompt implementation is making me switch back to Chrome regularly, we really need this functionality in Brave :)

[NejcZdovc]

@AlexRobinson- I would suggest that you extract anonymous function from here https://github.com/brave/browser-laptop/pull/13563/files#diff-70ff57a707bad6ba56cd9cf8373d0e3aR48 into another function. This way you can add uni tests for this function without needing to mock process.on

[AlexRobinson-]

Sorry everyone to hold you all up on this, haven't quite had much time to come back to this.

I have added some unit tests to the MessageBox component to reflect its changes

For the actual flow though I have been trying to copy the tests for the confirm dialog here

https://github.com/brave/browser-laptop/blob/master/test/app/renderer/components/messageBoxTest.js#L249

But your way @NejcZdovc sounds a bit easier

[AlexRobinson-]

Alright, so I moved the onWindowPrompt out into its out function and tested that independently to the rest of the system. Let me know what you all think!

[AlexRobinson-]

@bsclifton do you think this is ok to be merged in now?

[bsclifton]

@AlexRobinson- checking it out now... 😄

[bsclifton]

@AlexRobinson- this is great! I rebased your commits and squashed them down to two (the feature and the tests)

The only thing missing is that the textbox shown in the prompt should be highlighted when it's shown. So in your example, you should be able to click Try it and then type without having to manually focus the textbox. Could you please add this?

I added some manual test steps which you can check out here 😄 (basically what I ran through)
#94

Awesome job with this 😻

[AlexRobinson-]

The only thing missing is that the textbox shown in the prompt should be highlighted when it's shown

Completely fair point! I'll implement this tomorrow night

[AlexRobinson-]

@bsclifton All done! I have updated the PR description with a new gif showing this functionality

[bsclifton]

Works great! Awesome job on this one, @AlexRobinson- 😄 👍

[bsclifton]

@cezaraugusto @petemill would either of you like to give this a review before I merge?

[bsclifton]

Uplifted to 0.23.x with 88befe4