Fix scroll origin, overflow rects, and coordinate flipping for flexbox.

Fix scroll origin, overflow rects, and coordinate flipping  for flexbox.

PaintLayerScrollableArea uses a scroll origin to offset its scroll
position for RTL blocks, to ensure that a zero scroll position
always represents being scrolled all the way to the logical beginning
of the block.  This patch fixes scroll origin calculations for
writing-mode:vertical-rl and for row-reverse and column-reverse flex
directions.

It also fixes how overflow rects are calculated for flex containers,
by adding hasTopOverflow and hasLeftOverflow virtual methods on
LayoutBox, with appropriate overrides on LayoutFlexibleBox.

Finally, it has a couple of miscellaneous fixes for coordinate-flipping
for reverse flow.

BUG=webkit:76129
Committed: https://crrev.com/e8eed9fe26988a96498c92029b98dc980e5d15e1
Cr-Commit-Position: refs/heads/master@{#392981}

[szager1, 2016-04-28 21:30:07]

Description was changed from

==========
Fix scroll origin and scrolled content offset calculations.

PaintLayerScrollableArea uses a scroll origin to offset its scroll
position for RTL blocks, to ensure that a zero scroll position
always represents being scrolled all the way to the logical beginning
of the block.  This patch fixes scroll origin calculations for
writing-mode:vertical-rl and for row-reverse and column-reverse flex
directions.

BUG=webkit:76129
==========

to

==========
Fix scroll origin and scrolled content offset calculations.

PaintLayerScrollableArea uses a scroll origin to offset its scroll
position for RTL blocks, to ensure that a zero scroll position
always represents being scrolled all the way to the logical beginning
of the block.  This patch fixes scroll origin calculations for
writing-mode:vertical-rl and for row-reverse and column-reverse flex
directions.

BUG=webkit:76129
==========

[szager1, 2016-04-28 21:30:07]

szager@chromium.org changed reviewers:
+ cbiesinger@chromium.org, eae@chromium.org, leviw@chromium.org

[leviw_travelin_and_unemployed, 2016-04-28 22:12:12]

https://codereview.chromium.org/1921553008/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1921553008/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:960: if
(isHorizontalWritingMode() && shouldPlaceBlockDirectionScrollbarOnLogicalLeft())
Probably deserves a comment.

https://codereview.chromium.org/1921553008/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/1921553008/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:590: IntSize
PaintLayerScrollableArea::originAdjustmentForScrollbars() const
This deserves a big description about why it exists and what the cases are. I
know that's a bit of a pain to write up, but I don't want the arcane knowledge
you acquired to just be cargo-culted forever :)

