New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 757636 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner: ----
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 417782



Sign in to add a comment

[RLS] LocalFrameView::ScrollOrigin() should be 0,0

Project Member Reported by skobes@chromium.org, Aug 21 2017

Issue description

Log scroll origin in LocalFrameView::AdjustViewSize and run:

content_shell --root-layer-scrolls https://output.jsbin.com/pabuxig/quiet

It shows 848,0, then 863,0.  It should never be anything except 0,0 - the document's negative horizontal overflow should be applied to the layout viewport only.
 

Comment 1 by skobes@chromium.org, Aug 21 2017

Cc: sriram...@samsung.com satay...@samsung.com
Logs for RLS/Non-RLS case.

************************ RLS *************************
[1:1:0822/195644.230778:3401772401937:ERROR:LocalFrameView.cpp(757)] origin: 848, 0
[1:1:0822/195644.232002:3401772403130:ERROR:LocalFrameView.cpp(757)] origin: 863, 0
************************************************

*********************** Non RLS *********************
[1:1:0822/195802.643476:3401850814687:ERROR:LocalFrameView.cpp(757)] origin: 863, 0
[1:1:0822/195802.644224:3401850815374:ERROR:LocalFrameView.cpp(757)] origin: 863, 0
****************************************************
The only difference between RLS/NonRLS case is the first value 848 instead of 863.
So should we check that alone or should we check why 863 is coming instead of 0, in NonRLS case as well?

Comment 4 by skobes@chromium.org, Aug 23 2017

Without RLS, the scroll origin should be applied to the LocalFrameView, so 863 is correct.

With RLS, the scroll origin should be applied to the root PaintLayerScrollableArea instead of the LocalFrameView.  So the root PLSA should have a scroll origin of 863, and the LocalFrameView should have a scroll origin of 0.

Probably it is sufficient for LocalFrameView::AdjustViewSize to check the RLS setting and skip the call to SetScrollOrigin.  Currently it sets scroll origin based on LayoutView::DocumentRect which is the LayoutView's layout overflow rect.

Comment 5 by skobes@chromium.org, Aug 23 2017

I think the change from 848 to 863 is due to the creation of the root PLSA's vertical scrollbar.  There's a separate bug to start with a vertical scrollbar by default ( issue 711137 ).
Project Member

Comment 6 by bugdroid1@chromium.org, Aug 24 2017

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

commit e26a0a9d9246ab486707d49cd17a27688e239961
Author: srirama <srirama.m@samsung.com>
Date: Thu Aug 24 18:01:45 2017

[RLS] LocalFrameView::ScrollOrigin() should be 0,0

The document's negative horizontal overflow should be applied to the
layout viewport only. Currently it sets scroll origin based on
LayoutView::DocumentRect which is the LayoutView's layout overflow rect.
Incase of RLS it should be applied to the root PaintLayerScrollableArea only.
Added RLS enabled check to avoid wrong updation of scroll origin.

BUG= 757636 

Change-Id: I9e0f7ea31b71da7dcb81a40a661a45d5a150d65c
Reviewed-on: https://chromium-review.googlesource.com/628858
Reviewed-by: Steve Kobes <skobes@chromium.org>
Commit-Queue: srirama chandra sekhar <srirama.m@samsung.com>
Cr-Commit-Position: refs/heads/master@{#497120}
[modify] https://crrev.com/e26a0a9d9246ab486707d49cd17a27688e239961/third_party/WebKit/Source/core/exported/WebFrameTest.cpp
[modify] https://crrev.com/e26a0a9d9246ab486707d49cd17a27688e239961/third_party/WebKit/Source/core/frame/LocalFrameView.cpp

Comment 7 by skobes@chromium.org, Aug 24 2017

Status: Fixed (was: Available)

Sign in to add a comment