Closed Bug 1198135 Opened 9 years ago Closed 8 years ago

CSS z-transformed + scaled content affects scroll bounds

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox40 --- affected
firefox41 --- affected
firefox42 --- affected
firefox43 --- affected
firefox51 --- fixed

People

(Reporter: keith, Assigned: mattwoodrow)

References

(Blocks 2 open bugs)

Details

(Keywords: DevAdvocacy)

Attachments

(5 files, 3 obsolete files)

current contents of http://www.keithclark.co.uk/labs/css-tests/tests/3d-transforms-overflow-scale-down/overflow-scaled.html
2.75 KB, text/html
Details
variant testcase
1.57 KB, text/html
Details
Part 1: Recompute all required overflow areas when scrolling
1.56 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
Part 2: Compute the scrolled rect stored by ScrollFrameHelper as what will actually be scrollable v2
14.80 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
Set the size of HTMLScrollFrames earlier so that we compute perspective earlier
2.59 KB, patch
dbaron
: review+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/44.0.2403.157 Safari/537.36

Steps to reproduce:

Browse to: http://codepen.io/keithclark/full/xGvYbm/ and follow the test case instructions


Actual results:

z-transformed and scaled content affects the scroll bounds of parent elements allowing a user to scroll beyond the bottom of the page


Expected results:

The computed scroll area should match the height of the content.
I've documented this along with a self-hosted test: 

http://www.keithclark.co.uk/labs/css-tests/tests/3d-transforms-overflow-scale-down/overflow-scaled.html


This forms part of my ongoing CSS render test case list:

http://www.keithclark.co.uk/labs/css-tests/
Status: UNCONFIRMED → NEW
status-firefox40: --- → affected
status-firefox41: --- → affected
status-firefox42: --- → affected
status-firefox43: --- → affected
Component: Untriaged → Layout
Ever confirmed: true
Product: Firefox → Core
Version: 40 Branch → Trunk
I've updated my test case at http://www.keithclark.co.uk/labs/css-tests/tests/3d-transforms-overflow-scale-down/overflow-scaled.html to show the bug exists on horizontal scroll too. You may want to update your local copy.
I'm fairly sure this is because the perspective is being applied from an element that isn't scrolled, so the size of the scrolled content actually changes while scrolling.

We don't recompute overflow areas when scrolling, so we don't account for this, and we allow scrolling to the bottom of where the yellow element was when scrollTop==0.

The simple fix would be to recompute the overflow areas on each scroll change if we have perspective from outside the scroll container being applied, but then you'd see the scrollbars increase in size as you scroll.

Chrome appears to get the right scrollbar size from the start, so we should be able to do that too. It's not immediately obvious to me how to do that, but it's getting late, so I'll think more about this over the weekend.
If it helps, I've found that you can get Firefox to compute scrolling dimensions correctly if you use the devtools DOM inspector to toggle CSS rules for the `.example__group` elements.
Attached patch WIP partial fix (obsolete) — — Splinter Review 
This sort of 'works' for the non-APZ case, but you see the scrollbar change size as you scroll. APZ will still be broken, depending on how often the main-thread updates the compositor with the updated scroll range.

The main problems here is that RecomputePerspectiveOverflow seems to recompute the overflow areas on the children that use the perspective, but doesn't union these together to update the overflow on the parent frame (which will have changed).

The other problem is that we re-use the initial overflow from last time (which is too big) instead of starting again. It appears that FinishAndStoreOverflow for a preserve-3d frame takes in the initial overflow (the union of the children, but not accounting for preserve-3d correctly), and then adds all the children again (this time taking preserve-3d transforms into account). This will often give us an over-estimate of the true answer.

I think we need to refactor things such that FinishAndStoreOverflow's input only includes overflow specific to the current frame, and passes a flag (maybe unnecessary?) to specify that we should also union in the overflow of all child frames. This way we can only include the children once, and properly account for preserve-3d at the right time.

I also spent some time thinking about how to compute the correct scrollable overflow for the scrolled content in this case. I think we need to compute the visual overflow rect twice (at two different scroll positions) to determine the rate of change in the visual overflow rect. We can then find the intersection between this line and the line for 'bottom visible position' (scroll position + scroll port size).

