Correct overflow propagation in BTT and RTL writing-modes

Correct overflow propagation in BTT and RTL writing-modes

Overflow rectangles are not quite physical, not quite logical. This
means that we cannot use clientBoxRect() directly to represent a
rectangle that expresses exactly no overflow. This rectangle is the
padding box (relative to the border box) in vertical-lr and
horizontal-tb, but the block-direction borders need to be flipped in
vertical-rl and horizontal-bt.

fast/multicol/vertical-rl/rules-with-border-before.html now needs a
rebaseline, because it now renders differently, but correctly.

R=
BUG=236326
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=154945

[mstensho (USE GERRIT), 2013-07-10 09:30:13]

I closed https://codereview.chromium.org/14359018/ and opened this one instead
(please go there if you want to know the reasoning for doing so).

[Julien - ping for review, 2013-07-15 23:50:32]

Sorry for the very slow response, feel free to ping us on IRC next time so that
your change gets the attention it deserves.

This is going in the right direction, some comments / suggestions.

https://codereview.chromium.org/18720003/diff/1/LayoutTests/fast/css/overflow...
File LayoutTests/fast/css/overflow-rtl-border-after.html (right):

https://codereview.chromium.org/18720003/diff/1/LayoutTests/fast/css/overflow...
LayoutTests/fast/css/overflow-rtl-border-after.html:11:
-webkit-border-after:150px solid white;
I would rather have the test use physical coordinates (and non-proprietary
extensions as we will need to deal with them at some point)

https://codereview.chromium.org/18720003/diff/1/LayoutTests/fast/css/overflow...
LayoutTests/fast/css/overflow-rtl-border-after.html:29: document.body.innerHTML
= logicalHeight == 200 ? "PASS" : "FAIL - logicalHeight was " + logicalHeight;
I think this should be a check-layout.js test. This would remove the need for
the custom JS code altogether:

<div id="inner" data-expected-scroll-width="200">HEST</div>

The bonus: it would also work outside of DumpRenderTree as this code wouldn't be
run as-is.

https://codereview.chromium.org/18720003/diff/1/Source/core/rendering/RenderB...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/18720003/diff/1/Source/core/rendering/RenderB...
Source/core/rendering/RenderBox.cpp:4420: // Calculate physical padding box.
I don't think this comment adds much.

https://codereview.chromium.org/18720003/diff/1/Source/core/rendering/RenderB...
Source/core/rendering/RenderBox.cpp:4421: LayoutRect rect(left, top, width() -
left - right, height() - top - bottom);
This is very much:

LayoutRect physicalBorderBoxRect = this->borderBoxRect();
physicalBorderBoxRect.move(borderLeft(), borderTop());

(Note: I have no clue why borderBoxRect() doesn't start at (borderLeft(),
borderTop()) apart that it really looks like a bug).

https://codereview.chromium.org/18720003/diff/1/Source/core/rendering/RenderB...
Source/core/rendering/RenderBox.cpp:4422: // Flip block progression axis if
writing mode is vertical-rl or horizontal-bt.
Same comment about not adding much (it repeats what the code is doing). The
other comments are great though.

[jbroman, 2013-07-16 01:18:46]

