Bug 102359 – Caret is painted horizontally in vertical writing mode when there are no visible text

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Arpita Bahuguna]

Created attachment 174381 [details]
Testcase

When we try to add a new line at the end of some vertical text (in a contenteditable block), a vertical caret line is displayed on the new empty line, as if in accordance with horizontal text.
A horizontal caret line should instead be displayed for text in the vertical writing mode.

In the attached testcase, an enter should be pressed at the end of the text to reproduce the issue.
A vertical caret line would be displayed on the new line, as if for text in horizontal writing mode.

Also, the alignment of the caret changes once we start typing text in the new line.

[Arpita Bahuguna]

Created attachment 174392 [details]
WIP patch

[Arpita Bahuguna]

Editing bug title to make it more explanatory.

[Arpita Bahuguna]

Created attachment 175654 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 175654 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175654&action=review

> Source/WebCore/ChangeLog:3
> +        When the caret moves to a new line from the end of a vertical text, its orientation is not proper.

I just updated the bug title. Please update this line in the change log.

> Source/WebCore/ChangeLog:8
> +        When a new line is inserted at the end of a vertical text line, the caret

This isn't an accurate description of the bug. The bug reproduces whenever we have an empty paragraph.

> LayoutTests/editing/inserting/caret-alignment-for-vertical-text.html:31
> +    description('Testcase for bug <a href="https://bugs.webkit.org/show_bug.cgi?id=102359">102359</a>: When the caret moves to a new line from the end of a vertical text, its orientation is not proper.');
> +
> +    var editableContainer = document.getElementById('test');
> +    editableContainer.focus();
> +    
> +    if (window.internals) {
> +        startCaretRect = internals.absoluteCaretBounds(document);
> +
> +        window.getSelection().setPosition(editableContainer, 2);
> +        document.execCommand('InsertParagraph');
> +
> +        finalCaretRect = internals.absoluteCaretBounds(document);
> +        
> +        debug("The caret rect at the start of the new line should have the same orientation as on the existing one. We thus compare the width and the height of the caret rect at the two positions (one at the end of the existing text and the other at the start of the new line). The test case passes if they are the same.")
> +        shouldBe("startCaretRect.width", "finalCaretRect.width");
> +        shouldBe("startCaretRect.height", "finalCaretRect.height");
> +    }

This test doesn't work in browser at all. Ideally, the test case should be viewable in browser and DRT just provides a way to automatically verify it.
So, I'd prefer the test case having an empty paragraph with a description saying that the caret should render in vertical writing mode.
And then have script add some text and verify that the width and the height don't change.

[Arpita Bahuguna]

Created attachment 175785 [details]
Patch

[Arpita Bahuguna]

