New issue
Advanced search Search tips

Issue 675296 link

Starred by 1 user
Status: Assigned
Owner:
pdr@chromium.org
Cc:
szager@chromium.org
bokan@chromium.org
vmp...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

PaintLayerScrollableArea's m_scrollsOverflow is not recalculated on parent visibility changes

Project Member Reported by pdr@chromium.org, Dec 17 2016 
PaintLayerScrollableArea stores a local variable for whether it scrolls overflow. This is set in PaintLayerScrollableArea::updateScrollableAreaSet and depends on the owning frame's visibleToHitTesting value. When the owning frame's visibleToHitTesting changes, we fail to recalculate PaintLayerScrollableArea's m_scrollsOverflow.

Unit test:
TEST_P(PaintPropertyTreeBuilderTest, FrameVisibility) {
  if (!RuntimeEnabledFeatures::rootLayerScrollingEnabled())
    return;

  setBodyInnerHTML(
      "<style>body { margin: 0; }</style>"
      "<div id='iframeContainer' style='visibility: hidden;'>"
      "  <iframe id='iframe' style='width: 100px; height: 100px;'></iframe>"
      "</div>");
  setChildFrameHTML(
      "<style>body { margin: 0; }</style>"
      "<div id='forceScroll' style='height: 3000px;'></div>");

  FrameView* frameView = document().view();
  frameView->updateAllLifecyclePhases();

  FrameView* childFrameView = childDocument().view();
  EXPECT_FALSE(childFrameView->layoutView()->getScrollableArea()->scrollsOverflow());

  // Remove visibility: hidden so the child frame should have scrolls overflow.
  auto* iframeContainer = document().getElementById("iframeContainer");
  iframeContainer->setAttribute(HTMLNames::styleAttr, "");
  frameView->updateAllLifecyclePhases();

  EXPECT_TRUE(childFrameView->layoutView()->getScrollableArea()->scrollsOverflow()); // Fails

  // Force a layout which calls PaintLayerScrollableArea::updateScrollableAreaSet
  // and re-computes scrollsOverflow.
  childFrameView->setNeedsLayout();
  frameView->updateAllLifecyclePhases();

  EXPECT_TRUE(childFrameView->layoutView()->getScrollableArea()->scrollsOverflow()); / Passes
}
Project Member

Comment 1 by sheriffbot@chromium.org, Feb 16 2018

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 2 by dtapu...@chromium.org, Feb 22 2018

Cc: bokan@chromium.org szager@chromium.org
Is this still an issue?

Comment 3 by bokan@chromium.org, Feb 27 2018

Owner: pdr@chromium.org
Status: Assigned (was: Untriaged)
pdr@, mind re-triaging this?

Comment 4 by pdr@chromium.org, Feb 28 2018 

This is still a bug as PaintLayerScrollableArea's scrolls_overflow_ variable can be stale if CSS visibility properties change. The scrolls_overflow_ bit is used throughout paint and scroll code, so it could be in either bug component. I think this is correctly a P3 since we have never received a bugreport from it. I think we can leave this open and available in case anyone is in this area.

Here's an updated testcase:

TEST_P(PaintPropertyTreeBuilderTest, FrameVisibility) {
  if (!RuntimeEnabledFeatures::RootLayerScrollingEnabled())
    return;

  SetBodyInnerHTML(
      "<style>body { margin: 0; }</style>"
      "<div id='iframeContainer' style='visibility: hidden;'>"
      "  <iframe id='iframe' style='width: 100px; height: 100px;'></iframe>"
      "</div>");
  SetChildFrameHTML(
      "<style>body { margin: 0; }</style>"
      "<div id='forceScroll' style='height: 3000px;'></div>");

  LocalFrameView* frame_view = GetDocument().View();
  frame_view->UpdateAllLifecyclePhases();

  LocalFrameView* child_frame_view = ChildDocument().View();
  EXPECT_FALSE(child_frame_view->GetLayoutView()->GetScrollableArea()->ScrollsOverflow());

  // Remove visibility: hidden so the child frame should have scrolls overflow.
  auto* iframe_container = GetDocument().getElementById("iframeContainer");
  iframe_container->setAttribute(HTMLNames::styleAttr, "");
  frame_view->UpdateAllLifecyclePhases();

  EXPECT_TRUE(child_frame_view->GetLayoutView()->GetScrollableArea()->ScrollsOverflow()); // Fails

  // Force a layout which calls PaintLayerScrollableArea::updateScrollableAreaSet
  // and re-computes scrollsOverflow.
  child_frame_view->SetNeedsLayout();
  frame_view->UpdateAllLifecyclePhases();

  EXPECT_TRUE(child_frame_view->GetLayoutView()->GetScrollableArea()->ScrollsOverflow()); // Passes
}

Comment 5 by chrishtr@chromium.org, Feb 28 2018 

It seems the code was added in this CL:

https://chromium.googlesource.com/chromium/src/+/9de09a320e201ea651b23587850050a67654271f

Reading the corresponding bug, I don't see a great reason to include the
VisibleToHitTesting check, and suggest just removing it, which will fix this bug.
►

Sign in to add a comment