Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Rework alert/confirm to be tab-modal (instead of application-modal) #7107

Merged
merged 6 commits into from
Feb 28, 2017
Merged

Rework alert/confirm to be tab-modal (instead of application-modal) #7107

merged 6 commits into from
Feb 28, 2017

Conversation

bsclifton
Copy link
Member

@bsclifton bsclifton commented Feb 6, 2017

Test Plan

Testing alert

  1. Visit http://jsbin.com/fiyojusahu/edit?html,output
  2. In the output area, click the Click me to test an alert button
  3. Confirm the alert works as expected

Testing confirm

  1. Visit http://jsbin.com/sadunogefu/edit?html,output
  2. In the output area, click the Click me to test a confirm button
  3. Confirm the confirm works as expected (try clicking OK and also cancel)

Testing import

  1. Have a bookmarks file (.html) or other browser history which can be imported
  2. Launch Brave and go to Preferences > General
  3. Click import, choose appropriate settings (based on what you have in step 1)
  4. Click OK to start import process
  5. When done, you should see a tab-specific modal letting you know it's complete

Testing import (Firefox)

  1. Have Firefox installed and open
  2. Launch Brave and go to Preferences > General
  3. Click import, choose Firefox
  4. Click OK to start import process
  5. You should see warning alert

Issue specific tests

#3794 - Hard to exit/close Brave when site spams you with message box / alerts
#2755 - prevent js alert spoofing attacks
#6901 - alert() popups should appear below tabs
#7213 - Brave crashed on macOS 10.12.3 via.window.open()
#7280 - Invalid sync code alert needs proper heading

Automated tests

# unit tests
npm run unittest -- --grep="MessageBox component unit tests"
npm run unittest -- --grep="tabMessageBoxState unit tests"
npm run unittest -- --grep="tabMessageBox unit tests"
npm run unittest -- --grep="tabState unit tests"
npm run unittest -- --grep="Main component unit tests"
npm run unittest -- --grep="NavigationBar component unit tests"
npm run unittest -- --grep="UrlBar component unit tests"
npm run unittest -- --grep="UrlBarIcon component unit tests"

# webdriver tests
npm run uitest -- --grep="MessageBox component tests"

Description

  • 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).

Rework alert/confirm to be tab-modal (instead of application-modal)

Message box:

Other changes include:

  • Removed "An embedded page at " from source message (shown on every message)
  • Rename showMessageBox/hideMessageBox/clearMessageBoxes actions to showNotification/hideNotification/clearNotification
  • Converted most styles to Aphrodite

Auditors: @bridiver, @diracdeltas
Folks who may also be interested: @bbondy, @darkdh

Special note

Electron recently received a PR to implement an optional checkbox in electron.dialog.showMessageBox... but this is a better choice for us because it's tab-modal (you can switch tabs while the alert is up, etc).

Screenshots

screen shot 2017-02-06 at 10 32 02 am

screen shot 2017-02-06 at 10 30 04 am

screen shot 2017-02-06 at 10 27 50 am

screen shot 2017-02-06 at 11 48 20 am

cc: @ayumi

@luixxiul
Copy link
Contributor

luixxiul commented Feb 6, 2017

Nice work @bsclifton ! Does this PR fix the spoofing dialog like #4992? (I know it's labelled duplicated, I just wanted to make it clear)


close: (state, action) => {
state = makeImmutable(state)
action = makeImmutable(action)
Copy link
Member

@diracdeltas diracdeltas Feb 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[minor] setResponse calls makeImmutable on the args, so it's not necessary here

onClick={this.onDismiss.bind(this, this.tabId, index)} />)
}

return buttons
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[minor] could return this.buttons.map(...) instead of iterating through this.buttons

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did originally implement it like that- but the buttons have to be shown in reverse order (in order to keep the same "interface" as the electron dialog.showMessageBox).

ex: buttons would be ['OK', 'Cancel'] with cancel being index 1 (and the caller specifies cancelId === 1)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think you can do maxIndex - index to get the index reversed

@bsclifton
Copy link
Member Author

bsclifton commented Feb 6, 2017

@luixxiul yes- it does. By removing the logo and also showing the origin, I believe the concern raised in #4992 should be fixed. @diracdeltas can also confirm/deny 😄

const tabState = require('./tabState')
const {makeImmutable} = require('./immutableUtil')

const messageBoxDetail = 'messageBoxDetail'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this need to be added to docs/state.md? does it need to be cleaned from tabs on shutdown?

app/index.js Outdated
showSuppress: shouldDisplaySuppressCheckbox
}

MessageBox.show(tabId, detail, cb)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[minor] the enter key should be bound to OK

@diracdeltas
Copy link
Member

the test plan worked for me. 🎆