(In reply to comment #4)
> (From update of attachment 175654 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175654&action=review
> 
Thanks for the review rniwa.

> > Source/WebCore/ChangeLog:3
> > +        When the caret moves to a new line from the end of a vertical text, its orientation is not proper.
> 
> I just updated the bug title. Please update this line in the change log.
> 
Thanks for the suggestion. I had a hard time trying to figure out the right words to properly define the issue. :)

> > Source/WebCore/ChangeLog:8
> > +        When a new line is inserted at the end of a vertical text line, the caret
> 
> This isn't an accurate description of the bug. The bug reproduces whenever we have an empty paragraph.
> 
That's right. Have made the change.

> > LayoutTests/editing/inserting/caret-alignment-for-vertical-text.html:31
> > +    description('Testcase for bug <a href="https://bugs.webkit.org/show_bug.cgi?id=102359">102359</a>: When the caret moves to a new line from the end of a vertical text, its orientation is not proper.');
> > +
> > +    var editableContainer = document.getElementById('test');
> > +    editableContainer.focus();
> > +    
> > +    if (window.internals) {
> > +        startCaretRect = internals.absoluteCaretBounds(document);
> > +
> > +        window.getSelection().setPosition(editableContainer, 2);
> > +        document.execCommand('InsertParagraph');
> > +
> > +        finalCaretRect = internals.absoluteCaretBounds(document);
> > +        
> > +        debug("The caret rect at the start of the new line should have the same orientation as on the existing one. We thus compare the width and the height of the caret rect at the two positions (one at the end of the existing text and the other at the start of the new line). The test case passes if they are the same.")
> > +        shouldBe("startCaretRect.width", "finalCaretRect.width");
> > +        shouldBe("startCaretRect.height", "finalCaretRect.height");
> > +    }
> 
> This test doesn't work in browser at all. Ideally, the test case should be viewable in browser and DRT just provides a way to automatically verify it.
> So, I'd prefer the test case having an empty paragraph with a description saying that the caret should render in vertical writing mode.
> And then have script add some text and verify that the width and the height don't change.

Have now made the testcase viewable on browser.

[Ryosuke Niwa]

Comment on attachment 175785 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175785&action=review

> LayoutTests/editing/inserting/caret-alignment-for-vertical-text.html:24
> +    document.execCommand('InsertParagraph');        

This test shouldn't involve inserting a paragraph at all. You should have two editable regions one with text and another empty one and then just tell the user to replace carets in those two boxes and verify that the orientation of the carets are same in both boxes. In DRT, you can verify that automatically.

[Arpita Bahuguna]

Created attachment 175989 [details]
Patch

[Arpita Bahuguna]

(In reply to comment #7)
> (From update of attachment 175785 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175785&action=review
> 
> > LayoutTests/editing/inserting/caret-alignment-for-vertical-text.html:24
> > +    document.execCommand('InsertParagraph');        
> 
> This test shouldn't involve inserting a paragraph at all. You should have two editable regions one with text and another empty one and then just tell the user to replace carets in those two boxes and verify that the orientation of the carets are same in both boxes. In DRT, you can verify that automatically.

Thank-you for the review rniwa.

It was actually great that you suggested testing with an empty editable container (instead of inserting a paragraph) as that too had similar caret orientation issues.
Have tried to fix that as well in this patch.

For getting the caret rect for an empty element RenderBoxModelObject::localCaretRectForEmptyElement() is called which too needs a similar transposing of the final rect in case of vertical writing mode.
The change is similar to the one present in RenderText::localCaretRect().

[Ryosuke Niwa]

Comment on attachment 175989 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175989&action=review

> LayoutTests/editing/selection/caret-alignment-for-vertical-text.html:31
> +    document.execCommand('InsertParagraph');

Again, we shouldn't have to run an execCommand here. Just hard-code the DOM after inserting paragraph and use that.

[Arpita Bahuguna]

(In reply to comment #10)
> (From update of attachment 175989 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175989&action=review
> 
> > LayoutTests/editing/selection/caret-alignment-for-vertical-text.html:31
> > +    document.execCommand('InsertParagraph');
> 
> Again, we shouldn't have to run an execCommand here. Just hard-code the DOM after inserting paragraph and use that.

Hi rniwa, apologize for the delayed response.

Trying to hard-code in the dom exposes another issue with our code, which is that an editable div containing any empty node doesn't paint the caret at the proper position. For e.g.:
<div contenteditable="true"><p></p></div> or,
<div contenteditable="true"><span></span></div>
I need to study this issue further.

But, even for a <br> element, which does behave correctly in the horizontal writing mode, the caret is painted incorrectly in the vertical mode. Have raised a bug for that https://bugs.webkit.org/show_bug.cgi?id=103621.

Would appreciate your opinion on whether to wait for #103621, to be able to hard-code a <br> within our <div> or, to go ahead by inserting a new line with the execCommand() for this issue.

[Ryosuke Niwa]

Comment on attachment 175989 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175989&action=review

>>> LayoutTests/editing/selection/caret-alignment-for-vertical-text.html:31
>>> +    document.execCommand('InsertParagraph');
>> 
>> Again, we shouldn't have to run an execCommand here. Just hard-code the DOM after inserting paragraph and use that.
> 
> Hi rniwa, apologize for the delayed response.
> 
> Trying to hard-code in the dom exposes another issue with our code, which is that an editable div containing any empty node doesn't paint the caret at the proper position. For e.g.:
> <div contenteditable="true"><p></p></div> or,
> <div contenteditable="true"><span></span></div>
> I need to study this issue further.
> 
> But, even for a <br> element, which does behave correctly in the horizontal writing mode, the caret is painted incorrectly in the vertical mode. Have raised a bug for that https://bugs.webkit.org/show_bug.cgi?id=103621.
> 
> Would appreciate your opinion on whether to wait for #103621, to be able to hard-code a <br> within our <div> or, to go ahead by inserting a new line with the execCommand() for this issue.

What element does InsertParagraph insert? You should use exactly what InsertParagraph generates, not necessarily a an empty div with single br.

[Arpita Bahuguna]

Created attachment 176912 [details]
Patch

[Arpita Bahuguna]

Comment on attachment 176912 [details]
Patch

Many thanks for the review rniwa! :)

[WebKit Review Bot]

Comment on attachment 176912 [details]
Patch

Clearing flags on attachment: 176912

Committed r136225: <http://trac.webkit.org/changeset/136225>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Sudarsana Nagineni (babu)]

The newly added layout test editing/selection/caret-alignment-for-vertical-text.html is failing on GTK and EFl ports.

GTK:
http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r136225%20%2839844%29/editing/selection/caret-alignment-for-vertical-text-diff.txt

EFL:
http://build.webkit.org/results/EFL%20Linux%2064-bit%20Debug%20WK2/r136225%20%286524%29/editing/selection/caret-alignment-for-vertical-text-diff.txt

[Sudarsana Nagineni (babu)]

(In reply to comment #17)
> The newly added layout test editing/selection/caret-alignment-for-vertical-text.html is failing on GTK and EFl ports.

This is going to be fixed by bug 102374.