Closed Bug 976412 Opened 6 years ago Closed 6 years ago

Scrolled content incorrectly merged into a single layer when fixed-pos frames are in between

Categories

(Core :: Layout, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: roc, Assigned: roc)

Details

Attachments

(2 files)

While writing tests in bug 975931, I found a bug on trunk that should have been fixed by 969354 but wasn't. The testcase is:

<!DOCTYPE HTML>
<html reftest-async-scroll
      reftest-displayport-x="0" reftest-displayport-y="0"
      reftest-displayport-w="800" reftest-displayport-h="2000"
      reftest-async-scroll-x="0" reftest-async-scroll-y="400">
<body style="overflow:hidden; height:3000px">
  <!-- Test that scrolling content above position-fixed content but not
       intersecting it doesn't get moved down to a layer below the
       position-fixed content -->
  <div style="position:absolute; background:black; top:0; left:0; width:500px; height:200px; z-index:1"></div>
  <div style="position:fixed; background:blue; top:200px; left:0; width:500px; height:200px; z-index:2"></div>
  <div style="position:absolute; background:gray; top:500px; left:0; width:500px; height:200px; z-index:3"></div>
</body>
</html>

The second abs-pos DIV should be placed in its own layer, but it isn't. That's because the fixed-pos DIV doesn't trigger the "should-fix-to-viewport" logic of bug 969354 :-(.
Comment on attachment 8381330 [details] [diff] [review]
Part 2: Add more async scrolling tests

Review of attachment 8381330 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. A couple of minor issues below but I don't feel strongly about them.

::: layout/reftests/async-scrolling/position-fixed-1.html
@@ +4,5 @@
> +      reftest-displayport-w="800" reftest-displayport-h="2000"
> +      reftest-async-scroll-x="0" reftest-async-scroll-y="50">
> +<body style="overflow:hidden; height:3000px">
> +  <!-- Test that position:fixed content stays fixed when we async-scroll -->
> +  <div style="position:fixed; background:blue; top:0; left:0; width:200px; height:100px"></div>

This test (position-fixed-1) looks the same whether or not the async scroll is applied. It would be nice to add an element that is visible beside the fixed-position element in the top 50 pixels of the page and scrolls off once the async scroll is applied. That way if the async scroll properties are busted for whatever reason this test will actually fail.

::: layout/reftests/async-scrolling/position-fixed-2-ref.html
@@ +1,3 @@
> +<!DOCTYPE HTML>
> +<html>
> +<body style="overflow:hidden; height:3000px">

This reference page (and all the other position-fixed-*-ref.html files) doesn't need the overflow/height properties, right? For consistency I'd rather remove them.
Attachment #8381330 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8381329 [details] [diff] [review]
Part 1: Fix logic for determining when a display item can be placed in a layer

Review of attachment 8381329 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/FrameLayerBuilder.cpp
@@ +2197,5 @@
> +    // If the layer's drawn region intersects the item, stop now since no
> +    // lower layer will be usable. Do the same if the layer is subject to
> +    // async transforms, since we don't know where it will really be drawn.
> +    if (data->mDrawRegion.Intersects(aVisibleRect) ||
> +        data->IsSubjectToAsyncTransforms())

Maybe move these checks into a helper 'DrawRegionContains' similar to how DrawAboveRegionContains works.
Attachment #8381329 - Flags: review?(matt.woodrow) → review+
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.