https://codereview.chromium.org/1921553008/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:597: if
(box().isFlexibleBox()) {
It seems like it may be easier/clearer to handle the FlexBox case first and
separately (maybe even in another function that describes why it's different),
then the  non-flex case should seem easier.

The two intertwined like this makes it harder to understand the different cases.

[cbiesinger, 2016-04-29 20:16:52]

What Levi said, but otherwise looks good to me. Feel free to submit once he's
happy :-)

[eae, 2016-05-01 01:51:23]

As long as Levi is happy I'm happy :)

[szager1, 2016-05-09 23:18:36]

Description was changed from

==========
Fix scroll origin and scrolled content offset calculations.

PaintLayerScrollableArea uses a scroll origin to offset its scroll
position for RTL blocks, to ensure that a zero scroll position
always represents being scrolled all the way to the logical beginning
of the block.  This patch fixes scroll origin calculations for
writing-mode:vertical-rl and for row-reverse and column-reverse flex
directions.

BUG=webkit:76129
==========

to

==========
Fix scroll origin and overflow coordinates for flexbox.

PaintLayerScrollableArea uses a scroll origin to offset its scroll
position for RTL blocks, to ensure that a zero scroll position
always represents being scrolled all the way to the logical beginning
of the block.  This patch fixes scroll origin calculations for
writing-mode:vertical-rl and for row-reverse and column-reverse flex
directions.

BUG=webkit:76129
==========

[szager1, 2016-05-10 00:15:48]

Description was changed from

==========
Fix scroll origin and overflow coordinates for flexbox.

PaintLayerScrollableArea uses a scroll origin to offset its scroll
position for RTL blocks, to ensure that a zero scroll position
always represents being scrolled all the way to the logical beginning
of the block.  This patch fixes scroll origin calculations for
writing-mode:vertical-rl and for row-reverse and column-reverse flex
directions.

BUG=webkit:76129
==========

to

==========
Fix scroll origin, overflow rects, and coordinate flipping  for flexbox.

PaintLayerScrollableArea uses a scroll origin to offset its scroll
position for RTL blocks, to ensure that a zero scroll position
always represents being scrolled all the way to the logical beginning
of the block.  This patch fixes scroll origin calculations for
writing-mode:vertical-rl and for row-reverse and column-reverse flex
directions.

It also fixes how overflow rects are calculated for flex containers, by adding
hasTopOverflow and hasLeftOverflow virtual methods on LayoutBox, with
appropriate overrides on LayoutFlexibleBox.

Finally, it has a couple of miscellaneous fixes for coordinate-flipping for
reverse flow.

BUG=webkit:76129
==========

[szager1, 2016-05-10 00:17:10]

Well, that escalated quickly.

This patch should be an authoritative treatment of scrollbar adjustments for
flexbox.  The test case should keep us honest going forward.

PTAL

[leviw_travelin_and_unemployed, 2016-05-10 01:57:06]

Guest review! ;)

https://codereview.chromium.org/1921553008/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/1921553008/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.h:383: virtual bool
hasTopOverflow() const;
May be worth a comment about when this would be true?

Also, it appears from the code that the question is really "does this overflow
to the left/top" not "does this *have* left/top overflow," no?

[leviw_travelin_and_unemployed, 2016-05-10 01:57:30]

Nit: Your description also doesn't wrap in the 2nd paragraph.

[szager1, 2016-05-10 06:30:01]

Description was changed from

==========
Fix scroll origin, overflow rects, and coordinate flipping  for flexbox.

PaintLayerScrollableArea uses a scroll origin to offset its scroll
position for RTL blocks, to ensure that a zero scroll position
always represents being scrolled all the way to the logical beginning
of the block.  This patch fixes scroll origin calculations for
writing-mode:vertical-rl and for row-reverse and column-reverse flex
directions.

It also fixes how overflow rects are calculated for flex containers, by adding
hasTopOverflow and hasLeftOverflow virtual methods on LayoutBox, with
appropriate overrides on LayoutFlexibleBox.

Finally, it has a couple of miscellaneous fixes for coordinate-flipping for
reverse flow.

BUG=webkit:76129
==========

to

==========
Fix scroll origin, overflow rects, and coordinate flipping  for flexbox.

PaintLayerScrollableArea uses a scroll origin to offset its scroll
position for RTL blocks, to ensure that a zero scroll position
always represents being scrolled all the way to the logical beginning
of the block.  This patch fixes scroll origin calculations for
writing-mode:vertical-rl and for row-reverse and column-reverse flex
directions.

It also fixes how overflow rects are calculated for flex containers,
by adding hasTopOverflow and hasLeftOverflow virtual methods on LayoutBox,
with appropriate overrides on LayoutFlexibleBox.

Finally, it has a couple of miscellaneous fixes for coordinate-flipping
for reverse flow.

BUG=webkit:76129
==========

[szager1, 2016-05-10 06:30:47]

Description was changed from

==========
Fix scroll origin, overflow rects, and coordinate flipping  for flexbox.

PaintLayerScrollableArea uses a scroll origin to offset its scroll
position for RTL blocks, to ensure that a zero scroll position
always represents being scrolled all the way to the logical beginning
of the block.  This patch fixes scroll origin calculations for
writing-mode:vertical-rl and for row-reverse and column-reverse flex
directions.

