Bug 47514

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.

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.

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.

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 :)

(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.

(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.

(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)

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.

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.

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.

Created attachment 70778 [details] Patch to add topmostPosition() This patch lays the groundwork for fixing this bug in the way the Hyatt suggested.

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.

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

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.

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

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.

(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.

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.

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 }

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.

Comment on attachment 70790 [details] Patch to add topmostPosition() r=me!

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.

Created attachment 70985 [details] Pacth

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.

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

Created attachment 70986 [details] Patch Oops! Fixing that build error.

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

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

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).

(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.

Created attachment 71246 [details] Patch

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.

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.

(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.

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!)

Created attachment 71325 [details] Patch

Comment on attachment 71325 [details] Patch r=me

Committed with http://trac.webkit.org/changeset/70170 Thanks Dave!

I will file a follow-up bug about fixing LayoutTests/fast/transforms/scrollIntoView-transformed.html I'm working on it now.

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

(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.

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

<rdar://problem/5667587>

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

According to Luiz findings, this patch broke Google Images using iPhone UA. https://bugs.webkit.org/show_bug.cgi?id=55641