Skip to content
This repository has been archived by the owner. It is now read-only.

/ browser-laptop Archived

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

Merged
merged 1 commit into from May 9, 2017
Merged

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

merged 1 commit into from May 9, 2017
+200 −20

Conversation

@darkdh
Copy link
Member

darkdh commented May 8, 2017 •
edited

Display "Block autoplay" in bravey shield

Fix #8738
Address #8739

Auditors: @bbondy, @bsclifton, @jonathansampson

Test Plan:
Covered by automatic test

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

screen shot 2017-05-07 at 9 57 12 pm
@darkdh darkdh added this to the 0.15.3 milestone May 8, 2017
@darkdh darkdh self-assigned this May 8, 2017
@darkdh darkdh requested review from jonathansampson, bbondy and bsclifton May 8, 2017
@darkdh darkdh force-pushed the 8738 branch from e036c49 to e9158b5 May 8, 2017
@jonathansampson
jonathansampson suggested changes May 8, 2017
View changes
Copy link
Collaborator

jonathansampson left a comment •
edited

Everything looks really good, one question I have though is regarding the wording used in the notification:

"Media autoplay on has been blocked. Do you want to allow it?"

Contrast that with requests for full screen, location, or media capture:

"Allow to use fullscreen mode?"
"Allow to see your location?"
"Allow to use your camera and/or mic?"

I think we should adopt the same pattern:

"Allow to autoplay media?"
@darkdh
Copy link
Member Author

darkdh commented May 8, 2017

fixed in b8fc59c
@jonathansampson
jonathansampson approved these changes May 8, 2017
View changes
@cezaraugusto
cezaraugusto approved these changes May 8, 2017
View changes
Copy link
Contributor

cezaraugusto left a comment

Testing on YT, video kept loading until I decided what to do with notification. Deciding to disable autoplay, loading persists and pause button is shown rather than play button. Seems like video is not fully loaded and I had to click twice on the pause button to get it working. I felt confused by that.

Regarding UX the best scenario I can think of is having video stopped (play indicator visible) with video placeholder showing instead of forever loading until user decide what to do with notification.

Those are just my thoughts and my guess is that the above is beyond the scope of this PR and it's a great thing to have as-is, so LGTM
@luixxiul luixxiul mentioned this pull request May 9, 2017
Closed
3 of 4 tasks complete
@luixxiul
Copy link
Contributor

luixxiul commented May 9, 2017

@darkdh I changed #8739 into a tracking issue, so would you mind removing Fix #8739 from your commit message? thanks!
@darkdh
Notify users when autoplay has been blocked and provide option to all…
Verified
This commit was signed with a verified signature.
darkdh Anthony Tseng
GPG key ID: E14094A20D3AAEEC Learn about signing commits
Loading status checks…
490f31e 
…ow it

Display "Block autoplay" in bravey shield

Fix #8738

Auditors: @bbondy, @bsclifton, @jonathansampson

Test Plan:
Covered by automatic test
@darkdh darkdh force-pushed the 8738 branch from b8fc59c to 490f31e May 9, 2017
@darkdh
Copy link
Member Author

darkdh commented May 9, 2017

ok, done
@bbondy
bbondy reviewed May 9, 2017
View changes
js/actions/appActions.js
*/
autoplayBlocked: function (location, tabId) {
AppDispatcher.dispatch({
actionType: appConstants.APP_AUTOPLAY_BLOCKED,

This comment has been minimized.

Sign in to view
Copy link
@bbondy

bbondy May 9, 2017

Member

not blocking merging, but why is location needed here?

This comment has been minimized.

Sign in to view
Copy link
@darkdh

darkdh May 9, 2017

Author Member

It is for displaying origin for message box
https://github.com/brave/browser-laptop/pull/8751/files#diff-d4f09c262fdb2f86d8f7407afb926c8eR19

This comment has been minimized.

Sign in to view
Copy link
@bbondy

bbondy May 9, 2017

Member

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.

This comment has been minimized.

Sign in to view
Copy link
@bbondy

bbondy May 9, 2017

Member

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

This comment has been minimized.

Sign in to view
Copy link
@darkdh

darkdh May 9, 2017

Author Member

will do
@bbondy
Copy link
Member

bbondy commented May 9, 2017

++
@bbondy bbondy merged commit a3dd46f into master May 9, 2017
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@srirambv srirambv mentioned this pull request May 9, 2017
Closed
@bsclifton
bsclifton reviewed May 9, 2017
View changes
Copy link
Member

bsclifton left a comment

Comments left; changes look good 😄
app/browser/reducers/autoplayReducer.js
if (tabId) {
const tab = webContents.fromTabID(tabId)
if (tab && !tab.isDestroyed()) {
return tab.reload()

This comment has been minimized.

Sign in to view
Copy link
@bsclifton

bsclifton May 9, 2017

Member

What is being returned here?

This comment has been minimized.

Sign in to view
Copy link
@darkdh

darkdh May 9, 2017

Author Member

will do a follow-up along with

browser-laptop/js/flash.js

Line 83 in b2618c9

return tab.reload()

js/constants/appConfig.js
autoplay: {
enabled: false
noAutoplay: {
enabled: true

This comment has been minimized.

Sign in to view
Copy link
@bsclifton

bsclifton May 9, 2017

Member

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

This comment has been minimized.

Sign in to view
Copy link
@darkdh

darkdh May 9, 2017

Author Member

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.

This comment has been minimized.

Sign in to view
Copy link
@bbondy

bbondy May 9, 2017

Member

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

This comment has been minimized.

Sign in to view
Copy link
@bsclifton

bsclifton May 9, 2017

Member

My mistake- I think I missed part of the convo 😄 ++
@bsclifton bsclifton deleted the 8738 branch May 9, 2017
darkdh added a commit that referenced this pull request May 9, 2017
@darkdh
Follow-up of #8751
Verified
This commit was signed with a verified signature.
darkdh Anthony Tseng
GPG key ID: E14094A20D3AAEEC Learn about signing commits
Loading status checks…
e0dccf5 
Auditors: @bbondy, @bsclifton
bsclifton added a commit that referenced this pull request May 10, 2017
@darkdh @bsclifton
Follow-up of #8751
Unverified
This user has not uploaded their public key yet.
GPG key ID: 37FF5F19022427F2 Learn about signing commits
Loading status checks…
cf6cd7c 
Auditors: @bbondy, @bsclifton
bsclifton added a commit that referenced this pull request May 10, 2017
@darkdh @bsclifton
Follow-up of #8751
Unverified
This user has not uploaded their public key yet.
GPG key ID: 37FF5F19022427F2 Learn about signing commits
ff7cc2b 
Auditors: @bbondy, @bsclifton
kspearrin added a commit to kspearrin/browser-laptop that referenced this pull request May 11, 2017
@darkdh @kspearrin
Follow-up of brave#8751
bafb646 
Auditors: @bbondy, @bsclifton
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bbondy bbondy

@bsclifton bsclifton

@jonathansampson jonathansampson

@cezaraugusto cezaraugusto

Assignees

@darkdh darkdh

Labels
feature/autoplay
Projects
None yet
Milestone
  0.15.300 (Frozen,...
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@darkdh @luixxiul @bbondy @jonathansampson @cezaraugusto @bsclifton
You can’t perform that action at this time.