New issue
Advanced search Search tips

Issue 860718 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 16
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 802294
issue 860509



Sign in to add a comment

Picture-in-picture tests failing with User Activation v2

Project Member Reported by mustaq@chromium.org, Jul 6

Issue description

virtual/picture-in-picture/external/wpt/picture-in-picture/request-picture-in-picture.html

Looks like the requester code doesn't see a user activation.  Haven't looked into it, so not sure why.

 
Hmm, looking into the test setup, looks like we don't have a virtual/picture-in-picture suite, but instead there is a virtual/video-surface-layer test suite:
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/VirtualTestSuites?rcl=bd86e09382181645e7107433dff0f7da9c01d834&l=652

What am I missing here?
I recently removed the virtual/picture-in-picture/ in favour of virtual/video-surface-layer/.
Is this still an issue mustaq?
Yes.  I last checked on July 17, we have the same failure in virtual/video-surface-layer/... instead.

I just ran virtual/video-surface-layer/external/wpt/picture-in-picture/ with UserActivationV2 enabled by default and it worked fine (10 tests passed). Is this how the issue should be reproduced?
My July 17 run on linux_chromium_rel_ng was placed in "Didn't find text mismatch" bucket, but just noticed that there was no "crash log" either!



I did another run on Friday with the feature flag on (instead of the Blink one) and I couldn't get a test to fail :(
I couldn't repro either!

I will kick off another bot run, to see if it got fixed.
mustaq@ Has this been fixed?
No, this is still failing on linux_chromium_rel_ng.  Last tried 2 days ago:
https://ci.chromium.org/p/chromium/builders/luci.chromium.try/linux_chromium_rel_ng/192622
Cc: fbeaufort@chromium.org
For info, by having only this one test in a test file, wpt test passes:

promise_test(async t => {
  const video = await loadVideo();
  return promise_rejects(t, 'NotAllowedError', video.requestPictureInPicture());
}, 'request Picture-in-Picture requires a user gesture');

So there must be something else going on but I'm not sure what yet.
Yes, thanks, it makes sense now: it's UAv2's scope change that's causing the failure:
  https://mustaqahmed.github.io/user-activation-v2/

With UAv2, the click activation is carried on to the following tests since the state is no longer stack-scoped.


Cc: -fbeaufort@chromium.org mustaq@chromium.org
Owner: fbeaufort@chromium.org
I've uploaded WIP CL at https://chromium-review.googlesource.com/c/chromium/src/+/1244239 to fix this.
Project Member

Comment 14 by bugdroid1@chromium.org, Sep 25

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2353ff72a2efea58b334f7825551cad676673a44

commit 2353ff72a2efea58b334f7825551cad676673a44
Author: François Beaufort <beaufort.francois@gmail.com>
Date: Tue Sep 25 22:42:25 2018

Fix Picture-in-Picture test with User Activation V2

With User Activation V2, the click activation is carried on to the
following tests since the state is no longer stack-scoped. By
re-ordering tests, this CL makes sure test doesn't fail when User
Activation V2 is enabled.

Bug:  860718 
Change-Id: I54ebc6ca2d808323e4b87997b9687c56e2c8606d
Reviewed-on: https://chromium-review.googlesource.com/1244239
Commit-Queue: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594133}
[modify] https://crrev.com/2353ff72a2efea58b334f7825551cad676673a44/third_party/WebKit/LayoutTests/external/wpt/picture-in-picture/request-picture-in-picture.html

mustaq@ can you confirm that it is fixed?
* virtual/video-surface-layer/external/wpt/picture-in-picture/request-picture-in-picture.html is not failing anymore in https://ci.chromium.org/p/chromium/builders/luci.chromium.try/linux_chromium_rel_ng/194930 while it was in https://ci.chromium.org/p/chromium/builders/luci.chromium.try/linux_chromium_rel_ng/192622

However I still see some other PiP browser_tests failing: 
PictureInPictureLazyBackgroundPageApiTest.PictureInPictureInBackgroundPage
BrowserActionApiTest.TestPictureInPictureOnBrowserActionIconClick
Summary: Picture-in-picture tests failing with User Activation v2 (was: One picture-in-picture layout test failing with User Activation v2)
As fbeaufort@ confirmed, the layout test is now fixed.

The two browser tests started failing with UAv2 from sometime in August.  I will look into them in a few weeks (working on other failures now).
PictureInPictureLazyBackgroundPageApiTest.PictureInPictureInBackgroundPage

[31962:31962:0925/135230.506296:INFO:CONSOLE(39)] "Must be handling a user gesture to request picture in picture.", source: chrome-extension://mlmhpohlbpmlockpfeeimmckpamlamhl/background.js (39)

I believe this error means "BrowserActionTestUtil::Create(browser())->Press(0);" doesn't act as a user gesture anymore with UAv2.


BrowserActionApiTest.TestPictureInPictureOnBrowserActionIconClick

 ../../chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:1154: Failure
 Value of: catcher.GetNextResult()
 Actual: false
 Expected: true

This suggest the same error happens as "GetBrowserActionsBar()->Press(0);" seems to not enable user gesture as well.
Cc: -mustaq@chromium.org fbeaufort@chromium.org
Owner: mustaq@chromium.org
fbeaufort@chromium.org: Started working on this, then got confused with BrowserActionBar clicks: how is a click on browser UI element expected to activate a page?  IIUC, a button on BrowserActionBar is not associated with a specific tab, and in particular, users have no way to see this.  What am I missing here?

This is the Javascript code executed in extension background.js file: 

chrome.browserAction.onClicked.addListener(function(tab) {
    enterPictureInPicture();
});

It seems intuitive to me to think that user activation should work in that case.
The problem is that the (simulated) user action was not with the renderer associated with background.js.  Not sure why we want activation here.  Perhaps the use-case being tested here would help.  Let's sync next week about it.

Blocking: 860509
Labels: M-72 Pri-2
Raising the priority since we are planning to ship UAv2 in M72.
Project Member

Comment 25 by bugdroid1@chromium.org, Nov 16

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4c316baedb269c0b2a1f3b19ae3e88355ea1b39a

commit 4c316baedb269c0b2a1f3b19ae3e88355ea1b39a
Author: Mustaq Ahmed <mustaq@google.com>
Date: Fri Nov 16 14:31:28 2018

Forward browser action bar activation to background page.

This fixes the following three tests for UAv2:
- BrowserActionApiTest.TestPictureInPictureOnBrowserActionIconClick
- PictureInPictureLazyBackgroundPageApiTest.PictureInPictureInBackgroundPage
- NotificationsApiTest.TestUserGesture

Bug:  860718 
Change-Id: I004f7a3922b7b9f39f4371c0dfdd439c6f0b9a07
Reviewed-on: https://chromium-review.googlesource.com/c/1336029
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Mustaq Ahmed <mustaq@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608767}
[modify] https://crrev.com/4c316baedb269c0b2a1f3b19ae3e88355ea1b39a/extensions/renderer/dispatcher.cc

Status: Fixed (was: Assigned)

Sign in to add a comment