New issue
Advanced search Search tips

Issue 778084 link

Starred by 1 user

Issue metadata

Status: Duplicate
Merged: issue 785088
Owner: ----
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 417782



Sign in to add a comment

[root layer scrolls] content browsertests are failing

Project Member Reported by pdr@chromium.org, Oct 25 2017

Issue description

The following content browsertests are failing with root layer scrolling (set status: "stable" in runtime_enabled_features.json5):
RenderViewImplScaleFactorTest.PreferredSizeWithScaleFactor
DumpAccessibilityTreeTest.AccessibilityIframeCoordinates
RenderViewImplTest.PreferredSizeZoomed
DumpAccessibilityTreeTest.AccessibilityIframeTransformScrolled
AccessibilityHitTestingBrowserTest.CachingAsyncHitTestingInIframes
DumpAccessibilityTreeTest.AccessibilityIframeCoordinatesCrossProcess

ninja -j300 -C out/Debug content_browsertests
out/Debug/content_browsertests --gtest_filter=*RenderViewImplScaleFactorTest.PreferredSizeWithScaleFactor*

Example failure:
[ RUN      ] RenderViewImplScaleFactorTest.PreferredSizeWithScaleFactor
../../content/renderer/render_view_browsertest.cc:2304: Failure
      Expected: gfx::Size(400, 400)
      Which is: 400x400
To be equal to: size
      Which is: 415x400
../../content/renderer/render_view_browsertest.cc:2311: Failure
      Expected: gfx::Size(400, 400)
      Which is: 400x400
To be equal to: size
      Which is: 415x400
 
the additional 15 units is coming from ScrollbarLogicalWidth(), which returns 0 in normal case

as HasOverflowClip() is true for RLS, in normal case HasOverflowClip() is false
Could you share a stack trace of how ScrollbarLogicalWidth() is getting called?
Please find the Stack trace of how ScrollbarLogicalWidth() is called

#0 0x7f8880600b27 base::debug::StackTrace::StackTrace()
#1 0x7f887cc88e21 blink::LayoutBlock::ComputeIntrinsicLogicalWidths()
#2 0x7f887cc89c4e blink::LayoutBlock::ComputePreferredLogicalWidths()
#3 0x7f887ccaf826 blink::LayoutBox::MinPreferredLogicalWidth()
#4 0x7f887cc893fc blink::LayoutBlock::ComputeBlockPreferredLogicalWidths()
#5 0x7f887cc88c4d blink::LayoutBlock::ComputeIntrinsicLogicalWidths()
#6 0x7f887cc89c4e blink::LayoutBlock::ComputePreferredLogicalWidths()
#7 0x7f887ccaf826 blink::LayoutBox::MinPreferredLogicalWidth()
#8 0x7f887c9cd7cd blink::WebViewImpl::ContentsPreferredMinimumSize()
#9 0x7f887f5916aa content::RenderViewImpl::CheckPreferredSize()
#10 0x00000099f80e content::RenderViewImplScaleFactorTest_PreferredSizeWithScaleFactor_Test::TestBody()
#11 0x000000a497f6 testing::Test::Run()
#12 0x000000a4a1d0 testing::TestInfo::Run()
#13 0x000000a4a6b7 testing::TestCase::Run()
#14 0x000000a509b7 testing::internal::UnitTestImpl::RunAllTests()
#15 0x000000a50643 testing::UnitTest::Run()
#16 0x000000b0cc48 base::TestSuite::Run()
#17 0x000000addd73 content::ContentTestLauncherDelegate::RunTestSuite()
#18 0x000000af9463 content::LaunchTests()
#19 0x000000addd37 main
#20 0x7f8878de1830 __libc_start_main
#21 0x0000004ad029 <unknown>

So, preferred width includes auto scrollbar width on purpose (http://crrev.com/1980103002) to fix layout of auto-width scrollers.

But WebView::ContentsPreferredMinimumSize maybe shouldn't include it?  I'm not sure what this method is actually used for though.

Also, PreferredSizeWithScaleFactor calls SetCanHaveScrollbars(false).  Preferred width shouldn't include scrollbar width if scrollbars aren't actually present.  Maybe we're not plumbing SetCanHaveScrollbars properly?
In Normal case 
Layout happens after after SetCanHaveScrollbars() is set the Layout happens, the stack is 
#1 0x7f33e6c486d6 blink::PaintLayerScrollableArea::UpdateAfterLayout()
#2 0x7f33e6c33a41 blink::PaintLayer::UpdateSizeAndScrollingAfterLayout()
#3 0x7f33e6a3e99e blink::LayoutBlockFlow::UpdateBlockLayout()
#4 0x7f33e6a356af blink::LayoutBlock::UpdateLayout()
#5 0x7f33e6afbb66 blink::LayoutView::UpdateLayout()
#6 0x7f33e67ae88c blink::LocalFrameView::PerformLayout()
#7 0x7f33e67ac667 blink::LocalFrameView::UpdateLayout()
#8 0x7f33e67b252c blink::LocalFrameView::ScrollbarExistenceMaybeChanged()
#9 0x7f33e67ba496 blink::LocalFrameView::AdjustScrollbarExistence()
#10 0x7f33e67af3d0 blink::LocalFrameView::UpdateScrollbars()
#11 0x7f33e67abc2c blink::LocalFrameView::SetScrollbarModes()
#12 0x7f33e67a9cf5 blink::LocalFrameView::SetCanHaveScrollbars()
#13 0x00000099f68e content::RenderViewImplScaleFactorTest_PreferredSizeWithScaleFactor_Test::TestBody()

in RLS the flow returns in LocalFrameView::UpdateScrollbars()

Comment 6 by skobes@chromium.org, Nov 13 2017

The way it's supposed to work for SetCanHaveScrollbars(false) is that PLSA::ComputeScrollbarExistence calls LayoutView::CalculateScrollbarModes which has the following check:

    if (!frameView->CanHaveScrollbars())
      RETURN_SCROLLBAR_MODE(kScrollbarAlwaysOff);

It would be useful to understand where that's breaking here.
updated a patch https://chromium-review.googlesource.com/c/chromium/src/+/758480 as fix, Please Review
just this change fixes the test

@@ -3066,9 +3067,10 @@ WebSize WebViewImpl::ContentsPreferredMinimumSize() {
 
   // Needed for computing MinPreferredWidth.
   FontCachePurgePreventer fontCachePurgePreventer;
-  int width_scaled = document->GetLayoutViewItem()
-                         .MinPreferredLogicalWidth()
-                         .Round();  // Already accounts for zoom.
+  //int width_scaled = document->GetLayoutViewItem()
+  //                       .MinPreferredLogicalWidth()
+  //                       .Round();  // Already accounts for zoom.
+  int width_scaled = document->documentElement()->GetLayoutBox()->ScrollWidth().Round();

Comment 9 by skobes@chromium.org, Nov 27 2017

Layout objects track preferred sizes independent of actual size.  I think the LayoutView's preferred width can differ from its layout overflow width.
Mergedinto: 785088
Status: Duplicate (was: Available)
Let's use  issue 785088  for fixes to WebView::ContentsPreferredMinimumSize.

I'll file separate bugs for AccessibilityHitTestingBrowserTest and DumpAccessibilityTreeTest.

Sign in to add a comment