On 2013/07/15 23:50:32, Julien Chaffraix wrote:
> Sorry for the very slow response, feel free to ping us on IRC next time so
that
> your change gets the attention it deserves.
> 
> This is going in the right direction, some comments / suggestions.
> 
>
https://codereview.chromium.org/18720003/diff/1/LayoutTests/fast/css/overflow...
> File LayoutTests/fast/css/overflow-rtl-border-after.html (right):
> 
>
https://codereview.chromium.org/18720003/diff/1/LayoutTests/fast/css/overflow...
> LayoutTests/fast/css/overflow-rtl-border-after.html:11:
> -webkit-border-after:150px solid white;
> I would rather have the test use physical coordinates (and non-proprietary
> extensions as we will need to deal with them at some point)
> 
>
https://codereview.chromium.org/18720003/diff/1/LayoutTests/fast/css/overflow...
> LayoutTests/fast/css/overflow-rtl-border-after.html:29:
document.body.innerHTML
> = logicalHeight == 200 ? "PASS" : "FAIL - logicalHeight was " + logicalHeight;
> I think this should be a check-layout.js test. This would remove the need for
> the custom JS code altogether:
> 
> <div id="inner" data-expected-scroll-width="200">HEST</div>
> 
> The bonus: it would also work outside of DumpRenderTree as this code wouldn't
be
> run as-is.
> 
>
https://codereview.chromium.org/18720003/diff/1/Source/core/rendering/RenderB...
> File Source/core/rendering/RenderBox.cpp (right):
> 
>
https://codereview.chromium.org/18720003/diff/1/Source/core/rendering/RenderB...
> Source/core/rendering/RenderBox.cpp:4420: // Calculate physical padding box.
> I don't think this comment adds much.
> 
>
https://codereview.chromium.org/18720003/diff/1/Source/core/rendering/RenderB...
> Source/core/rendering/RenderBox.cpp:4421: LayoutRect rect(left, top, width() -
> left - right, height() - top - bottom);
> This is very much:
> 
> LayoutRect physicalBorderBoxRect = this->borderBoxRect();
> physicalBorderBoxRect.move(borderLeft(), borderTop());
> 
> (Note: I have no clue why borderBoxRect() doesn't start at (borderLeft(),
> borderTop()) apart that it really looks like a bug).
> 
>
https://codereview.chromium.org/18720003/diff/1/Source/core/rendering/RenderB...
> Source/core/rendering/RenderBox.cpp:4422: // Flip block progression axis if
> writing mode is vertical-rl or horizontal-bt.
> Same comment about not adding much (it repeats what the code is doing). The
> other comments are great though.

I've been working on a similar (slightly larger, but otherwise the same) change
for several days – so thanks for this, it saves me some debugging on my change.
I have but one drive-by comment (and a comment on jchaffraix's comment).

[jbroman, 2013-07-16 01:19:05]

https://codereview.chromium.org/18720003/diff/1/Source/core/rendering/RenderB...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/18720003/diff/1/Source/core/rendering/RenderB...
Source/core/rendering/RenderBox.cpp:4231:
m_overflow->setLayoutOverflow(borderBoxRect());
This should be noOverflowRect() as well.

https://codereview.chromium.org/18720003/diff/1/Source/core/rendering/RenderB...
Source/core/rendering/RenderBox.cpp:4421: LayoutRect rect(left, top, width() -
left - right, height() - top - bottom);
On 2013/07/15 23:50:33, Julien Chaffraix wrote:
> This is very much:
> 
> LayoutRect physicalBorderBoxRect = this->borderBoxRect();
> physicalBorderBoxRect.move(borderLeft(), borderTop());
> 
> (Note: I have no clue why borderBoxRect() doesn't start at (borderLeft(),
> borderTop()) apart that it really looks like a bug).

Comment-on-a-comment: the border box is the rect that surrounds the border, not
the rect surrounded by the border. This rect is the rect just within the border.

[mstensho (USE GERRIT), 2013-07-17 13:39:36]

Hi, thanks for the review, and sorry for the delay. I got so intrigued by one of
my own comments (about the scrollbars) in this patch, I had to investigate
further, and didn't stop until I realized that I was about to fix
https://bugs.webkit.org/show_bug.cgi?id=76129

It wouldn't be right to do that as part of this patch, though (I just improved
the comment and made the tests slightly more evil instead), but I think that
would be a natural next thing for me to do, unless that's related to what you're
already working on, jbroman.

https://codereview.chromium.org/18720003/diff/1/LayoutTests/fast/css/overflow...
File LayoutTests/fast/css/overflow-rtl-border-after.html (right):

https://codereview.chromium.org/18720003/diff/1/LayoutTests/fast/css/overflow...
LayoutTests/fast/css/overflow-rtl-border-after.html:11:
-webkit-border-after:150px solid white;
On 2013/07/15 23:50:33, Julien Chaffraix wrote:
> I would rather have the test use physical coordinates (and non-proprietary
> extensions as we will need to deal with them at some point)

Done.

https://codereview.chromium.org/18720003/diff/1/LayoutTests/fast/css/overflow...
LayoutTests/fast/css/overflow-rtl-border-after.html:29: document.body.innerHTML
= logicalHeight == 200 ? "PASS" : "FAIL - logicalHeight was " + logicalHeight;
On 2013/07/15 23:50:33, Julien Chaffraix wrote:
> I think this should be a check-layout.js test. This would remove the need for
> the custom JS code altogether:
> 
> <div id="inner" data-expected-scroll-width="200">HEST</div>
> 
> The bonus: it would also work outside of DumpRenderTree as this code wouldn't
be
> run as-is.

Done.

And thanks for the tip.

