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

Issue 711474 link

Starred by 2 users

Issue metadata

Status: Duplicate
Merged: issue 711468
Owner:
Last visit > 30 days ago
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 2
Type: Bug

Blocking:
issue 711468



Sign in to add a comment

[root layer scrolls] fast/scrolling failures

Project Member Reported by skobes@chromium.org, Apr 13 2017

Issue description

sataya.m@samsung.com has volunteered to look into the following test failures:

virtual/rootlayerscrolls/fast/scrolling/scrollable-area-frame-overflow-hidden.html
virtual/rootlayerscrolls/fast/scrolling/scrollable-area-frame-overried-inherited-visibility-hidden.html
virtual/rootlayerscrolls/fast/scrolling/scrollable-area-frame-scrolling-no-overried-inherited-visibility-hidden.html
virtual/rootlayerscrolls/fast/scrolling/scrollable-area-frame-scrolling-no-visibility-hidden-child.html
virtual/rootlayerscrolls/fast/scrolling/scrollable-area-frame-scrolling-no.html
virtual/rootlayerscrolls/fast/scrolling/scrollable-area-frame-scrolling-yes.html
virtual/rootlayerscrolls/fast/scrolling/scrollable-area-frame-visibility-hidden-child.html
virtual/rootlayerscrolls/fast/scrolling/scrollable-area-frame-zero-size-and-border.html
 
Cc: sriram...@samsung.com
These are the only vaild failures !!!

virtual/rootlayerscrolls/fast/scrolling/scrollable-area-frame-scrolling-no-overried-inherited-visibility-hidden.html
virtual/rootlayerscrolls/fast/scrolling/scrollable-area-frame-scrolling-no-visibility-hidden-child.html
virtual/rootlayerscrolls/fast/scrolling/scrollable-area-frame-scrolling-no.html
virtual/rootlayerscrolls/fast/scrolling/scrollable-area-frame-zero-size-and-border.html

These cases are passing !!!

virtual/rootlayerscrolls/fast/scrolling/scrollable-area-frame-overflow-hidden.html
virtual/rootlayerscrolls/fast/scrolling/scrollable-area-frame-overried-inherited-visibility-hidden.html
virtual/rootlayerscrolls/fast/scrolling/scrollable-area-frame-scrolling-yes.html
virtual/rootlayerscrolls/fast/scrolling/scrollable-area-frame-visibility-hidden-child.html


i shall remove these test cases from the TestExpectations !!!
Project Member

Comment 3 by bugdroid1@chromium.org, Apr 17 2017

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

commit 6f057f40c594d751bc68e5808c66525d0f195c72
Author: sataya.m <sataya.m@samsung.com>
Date: Mon Apr 17 08:26:32 2017

Remove RLS Passing Test Cases from TestExceptions.

Removing RLS passing test cases from the test exceptions.

BUG= 711474 