David, does that make sense to you? Any better ideas, since this all seems fairly complex?
Flags: needinfo?(dbaron)
I guess I sort of follow the nsGfxScrollFrame change, but I don't understand the motivation for the nsFrame.cpp change at all.  Could you explain?  (And maybe explain a little more clearly what it is you're trying to accomplish with the change?)
Flags: needinfo?(matt.woodrow)
As an initial disclaimer, I'm fairly sure the nsFrame change is incorrect for some cases. It's relying on the assumption that the value of aOverflowAreas for a frame that returns true for Extend3DContext() consists only of the union of the overflow areas of its children. I'm sure there are frame types that add other area there, making this invalid (my suggested solution is the 4th paragraph of comment 6).

Our (simplified) frame tree for this example looks like this:

ScrollFrame - perspective
    Block-1(mScrolledFrame)
        Block-2(preserve-3d)
            Block-3(Green square)
            Block-4(Orange square, transformed)

When we reflow this initially, we call into FinishAndStoreOverflow for Block-2 with aOverflowAreas containing the union of Block-3 and Block-4's areas. We also store this area in the 'InitialOverflowProperty' on the frame.

On a scroll, we call RecomputePerspectiveChildrenOverflow on the scroll frame, which finds descendants on which the perspective is actually applied (Block-2) and then calls FinishAndStoreOverflow on them (using the value we cached in the InitialOverflowProperty).

FinishAndStoreOverflow then works with this value, maybe union-ing in more area, but never removing anything.

In this example however, downwards scrolling moves Block-4 more than Block-3 causing them to overlap and reducing the real overflow.

My patch stops using the 'InitialOverflowProperty' value, and reverts to just using the frame size. We make up for this further down in FinishAndStoreOverflow by calling ComputePreserve3DChildrenOverflow which will union all the children's *new* overflow areas.
Flags: needinfo?(matt.woodrow)
OK, just had a chat with Matt about this.

The most significant conclusion is that what looks like a clear way forward is to refactor the UpdateOverflow methods so that the part of UpdateOverflow that adds in a frame's "custom" overflow is a separate virtual method.  This will hopefully (following inspection of what the various UpdateOverflow methods do) allow UpdateOverflow itself to become non-virtual.

That in turn will allow nsIFrame::RecomputePerspectiveChildrenOverflow to start from the bounds, and then FinishAndStoreOverflow to blow away its input (which I suspect may still be needed for the Reflow codepath even if RecomputePerspectiveChildrenOverflow starts from the bounds -- although maybe not), add the bounds and the custom overflow (instead of having RecomputePerspectiveChildrenOverflow start from the InitialOverflowProperty which includes the old, now-incorrect overflow for the children, since perspective has changed that), and then let the special 3-D code in FinishAndStoreOverflow account for the children correctly.  Or something like that ... Matt can figure it out. :-)

There may also be a need to adjust some code in OverflowChangedTracker::Flush.

It's probably worth running variants of this testcase through both Reflow and UpdateOverflow codepaths to make sure they work correctly.



Another thing we discussed was how to make the overflow area correct for APZ.  One thing of interest is that Chrome seems to be ignoring perspective when computing the overflow area.  That works fine in the current attached testcase, since the desired scrollable extents are those of the non-transformed elements.  However, I'll attach a variant of it in which Chrome can't scroll to the edge of the orange box.

This still doesn't give us the right size for the scrollbar from the beginning.  I was thinking when Matt and I were talking that this was pretty messy because elements could interact, but I realize that was wrong and the math should be reasonably simple, assuming we have access to each of the elements separately from the scrollframe itself; it just requires how far in each direction you need to scroll in order to align the edge of the child with the edge of the scrollable area.  (My comment about having access to each of the elements relates a bit to previous discussions about the parenting of elements in a preserve-3d scene; this does require considering each 3-D element separately.)
Flags: needinfo?(dbaron)
Attached file variant testcase — 
This testcase shows that Chrome doesn't scroll to show elements with perspective; note that if you delete the two perspective lines below the "NOTE" comment in the source, that will show you what it looks like without the perspective, which explains Chrome's scrolling extents.
Oh, and I also wonder if this will allow getting rid of the InitialOverflowProperty.
Thanks for adding the test case variant — I hadn't come across that one. Is it ok if I add that to my test suite?