https://codereview.chromium.org/18720003/diff/1/Source/core/rendering/RenderB...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/18720003/diff/1/Source/core/rendering/RenderB...
Source/core/rendering/RenderBox.cpp:4231:
m_overflow->setLayoutOverflow(borderBoxRect());
On 2013/07/16 01:19:05, jbroman wrote:
> This should be noOverflowRect() as well.

Done.

https://codereview.chromium.org/18720003/diff/1/Source/core/rendering/RenderB...
Source/core/rendering/RenderBox.cpp:4420: // Calculate physical padding box.
On 2013/07/15 23:50:33, Julien Chaffraix wrote:
> I don't think this comment adds much.

Done.

https://codereview.chromium.org/18720003/diff/1/Source/core/rendering/RenderB...
Source/core/rendering/RenderBox.cpp:4421: LayoutRect rect(left, top, width() -
left - right, height() - top - bottom);
On 2013/07/16 01:19:05, jbroman wrote:
> On 2013/07/15 23:50:33, Julien Chaffraix wrote:
> > This is very much:
> > 
> > LayoutRect physicalBorderBoxRect = this->borderBoxRect();
> > physicalBorderBoxRect.move(borderLeft(), borderTop());
> > 
> > (Note: I have no clue why borderBoxRect() doesn't start at (borderLeft(),
> > borderTop()) apart that it really looks like a bug).
> 
> Comment-on-a-comment: the border box is the rect that surrounds the border,
not
> the rect surrounded by the border. This rect is the rect just within the
border.

A good method to use here would be paddingBoxRect(), but there are some
complications with scrollbars, due to
https://bugs.webkit.org/show_bug.cgi?id=76129 , so I think we're better off
building the rectangle on our own for now. See new patch regarding the
scrollbars; I've tried to explain it better.

https://codereview.chromium.org/18720003/diff/1/Source/core/rendering/RenderB...
Source/core/rendering/RenderBox.cpp:4422: // Flip block progression axis if
writing mode is vertical-rl or horizontal-bt.
On 2013/07/15 23:50:33, Julien Chaffraix wrote:
> Same comment about not adding much (it repeats what the code is doing). The
> other comments are great though.

Done.

https://codereview.chromium.org/18720003/diff/1/Source/core/rendering/RenderB...
Source/core/rendering/RenderBox.cpp:4424: // Subtract space occupied by
scrollbars. They are at their physical edge in this coordinate
Raising an issue myself. :)
I didn't understand this comment myself, so I took a closer look at what's going
on (which is why it took me so long to follow up on the review). What we do here
is strange, but it's all due to https://bugs.webkit.org/show_bug.cgi?id=76129

[jbroman, 2013-07-23 14:11:46]

Who is this change blocked on right now? It hasn't been touched in almost a
week.

[mstensho (USE GERRIT), 2013-07-23 14:20:45]

On 2013/07/23 14:11:46, jbroman wrote:
> Who is this change blocked on right now? It hasn't been touched in almost a
> week.

I'm waiting for review. I'd also like to ask you what you were referring to when
you said that you were working on something similar to this patch.

[jbroman, 2013-07-23 18:54:49]

On 2013/07/23 14:20:45, Morten Stenshorne wrote:
> I'm waiting for review. I'd also like to ask you what you were referring to
when
> you said that you were working on something similar to this patch.

While working in RenderOverflow, I ran into problems that needed to be fixed
(else my changes would break layout tests).

My patch did the following:
- introduces a box equivalent to your noOverflowRect (albeit with a different
name)
- fixes repaint rects (applyCachedClipAndScrollOffsetForRepaint doesn't seem
correct in the presence of flipped writing mode)
- fixes has{Horizontal,Vertical}LayoutOverflow, which are used for the
overflowchanged event
- remove a bunch of unused methods in RenderOverflow (this is totally tangential
to the above)

I didn't get the first part quite correct (I was having trouble working out why
the scrollbar removing-and-adding trick was necessary), which is why I hadn't
cleaned/mailed it for review by the time Julien told me about your patch. That
(unpolished) change is at http://crrev.com/17722002.

[mstensho (USE GERRIT), 2013-07-23 19:20:54]

On 2013/07/23 18:54:49, jbroman wrote:

> (unpolished) change is at http://crrev.com/17722002.

Thanks! So is your plan to finish it when my patch has landed?

[jbroman, 2013-07-23 19:21:36]

