Repaint issues with vertical text which has absolute (or fixed) position specified for it.

Repaint issues with vertical text which has absolute (or fixed) position specified for it.

For a writing mode root editable box which is absolute (or fixed) positioned
and which contains vertically aligned text within, trying to either insert
a paragraph in-between the text or trying to select part of the text does
not visually manifest the expected behavior.
An extra repaint is required to demonstrate the expected behavior.

This occurs because the repaint rect being computed in case of absolutely or
fixed positioned writing mode editable roots is not flipping the rect as per
the writing mode.

Removed the extra condition check for writing mode root box that flipped the
computed rect (based on the writing mode) only if the renderer was not absolute (or fixed) positioned.

R=leviw@chromium.org,eseidel@chromium.org,ojan@chromium.org
BUG=259352, 230580
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177870

[ojan, 2013-07-15 23:54:12]

The C++ change seems reasonable assuming the trybots say all the tests pass.
Please minimize the testcase and remove the setTimeouts though.

https://codereview.chromium.org/18254006/diff/1/LayoutTests/editing/inserting...
File
LayoutTests/editing/inserting/insert-paragraph-between-vertical-text-with-positioned-root.html
(right):

https://codereview.chromium.org/18254006/diff/1/LayoutTests/editing/inserting...
LayoutTests/editing/inserting/insert-paragraph-between-vertical-text-with-positioned-root.html:16:
outline-style: none;
This isn't needed, right?

https://codereview.chromium.org/18254006/diff/1/LayoutTests/editing/inserting...
LayoutTests/editing/inserting/insert-paragraph-between-vertical-text-with-positioned-root.html:20:
line-height: 1;
Are the font-size and line-height needed?