It also fixes how overflow rects are calculated for flex containers,
by adding hasTopOverflow and hasLeftOverflow virtual methods on LayoutBox,
with appropriate overrides on LayoutFlexibleBox.

Finally, it has a couple of miscellaneous fixes for coordinate-flipping
for reverse flow.

BUG=webkit:76129
==========

to

==========
Fix scroll origin, overflow rects, and coordinate flipping  for flexbox.

PaintLayerScrollableArea uses a scroll origin to offset its scroll
position for RTL blocks, to ensure that a zero scroll position
always represents being scrolled all the way to the logical beginning
of the block.  This patch fixes scroll origin calculations for
writing-mode:vertical-rl and for row-reverse and column-reverse flex
directions.

It also fixes how overflow rects are calculated for flex containers,
by adding hasTopOverflow and hasLeftOverflow virtual methods on
LayoutBox, with appropriate overrides on LayoutFlexibleBox.

Finally, it has a couple of miscellaneous fixes for coordinate-flipping
for reverse flow.

BUG=webkit:76129
==========

[szager1, 2016-05-10 07:33:09]

Well, look who's working for free now!

I added some comments to the source, and some docs with ascii art to README.md.

https://codereview.chromium.org/1921553008/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/1921553008/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.h:383: virtual bool
hasTopOverflow() const;
On 2016/05/10 01:57:06, leviw_travelin_and_unemployed wrote:
> May be worth a comment about when this would be true?
> 
> Also, it appears from the code that the question is really "does this overflow
> to the left/top" not "does this *have* left/top overflow," no?

Added a comment.

[leviw_travelin_and_unemployed, 2016-05-10 17:52:43]

LGTM dude! This is amazing work. Work so good I'd review it for free!

https://codereview.chromium.org/1921553008/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/css3/flexbox/flexbox-overflow-auto-expected.html
(left):

