Bug 47514 – CSS transforms should affect scrolling

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Beth Dakin]

lowestPosition(), rightmostPosition(), and leftmostPosition() should take CSS transforms into account.

[Beth Dakin]

Created attachment 70491 [details]
Test case

To reproduce the problem with the attached test case:

1. Open the test case.
2. Size the browser window to cut across the middle of the purple square.
3. Note the lack of a scroll bar to scroll the the rest of the blue square. That's the bug.

[Beth Dakin]

Created attachment 70492 [details]
Patch

A few things about the updated test results in this patch:

LayoutTests/fast/transforms/scrollIntoView-transformed.html --> After examining this test for a littler while, I feel confident that the updated results are correct. However, the test should probably be updated since it now claims that the fails on some of the sub-tests. I didn't change the test yet only because I know changing tests is always controversial. I am willing to change the test or do whatever makes the most sense.

I feel pretty confident that the other test results are not controversial, but of course the reviewer should look over them too.

[Dean Jackson]

I wouldn't hesitate about fixing the scrollIntoView-transformed test. I think I wrote that, and wasn't completely sure it was testing everything I was trying to fix at the time.

[Dean Jackson]

I'm not so sure that transformed elements should always participate in the scrollbariness of content. I worry that this fix will cause elements that intentionally off the page to cause scrollbars to appear. eg. what will happen if I translateX(-1000px) ?

(note that I didn't apply the patch so I'm not sure if your fix will actually do what I fear :)

[Beth Dakin]

