Picture-in-picture tests failing with User Activation v2 |
||||||||
Issue descriptionvirtual/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.
,
Jul 6
I recently removed the virtual/picture-in-picture/ in favour of virtual/video-surface-layer/.
,
Jul 27
Is this still an issue mustaq?
,
Jul 27
Yes. I last checked on July 17, we have the same failure in virtual/video-surface-layer/... instead.
,
Jul 27
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?
,
Jul 27
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!
,
Jul 30
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 :(
,
Jul 31
I couldn't repro either! I will kick off another bot run, to see if it got fixed.
,
Sep 25
mustaq@ Has this been fixed?
,
Sep 25
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
,
Sep 25
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.
,
Sep 25
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.
,
Sep 25
I've uploaded WIP CL at https://chromium-review.googlesource.com/c/chromium/src/+/1244239 to fix this.
,
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
,
Sep 26
mustaq@ can you confirm that it is fixed?
,
Sep 26
* 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
,
Sep 26
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).
,
Sep 27
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.
,
Sep 27
,
Nov 9
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?
,
Nov 9
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.
,
Nov 9
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.
,
Nov 14
,
Nov 14
Raising the priority since we are planning to ship UAv2 in M72.
,
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
,
Nov 16
|
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by mustaq@chromium.org
, Jul 6