https://codereview.chromium.org/1921553008/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/css3/flexbox/flexbox-overflow-auto-expected.html:70:
Array.prototype.forEach.call(document.querySelectorAll(".right-almost"),
function(element) {
Squeeee!

https://codereview.chromium.org/1921553008/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/1921553008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.h:387: virtual bool
hasTopOverflow() const;
The name still strikes me as confusing (seems like if we called them
overflowFlowsLeft/Top or something you may not need as much comment), but I
won't block an otherwise amazing patch over this.

https://codereview.chromium.org/1921553008/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp (right):

https://codereview.chromium.org/1921553008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:256: if
(flexDirection == FlowRow) {
This is super easy to follow now, but I can totally see how it was hard to get
to!

[leviw_travelin_and_unemployed, 2016-05-10 17:54:04]

One more thought: is the old flexbox-overflow-auto redundant now? Doesn't your
new test cover everything it did and more? May be worth just deleting the old
one.

[szager1, 2016-05-10 23:01:18]

The CQ bit was checked by szager@chromium.org

[szager1, 2016-05-10 23:01:51]

On 2016/05/10 17:54:04, leviw_travelin_and_unemployed wrote:
> One more thought: is the old flexbox-overflow-auto redundant now? Doesn't your
> new test cover everything it did and more? May be worth just deleting the old
> one.

Actually, I did at one point end up in a state where the new test passed, but
flexbox-overflow-auto failed, so I'm gonna keep it for now.

[commit-bot: I haz the power, 2016-05-10 23:02:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1921553008/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1921553008/100001

[commit-bot: I haz the power, 2016-05-11 03:33:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-11 03:33:55]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[szager1, 2016-05-11 17:45:16]

The CQ bit was checked by szager@chromium.org

[commit-bot: I haz the power, 2016-05-11 17:45:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1921553008/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1921553008/100001

[commit-bot: I haz the power, 2016-05-11 17:51:51]

Description was changed from

==========
Fix scroll origin, overflow rects, and coordinate flipping  for flexbox.

PaintLayerScrollableArea uses a scroll origin to offset its scroll
position for RTL blocks, to ensure that a zero scroll position
always represents being scrolled all the way to the logical beginning
of the block.  This patch fixes scroll origin calculations for
writing-mode:vertical-rl and for row-reverse and column-reverse flex
directions.

It also fixes how overflow rects are calculated for flex containers,
by adding hasTopOverflow and hasLeftOverflow virtual methods on
LayoutBox, with appropriate overrides on LayoutFlexibleBox.

Finally, it has a couple of miscellaneous fixes for coordinate-flipping
for reverse flow.

BUG=webkit:76129
==========

to

==========
Fix scroll origin, overflow rects, and coordinate flipping  for flexbox.

PaintLayerScrollableArea uses a scroll origin to offset its scroll
position for RTL blocks, to ensure that a zero scroll position
always represents being scrolled all the way to the logical beginning
of the block.  This patch fixes scroll origin calculations for
writing-mode:vertical-rl and for row-reverse and column-reverse flex
directions.

It also fixes how overflow rects are calculated for flex containers,
by adding hasTopOverflow and hasLeftOverflow virtual methods on
LayoutBox, with appropriate overrides on LayoutFlexibleBox.

Finally, it has a couple of miscellaneous fixes for coordinate-flipping
for reverse flow.

BUG=webkit:76129
==========

[commit-bot: I haz the power, 2016-05-11 17:51:54]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-05-11 17:53:12]

Description was changed from

==========
Fix scroll origin, overflow rects, and coordinate flipping  for flexbox.

PaintLayerScrollableArea uses a scroll origin to offset its scroll
position for RTL blocks, to ensure that a zero scroll position
always represents being scrolled all the way to the logical beginning
of the block.  This patch fixes scroll origin calculations for
writing-mode:vertical-rl and for row-reverse and column-reverse flex
directions.

It also fixes how overflow rects are calculated for flex containers,
by adding hasTopOverflow and hasLeftOverflow virtual methods on
LayoutBox, with appropriate overrides on LayoutFlexibleBox.

Finally, it has a couple of miscellaneous fixes for coordinate-flipping
for reverse flow.

BUG=webkit:76129
==========

to

==========
Fix scroll origin, overflow rects, and coordinate flipping  for flexbox.

PaintLayerScrollableArea uses a scroll origin to offset its scroll
position for RTL blocks, to ensure that a zero scroll position
always represents being scrolled all the way to the logical beginning
of the block.  This patch fixes scroll origin calculations for
writing-mode:vertical-rl and for row-reverse and column-reverse flex
directions.

It also fixes how overflow rects are calculated for flex containers,
by adding hasTopOverflow and hasLeftOverflow virtual methods on
LayoutBox, with appropriate overrides on LayoutFlexibleBox.

Finally, it has a couple of miscellaneous fixes for coordinate-flipping
for reverse flow.

BUG=webkit:76129

Committed: https://crrev.com/e8eed9fe26988a96498c92029b98dc980e5d15e1
Cr-Commit-Position: refs/heads/master@{#392981}
==========

[commit-bot: I haz the power, 2016-05-11 17:53:13]

Patchset 6 (id:??) landed as
https://crrev.com/e8eed9fe26988a96498c92029b98dc980e5d15e1
Cr-Commit-Position: refs/heads/master@{#392981}

[cbiesinger, 2016-05-11 19:44:00]

I'm a little late to this patch, and I only have two comments:

1) Thanks so much for doing all this!! This code makes a lot more sense now and
I'm glad we got to fix an actual bug here too. Also thanks for writing the
readme!

2) I agree with Levi re the name... maybe we can rename it in a separate patch?
See suggestion below.

https://codereview.chromium.org/1921553008/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/1921553008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.h:383: // These methods don't
mean the box *actually* has top/left overflow.  They mean that
maybe rename to canOverflowTo{Top,Left}?

[cbiesinger, 2016-05-11 19:44:15]

to be clear: lgtm :)