LayoutTests/editing/inserting/insert-paragraph-between-vertical-text-with-positioned-root.html - Issue 18254006: Repaint issues with vertical text which has absolute (or fixed) position specified for it.

Side by Side Diff: LayoutTests/editing/inserting/insert-paragraph-between-vertical-text-with-positioned-root.html

Issue 18254006: Repaint issues with vertical text which has absolute (or fixed) position specified for it. (Closed) Base URL: https://chromium.googlesource.com/chromium/blink.git@master

Patch Set: Created 7 years, 5 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch

OLD	NEW
(Empty)
	1 <!DOCTYPE html>

	2 <html>

	3 <head>

	4 <script>jsTestIsAsync = true;</script>

	5 <script src="../../fast/js/resources/js-test-pre.js"></script>

	6 <style>

	7 @font-face {

	8 font-family: 'Ahem';

	9 src: url(../../resources/Ahem.ttf);

	10 }

	11 .textArea {

	12 position: absolute; left: 0px; top: 200px;

	13 -webkit-writing-mode: vertical-rl;

	14 width: 200px;

	15 height: 300px;

	16 outline-style: none;
	ojan 2013/07/15 23:54:12 This isn't needed, right? This isn't needed, right? arpitab_ 2013/08/08 14:22:30 Yes, and I shall remove the same in the next patch Show quoted text 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.
	17 border: 1px solid black;

	18 font-family: Ahem;

	19 font-size: 12px;

	20 line-height: 1;
	ojan 2013/07/15 23:54:12 Are the font-size and line-height needed? Are the font-size and line-height needed? arpitab_ 2013/08/08 14:22:30 I was trying to make a platform independent testca Show quoted text 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. ojan 2013/09/12 23:40:15 Instead of using text in your page, you can use fi Show quoted text 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.
	21 }

	22 </style>

	23 <script>

	24 description("Testcase for bug http://crbug.com/259352: Repaint issues with verti cal text which has absolute (or fixed) position specified for it.\nTwo lines sho uld be visible within the box. To manually verify the issue, try to insert a lin e break in between the vertically aligned text. The line should break as expecte d.");

	25

	26 function start() {

	27 var testElement = document.getElementById('test');

	28 testElement.focus();

	29

	30 window.setTimeout(function() {
	ojan 2013/07/15 23:54:12 Why is this setTimeout needed? Why is this setTimeout needed? arpitab_ 2013/08/08 14:22:30 This particular setTimeout can be removed. Show quoted text On 2013/07/15 23:54:12, ojan wrote: > Why is this setTimeout needed? This particular setTimeout can be removed.
	31 var selection = window.getSelection();

	32 selection.collapse(testElement, 1);

	33 selection.modify("move", "backward", "character");

	34 document.execCommand("InsertParagraph");

	35

	36 window.setTimeout(logRepaints, 200);
	ojan 2013/07/15 23:54:12 I think you can avoid this setTimeout by calling d 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. arpitab_ 2013/08/08 14:22:30 This setTimeout is required since we need some tim Show quoted text 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. yosin_UTC9 2013/09/10 02:20:15 Can we call logRepaints until test condition satis 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 Something like this would be good. Even better wou Show quoted text 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.
	37 }, 0);

	38

	39 if (!window.testRunner \|\| !window.internals)

	40 return;

	41

	42 window.internals.startTrackingRepaints(document);

	43 window.testRunner.display();

	44 }

	45 function logRepaints() {

	46 if (!window.internals)

	47 return;

	48

	49 repaintRects = window.internals.repaintRectsAsText(document);

	50 window.internals.stopTrackingRepaints(document);

	51 shouldNotBe("repaintRects.indexOf('177 343')", "-1");

	52 finishJSTest();

	53 }

	54 </script>

	55 </head>

	56 <body onload="start()">

	57 <div class="textArea" contenteditable="true">

	58 <p>

	59 <span id="test">Some text in vertical mode</span>

	60 </p>

	61 </div>

	62 <div id="description"></div>

	63 <p id="console"></p>
	ojan 2013/07/15 23:54:12 The console and description elements are not neede The console and description elements are not needed. js-test-pre will insert them for you. arpitab_ 2013/08/08 14:22:30 I shall remove the same in the next patch. Show quoted text 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.
	64 <script src="../../fast/js/resources/js-test-post.js"></script>

	65 </body>

	66 </html>

OLD	NEW