(In reply to comment #3)
> I wouldn't hesitate about fixing the scrollIntoView-transformed test. I think I wrote that, and wasn't completely sure it was testing everything I was trying to fix at the time.

Hi Dean! Thanks for the input. You did write that test, so it's good to know you didn't have something else in mind that is now broken.

(In reply to comment #4)
> I'm not so sure that transformed elements should always participate in the scrollbariness of content. I worry that this fix will cause elements that intentionally off the page to cause scrollbars to appear. eg. what will happen if I translateX(-1000px) ?
> 
> (note that I didn't apply the patch so I'm not sure if your fix will actually do what I fear :)

The patch only adds scrollbars when the transformed content moves off of the right or bottom edge of the screen. In this way, it matches, say, absolutely positioned content. If an element is positioned absolutely to start at a negative coordinate, then scrollbars are not added, but they are for positive coordinates that are beyond the current window. And that's what this patch does for transformed content as well.

[Simon Fraser (smfr)]

(In reply to comment #4)
> I'm not so sure that transformed elements should always participate in the scrollbariness of content. I worry that this fix will cause elements that intentionally off the page to cause scrollbars to appear. eg. what will happen if I translateX(-1000px) ?

You should have used overflow:hidden in this case. The spec says that transforms affect overflow, so we need to implement this. We should talk about whether it's OK to affect existing content though.

[Dean Jackson]

(In reply to comment #5)
> (In reply to comment #3)
> > I wouldn't hesitate about fixing the scrollIntoView-transformed test. I think I wrote that, and wasn't completely sure it was testing everything I was trying to fix at the time.
> 
> Hi Dean! Thanks for the input. You did write that test, so it's good to know you didn't have something else in mind that is now broken.

I should add that I also wasn't sure exactly what was correct behaviour vs existing behaviour at the time :)

That's why the test is so pathetic.

> 
> (In reply to comment #4)
> > I'm not so sure that transformed elements should always participate in the scrollbariness of content. I worry that this fix will cause elements that intentionally off the page to cause scrollbars to appear. eg. what will happen if I translateX(-1000px) ?
> > 
> > (note that I didn't apply the patch so I'm not sure if your fix will actually do what I fear :)
> 
> The patch only adds scrollbars when the transformed content moves off of the right or bottom edge of the screen. In this way, it matches, say, absolutely positioned content. If an element is positioned absolutely to start at a negative coordinate, then scrollbars are not added, but they are for positive coordinates that are beyond the current window. And that's what this patch does for transformed content as well.

OK. That still means transforms off the bottom and right will now cause scrollbars to appear, which could be a big change to existing content. I agree it is the right thing to do though (people should overflow:hidden if they don't want that behaviour)

[Dave Hyatt]

Comment on attachment 70492 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=70492&action=review

Discussed on IRC.

> WebCore/rendering/RenderBlock.cpp:3332
> +

Shouldn't need the ((RenderBox*)this) here since the variable name and the function name aren;'t the same.

[Dave Hyatt]

Basically in order to handle the transform properly on a RenderBlock (and any container really), you need to compute the rightmost/leftmost/lowest/topmost positions all without a transform applied.  Then you apply your transform to the box that includes all of that overflow.  Your lowest position will then be the bottom of that transformed box.

Your patch is taking the transformed RenderBlock frameRect and then just maxing with children values (which are untransformed), so that gives you wrong results.  You have to just compute everything (in all directions) without the transform, and only at the end apply the transform.

[Dave Hyatt]

You'll probably have to add an implementation of topmostPosition as well.  If you flip a box 180 degrees, the top becomes the bottom, so that overflow would become the lowestPosition.

[Beth Dakin]

Created attachment 70778 [details]
Patch to add topmostPosition()

This patch lays the groundwork for fixing this bug in the way the Hyatt suggested.

[WebKit Review Bot]

Attachment 70778 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/rendering/RenderBlock.cpp:3402:  Extra space before ) in for  [whitespace/parens] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Beth Dakin]

Created attachment 70782 [details]
Patch to add topmostPosition()

[Dave Hyatt]

Comment on attachment 70782 [details]
Patch to add topmostPosition()

View in context: https://bugs.webkit.org/attachment.cgi?id=70782&action=review

Everything else looks good.  One more round.

> WebCore/rendering/RenderBlock.cpp:3385
> +                    int tp = r->y() + r->topmostPosition(false);

This check isn't quite right.  It needs to be like the one in lowestPosition.  The comment is wrong too.

Should be something like this:

// If a positioned object lies completely to the left of the root it will be unreachable via scrolling.
// Therefore we should not allow it to contribute to the topmost position.
if (!isRenderView() || r->x() + r->width() > 0 || r->x() + r->rightmostPosition(false) > 0) {

> WebCore/rendering/RenderBlock.cpp:3416
> +        top = min(top, relativeOffset - paddingTop());

Don't include "- paddingTop()" here. Just top = min(top, relativeOffset);

> WebCore/rendering/RenderBlock.cpp:3419
> +                int childTopEdge = lastRootBox()->selectionTop();

You should be using firstRootBox() here not lastRootBox().

> WebCore/rendering/RenderBlock.cpp:3421
> +            }

No need for the "- paddingTop()" here.

> WebCore/rendering/RenderBlock.cpp:3430
> +    }

I don't think this else check was needed at all.  Sorry for leading you astray there.

I think this whole check should just reduce to:

if (!includeSelf && firstRootBox())
     top = min(top, firstRootBox()->selectionTop() + relativeOffset);

There may be more I'm missing, but that should be good enough for now.

[Beth Dakin]

Created attachment 70786 [details]
Patch to add topmostPosition()

Good catches!

[Simon Fraser (smfr)]

It would be great to have a layout test that doesn't require a pixel test. Maybe something that tests the scrollHeight/width on an overflow:scroll div containing at transformed element.

[Beth Dakin]

(In reply to comment #16)
> It would be great to have a layout test that doesn't require a pixel test. Maybe something that tests the scrollHeight/width on an overflow:scroll div containing at transformed element.

Good idea. I will try to make a test like that.

I should add that this current patch (which is just laying the groundwork for the real patch) doesn't have a layout test because it adds code that isn't yet used. I will be sure to write a layout that exercises it once I have the patch to fix this bug though.

[Dave Hyatt]

Comment on attachment 70786 [details]
Patch to add topmostPosition()

View in context: https://bugs.webkit.org/attachment.cgi?id=70786&action=review

> WebCore/rendering/RenderBlock.cpp:3387
> +                }

This is still not right.  The if statement should be identical to the one in lowestPosition.  You should be checking x + width and x + rightmostPosition, and you're looking for objects to the left of the root (just like what lowestPosition does).  What you have in the body of the if statement is correct.

> WebCore/rendering/RenderBlock.cpp:3413
> +    }

Just yank these three lines.  The following check supersedes them.

[Dave Hyatt]

This check:

if (!isRenderView() || r->x() + r->width() > 0 || r->x() + r->rightmostPosition(false) > 0) {

From lowestPosition should be unchanged.  You're looking for objects to the left of the root still.

This is the check that can just be yanked:


  if (!includeSelf && firstLineBox()) {
 3411         for (InlineFlowBox* currBox = firstLineBox(); currBox; currBox = currBox->nextLineBox())
 3412             top = min(top, (int)currBox->y() + relativeOffset);
 3413     }

[Beth Dakin]

Created attachment 70790 [details]
Patch to add topmostPosition()

Ugh, sorry. I meant to fix both of those things last time, but careless editing prevented it.

[Dave Hyatt]

Comment on attachment 70790 [details]
Patch to add topmostPosition()

r=me!

[Beth Dakin]

Yay! I committed this patch with http://trac.webkit.org/changeset/69822

Keeping the bug open since that is just one of two patches that will be necessary.

[Beth Dakin]

Created attachment 70985 [details]
Pacth

[Beth Dakin]

A note about some tests:

fast/transforms/scrollIntoView-transformed.html --> As Dean and I discussed earlier in the bug, I believe that this test passes even though the expected results now say they fail. I think I should update the test to reflect that, but I haven't done that yet.

fast/borders/border-image-rotate-transform-expected.png
-and-
/fast/borders/border-image-scale-transform-expected.png --> When Hyatt and I talked on irc before, he thought the results of these tests should not have a scrollbar. However, I believe they correctly do have a scrollbar because in both tests, the body itself is rotated, so some of it is always necessarily offscreen, which is sort of weird and unfortunate, but I think that's what you sign up for when you transform the body.

fast/forms/search-transformed-expected.png --> Similarly, this test has a translated <p>, hence the scrollbar.

[Early Warning System Bot]

Attachment 70985 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4445059

[Beth Dakin]

Created attachment 70986 [details]
Patch

Oops! Fixing that build error.

[Eric Seidel (no email)]

Attachment 70985 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4389068

[WebKit Review Bot]

Attachment 70985 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4498008

[Dave Hyatt]

Comment on attachment 70986 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=70986&action=review

LayoutTests/platform/mac/fast/borders/border-image-rotate-transform-expected.png

The results for this one look odd to me.  Why are there scrollbars?

Other comments inline.

> WebCore/rendering/RenderBlock.cpp:3524
> +    if (layer() && layer()->hasTransform()) {
> +        IntRect transformRect = applyLayerTransformToRect(IntRect(x(), y(), width(), bottom));
> +        return transformRect.height();
> +    }

Ok, this check needs to take into account the other positions.  You can't just use x, y and width.  You have to use the *untransformed* leftmostPosition, topmostPosition, rightmostPosition and lowestPosition to make the rect.  Then you transform that rect.  The other checks have the same issue.

> WebCore/rendering/RenderMedia.cpp:608
> +    return min(top,  m_controlsShadowRoot->renderBox()->transformedFrameRect().y() + m_controlsShadowRoot->renderBox()->topmostPosition(includeOverflowInterior, includeSelf));

Can't the RenderMedia be transformed?  It seems like you have to account for a transform here.  The base class call gives you the transformed rectangle's top.  You can't just add the controls root position to that, since it was also affected by the transform.

This needs to be a bit more like RenderBlock.  You have to apply the transform at the end (and just as with RenderBlock, overflow out all four sides has to be considered).

[Beth Dakin]

(In reply to comment #29)
> (From update of attachment 70986 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70986&action=review
> 
> LayoutTests/platform/mac/fast/borders/border-image-rotate-transform-expected.png
> 
> The results for this one look odd to me.  Why are there scrollbars?

This test has scrollbar because the body is rotated.

[Beth Dakin]

Created attachment 71246 [details]
Patch

[Beth Dakin]

I am a bit confused about what topmost, rightmost, leftmost, and lowest are returning in the case where includeSelf is false. right and bottom are usually smaller numbers than top and left in those cases, which is odd. So this patch doesn't apply transforms when includeSelf is false in RenderBlock, which is almost certainly wrong.

[Dave Hyatt]

Comment on attachment 71246 [details]
Patch

This is really close.  The only bug I see now is that you shouldn't be passing the applyTransform argument to children.  That will turn off transforms in descendants, when the purpose of the enum was just to exclude your transform (not all transforms).  You always want to pass "IncludeTransform" for kids.

[Dave Hyatt]

(In reply to comment #32)
> I am a bit confused about what topmost, rightmost, leftmost, and lowest are returning in the case where includeSelf is false. right and bottom are usually smaller numbers than top and left in those cases, which is odd. So this patch doesn't apply transforms when includeSelf is false in RenderBlock, which is almost certainly wrong.

This is not wrong.  It's correct.  When people call the function with includeSelf = false they are wanting to compute their own scroll dimensions, so they don't want to apply the transform if they have one.

[Simon Fraser (smfr)]

Comment on attachment 71246 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=71246&action=review

> WebCore/rendering/RenderBlock.h:105
> -    virtual int topmostPosition(bool includeOverflowInterior = true, bool includeSelf = true) const;
> -    virtual int lowestPosition(bool includeOverflowInterior = true, bool includeSelf = true) const;
> -    virtual int rightmostPosition(bool includeOverflowInterior = true, bool includeSelf = true) const;
> -    virtual int leftmostPosition(bool includeOverflowInterior = true, bool includeSelf = true) const;
> +    virtual int topmostPosition(bool includeOverflowInterior = true, bool includeSelf = true, ApplyTransform = IncludeTransform) const;
> +    virtual int lowestPosition(bool includeOverflowInterior = true, bool includeSelf = true, ApplyTransform = IncludeTransform) const;
> +    virtual int rightmostPosition(bool includeOverflowInterior = true, bool includeSelf = true, ApplyTransform = IncludeTransform) const;
> +    virtual int leftmostPosition(bool includeOverflowInterior = true, bool includeSelf = true, ApplyTransform = IncludeTransform) const;

This would be cleaner using a single bitmask (and more self-documenting!)

[Beth Dakin]

Created attachment 71325 [details]
Patch

[Dave Hyatt]

Comment on attachment 71325 [details]
Patch

r=me

[Beth Dakin]

Committed with http://trac.webkit.org/changeset/70170

Thanks Dave!

[Beth Dakin]

I will file a follow-up bug about fixing LayoutTests/fast/transforms/scrollIntoView-transformed.html

I'm working on it now.

[WebKit Review Bot]

http://trac.webkit.org/changeset/70170 might have broken GTK Linux 64-bit Debug
The following tests are not passing:
fast/backgrounds/repeat/negative-offset-repeat-transformed.html
fast/borders/border-image-rotate-transform.html
fast/borders/border-image-scale-transform.html
fast/transforms/scrollIntoView-transformed.html
transforms/2d/hindi-rotated.html

[Beth Dakin]

(In reply to comment #40)
> http://trac.webkit.org/changeset/70170 might have broken GTK Linux 64-bit Debug
> The following tests are not passing:
> fast/backgrounds/repeat/negative-offset-repeat-transformed.html
> fast/borders/border-image-rotate-transform.html
> fast/borders/border-image-scale-transform.html
> fast/transforms/scrollIntoView-transformed.html
> transforms/2d/hindi-rotated.html

Doh! These need updated results.

[Simon Fraser (smfr)]

*** Bug 22766 has been marked as a duplicate of this bug. ***

[Simon Fraser (smfr)]

<rdar://problem/5667587>

[Nico Weber]

This caused https://bugs.webkit.org/show_bug.cgi?id=49220

[Kenneth Rohde Christiansen]

According to Luiz findings, this patch broke Google Images using iPhone UA.

https://bugs.webkit.org/show_bug.cgi?id=55641