Bug 102359

Created attachment 174392 [details] WIP patch

Editing bug title to make it more explanatory.

Created attachment 175654 [details] Patch

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.

Created attachment 175785 [details] Patch

(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 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.

Created attachment 175989 [details] Patch

(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 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.

(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 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.

Created attachment 176912 [details] Patch

Comment on attachment 176912 [details] Patch Many thanks for the review rniwa! :)

Comment on attachment 176912 [details] Patch Clearing flags on attachment: 176912 Committed r136225: <http://trac.webkit.org/changeset/136225>

All reviewed patches have been landed. Closing bug.

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

(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.