Review-Url: https://codereview.chromium.org/2820023002
Cr-Commit-Position: refs/heads/master@{#464904}

[modify] https://crrev.com/6f057f40c594d751bc68e5808c66525d0f195c72/third_party/WebKit/LayoutTests/TestExpectations

Hi Skobes,

Here is todays findings.

started working on this virtual/rootlayerscrolls/fast/scrolling/scrollable-area-frame-scrolling-no-overried-inherited-visibility-hidden.html

Found few bug's with in this test case.

#Bug 1

<iframe width=120 scrolling=no style="visibility: visible;" src="resources/generic-scrollable-content.html"></iframe>

Means iframe scrolling mode is no, means no scrollbars shouldnt be created but.

with RLS :  IFrame creates horizontal scrollbar. => Reason.
        void PaintLayerScrollableArea::ComputeScrollbarExistence(){
		        ....
				....
				needs_horizontal_scrollbar = Box().ScrollsOverflowX(); // This condition makes it true.
				....
				  if (Box().IsLayoutView()) {
					if (LocalFrame* frame = Box().GetFrame()) {
					  if (FrameView* frame_view = frame->View()) {
						ScrollbarMode h_mode;
						ScrollbarMode v_mode;
						frame_view->CalculateScrollbarModes(h_mode, v_mode); 
						/* Bug is here. CalculateScrollbarModes returns h_mode & v_mode
						   with kScrollbarAlwaysOff as frame owner scrolling mode is off.
						   but we are not setting needs_horizontal_scrollbar to false,
						   as h_mode is kScrollbarAlwaysOff, due to which Horizontal Scrollbar
						   is created.
						*/
						if (h_mode == kScrollbarAlwaysOn)
						  needs_horizontal_scrollbar = true;
						if (v_mode == kScrollbarAlwaysOn)
						  needs_vertical_scrollbar = true;
					  }
					}
				  }
		}

Without RLS: IFrame doesnt create horizontal scrollbar => Reason
        void FrameView::Init(){
		          ....
				  ....
				  ....
				  
				  if (frame_->Owner() &&
					  frame_->Owner()->ScrollingMode() == kScrollbarAlwaysOff)
					SetCanHaveScrollbars(false);
			    }
				
				
# Bug 2 .
<iframe width=120 scrolling=no style="visibility: visible;" src="resources/generic-scrollable-content.html"></iframe>
where generic-scrollable-content.html content is just "content content content content .... " without any textarea.

with RLS :  Vertical scrollbar is created and we are able to scroll.

without RLS: No Scrollbar is created and we are not able to scroll.

For  Bug#2  we can stop creating scrollbars with below fix, setting needs_vertical_scrollbar = false in kScrollbarAlwaysOff mode.
Need pointers on how to deregister from scrolling.

 void PaintLayerScrollableArea::ComputeScrollbarExistence(){
        ....
		....
		....
		
				  if (Box().IsLayoutView()) {
					if (LocalFrame* frame = Box().GetFrame()) {
					  if (FrameView* frame_view = frame->View()) {
						ScrollbarMode h_mode;
						ScrollbarMode v_mode;
						frame_view->CalculateScrollbarModes(h_mode, v_mode); 
						if (h_mode == kScrollbarAlwaysOn)
						  needs_horizontal_scrollbar = true;
						else if(h_mode == kScrollbarAlwaysOff)
						  needs_horizontal_scrollbar = false;
						if (v_mode == kScrollbarAlwaysOn)
						  needs_vertical_scrollbar = true;
						else if(v_mode == kScrollbarAlwaysOff)
						  needs_vertical_scrollbar = false;
					  }
					}
				  }
    }
	
	
Share your inputs.	


Thanks,
MuVen

Comment 5 by skobes@chromium.org, Apr 20 2017

Thanks, I agree with your analysis.  PLSA::ComputeScrollbarExistence should be updated to handle the case where FrameView::CalculateScrollbarModes returns kScrollbarAlwaysOff.

It looks like "scrolling=no" disables user scrolls by making FrameView::UserInputScrollable return false.  I think PLSA::UserInputScrollable should be updated to return false if FrameView::CalculateScrollbarModes returns kScrollbarAlwaysOff (if it is the root layer).
I'll be on leave for next 10 days. @srirama, please make the necessary changes along with the test cases and submit it for review (to Skobes).
@srirama, Please verify if this patch fixes the above failing test cases !!! If it doesn't fix, debug and update your findings !!! 
Project Member

Comment 8 by bugdroid1@chromium.org, Apr 26 2017

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

commit 31eb41c176f57dbdbf8d5ce4edd45d21a1c9a45c
Author: srirama.m <srirama.m@samsung.com>
Date: Wed Apr 26 02:25:48 2017

[RLS] Update iframe scrollbar behaviour based on scrolling attribute

Currently in RLS path, scrollbar existence is not updated when
"scrolling" attribute is "no". Updated PLSA::ComputeScrollbarExistence
and PLSA::UserInputScrollable to consider scrollbar modes to turn of
scrolling/scrollbars for root layers.

BUG= 711474 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2831313002
Cr-Commit-Position: refs/heads/master@{#467207}

[modify] https://crrev.com/31eb41c176f57dbdbf8d5ce4edd45d21a1c9a45c/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/31eb41c176f57dbdbf8d5ce4edd45d21a1c9a45c/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp

Project Member

Comment 9 by bugdroid1@chromium.org, Jul 18 2017

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

commit 03a08a02a028bdc0042b34cf36d229f02f9ce5fc
Author: sataya.m <sataya.m@samsung.com>
Date: Tue Jul 18 18:13:19 2017

[RLS] Don't add to ScrollableAreaSet if the size is zero

Add ScrollableArea only if the box size is not zero and
there is a horizontal or vertical overflow scroll.

BUG= 711474 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2857583007
Cr-Commit-Position: refs/heads/master@{#487525}

[modify] https://crrev.com/03a08a02a028bdc0042b34cf36d229f02f9ce5fc/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/03a08a02a028bdc0042b34cf36d229f02f9ce5fc/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
[modify] https://crrev.com/03a08a02a028bdc0042b34cf36d229f02f9ce5fc/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h

Can we mark this fixed now?
Please let us know if you would like us to work on any other issues.
Thanks for the patches so far!  Here are the remaining failures in fast/scrolling that I think we should look at.  (Or if they are passing, we should remove them from TestExpectations.)

virtual/rootlayerscrolls/fast/scrolling/scrollbar-tickmarks-styled.html
virtual/rootlayerscrolls/fast/scrolling/scrollbar-tickmarks-styled-after-onload.html
virtual/rootlayerscrolls/fast/scrolling/scrollbar-prevent-default.html
I have debugged first two test cases. Reason looks to be somewhat similar for both cases.
These test cases are trying to paint textmatchmarker on top of scrollbar using internals API (internals.addTextMatchMarker). This function is adding the marker and then calling InvalidatePaintForTickmarks() in LocalFrameView.cpp which is trying to invalidate vertical scrollbar using SetNeedsPaintInvalidation().
In case of RLS, this scrollbar is coming NULL. So i changed the code to get the correct scrollbar using LayoutViewportScrollableArea(). Now it is reaching till ScrollableArea::SetScrollbarNeedsPaintInvalidation() and the flags are set properly on graphics layer but still the marker is not displayed.

Any inputs for further debugging?


I think the tickmarks are painted by ScrollbarTheme::PaintTickmarks, you could try looking there.
Thanks for the quick suggestion. I have fixed these test cases now at https://chromium-review.googlesource.com/c/578831
But there is a new test failure text-match-highlight.html
It is crashing with the below callstack. Couldn't understand what exactly is this paintunderinvalidationcheck. Probably that also needs to be fixed incase of RLS flow?
***************************************************
STDERR: [1:1:0720/013635.711825:48546148475:FATAL:PaintController.cpp(464)] Check failed: false. Can't find cached display item
STDERR: #0 0x000001a57617 base::debug::StackTrace::StackTrace()
STDERR: #1 0x000001a6e601 logging::LogMessage::~LogMessage()
STDERR: #2 0x0000030a0158 blink::PaintController::FindOutOfOrderCachedItemForward()
STDERR: #3 0x00000309d6d7 blink::PaintController::FindCachedItem()
STDERR: #4 0x00000309d455 blink::PaintController::UseCachedDrawingIfPossible()
STDERR: #5 0x0000030e601b blink::ScrollbarTheme::PaintTickmarks()
STDERR: #6 0x0000030e5aa3 blink::ScrollbarTheme::Paint()
STDERR: #7 0x0000030e3fe3 blink::Scrollbar::Paint()
STDERR: #8 0x000003ad6253 blink::ScrollableAreaPainter::PaintOverflowControls()
STDERR: #9 0x000003a6a9ba blink::BlockPainter::PaintOverflowControlsIfNeeded()
**************************************************************
Project Member

Comment 15 by bugdroid1@chromium.org, Jul 22 2017

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

commit 8af4ef8b433ae41baae4cea697ad2ce66c530da2
Author: Munukutla Subrahmanya Praveen <sataya.m@samsung.com>
Date: Sat Jul 22 19:09:53 2017

[RLS] Remove scrollbar-prevent-default.html from TestExpectations

Removing scrollbar-prevent-default.html from TestExpectations.

BUG= 711474 

Change-Id: Ia38c40d7cfb3ddb49d607a53a84094f2dd1b4055
Reviewed-on: https://chromium-review.googlesource.com/581327
Reviewed-by: Steve Kobes <skobes@chromium.org>
Commit-Queue: Mu Ven <sataya.m@samsung.com>
Cr-Commit-Position: refs/heads/master@{#488864}
[modify] https://crrev.com/8af4ef8b433ae41baae4cea697ad2ce66c530da2/third_party/WebKit/LayoutTests/TestExpectations

@skobes, Can you share next set of testcases which we can look in to.

Thanks,

Comment 17 Deleted

Project Member

Comment 18 by bugdroid1@chromium.org, Jul 31 2017

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

commit 652a75d70e8f275bc795e2bbed301a42820af1b8
Author: Sriram <srirama.m@samsung.com>
Date: Mon Jul 31 07:47:35 2017

[RLS] Implement GetTickmarks for RootLayerScrolling

Currently GetTickmarks is not implemented in PLSA so the tickmarks are
not getting painted for RLS flow.
Implemented this function now and updated testExpectations.

BUG= 711474 

Change-Id: I150dfd413d2ae015e7e9e2dad94d1116df049fb3
Reviewed-on: https://chromium-review.googlesource.com/578831
Reviewed-by: Steve Kobes <skobes@chromium.org>
Commit-Queue: srirama chandra sekhar <srirama.m@samsung.com>
Cr-Commit-Position: refs/heads/master@{#490724}
[modify] https://crrev.com/652a75d70e8f275bc795e2bbed301a42820af1b8/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/652a75d70e8f275bc795e2bbed301a42820af1b8/third_party/WebKit/Source/core/frame/LocalFrameView.cpp
[modify] https://crrev.com/652a75d70e8f275bc795e2bbed301a42820af1b8/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
[modify] https://crrev.com/652a75d70e8f275bc795e2bbed301a42820af1b8/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h

@Skobes, we will not be able to check virtual/rootlayerscrolls/fast/scrolling/scrollbar-prevent-default.html as it is working fine in Linux and fails only in mac and windows.
Right now we have only Linux setup.
Can you please share other test cases if any?
Blockedon: 773373
Mergedinto: 711468
Status: Duplicate (was: Assigned)
Blockedon: -773373

Sign in to add a comment