https://codereview.chromium.org/18254006/diff/1/LayoutTests/editing/inserting...
LayoutTests/editing/inserting/insert-paragraph-between-vertical-text-with-positioned-root.html:30:
window.setTimeout(function() {
Why is this setTimeout needed?

https://codereview.chromium.org/18254006/diff/1/LayoutTests/editing/inserting...
LayoutTests/editing/inserting/insert-paragraph-between-vertical-text-with-positioned-root.html:36:
window.setTimeout(logRepaints, 200);
I think you can avoid this setTimeout by calling document.body.offsetWidth to
force a layout and then running logRepaints synchronously. setTimeouts are
invariably a cause of test flakiness and should be avoided except where
absolutely necessary. Also, they make for slower running tests.

https://codereview.chromium.org/18254006/diff/1/LayoutTests/editing/inserting...
LayoutTests/editing/inserting/insert-paragraph-between-vertical-text-with-positioned-root.html:63:
<p id="console"></p>
The console and description elements are not needed. js-test-pre will insert
them for you.

[arpitab_, 2013-08-08 14:22:30]

https://codereview.chromium.org/18254006/diff/1/LayoutTests/editing/inserting...
File
LayoutTests/editing/inserting/insert-paragraph-between-vertical-text-with-positioned-root.html
(right):

https://codereview.chromium.org/18254006/diff/1/LayoutTests/editing/inserting...
LayoutTests/editing/inserting/insert-paragraph-between-vertical-text-with-positioned-root.html:16:
outline-style: none;
On 2013/07/15 23:54:12, ojan wrote:
> This isn't needed, right?
Yes, and I shall remove the same in the next patch.

https://codereview.chromium.org/18254006/diff/1/LayoutTests/editing/inserting...
LayoutTests/editing/inserting/insert-paragraph-between-vertical-text-with-positioned-root.html:20:
line-height: 1;
On 2013/07/15 23:54:12, ojan wrote:
> Are the font-size and line-height needed?
I was trying to make a platform independent testcase however, simply using the
ahem font doesn't help.
R Niwa suggested to specify the font-size explicitly but am afraid even that
doesn't help in making this testcase platform independent as the repaint rects
on different platforms don't match.

Would really appreciate your help with this, if you have any pointers on how to
make this testcase platform independent.

https://codereview.chromium.org/18254006/diff/1/LayoutTests/editing/inserting...
LayoutTests/editing/inserting/insert-paragraph-between-vertical-text-with-positioned-root.html:30:
window.setTimeout(function() {
On 2013/07/15 23:54:12, ojan wrote:
> Why is this setTimeout needed?
This particular setTimeout can be removed.

https://codereview.chromium.org/18254006/diff/1/LayoutTests/editing/inserting...
LayoutTests/editing/inserting/insert-paragraph-between-vertical-text-with-positioned-root.html:36:
window.setTimeout(logRepaints, 200);
On 2013/07/15 23:54:12, ojan wrote:
> I think you can avoid this setTimeout by calling document.body.offsetWidth to
> force a layout and then running logRepaints synchronously. setTimeouts are
> invariably a cause of test flakiness and should be avoided except where
> absolutely necessary. Also, they make for slower running tests.
This setTimeout is required since we need some time interval for capturing the
generated repaintRects (triggered by the window.testRunner.display() call which
shall force a layout).
If logRepaints is called synchronously, no repaint rects are captured.

https://codereview.chromium.org/18254006/diff/1/LayoutTests/editing/inserting...
LayoutTests/editing/inserting/insert-paragraph-between-vertical-text-with-positioned-root.html:63:
<p id="console"></p>
On 2013/07/15 23:54:12, ojan wrote:
> The console and description elements are not needed. js-test-pre will insert
> them for you.
I shall remove the same in the next patch.

[yosin_UTC9, 2013-09-10 02:20:14]

https://codereview.chromium.org/18254006/diff/1/LayoutTests/editing/inserting...
File
LayoutTests/editing/inserting/insert-paragraph-between-vertical-text-with-positioned-root.html
(right):

https://codereview.chromium.org/18254006/diff/1/LayoutTests/editing/inserting...
LayoutTests/editing/inserting/insert-paragraph-between-vertical-text-with-positioned-root.html:36:
window.setTimeout(logRepaints, 200);
Can we call logRepaints until test condition satisfied rather than checking
once? On heavy load bot, 200ms is too short for completing change.

For example:

window.setTimeout(check, 200);

function check() 
{
  repaintRects = window.internals.repaintRectsAsText(document);
  if (repaintRects.indexOf('177 343') == -1) {
      window.setTimeout(check, 200);
      return;
  }
  
  testPassed('Repaint succeeded');
  finishJSTest();
}

[ojan, 2013-09-12 23:40:15]

https://codereview.chromium.org/18254006/diff/1/LayoutTests/editing/inserting...
File
LayoutTests/editing/inserting/insert-paragraph-between-vertical-text-with-positioned-root.html
(right):

https://codereview.chromium.org/18254006/diff/1/LayoutTests/editing/inserting...
LayoutTests/editing/inserting/insert-paragraph-between-vertical-text-with-positioned-root.html:20:
line-height: 1;
On 2013/08/08 14:22:30, arpitab_ wrote:
> On 2013/07/15 23:54:12, ojan wrote:
> > Are the font-size and line-height needed?
> I was trying to make a platform independent testcase however, simply using the
> ahem font doesn't help.
> R Niwa suggested to specify the font-size explicitly but am afraid even that
> doesn't help in making this testcase platform independent as the repaint rects
> on different platforms don't match.
> 
> Would really appreciate your help with this, if you have any pointers on how
to
> make this testcase platform independent. 

Instead of using text in your page, you can use fixed size inline-blocks, e.g.:

<div class=textArea contentEditable>
<div style="display:inline-block; width: 100px; height: 20px"></div>
<div style="display:inline-block; width: 100px; height: 20px"></div>
<div style="display:inline-block; width: 100px; height: 20px"></div>
</div>

If there's no text, the test should be platform independent.

https://codereview.chromium.org/18254006/diff/1/LayoutTests/editing/inserting...
LayoutTests/editing/inserting/insert-paragraph-between-vertical-text-with-positioned-root.html:36:
window.setTimeout(logRepaints, 200);
On 2013/09/10 02:20:15, Yoshifumi Inoue wrote:
> Can we call logRepaints until test condition satisfied rather than checking
> once? On heavy load bot, 200ms is too short for completing change.
> 
> For example:
> 
> window.setTimeout(check, 200);
> 
> function check() 
> {
>   repaintRects = window.internals.repaintRectsAsText(document);
>   if (repaintRects.indexOf('177 343') == -1) {
>       window.setTimeout(check, 200);
>       return;
>   }
>   
>   testPassed('Repaint succeeded');
>   finishJSTest();
> }

Something like this would be good. Even better would be to use a timeout of 0 so
that this test is as fast as possible.

[arpitabahuguna, 2014-07-01 09:04:23]

@leviw, @ojan, @yosin have uploaded an updated patch for this issue.

The repaint layout test has been revised using the new text based harness
in LayoutTests/fast/repaint/resources/text-based-repaint.js

Please review the same.

[arpitabahuguna, 2014-07-03 04:45:55]

ping..

[arpitab_, 2014-07-03 08:13:33]

Sorry, I forgot to add an entry for the layout testcase in the TestExpectations
file.
Uploaded another patch with that change.
PTAL.

[yosin_UTC9, 2014-07-04 01:13:09]

LGTM

Please wait for OWNERS review for committing.

https://codereview.chromium.org/18254006/diff/38001/LayoutTests/editing/inser...
File
LayoutTests/editing/inserting/insert-paragraph-between-text-with-positioned-root.html
(right):

https://codereview.chromium.org/18254006/diff/38001/LayoutTests/editing/inser...
LayoutTests/editing/inserting/insert-paragraph-between-text-with-positioned-root.html:20:
var testElement = document.getElementById('test');
nit: Please use double-quote because other parts of JavaScript use double-quote.

[arpitab_, 2014-07-04 08:53:51]

Thank-you for the review @Yosin! Have uploaded another patch that takes care of
the nit.

Shall ask for owner reviews.

[arpitab_, 2014-07-04 08:55:57]

@ojan, @eseidel This needs an owner's review. Please take a look.

[arpitab_, 2014-07-08 05:15:22]

Awaiting an owner's review.

[eseidel, 2014-07-08 15:45:20]

leviw is really the correct reviewer, but it seems he's out.

lgtm.

[ojan, 2014-07-08 23:53:30]

https://codereview.chromium.org/18254006/diff/58001/LayoutTests/editing/inser...
File
LayoutTests/editing/inserting/insert-paragraph-between-text-with-positioned-root.html
(right):

https://codereview.chromium.org/18254006/diff/58001/LayoutTests/editing/inser...
LayoutTests/editing/inserting/insert-paragraph-between-text-with-positioned-root.html:3:
<head>
Please remove the html and head element as this test doesn't depend on them.

https://codereview.chromium.org/18254006/diff/58001/LayoutTests/editing/inser...
LayoutTests/editing/inserting/insert-paragraph-between-text-with-positioned-root.html:4:
<script src="../../fast/repaint/resources/text-based-repaint.js"
type="text/javascript"></script>
No need for the type attribute.

https://codereview.chromium.org/18254006/diff/58001/LayoutTests/editing/inser...
LayoutTests/editing/inserting/insert-paragraph-between-text-with-positioned-root.html:7:
position: absolute; left: 0px; top: 200px;
Please put each of these on their own line.

https://codereview.chromium.org/18254006/diff/58001/LayoutTests/editing/inser...
LayoutTests/editing/inserting/insert-paragraph-between-text-with-positioned-root.html:26:
document.execCommand("InsertParagraph");
execCommands and selections are complicated and make for a hard to maintain
test. Can you make a test case that doesn't depend on selections or execCommand?
It should be possible to create one that does the same thing with regular DOM
operations.

[leviw_travelin_and_unemployed, 2014-07-09 00:34:18]

On 2014/07/08 at 23:53:30, ojan wrote:
>
https://codereview.chromium.org/18254006/diff/58001/LayoutTests/editing/inser...
>
LayoutTests/editing/inserting/insert-paragraph-between-text-with-positioned-root.html:26:
document.execCommand("InsertParagraph");
> execCommands and selections are complicated and make for a hard to maintain
test. Can you make a test case that doesn't depend on selections or execCommand?
It should be possible to create one that does the same thing with regular DOM
operations.

Sorry to be late to the party. If this can be demonstrated with selection
repaint as the description seems to suggest, why not do that?

Also, I'd change the description to specifically talk about what the code was
doing incorrectly instead of specifically mentioning that "insert[ing] a
paragraph in-between the text" tickles the bug, as I'm sure there are any number
of ways to trigger this repaint bug.

[arpitab_, 2014-07-09 13:57:26]

Thanks for the review ojan and leviw!
Have uploaded another patch incorporating the review comments.

Have modified the testcase to a selection based one and edited the name to more
reflect the issue.

PTAL.

https://codereview.chromium.org/18254006/diff/58001/LayoutTests/editing/inser...
File
LayoutTests/editing/inserting/insert-paragraph-between-text-with-positioned-root.html
(right):

https://codereview.chromium.org/18254006/diff/58001/LayoutTests/editing/inser...
LayoutTests/editing/inserting/insert-paragraph-between-text-with-positioned-root.html:3:
<head>
On 2014/07/08 23:53:30, ojan-only-code-yellow-reviews wrote:
> Please remove the html and head element as this test doesn't depend on them.

Done.

https://codereview.chromium.org/18254006/diff/58001/LayoutTests/editing/inser...
LayoutTests/editing/inserting/insert-paragraph-between-text-with-positioned-root.html:4:
<script src="../../fast/repaint/resources/text-based-repaint.js"
type="text/javascript"></script>
On 2014/07/08 23:53:30, ojan-only-code-yellow-reviews wrote:
> No need for the type attribute.

Done.

https://codereview.chromium.org/18254006/diff/58001/LayoutTests/editing/inser...
LayoutTests/editing/inserting/insert-paragraph-between-text-with-positioned-root.html:7:
position: absolute; left: 0px; top: 200px;
On 2014/07/08 23:53:30, ojan-only-code-yellow-reviews wrote:
> Please put each of these on their own line.

Done.

[ojan, 2014-07-09 17:23:08]

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

https://codereview.chromium.org/18254006/diff/78001/Source/core/rendering/Ren...
Source/core/rendering/RenderBox.cpp:2025: if (isWritingModeRoot() &&
style()->isFlippedBlocksWritingMode())
flipForWritingMode already has an early return for
style()->isFlippedBlocksWritingMode(), right? So it doesn't need to be checked
here.

[arpitab_, 2014-07-10 13:17:15]

Thanks for the review ojan. Have uploaded another patch. PTAL

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

https://codereview.chromium.org/18254006/diff/78001/Source/core/rendering/Ren...
Source/core/rendering/RenderBox.cpp:2025: if (isWritingModeRoot() &&
style()->isFlippedBlocksWritingMode())
On 2014/07/09 17:23:08, ojan-only-code-yellow-reviews wrote:
> flipForWritingMode already has an early return for
> style()->isFlippedBlocksWritingMode(), right? So it doesn't need to be checked
> here.

Done.

[ojan, 2014-07-10 17:52:12]

lgtm

[commit-bot: I haz the power, 2014-07-10 17:52:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/a.bah@samsung.com/18254006/98001

[commit-bot: I haz the power, 2014-07-10 18:58:04]

Change committed as 177870

[leviw_travelin_and_unemployed, 2014-07-15 20:56:30]

This exposes some bugs in our invalidation logic. If you disable the
PaintInvalidationState optimization (forcing this code to always be exercised),
we fail a bunch of tests.

crbug.com/393762