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

Notify users when autoplay has been blocked and provide option to allow it #8751

Merged
merged 1 commit into from
May 9, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 74 additions & 0 deletions app/browser/reducers/autoplayReducer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

'use strict'

const appConstants = require('../../../js/constants/appConstants')
const {makeImmutable} = require('../../common/state/immutableUtil')
const {ipcMain, webContents} = require('electron')
const AppStore = require('../../../js/stores/appStore')
const siteSettings = require('../../../js/state/siteSettings')
const appActions = require('../../../js/actions/appActions')
const {getOrigin} = require('../../../js/state/siteUtil')
const locale = require('../../locale')
const messages = require('../../../js/constants/messages')
const urlParse = require('../../common/urlParse')

const showAutoplayMessageBox = (location, tabId) => {
const origin = getOrigin(location)
const originSettings = siteSettings.getSiteSettingsForURL(AppStore.getState().get('siteSettings'), origin)
if (originSettings && originSettings.get('noAutoplay') === true) {
return
}
const message = locale.translation('allowAutoplay', {origin})

appActions.showNotification({
buttons: [
{text: locale.translation('yes')},
{text: locale.translation('no')}
],
message,
frameOrigin: origin,
options: {
persist: true
}
})

ipcMain.once(messages.NOTIFICATION_RESPONSE, (e, msg, buttonIndex, persist) => {
if (msg === message) {
appActions.hideNotification(message)
let ruleKey = origin
const parsedUrl = urlParse(location)
if ((parsedUrl.protocol === 'https:' || parsedUrl.protocol === 'http:')) {
ruleKey = `https?://${parsedUrl.host}`
}
if (buttonIndex === 0) {
appActions.changeSiteSetting(ruleKey, 'noAutoplay', false)

if (tabId) {
const tab = webContents.fromTabID(tabId)
if (tab && !tab.isDestroyed()) {
return tab.reload()
Copy link
Member

Choose a reason for hiding this comment

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

What is being returned 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.

will do a follow-up along with

return tab.reload()

}
}
} else {
if (persist) {
appActions.changeSiteSetting(ruleKey, 'noAutoplay', true)
}
}
}
})
}

const autoplayReducer = (state, action, immutableAction) => {
action = immutableAction || makeImmutable(action)
switch (action.get('actionType')) {
case appConstants.APP_AUTOPLAY_BLOCKED:
showAutoplayMessageBox(action.get('location'), action.get('tabId'))
break
}
return state
}

module.exports = autoplayReducer
1 change: 1 addition & 0 deletions app/extensions/brave/locales/en-US/app.properties
Original file line number Diff line number Diff line change
Expand Up @@ -260,3 +260,4 @@ copied=Copied!
connectionError=Server connection failed. Please make sure you are connected to the Internet.
unknownError=Oops, something went wrong.
browserActionButton.title={{name}}
allowAutoplay=Allow {{origin}} to autoplay media?
2 changes: 1 addition & 1 deletion app/extensions/brave/locales/en-US/bravery.properties
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,4 @@ httpReroutes={[plural(httpsUpgradeCount)]}
httpReroutes[one]=HTTPS Upgrade
httpReroutes[other]=HTTPS Upgrades
editBraveryGlobalSettings=Edit default shield settings…
allowAutoplay=Allow Autoplay Media
noAutoplay=Block Autoplay
3 changes: 2 additions & 1 deletion app/locale.js
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,8 @@ var rendererIdentifiers = function () {
'closeFirefoxWarningOk',
'importSuccessOk',
'connectionError',
'unknownError'
'unknownError',
'allowAutoplay'
]
}

Expand Down
12 changes: 12 additions & 0 deletions docs/appActions.md
Original file line number Diff line number Diff line change
Expand Up @@ -1036,6 +1036,18 @@ because ESC was pressed.



### autoplayBlocked(location, tabId)

Notifies autoplay has been blocked

**Parameters**

**location**: `string`, Location of current frame

**tabId**: `number`, Tab id of current frame




* * *