I'll raise an issue on the Chromium bug tracker about the Chrome bug.
(In reply to keith from comment #12)
> Thanks for adding the test case variant — I hadn't come across that one. Is
> it ok if I add that to my test suite?

Sure.

(Is there a link to this test suite that you can share?)
I mentioned it above: http://www.keithclark.co.uk/labs/css-tests/
I spent some time looking at the current UpdateOverflow implementations and it appears that some of them don't really implement it and just request a reflow instead. nsMathMLContainerFrame and ScrollFrameHelper in particular seem to do this.

It's not obvious how to convert these functions into a 'custom custom overflow' version.

Do you have any ideas David?
Flags: needinfo?(dbaron)
So ScrollFrameHelper does what it does so that we can do a reflow that updates the scrollbars, and then terminate the overflow updating.  So effectively it has a custom overflow behavior of being a frame class that clips overflow.  (I think the other things that clip overflow are cross-class and in FinishAndStoreOverflow.)  It's a bit of extra data, but it should be possible to encode that in a reasonable way in a factored-out method.

I'm not sure why the MathML code does what it does -- can you try reading code and/or poking around in the mercurial or (probably) gecko-dev history?  It may be doing something tricky that we just didn't want to figure out how to handle.
Flags: needinfo?(dbaron)
Depends on: 1243610
Now that bug 1243610 has landed, I'm getting closer to fixing this.

I have an updated patch that uses ComputeCustomOverflow and it fixes the original testcase.

dbaron's extra testcase however tends to bounce a lot when you reach the bottom scroll position.

When we scroll down, we don't know the final extents of the scrollable (which shrink as we go), so we can end up overscrolling to positions past where we should.

This triggers a reflow (as ScrollFrameHelper::CustomCustomOverflow returns false), where we recompute the scrollable extents and clamp our scroll position to within them.

In the original testcase, the extents don't shrink in overscroll since we have the green rectangle there to keep them the right size. In the modified test case, the yellow rect is still the furthest down, so the scrollable extents continue to shink in overscroll.

In the first testcase, this clamping moves us to the end of the scroll container and we're done. In the modified one, the extra shortening means we clamp to a position before the 'real' end, and then when we update overflow to reflow this.

More scroll events get processed, and we repeat the cycle of overscrolling and then clamping back to a position before the end.

I'm going to have a go at fixing nsLayoutUtils::GetScrolledRect to return the 'real' scrollable extents rather than the current overflow area of the children to fix this.
Assignee: nobody → matt.woodrow
Bug 1274962 simplified some things and made this a lot easier.

This patches fixes the original testcase, but has the jittering problems on the variant.
Attachment #8675984 - Attachment is obsolete: true
Depends on: 1274962
Comment on attachment 8755717 [details] [diff] [review]
Part 1: Recompute all required overflow areas when scrolling

Note this this is a lot simpler than we originally discussed, and doesn't actually require us to throw away the input overflow areas in FinishAndStoreOverflow, nor use ComputeCustomOverflow. This is because of the patches in bug 1274962.

This is because the input overflow area no longer contains preserve-3d children (since they all have empty overflow areas), and we always compute the manually within FinishAndStoreOverflow.
Attachment #8755717 - Flags: review?(dbaron)
WIP fix for the jittering issue and APZ.

I still need to do some cleanup here and add some tests, it seems to work really well though.
Blocks: 1254260
Attachment #8760673 - Flags: review?(dbaron)
Some preliminary comments; I need to get back and do a full review:

 - is this testing for perspective in places where it also needs
 to test for transforms with the perspective component?  It seems
 like the code is ok but perhaps the comments are wrong?  Or is the
 call to ChildrenHavePerspective() a problem?

 - needs comments about how it updates the ScrollFrameHelper member
 but not the overflow areas on the scrolled frame

 - should perhaps also modify nsXULScrollFrame, although it's not
 immediately clear to me how.  Maybe not worth the bother, but at
 least leave a comment somewhere that it's broken?
 (Maybe somewhere near LayoutScrollArea or ClampAndSetBounds?)
curious if you had thoughts on comment 22
Flags: needinfo?(matt.woodrow)
(In reply to David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) from comment #22)
> Some preliminary comments; I need to get back and do a full review:
> 
>  - is this testing for perspective in places where it also needs
>  to test for transforms with the perspective component?  It seems
>  like the code is ok but perhaps the comments are wrong?  Or is the
>  call to ChildrenHavePerspective() a problem?

I don't believe so. The standalone perspective property is special in that it acts like preserve-3d (computes a transform using the size/position of the current element, but then combines that with the transform of its children), but doesn't have the same restrictions (isn't disabled if the element has overflow:scroll/hidden clipping). Only the perspective property has the effect where scrolling can change the computed transform on the child.

I can add more comments to make this clearer.

> 
>  - needs comments about how it updates the ScrollFrameHelper member
>  but not the overflow areas on the scrolled frame
> 
>  - should perhaps also modify nsXULScrollFrame, although it's not
>  immediately clear to me how.  Maybe not worth the bother, but at
>  least leave a comment somewhere that it's broken?
>  (Maybe somewhere near LayoutScrollArea or ClampAndSetBounds?)

I'll update these comments and upload a new patch.
Flags: needinfo?(matt.woodrow)
Adding the DevAdvocacy keyword, as references to this bug showed up while researching CSS-only parallax approaches.
Keywords: DevAdvocacy
(In reply to Matt Woodrow (:mattwoodrow) from comment #24)
> (In reply to David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) from comment #22)
> >  - is this testing for perspective in places where it also needs
> >  to test for transforms with the perspective component?  It seems
> >  like the code is ok but perhaps the comments are wrong?  Or is the
> >  call to ChildrenHavePerspective() a problem?
> 
> I don't believe so. The standalone perspective property is special in that
> it acts like preserve-3d (computes a transform using the size/position of
> the current element, but then combines that with the transform of its
> children), but doesn't have the same restrictions (isn't disabled if the
> element has overflow:scroll/hidden clipping). Only the perspective property
> has the effect where scrolling can change the computed transform on the
> child.

OK, I get this now.  The basic thing is that in the loop in:
https://drafts.csswg.org/css-transforms/#accumulated-3d-transformation-matrix-computation
the only thing that happens after the:
> 2. Compute a translation matrix which represents the offset of current element from its ancestor block, and pre-multiply that matrix into the transform.
that does the offset for the scroll frame (which is the offset that moves as you scroll) is the multiplication by the perspective matrix of the scrolling element.
Comment on attachment 8771846 [details] [diff] [review]
Part 2: Compute the scrolled rect stored by ScrollFrameHelper as what will actually be scrollable v2

>+  // account. Note that this only recompute the overflow areas stored on the helper

recompute -> recomputes

also, wrap at <80 columns
(and same for the later comment too, and a few bits of code)


>+// changes as we scroll. These perspective transform can cause the element to move relative to the scrolled

transform -> transforms or These -> This

>+// inner frame, and the scrollable length changes during scrolling.

maybe the "and the" should be something more like "which if we didn't do
anything to handle it, would cause"



>+    nsFrameList children = childLists.CurrentList();
>+    for (nsFrameList::Enumerator e(children); !e.AtEnd(); e.Next()) {
>+      nsIFrame* child = e.get();

I think this could just be:

    for (nsIFrame* child : childLists.CurrentList()) {


>+        // If preserve-3d then recurse, since we want to only consider 'leaf' frames

This is a little confusing to me.  Don't we now handle overflow for
preserve-3d by not trying to add the overflow to the parent, but only
to the element that establishes the 3d context?  So we really *have* to
recurse to get the overflow at all, right?

>+        if(finalScrollPos.x > 0 && finalScrollPos.y > 0) {

Space after "if".


I initially found the meaning of aOffset confusing.  But I presume it's
done the way it is (using the offset of the scrolled frame to its child)
because our handling of 3D transforms in TransformRect transforms the
rect to be relative to the root of the 3D scene (if the frame isn't the
root of its own 3D context), which explains (I think) why you'd use that
particular offset for all 3D descendants of a particular child of the
scrolled frame.  (I'm far from being able to check the math for that,
but I think I follow why you're doing it.  But maybe some more comments
would help.)


>+        // Compute how many app units the scroll position needs to change by to move the overflow rect
>+        // by 1 app unit.
>+        double deltaX = (postScroll.XMost() - preScroll.XMost() + 600.0) / 600.0;
>+        double deltaY = (postScroll.YMost() - preScroll.YMost() + 600.0) / 600.0;

Considering the right and bottom edges isn't going to be correct for
languages where scrollbars extend different directions, e.g., Arabic
or vertical Japanese, where we scroll leftwards.

In theory, the cleanest thing to do is probably just to do this for all
4 sides, by renaming deltaX to rightDelta, etc., and then handling all
four sides rather than treating finalScrollPos as a point.

(You can presumably remove the finalScrollPos.x > 0 optimization.  You'll
still need to avoid adding rectiangles with height or width < 0, although
I think *maybe* it's OK to just skip such rectangles, but that might
require a little thought.  Are those only rectangles that are "higher"
than the perspective point and thus invisible?)



Also, I think you need to handle deltaX or deltaY being 0.

r=dbaron with that
Attachment #8771846 - Flags: review?(dbaron) → review+
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d6ede0caa26
Part 1: Recompute all required overflow areas when scrolling. r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/05d5dee06384
Part 2: Compute the scrolled rect stored by ScrollFrameHelper as what will actually be scrollable. r=dbaron
And also on Linux.

Also a scroll height failure, so far on Linux, https://treeherder.mozilla.org/logviewer.html#?job_id=34192632&repo=mozilla-inbound
This failed the mochitest (when run in a group, but not when run individually) because we try compute the effective transform on child frames before we've set the size on the scroll frame.

This calculation needs the size of the scroll frame (see comment at [1] asking about exactly this), so computes the wrong matrix.

If we're lucky we reflow again later and get it right, but if we're not then we get stuck with the wrong values.

I checked that nothing else modifies the effective value of aDesiredSize in the code that is now run after we compute it. I don't think setting the size early breaks anything, but I could be wrong.


[1] http://searchfox.org/mozilla-central/source/layout/base/nsDisplayList.cpp#5631
Attachment #8783910 - Flags: review?(dbaron)
Comment on attachment 8783910 [details] [diff] [review]
Set the size of HTMLScrollFrames earlier so that we compute perspective earlier

Once we get rid of the last bits of reflow-based invalidation in tables, we should change the reflow protocol in general to set the size earlier.  Doing some of that now is fine.  (And this isn't the first such change, but might be the second.)
Attachment #8783910 - Flags: review?(dbaron) → review+
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c24738ca092
Part 1: Recompute all required overflow areas when scrolling. r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/a08b7dc31691
Part 2: Compute the scrolled rect stored by ScrollFrameHelper as what will actually be scrollable. r=dbaron
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e93865a03250
Part 3:  Set the size of HTMLScrollFrames earlier so that we compute perspective earlier. r=dbaron
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3cbb10dbf58d
Part 1: Recompute all required overflow areas when scrolling. r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c2daff688e3
Part 2: Compute the scrolled rect stored by ScrollFrameHelper as what will actually be scrollable. r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/0515ffa765a6
Part 3:  Set the size of HTMLScrollFrames earlier so that we compute perspective earlier. r=dbaron
There appears to be an issue when scrolling elements with perspective in latest OSX Nightly (51.0a1 (2016-08-30)). The scrollbar changes size as you scroll and it's not possible to reach the end of the document.

You can see the effect in these demos:

* http://www.keithclark.co.uk/labs/css-tests/tests/3d-transforms-overflow-scale-down/overflow-scaled.html
* http://keithclark.co.uk/articles/practical-css-parallax/smooth-scroll/

Is the fix in the latest nightly build yet? If so, is this behaviour related to this fix?
(In reply to keith from comment #39)
> There appears to be an issue when scrolling elements with perspective in
> latest OSX Nightly (51.0a1 (2016-08-30)). The scrollbar changes size as you
> scroll and it's not possible to reach the end of the document.

I can verify that the same issue is still present in Windows 10, using the latest Firefox Nightly.
Depends on: 1300611
(In reply to keith from comment #39)
> There appears to be an issue when scrolling elements with perspective in
> latest OSX Nightly (51.0a1 (2016-08-30)). The scrollbar changes size as you
> scroll and it's not possible to reach the end of the document.
> 
> You can see the effect in these demos:
> 
> *
> http://www.keithclark.co.uk/labs/css-tests/tests/3d-transforms-overflow-
> scale-down/overflow-scaled.html
> * http://keithclark.co.uk/articles/practical-css-parallax/smooth-scroll/
> 
> Is the fix in the latest nightly build yet? If so, is this behaviour related
> to this fix?

I have filed a new bug about the above, see Bug 1300611
Could this work be responsible for fixing related Bug 1090566?, which now seems to be ok in the latest nightly.
Depends on: 1543140
No longer depends on: 1543140
Regressions: 1543140
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: