New issue
Advanced search Search tips

Issue 816957 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 417782



Sign in to add a comment

[RLS] virtual/android/fullscreen tests are failing

Project Member Reported by pdr@chromium.org, Feb 27 2018

Issue description

The following tests are failing:
virtual/android/fullscreen/compositor-touch-hit-rects-fullscreen-video-controls.html
virtual/android/fullscreen/full-screen-iframe-allowed-video.html
virtual/android/fullscreen/video-controls-timeline.html
virtual/android/fullscreen/video-scrolled-iframe.html

I tested "virtual/android/fullscreen/full-screen-iframe-allowed-video.html" on a real device and it does not fail, so I suspect these may be bugs in the test harness.
 

Comment 1 by bokan@chromium.org, Mar 1 2018

Landed CL that fixes all but the touch-hit-rects test. Forgot to link to this bug though so pasted below:

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

commit 48ec74d7989862a82990827d4898d016d2942615
Author: David Bokan <bokan@chromium.org>
Date: Thu Mar 01 18:36:14 2018

[root-layer-scrolls] Fix android fullscreen video

Android uses an "overlay" composited video layer when a video is
fullscreened. This layer is parented to the PaintLayerCompositor's
root_content_layer and the actual content (LayoutView) is detached.

With root-layer-scrolling enabled, PLC doesn't create a
root_content_layer and the top-most GraphicsLayer in PLC is that of the
LayoutView. This CL connects the video layer to the PLC's parent
instead. This is currently the outer viewport scroll layer. We also need
to make sure painting starts from the child of this layer, rather than
the PLC's root layer since the video layer is no longer connected to the
root. This doesn't matter for the video itself, however, the media
player controls need to be painted from Blink.

These changes exposed some brittleness in the compositor and the Android
fullscreen video layout tests were hitting the DCHECK in
ScrollbarLayerImplBase::CanScrollOrientation. This is because the
viewport scrollbars use the outer/layout viewport as their ElementId but
are owned by the inner/visual viewport. So when we disconnect the PLC's
content layers, we remove the ScrollNode in the compositor but the
visual viewport and its scrollbar layers live on. The rest of this patch
changes the visual viewport scrollbars to use the visual viewport's
element id.

TEST=virtual/android/fullscreen/video-scrolled-iframe.html,
     virtual/android/fullscreen/video-controls-timeline.html,
     virtual/android/fullscreen/full-screen-iframe-allowed-video.html

Bug:  711468 , 811024 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: Ic1c52e1853f14842ee3ae19119476b8945ecae17
Reviewed-on: https://chromium-review.googlesource.com/923397
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Commit-Queue: Chris Harrelson <chrishtr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540226}
[modify] https://crrev.com/48ec74d7989862a82990827d4898d016d2942615/cc/input/scrollbar_animation_controller.cc
[modify] https://crrev.com/48ec74d7989862a82990827d4898d016d2942615/cc/trees/layer_tree_host_impl.cc
[modify] https://crrev.com/48ec74d7989862a82990827d4898d016d2942615/cc/trees/layer_tree_impl.cc
[modify] https://crrev.com/48ec74d7989862a82990827d4898d016d2942615/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/48ec74d7989862a82990827d4898d016d2942615/third_party/WebKit/Source/core/exported/WebViewImpl.cpp
[modify] https://crrev.com/48ec74d7989862a82990827d4898d016d2942615/third_party/WebKit/Source/core/frame/LocalFrameView.cpp
[modify] https://crrev.com/48ec74d7989862a82990827d4898d016d2942615/third_party/WebKit/Source/core/frame/VisualViewport.cpp
[modify] https://crrev.com/48ec74d7989862a82990827d4898d016d2942615/third_party/WebKit/Source/core/frame/VisualViewport.h
[modify] https://crrev.com/48ec74d7989862a82990827d4898d016d2942615/third_party/WebKit/Source/core/paint/compositing/PaintLayerCompositor.cpp
[modify] https://crrev.com/48ec74d7989862a82990827d4898d016d2942615/third_party/WebKit/Source/core/paint/compositing/PaintLayerCompositor.h

Project Member

Comment 2 by bugdroid1@chromium.org, Mar 3 2018

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

commit 059794950cf2bc8e148db283189ba604f320d892
Author: David Bokan <bokan@chromium.org>
Date: Sat Mar 03 00:16:07 2018

[root-layer-scrolls] Fix android fullscreen test

This patch fixes compositor-touch-hit-rects-fullscreen-video-controls
when run with root layer scrolls. This test was added for
 crbug.com/372314 . An explanation of that is helpful to understand the
fix here:

When Blink calculates the touch event handler rects it
has a special optimization when the document has a handler. In that
case, it doesn't bother checking anything else and places a content-
sized rect onto the document layer.

However, in the case of an Android fullscreen video, the compositor
detaches the document layer hierarchy and attaches the video and its
controls. Because of the optimization above, the compositor doesn't
bother calculating touch handler rects on these new layers, but because
the document layer is detached it seems as though there aren't any touch
handler regions at all. The fix in that bug was to disable the
optimization when a fullscreen video overlay is active to force
computing rects from the media control layers.

This test uses Internals::touchEventTargetLayerRects to calculate the
rects on the layer tree. This method uses
PaintLayerCompositor::RootGraphicsLayer as its starting point. Pre-RLS,
this would be the overflow controls host layer in the PLC. This was ok
because the document layer was connected to it's descendants and
disconnected when an overlay video was attached. With RLS,
RootGraphicsLayer returns explicitly the graphics layer for the
LayoutView which may not be attached at all. Thus the test was seeing
that a handler still existed on the document because that's where it
would start its search. The fix is to start the search from the painting
root which is above the PaintLayerCompositor and thus, wont descend into
the detached document layer.

I don't think this test is a good guard against the regression. Since
the document layer is detached we don't actually check that we correctly
calculate the touch rects on the video controls. It appears that they've
since been modified to not require touch handlers and the expectation
reflects that. As such, this test can't tell the difference between the
controls not having touch handlers and Blink failing to calculate touch
handler rects. I've filed https://crbug.com/818330 for this problem.

Bug:  816957 
Change-Id: I088f960edb47af2d6d539e50a21b7e39067efb5f
Reviewed-on: https://chromium-review.googlesource.com/947052
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540694}
[modify] https://crrev.com/059794950cf2bc8e148db283189ba604f320d892/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/059794950cf2bc8e148db283189ba604f320d892/third_party/WebKit/LayoutTests/fullscreen/compositor-touch-hit-rects-fullscreen-video-controls.html
[modify] https://crrev.com/059794950cf2bc8e148db283189ba604f320d892/third_party/WebKit/LayoutTests/platform/linux/virtual/android/fullscreen/compositor-touch-hit-rects-fullscreen-video-controls-expected.txt
[modify] https://crrev.com/059794950cf2bc8e148db283189ba604f320d892/third_party/WebKit/LayoutTests/virtual/android/fullscreen/compositor-touch-hit-rects-fullscreen-video-controls-expected.txt
[modify] https://crrev.com/059794950cf2bc8e148db283189ba604f320d892/third_party/WebKit/Source/core/testing/Internals.cpp

Comment 3 by bokan@chromium.org, Mar 5 2018

Status: Fixed (was: Assigned)

Sign in to add a comment