Bug 102359 - Caret is painted horizontally in vertical writing mode when there are no visible text
Summary: Caret is painted horizontally in vertical writing mode when there are no visi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Arpita Bahuguna
URL:
Keywords:
Depends on:
Blocks: 49408 108053
  Show dependency treegraph
 
Reported: 2012-11-15 02:32 PST by Arpita Bahuguna
Modified: 2013-01-28 03:33 PST (History)
8 users (show)

See Also:


Attachments
Testcase (687 bytes, text/html)
2012-11-15 02:32 PST, Arpita Bahuguna
no flags Details
WIP patch (1.40 KB, patch)
2012-11-15 03:18 PST, Arpita Bahuguna
no flags Details | Formatted Diff | Diff
Patch (5.51 KB, patch)
2012-11-22 05:54 PST, Arpita Bahuguna
no flags Details | Formatted Diff | Diff
Patch (5.80 KB, patch)
2012-11-23 05:00 PST, Arpita Bahuguna
no flags Details | Formatted Diff | Diff
Patch (7.42 KB, patch)
2012-11-26 06:20 PST, Arpita Bahuguna
no flags Details | Formatted Diff | Diff
Patch (7.28 KB, patch)
2012-11-30 00:41 PST, Arpita Bahuguna
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Arpita Bahuguna 2012-11-15 02:32:46 PST
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.
Comment 1 Arpita Bahuguna 2012-11-15 03:18:16 PST
Created attachment 174392 [details]
WIP patch
Comment 2 Arpita Bahuguna 2012-11-22 01:27:49 PST
Editing bug title to make it more explanatory.
Comment 3 Arpita Bahuguna 2012-11-22 05:54:23 PST
Created attachment 175654 [details]
Patch
Comment 4 Ryosuke Niwa 2012-11-22 08:55:16 PST
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.
Comment 5 Arpita Bahuguna 2012-11-23 05:00:57 PST
Created attachment 175785 [details]
Patch
Comment 6 Arpita Bahuguna 2012-11-23 07:00:15 PST
(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.
Comment 7 Ryosuke Niwa 2012-11-23 18:30:16 PST
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.
Comment 8 Arpita Bahuguna 2012-11-26 06:20:04 PST
Created attachment 175989 [details]
Patch
Comment 9 Arpita Bahuguna 2012-11-26 06:40:52 PST
(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().
Comment 10 Ryosuke Niwa 2012-11-26 09:58:25 PST
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.
Comment 11 Arpita Bahuguna 2012-11-29 06:57:46 PST
(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.
Comment 12 Ryosuke Niwa 2012-11-29 10:15:17 PST
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.
Comment 13 Arpita Bahuguna 2012-11-30 00:41:44 PST
Created attachment 176912 [details]
Patch
Comment 14 Arpita Bahuguna 2012-11-30 01:55:49 PST
Comment on attachment 176912 [details]
Patch

Many thanks for the review rniwa! :)
Comment 15 WebKit Review Bot 2012-11-30 04:33:04 PST
Comment on attachment 176912 [details]
Patch

Clearing flags on attachment: 176912

Committed r136225: <http://trac.webkit.org/changeset/136225>
Comment 16 WebKit Review Bot 2012-11-30 04:33:09 PST
All reviewed patches have been landed.  Closing bug.
Comment 18 Sudarsana Nagineni (babu) 2012-11-30 07:24:32 PST
(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.