however, now that the modal is below the line-of-trust (AKA fully in the webview area), it can be fully spoofed by the page. this is not the case in Chrome/etc. because the modal is its own window, which can be moved outside the webview area; in our case it is immovable.

i see two potential downsides:

  1. this can be abused by the page to make it appear that Brave keeps showing dialogs even though the user has checked the 'suppress' checkbox.
  2. if the alert box is covering something that the user wants to read, they have to close the alert to read it.

@bridiver
Copy link
Collaborator

bridiver commented Feb 7, 2017

accidentally made the comments in your repo @bsclifton, but looks like they still appear above

@bsclifton
Copy link
Member Author

Thanks for the feedback, folks! I'm working on tests and am/will be addressing all of the above items 😄

},
'flyoutDialog': {
backgroundColor: `${globalStyles.color.toolbarBackground}`,
borderRadius: `${globalStyles.radius.borderRadius}`,
Copy link
Contributor

@luixxiul luixxiul Feb 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the blackets here? I think this should work: backgroundColor: globalStyles.color.toolbarBackground, and borderRadius: globalStyles.radius.borderRadius,. cf: https://github.com/brave/browser-laptop/pull/7107/files#diff-fa59d1e0221a5bee94fd2abd9b96dcfdR12

padding: '10px 30px',
position: 'absolute',
textAlign: 'left',
top: `${globalStyles.spacing.dialogTopOffset}`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

marginBottom: '1.5em'
},
'buttons': {
float: 'right',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works enough but I would not set with float to avoid the possibility of the case like this: #6749 (comment). I would instead use display: flex and justify-content: flex-end

@@ -19,6 +19,17 @@ const styles = StyleSheet.create({
outline: 'none',
padding: '0.4em',
width: '100%'
},
'flyoutDialog': {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to create a tracking issue to delete

.flyoutDialog {
background-color: @toolbarBackground;
border-radius: @borderRadius;
box-shadow: 2px 2px 8px #3b3b3b;
color: #000;
font-size: 13px;
padding: 10px 30px;
position: absolute;
text-align: left;
top: @dialogTopOffset;
}
in favor of these lines :-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

posted here: #7114

@bridiver
Copy link
Collaborator

bridiver commented Feb 7, 2017

@diracdeltas I'm not sure if we need to address those issues right now. A page can spoof an alert dialog and even if it doesn't resemble what Brave does, a lot of users don't know the difference anyway. The big issue I think is preventing people from navigating away when that happens and this fixes that issue. For the second issue, the old dialog could potentially cover content as well. Maybe file separate issues for them as improvements?

@bsclifton
Copy link
Member Author

bsclifton commented Feb 7, 2017

Round 1 of fixes done w/ 5f2d239

Coming up next:

  • binding enter/escape keys
  • webdriver tests enforcing the behaviors called out above in feedback

@luixxiul regarding the float: right comment, did you want to push a fix? (my flexbox skills are not the best) I'd love to have a proper style update (and you're welcome to commit one 😄 )

@bsclifton
Copy link
Member Author

Also one open I had for you all ( @bridiver, @diracdeltas ):

While writing my tests, I noticed that (because the UI isn't blocked), you can type in the URL bar (including hitting enter) when an alert is up. Nothing happens of course... at least, until you close the alert/confirm.

How should we address this behavior? This could be a chance to address the feedback about pages being able to fake the alert. We can update our alert to disable/gray out the URL bar or any other action we'd like. I do think it's worth keeping the ability to switch tabs though

cc: @bradleyrichter

@luixxiul
Copy link
Contributor

luixxiul commented Feb 7, 2017

regarding the float: right comment, did you want to push a fix?

I will do with a follow-up task!

@bradleyrichter
Copy link
Contributor

@bsclifton It would be better to make the URL bar non-editable and perhaps disabled in appearance for this case if you can not actually execute a new search or page change in this state.

@diracdeltas
Copy link
Member

Maybe file separate issues for them as improvements?

sgtm

@bsclifton
Copy link
Member Author

Ready for re-review! cc: @bridiver, @diracdeltas, @bbondy

This PR is mostly tests- it adds 93 tests (81 unit tests, 12 webdriver). Be sure to check all the scenarios tested... they should be covered 😄 If you want to run the tests, you'll find the npm run commands in the top post here ^^

I've also manually tested each scenario. Should be good to go 😄

const detail = {
message,
title,
buttons: ['OK'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add translated string here

const detail = {
message,
title,
buttons: ['OK', 'Cancel'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add translated string here

}

get buttons () {
return (this.props.detail && this.props.detail.get('buttons')) || makeImmutable(['OK'])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add translated string here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch! It already is being translated (these strings get passed to l10n-Id in render)... however, it's expected (per the app.properties file) to be lowercase. I fixed and verified it works by changing the string to nonsense 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I notice that button component has I10n-Id, but when I changed to the another language there was still the same string.

docs/state.md Outdated
canGoForward: boolean, // the tab can be navigated forward
muted: boolean, // is the tab muted
windowId: number, // the windowId that contains the tab
messageBoxDetail: { // fields used if showing a message box for a tab
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please order these and child properties alphabetically

process.on('window-prompt', (webContents, extraData, title, message, defaultPromptText,
shouldDisplaySuppressCheckbox, isBeforeUnloadDialog, isReload, cb) => {
console.warn('window.prompt is not supported yet')
let suppress = false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use const

const tabId = action.getIn(['tabValue', 'tabId'])
if (tabId) {
// remove callback; call w/ defaults
let cb = messageBoxCallbacks[tabId]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use const

// remove detail from state
state = tabMessageBoxState.removeDetail(state, removeAction)
// remove callback; call w/ defaults
let cb = messageBoxCallbacks[tabId]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use const

class MessageBox extends ImmutableComponent {
constructor () {
super()
this.onClick = this.onClick.bind(this)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is not necessary, because you are just stopping propagation in this function

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call on this- I copied from another modal and it was unnecessary. It may have originally been put there to prevent the modal from closing on click... but that doesn't happen here 👍

}

render () {
return <Dialog className='noOutline'>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use Aphrodite here as well

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also good catch- I wasn't sure about it at first, because Dialog is not an Aphrodite component... but adding a className using Aphrodite works just fine 👍

@@ -16,6 +16,10 @@
align-items: center;
background: rgba(0, 0, 0, 0.15);

&.noOutline {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use Aphrodite

/>
)
const instance = wrapper.instance()
assert.equal(instance.title, 'Brave says:')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++

@diracdeltas
Copy link
Member

this seems to make the notificationBar login form tests fail: https://travis-ci.org/brave/browser-laptop/builds/205211560#L5533.

they pass on master with npm run test -- --grep='notificationBar' but the same command also fails locally in this branch

}

const styles = StyleSheet.create({
'container': {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to use quotes on keys for Aphrodite

},
'title': {
fontWeight: 'bold',
fontSize: '12pt',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the reason of using pt instead of px?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question- I was thinking it would be nice to use official point sizes and then have everything else relative using em. @NejcZdovc suggested using rem for font size. We should work together and propose a standard cc: @luixxiul

fontSize: '12pt',
marginBottom: '1em',
marginTop: '0.5em',
'-webkit-user-select': 'text'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be better using WebkitUserSelect instead of stringifying

Copy link
Member Author

@bsclifton bsclifton Feb 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cezaraugusto does this work even for proprietary extensions? (vendor prefix; starting with a -)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, you can double-check looking at devTools for resulted code. I guess Aphrodite uses React built-in styles vendor mechanism. For reference:

https://facebook.github.io/react/docs/dom-elements.html#style

marginTop: '1.5em',
minWidth: '425px',
marginBottom: '1.5em',
'-webkit-user-select': 'text'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

onDragStart={this.onDragStart}
draggable
onClick={this.onClick}
{...props}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++ spread operator ftw

docs/state.md Outdated
@@ -279,16 +279,16 @@ AppStore
audible: boolean, // is audio playing (muted or not)
canGoBack: boolean, // the tab can be navigated back
canGoForward: boolean, // the tab can be navigated forward
muted: boolean, // is the tab muted
windowId: number, // the windowId that contains the tab
messageBoxDetail: { // fields used if showing a message box for a tab
message: string,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These properties should be alphabetize as well

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a comment showing some which are session and some which are persistent. We can alphabetize but then remove that label, but I don't see value with that

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tabs: [{
    // persistent properties
    active: boolean,  // whether the tab is selected
    favIconUrl: string,
    id: number,
    index: number,  // the position of the tab in the window
    title: string,
    url: string,
    windowUUID: string,  // the permanent identifier for the window
    // session properties
    audible: boolean, // is audio playing (muted or not)
    canGoBack: boolean, // the tab can be navigated back
    canGoForward: boolean, // the tab can be navigated forward
    messageBoxDetail: { // fields used if showing a message box for a tab
      message: string,
      title: string, // title is the source; ex: "brave.com says:"
      buttons: [string], // array of buttons as string; code only handles 1 or 2
      suppress: boolean, // if true, show a suppress checkbox (defaulted to not checked)
      showSuppress: boolean, // final result of the suppress checkbox
      cancelId: number // optional: used for a confirm message box
    },
    muted: boolean, // is the tab muted
    windowId: number // the windowId that contains the tab
  }],

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am talking about these properties:

messageBoxDetail: { // fields used if showing a message box for a tab
      message: string,
      title: string, // title is the source; ex: "brave.com says:"
      buttons: [string], // array of buttons as string; code only handles 1 or 2
      suppress: boolean, // if true, show a suppress checkbox (defaulted to not checked)
      showSuppress: boolean, // final result of the suppress checkbox
      cancelId: number // optional: used for a confirm message box
    }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doh my bad- fixed 😄

Message box:
- shows an on/off switch for "Prevent this page from creating additional dialogs". Fixes #3794
- does not include Brave logo; shows origin; does not need to show "switch to tab" because it's already bound to tab. Fixes #2755
- is shown BELOW tabs now. Fixes #6901
- Fixes #7213

Other changes include:
- Removed "An embedded page at " from source message (shown on every message)
- Rename showMessageBox/hideMessageBox/clearMessageBoxes actions to showNotification/hideNotification/clearNotification
- Converted most styles to Aphrodite

When active tab has an alert:
- URL bar puts itself in title mode
- disable / gray out lion icon
- back/forward buttons are disabled
- URL bar icon (lock, etc) is no longer clickable/draggable

Includes first round of fixes (post-feedback)
- renamed actions
- unit tests for messageBoxState
- component tests for MessageBox
- update to state.md
- MessageBox control responds properly to keyboard (escape / enter)
- webdriver tests for alert/confirm

TODO / WIP:
- call callback if navigating away (click link, close tab, etc)
- tests which ensure callback is cleaned up on crash/close of tab
- Added webdriver tests for opening new tab / navigating when alert is open
- created tests for the `app/browser/messageBox` code
- created new tabMessageBoxReducer
- renamed msgbox actions to be declarative
- updated onDestroy/onCrashed to call callback and remove callback
- added test to ensure on() handler is registered for webcontents
- Moved `process.on` handlers for message box into the new reducer (`window-alert`, `window-confirm`, `window-prompt`)

Includes some renaming to make things consistent

Auditors: @bbondy, @bridiver
- Main.js component now properly disables shields button and hides extensions when active tab is showing an alert/confirm.
- Message box for tab is now cleared (callback called + removed) when tab showing alert navigates
- Removed destroy/crash handlers for webcontents in favor of APP_TAB_CLOSED action
- Removed session test; replaced with a proper tab-specific session test
- "ok"/"cancel" are now localized
- tests for notificationBar work great now (miss on my part after rebase)
- aphrodite the dialog / drop the less change
- const over let
- no quotes on keys
- alphabetize state.md

Auditors: @NejcZdovc, @diracdeltas, @cezaraugusto
@NejcZdovc
Copy link
Contributor

NejcZdovc commented Feb 28, 2017

We should change this two as well:

Auditors: @darkdh, @NejcZdovc

I was easily able to test the happy path (showImportSuccess), but I am
not sure how to test the other (showImportWarning).

Also (per the comments in place), I don't anticipate any issues with
the menu, but would like your input @darkdh :)
@bsclifton
Copy link
Member Author

@NejcZdovc yes- great catch 😄 I went ahead and updated the import code to also use this alert.

@bbondy
Copy link
Member

bbondy commented Feb 28, 2017

I assume there will be some follow ups but this is looking very good, great work on this. I'll merge now.

@bbondy bbondy merged commit 67b7687 into brave:master Feb 28, 2017
@bsclifton bsclifton deleted the prevent-alert-spam branch February 28, 2017 15:56
bridiver pushed a commit that referenced this pull request Apr 6, 2017
Closes #7949

Also modify messageBox.js, which was introduced with #7107

- Introduced globalStyles.spacing.dialogInsideMargin
  As the marginTop of title and marginBottom of buttons on messageBox.js had been set to 0.5em, I removed and added them to padding of flyoutDialog. The vaule was calculated this way: calc(10px + 0.5rem) = 18px. Also I applied it to the elements inside messageBox.js

- Aligned the buttons to the right (same as messageBox.js)

- Introduced siteInfo__wrapperLarge
  As the buttons on mixed content info dialog are so huge that sometimes they are wrapped, epecially on foreign language like Japanese. This is a temporary workaround to avoid it.

- Moved 'denyRunInsecureContent' button to the right to improve UX

- Updated automated test code

Auditors:

Test Plan:
1. Open https://apple.com
2. Click the lock icon on the URL bar
3. Make sure the title and description are aligned
4. Make sure the button is aligned to the right
5. Open http://apple.com
6. Click the unlock icon on the URL bar
7. Make sure the title and description are aligned
8. Open https://mixed-script.badssl.com
9. Click the lock icon
10. Make sure the buttons are aligned to the right and they are not wrapped
11. Click "Load Unsafe Script"
12. Click the unlock icon on the URL bar
13. Click "Stop Loading Unsafe Script"
14. Visit http://jsbin.com/fiyojusahu/edit?html,output
15. Make sure the elements are aligned well
16. Visit http://jsbin.com/sadunogefu/edit?html,output
17. Make sure the switch next to "Prevent this page from creating additional dialogs" is aligned with the body text
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
8 participants