Expand Down
9 changes: 5 additions & 4 deletions js/about/preferences.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ const adInsertion = appConfig.resourceNames.AD_INSERTION
const trackingProtection = appConfig.resourceNames.TRACKING_PROTECTION
const httpsEverywhere = appConfig.resourceNames.HTTPS_EVERYWHERE
const safeBrowsing = appConfig.resourceNames.SAFE_BROWSING
const autoplay = appConfig.resourceNames.AUTOPLAY
const noAutoplay = appConfig.resourceNames.NOAUTOPLAY
const noScript = appConfig.resourceNames.NOSCRIPT
const flash = appConfig.resourceNames.FLASH

Expand Down Expand Up @@ -88,7 +88,8 @@ const braveryPermissionNames = {
'safeBrowsing': ['boolean'],
'httpsEverywhere': ['boolean'],
'fingerprintingProtection': ['boolean'],
'noScript': ['boolean', 'number']
'noScript': ['boolean', 'number'],
'noAutoplay': ['boolean']
}

class GeneralTab extends ImmutableComponent {
Expand Down Expand Up @@ -497,7 +498,7 @@ class ShieldsTab extends ImmutableComponent {
this.onChangeAdControl = this.onChangeAdControl.bind(this)
this.onToggleHTTPSE = this.onToggleSetting.bind(this, httpsEverywhere)
this.onToggleSafeBrowsing = this.onToggleSetting.bind(this, safeBrowsing)
this.onToggleAutoplay = this.onToggleSetting.bind(this, autoplay)
this.onToggleNoAutoplay = this.onToggleSetting.bind(this, noAutoplay)
this.onToggleNoScript = this.onToggleSetting.bind(this, noScript)
}
onChangeAdControl (e) {
Expand Down Expand Up @@ -548,7 +549,7 @@ class ShieldsTab extends ImmutableComponent {
<SettingCheckbox checked={this.props.braveryDefaults.get('safeBrowsing')} dataL10nId='safeBrowsing' onChange={this.onToggleSafeBrowsing} />
<SettingCheckbox checked={this.props.braveryDefaults.get('noScript')} dataL10nId='noScriptPref' onChange={this.onToggleNoScript} />
<SettingCheckbox dataL10nId='blockCanvasFingerprinting' prefKey={settings.BLOCK_CANVAS_FINGERPRINTING} settings={this.props.settings} onChangeSetting={this.props.onChangeSetting} />
<SettingCheckbox checked={this.props.braveryDefaults.get('autoplay')} dataL10nId='allowAutoplay' onChange={this.onToggleAutoplay} />
<SettingCheckbox checked={this.props.braveryDefaults.get('noAutoplay')} dataL10nId='noAutoplay' onChange={this.onToggleNoAutoplay} />
<Button l10nId='manageAdblockSettings' className='primaryButton manageAdblockSettings'
onClick={aboutActions.createTabRequested.bind(null, {
url: 'about:adblock'
Expand Down
13 changes: 13 additions & 0 deletions js/actions/appActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -1292,6 +1292,19 @@ const appActions = {
AppDispatcher.dispatch({
actionType: appConstants.APP_DRAG_CANCELLED
})
},

/**
* Notifies autoplay has been blocked
* @param {string} location - Location of current frame
* @param {number} tabId - Tab id of current frame
*/
autoplayBlocked: function (location, tabId) {
AppDispatcher.dispatch({
actionType: appConstants.APP_AUTOPLAY_BLOCKED,
Copy link
Member

Choose a reason for hiding this comment

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

not blocking merging, but why is location needed 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.

Copy link
Member

Choose a reason for hiding this comment

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

I just mean that the browser process already has this info so it is not needed to send from the window renderer. Pls do for a follow up but not high priority.

Copy link
Member

Choose a reason for hiding this comment

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

Basically it just leaves the chance that someone will use the API wrongly at some point, so better not to include it.

Copy link
Member Author

Choose a reason for hiding this comment

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

will do

location,
tabId
})
}
}

Expand Down
7 changes: 4 additions & 3 deletions js/components/braveryPanel.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ class BraveryPanel extends ImmutableComponent {
this.onToggleCookieControl = this.onToggleSiteSetting.bind(this, 'cookieControl')
this.onToggleHTTPSE = this.onToggleSiteSetting.bind(this, 'httpsEverywhere')
this.onToggleFp = this.onToggleSiteSetting.bind(this, 'fingerprintingProtection')
this.onToggleAutoplay = this.onToggleSiteSetting.bind(this, 'autoplay')
this.onToggleNoAutoplay = this.onToggleSiteSetting.bind(this, 'noAutoplay')
this.onReload = this.onReload.bind(this)
this.onEditGlobal = this.onEditGlobal.bind(this)
this.onInfoClick = this.onInfoClick.bind(this)
Expand Down Expand Up @@ -151,6 +151,7 @@ class BraveryPanel extends ImmutableComponent {
if (setting !== 'noScript' && (parsedUrl.protocol === 'https:' || parsedUrl.protocol === 'http:')) {
ruleKey = `https?://${parsedUrl.host}`
}
console.log(e.target.value)
appActions.changeSiteSetting(ruleKey, setting, e.target.value, this.isPrivate)
this.onReload()
}
Expand All @@ -165,7 +166,7 @@ class BraveryPanel extends ImmutableComponent {
const shieldsUp = this.props.braverySettings.shieldsUp
const noScriptEnabled = this.props.braverySettings.noScript
const httpseEnabled = this.props.braverySettings.httpsEverywhere
const autoplayEnabled = this.props.braverySettings.autoplay
const noAutoplayEnabled = this.props.braverySettings.noAutoplay
const adControl = this.props.braverySettings.adControl
const fpEnabled = this.props.braverySettings.fingerprintingProtection
const adsBlockedStat = (this.blockedAds ? this.blockedAds.size : 0) + (this.blockedByTrackingList ? this.blockedByTrackingList.size : 0)
Expand Down Expand Up @@ -299,7 +300,7 @@ class BraveryPanel extends ImmutableComponent {
</FormDropdown>
<SwitchControl onClick={this.onToggleHTTPSE} rightl10nId='httpsEverywhere' checkedOn={httpseEnabled} disabled={!shieldsUp} className='httpsEverywhereSwitch' />
<SwitchControl onClick={this.onToggleNoScript} rightl10nId='noScript' checkedOn={noScriptEnabled} disabled={!shieldsUp} className='noScriptSwitch' />
<SwitchControl onClick={this.onToggleAutoplay} rightl10nId='allowAutoplay' checkedOn={autoplayEnabled} disabled={!shieldsUp} className='allowAutoplay' />
<SwitchControl onClick={this.onToggleNoAutoplay} rightl10nId='noAutoplay' checkedOn={noAutoplayEnabled} disabled={!shieldsUp} className='noAutoplaySwitch' />
</div>
<div className='braveryControlGroup'>
<div className={cx({
Expand Down
3 changes: 3 additions & 0 deletions js/components/frame.js
Original file line number Diff line number Diff line change
Expand Up @@ -528,6 +528,9 @@ class Frame extends React.Component {
if (e.details[0] === 'javascript' && e.details[1]) {
windowActions.setBlockedBy(this.frame, 'noScript', e.details[1])
}
if (e.details[0] === 'autoplay') {
appActions.autoplayBlocked(this.frame.get('location'), this.frame.get('tabId'))
}
})
this.webview.addEventListener('did-block-run-insecure-content', (e) => {
if (this.frame.isEmpty()) {
Expand Down
6 changes: 3 additions & 3 deletions js/constants/appConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ module.exports = {
quitTimeout: 10 * 1000,
resourceNames: {
ADBLOCK: 'adblock',
AUTOPLAY: 'autoplay',
NOAUTOPLAY: 'noAutoplay',
SAFE_BROWSING: 'safeBrowsing',
HTTPS_EVERYWHERE: 'httpsEverywhere',
TRACKING_PROTECTION: 'trackingProtection',
Expand All @@ -36,8 +36,8 @@ module.exports = {
cookieblock: {
enabled: true
},
autoplay: {
enabled: false
noAutoplay: {
enabled: true
Copy link
Member

Choose a reason for hiding this comment

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

Per our discussions, we want to default this to false, right? So that content isn't blocked
cc: @bradleyrichter

Copy link
Member Author

Choose a reason for hiding this comment

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

If the default is false, the notification won't popup in most of the cases because https://github.com/brave/browser-laptop/pull/8751/files#diff-d4f09c262fdb2f86d8f7407afb926c8eR21
The only case it will show up is users toggle global autoplay to block and without site setting of autoplay to the website

The setting is similar to our default fullscreen setting.

Copy link
Member

Choose a reason for hiding this comment

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

I was under the understanding we were defaulting to on with notification bar.

Copy link
Member

Choose a reason for hiding this comment

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

My mistake- I think I missed part of the convo 😄 ++

},
cookieblockAll: {
enabled: false
Expand Down
3 changes: 2 additions & 1 deletion js/constants/appConstants.js
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,8 @@ const appConstants = {
APP_ON_GO_FORWARD: _,
APP_ON_GO_TO_INDEX: _,
APP_ON_GO_BACK_LONG: _,
APP_ON_GO_FORWARD_LONG: _
APP_ON_GO_FORWARD_LONG: _,
APP_AUTOPLAY_BLOCKED: _
}

module.exports = mapValuesByKeys(appConstants)
6 changes: 3 additions & 3 deletions js/state/contentSettings.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ const getDefaultUserPrefContentSettings = (braveryDefaults, appSettings, appConf
braveryDefaults = makeImmutable(braveryDefaults)
return Immutable.fromJS({
autoplay: [{
setting: braveryDefaults.get('autoplay') ? 'allow' : 'block',
setting: braveryDefaults.get('noAutoplay') ? 'block' : 'allow',
primaryPattern: '*'
}],
cookies: getDefault3rdPartyStorageSettings(braveryDefaults, appSettings, appConfig),
Expand Down Expand Up @@ -281,8 +281,8 @@ const siteSettingsToContentSettings = (currentSiteSettings, defaultContentSettin
if (typeof siteSetting.get('widevine') === 'number' && braveryDefaults.get('widevine')) {
contentSettings = addContentSettings(contentSettings, 'plugins', primaryPattern, '*', 'allow', appConfig.widevine.resourceId)
}
if (typeof siteSetting.get('autoplay') === 'boolean') {
contentSettings = addContentSettings(contentSettings, 'autoplay', primaryPattern, '*', siteSetting.get('autoplay') ? 'allow' : 'block')
if (typeof siteSetting.get('noAutoplay') === 'boolean') {
contentSettings = addContentSettings(contentSettings, 'autoplay', primaryPattern, '*', siteSetting.get('noAutoplay') ? 'block' : 'allow')
}
})
// On the second pass we consider only shieldsUp === false settings since we want those to take precedence.
Expand Down
1 change: 1 addition & 0 deletions js/stores/appStore.js
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,7 @@ const handleAppAction = (action) => {
reducers = [
require('../../app/browser/reducers/downloadsReducer'),
require('../../app/browser/reducers/flashReducer'),
require('../../app/browser/reducers/autoplayReducer'),
// tabs, sites and windows reducers need to stay in that order
// until we have a better way to manage dependencies.
// tabsReducer must be above dragDropReducer.
Expand Down
78 changes: 75 additions & 3 deletions test/bravery-components/braveryPanelTest.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/* global describe, it, beforeEach */

const Brave = require('../lib/brave')
const {cookieControl, allowAllCookiesOption, blockAllCookiesOption, urlInput, braveMenu, braveMenuDisabled, adsBlockedStat, adsBlockedControl, showAdsOption, blockAdsOption, braveryPanel, httpsEverywhereStat, noScriptStat, noScriptSwitch, fpSwitch, autoplaySwitch, fpStat, noScriptNavButton, customFiltersInput} = require('../lib/selectors')
const {cookieControl, allowAllCookiesOption, blockAllCookiesOption, urlInput, braveMenu, braveMenuDisabled, adsBlockedStat, adsBlockedControl, showAdsOption, blockAdsOption, braveryPanel, httpsEverywhereStat, noScriptStat, noScriptSwitch, fpSwitch, noAutoplaySwitch, fpStat, noScriptNavButton, customFiltersInput, notificationBar, reloadButton} = require('../lib/selectors')
const {getTargetAboutUrl} = require('../../js/lib/appUrlUtil')

describe('Bravery Panel', function () {
Expand Down Expand Up @@ -518,6 +518,11 @@ describe('Bravery Panel', function () {
return status === ''
})
})
.windowByUrl(Brave.browserWindowUrl)
.waitForExist(notificationBar)
.waitUntil(function () {
return this.getText(notificationBar).then((val) => val.includes('autoplay media'))
})
})

it('allow autoplay in regular tab', function * () {
Expand All @@ -531,8 +536,13 @@ describe('Bravery Panel', function () {
return status === ''
})
})
.windowByUrl(Brave.browserWindowUrl)
.waitForExist(notificationBar)
.waitUntil(function () {
return this.getText(notificationBar).then((val) => val.includes('autoplay media'))
})
.openBraveMenu(braveMenu, braveryPanel)
.click(autoplaySwitch)
.click(noAutoplaySwitch)
.keys(Brave.keys.ESCAPE)
.tabByUrl(url)
.waitUntil(function () {
Expand All @@ -554,8 +564,13 @@ describe('Bravery Panel', function () {
return status === ''
})
})
.windowByUrl(Brave.browserWindowUrl)
.waitForExist(notificationBar)
.waitUntil(function () {
return this.getText(notificationBar).then((val) => val.includes('autoplay media'))
})
.openBraveMenu(braveMenu, braveryPanel)
.click(autoplaySwitch)
.click(noAutoplaySwitch)
.keys(Brave.keys.ESCAPE)
.tabByUrl(url)
.waitUntil(function () {
Expand All @@ -565,5 +580,62 @@ describe('Bravery Panel', function () {
})
})
})

it('allow autoplay in notification bar', function * () {
const url = Brave.server.url('autoplay.html')
yield this.app.client
.tabByIndex(0)
.loadUrl(url)
.waitUntil(function () {
return this.getText('div[id="status"]')
.then((status) => {
return status === ''
})
})
.windowByUrl(Brave.browserWindowUrl)
.waitForExist(notificationBar)
.waitUntil(function () {
return this.getText(notificationBar).then((val) => val.includes('autoplay media'))
})
.click('button=Yes')
.tabByUrl(url)
.waitUntil(function () {
return this.getText('div[id="status"]')
.then((status) => {
return status === 'Autoplay playing'
})
})
})

it('Remember block autoplay in notification bar', function * () {
const url = Brave.server.url('autoplay.html')
yield this.app.client
.tabByIndex(0)
.loadUrl(url)
.waitUntil(function () {
return this.getText('div[id="status"]')
.then((status) => {
return status === ''
})
})
.windowByUrl(Brave.browserWindowUrl)
.waitForExist(notificationBar)
.waitUntil(function () {
return this.getText(notificationBar).then((val) => val.includes('autoplay media'))
})
.click('[data-l10n-id=rememberDecision]')
.click('button=No')
.windowByUrl(Brave.browserWindowUrl)
.click(reloadButton)
.tabByUrl(url)
.waitUntil(function () {
return this.getText('div[id="status"]')
.then((status) => {
return status === ''
})
})
.windowByUrl(Brave.browserWindowUrl)
.waitForElementCount('.notificationItem', 0)
})
})
})
2 changes: 1 addition & 1 deletion test/lib/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ module.exports = {
noScriptAllowOnceButton: '[data-l10n-id="allowScriptsOnce"]',
noScriptAllowButton: '[data-l10n-id="allowScripts"]',
safeBrowsingSwitch: '.safeBrowsingSwitch .switchMiddle',
autoplaySwitch: '.allowAutoplay .switchMiddle',
noAutoplaySwitch: '.noAutoplaySwitch .switchMiddle',
backButton: '.backforward .backButton',
forwardButton: '.backforward .forwardButton',
reloadButton: '.reloadButton',
Expand Down