On 2013/07/23 19:20:54, Morten Stenshorne wrote:
> On 2013/07/23 18:54:49, jbroman wrote:
> 
> > (unpolished) change is at http://crrev.com/17722002.
> 
> Thanks! So is your plan to finish it when my patch has landed?

Yeah, I'll rebase and then send out the remaining bits for review. :)

[mstensho (USE GERRIT), 2013-07-23 19:42:55]

On 2013/07/23 19:21:36, jbroman wrote:
> On 2013/07/23 19:20:54, Morten Stenshorne wrote:
> > On 2013/07/23 18:54:49, jbroman wrote:
> > 
> > > (unpolished) change is at http://crrev.com/17722002.
> > 
> > Thanks! So is your plan to finish it when my patch has landed?
> 
> Yeah, I'll rebase and then send out the remaining bits for review. :)

Great! I've CCed myself there now. :)

[Julien - ping for review, 2013-07-24 22:22:28]

lgtm

https://codereview.chromium.org/18720003/diff/11001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/18720003/diff/11001/Source/core/rendering/Ren...
Source/core/rendering/RenderBox.cpp:4461: LayoutRect RenderBox::noOverflowRect()
const
Not totally satisfied with this name as it doesn't explicitly state which box we
return or which coordinate system it is in but I couldn't come with a better
one. Though the box is easy (client box), I don't think we have a name for this
coordinate system so it would be tricky to put it in the name.

Btw, this requires a FIXME in this function as we would really like to infer
this box from clientBoxRect.

[mstensho (USE GERRIT), 2013-07-25 12:36:39]

https://codereview.chromium.org/18720003/diff/11001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/18720003/diff/11001/Source/core/rendering/Ren...
Source/core/rendering/RenderBox.cpp:4461: LayoutRect RenderBox::noOverflowRect()
const
On 2013/07/24 22:22:28, Julien Chaffraix wrote:
> Not totally satisfied with this name as it doesn't explicitly state which box
we
> return or which coordinate system it is in but I couldn't come with a better
> one. Though the box is easy (client box), I don't think we have a name for
this
> coordinate system so it would be tricky to put it in the name.

I'm not impressed by the name myself. :) But yeah, I couldn't come up with
anything that sucked less. This type of coordinate system certainly deserves a
name. It's widely used throughout the code, e.g. in RenderBox::m_frameRect.

> Btw, this requires a FIXME in this function as we would really like to infer
> this box from clientBoxRect.

Well... or from paddingBoxRect(). I don't understand why we use clientBoxRect()
and other methods marked with "More IE extensions" internally. But in order to
be able to use any of these, we probably have to fix
https://bugs.webkit.org/show_bug.cgi?id=76129 first.

I've added a FIXME.

[mstensho (USE GERRIT), 2013-07-25 12:58:22]

I'm trying to get this one committed, but it doesn't seem to work. Is it because
I added the FIXME after I got the lgtm? It was my understanding that one was
allowed to make such minor changes in cases where the patch was lgtmed with
minor issues (without waiting for another lgtm), but maybe that's not true.

[jbroman, 2013-07-25 13:27:59]

On 2013/07/25 12:58:22, Morten Stenshorne wrote:
> I'm trying to get this one committed, but it doesn't seem to work. Is it
because
> I added the FIXME after I got the lgtm? It was my understanding that one was
> allowed to make such minor changes in cases where the patch was lgtmed with
> minor issues (without waiting for another lgtm), but maybe that's not true.

That's odd...CQ think the commit box was unchecked.
https://chromium-status.appspot.com/cq/mstensho@opera.com/18720003

[mstensho (USE GERRIT), 2013-07-25 13:32:51]

On 2013/07/25 13:27:59, jbroman wrote:
> On 2013/07/25 12:58:22, Morten Stenshorne wrote:
> > I'm trying to get this one committed, but it doesn't seem to work. Is it
> because
> > I added the FIXME after I got the lgtm? It was my understanding that one was
> > allowed to make such minor changes in cases where the patch was lgtmed with
> > minor issues (without waiting for another lgtm), but maybe that's not true.
> 
> That's odd...CQ think the commit box was unchecked.
> https://chromium-status.appspot.com/cq/mstensho%40opera.com/18720003

I think that message always appears. At least for me:

https://chromium-status.appspot.com/cq/mstensho@opera.com

[commit-bot: I haz the power, 2013-07-25 18:42:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mstensho@opera.com/18720003/22001

[commit-bot: I haz the power, 2013-07-25 23:28:07]

Change committed as 154945