106827 – Pressing the down cursor key does not move the caret in contenteditable div when there is a wrapped image

NEW 106827

Pressing the down cursor key does not move the caret in contenteditable div when there is a wrapped image

https://bugs.webkit.org/show_bug.cgi?id=106827

Summary Pressing the down cursor key does not move the caret in contenteditable div w...

testing9999

Reported 2013-01-14 14:13:31 PST

Repro steps: 1. Paste the following into the address bar data:text/html,<html><div contenteditable="true" style="height:400px; width:200px; border: solid black 1px">testing wrapped picture on the same line <img src="http://farm8.staticflickr.com/7239/7171902074_2d9b462d9c_q.jpg" /><br/>2nd line here</div></html> 2. Use the mouse and click on after the word "line" 3. Press the down arrow key Expected Caret is stuck and does not move. Actual Caret move into another line, ideally right before or after the image

Attachments
Patch (6.65 KB, patch) 2013-07-16 04:22 PDT, Arpita Bahuguna	no flags	Details Formatted Diff Diff
Patch (6.88 KB, patch) 2013-07-17 04:54 PDT, Arpita Bahuguna	no flags	Details Formatted Diff Diff
Test case extracted from the patch (456 bytes, text/html) 2013-07-17 10:55 PDT, Ryosuke Niwa	no flags	Details
Patch (6.70 KB, patch) 2013-07-24 22:06 PDT, Arpita Bahuguna	no flags	Details Formatted Diff Diff
Patch (10.42 KB, patch) 2013-07-24 22:55 PDT, Arpita Bahuguna	no flags	Details Formatted Diff Diff
Patch (11.72 KB, patch) 2013-07-25 04:11 PDT, Arpita Bahuguna	no flags	Details Formatted Diff Diff
Patch (12.44 KB, patch) 2013-07-31 01:51 PDT, Arpita Bahuguna	darin: review- buildbot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (677.22 KB, application/zip) 2013-07-31 05:52 PDT, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (751.32 KB, application/zip) 2013-07-31 08:21 PDT, Build Bot	no flags	Details
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Arpita Bahuguna

Comment 1 2013-07-16 04:22:18 PDT

Created attachment 206758 [details] Patch

Arpita Bahuguna

Comment 2 2013-07-17 04:54:43 PDT

Created attachment 206884 [details] Patch

Arpita Bahuguna

Comment 3 2013-07-17 04:55:13 PDT

Comment on attachment 206884 [details] Patch Patch with small layout test modifications

Arpita Bahuguna

Comment 4 2013-07-17 04:55:53 PDT

The patch will make our behavior similar to that of Firefox.

Ryosuke Niwa

Comment 5 2013-07-17 10:55:08 PDT

Comment on attachment 206884 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206884&action=review > Source/WebCore/editing/VisibleUnits.cpp:1025 > + if (beforeNodePosition == visiblePosition) > + return positionInParentAfterNode(node); We should probably add the fix to previousLinePosition as well if anything for the consistency's sake.

Ryosuke Niwa

Comment 6 2013-07-17 10:55:34 PDT

Created attachment 206901 [details] Test case extracted from the patch

Ryosuke Niwa

Comment 7 2013-07-17 10:55:47 PDT

Comment on attachment 206884 [details] Patch Please fix previousLinePosition before you land the patch.

Radar WebKit Bug Importer

Comment 8 2013-07-17 10:56:16 PDT

<rdar://problem/14470612>

Arpita Bahuguna

Comment 9 2013-07-19 04:48:00 PDT

(In reply to comment #7) > (From update of attachment 206884 [details]) > Please fix previousLinePosition before you land the patch. Hi Ryosuke, many thanks for the review! I had actually already opened a different bug for fixing previousLinePosition https://bugs.webkit.org/show_bug.cgi?id=118786 but it gives me a regression. Am working on that.

Arpita Bahuguna

Comment 10 2013-07-24 22:06:22 PDT

Created attachment 207432 [details] Patch

Arpita Bahuguna

Comment 11 2013-07-24 22:36:35 PDT

Another issue exists with moving the caret to the previous line. For the attached testcase place the caret at the end of the third line and then press the up arrow key. The caret moves to the end of the first line instead of at the end of the second line containing the image.

Arpita Bahuguna

Comment 12 2013-07-24 22:55:27 PDT

Created attachment 207434 [details] Patch

Arpita Bahuguna

Comment 13 2013-07-25 04:11:59 PDT

Created attachment 207447 [details] Patch

Arpita Bahuguna

Comment 14 2013-07-25 04:25:53 PDT

Hi rniwa! Have submitted another patch that clubs the changes for both previousLinePosition() and nextLinePosition(). It also handles the scenario that you had mentioned on IRC for nextLinePosition(). That is for nextLinePosition we should take the position before node and for prevLinePosition we should take the position after the node. Have accordingly modified the test case for next line position as well to verify that scenario as well.

Ryosuke Niwa

Comment 15 2013-07-25 17:30:58 PDT

Comment on attachment 207447 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207447&action=review > Source/WebCore/ChangeLog:26 > + However, if the computed node is either a <br> element or a form control > + element we should always return the before position for such nodes. This > + is because we cannot have a caret position lying at the end of such > + elements. I don't understand this. It should be perfectly fine to have a position in the parent after br or form control element. What makes you say that we can't have such positions? > Source/WebCore/ChangeLog:29 > + then return the other side position. return the position on the other side.

Ryosuke Niwa

Comment 16 2013-07-25 17:32:03 PDT

Comment on attachment 207447 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207447&action=review >> Source/WebCore/ChangeLog:26 >> + elements. > > I don't understand this. It should be perfectly fine to have a position in the parent after br or form control element. > What makes you say that we can't have such positions? Also, we probably need to call isAtomicNode instead of manually checking whether they're br & form control element. In fact, if that reintroduces the bug or introduces another problem, then we probably haven't completely understood the problem yet.

Arpita Bahuguna

Comment 17 2013-07-31 01:51:01 PDT

Created attachment 207820 [details] Patch

Arpita Bahuguna

Comment 18 2013-07-31 01:54:44 PDT

(In reply to comment #16) > (From update of attachment 207447 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=207447&action=review > Hi Ryosuke, Have uploaded another patch which hopefully is a more appropriate fix for the issue. > >> Source/WebCore/ChangeLog:26 > >> + elements. > > > > I don't understand this. It should be perfectly fine to have a position in the parent after br or form control element. > > What makes you say that we can't have such positions? > > Also, we probably need to call isAtomicNode instead of manually checking whether they're br & form control element. > In fact, if that reintroduces the bug or introduces another problem, then we probably haven't completely understood the problem yet.

Ryosuke Niwa

Comment 19 2013-07-31 02:12:16 PDT

Comment on attachment 207820 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207820&action=review > Source/WebCore/ChangeLog:43 > + next/previous line is offset within a non-editable node. Poistions that are Typo: Poistions > Source/WebCore/editing/VisibleUnits.cpp:966 > + VisiblePosition prevLineVisiblePosition = renderer->positionForPoint(pointInLine); > + if (node && editingIgnoresContent(node) && (prevLineVisiblePosition.deepEquivalent().anchorType() == Position::PositionIsOffsetInAnchor)) { I'm not certain what guarantees node to be the anchor node of prevLineVisiblePosition. Perhaps you might be interested in reading https://rniwa.com/2011-06-26/position-and-anchor-types/ At minimum, we should be using deepEquivalent's containerNode instead. But it seems like you want to catch the case where we have a legacy editing position given you're looking for a position inside an atomic node.

Arpita Bahuguna

Comment 20 2013-07-31 05:02:06 PDT

(In reply to comment #19) > (From update of attachment 207820 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=207820&action=review > Hi rniwa! Thanks for the review and for pointing me to your blog! > > Source/WebCore/ChangeLog:43 > > + next/previous line is offset within a non-editable node. Poistions that are > > Typo: Poistions > > > Source/WebCore/editing/VisibleUnits.cpp:966 > > + VisiblePosition prevLineVisiblePosition = renderer->positionForPoint(pointInLine); > > + if (node && editingIgnoresContent(node) && (prevLineVisiblePosition.deepEquivalent().anchorType() == Position::PositionIsOffsetInAnchor)) { > > I'm not certain what guarantees node to be the anchor node of prevLineVisiblePosition. > Perhaps you might be interested in reading https://rniwa.com/2011-06-26/position-and-anchor-types/ > > At minimum, we should be using deepEquivalent's containerNode instead. But it seems like you want to catch the case where we have a legacy editing position > given you're looking for a position inside an atomic node. Yes, node and the anchorNode from renderer->positionForPoint(pointInLine) shall not necessarily be the same but for my case (moving to next line from end of the first line containing text and next line containing an image) renderer->positionForPoint(pointInLine) returns a position with anchorNode as the text node, offset as 40, anchorType as PositionIsOffsetInAnchor and is a legacy editing position. In this case the anchorNode returned is an editable node and the node corresponding to the renderer is a non-editable node (image element). I had initially planned on trying to verify if the returned position is a legacy editing position, then trying to get the anchorType (offset is 0 then positionIsBeforeAnchor else positionIsAfterAnchor) and based on that returning either positionInParentBeforeNode() or positionInParentAfterNode(). But this would make use of the renderer's node and not the anchorNode returned in renderer->positionForPoint(pointInLine). Also, I get situations wherein the position returned is for a text control element (anchorNode) with offset as 0 and anchorType as positionIsOffsetInAnchor. Thus I used that specific check: (prevLineVisiblePosition.deepEquivalent().anchorType() == Position::PositionIsOffsetInAnchor) I understand that the renderer's node and the position's anchorNode shall not be the same but I couldn't find a way to make this work by just using the anchorNode.

Build Bot

Comment 21 2013-07-31 05:52:48 PDT

Comment on attachment 207820 [details] Patch Attachment 207820 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1299642 New failing tests: editing/selection/3690703-2.html editing/selection/replaced-boundaries-3.html

Build Bot

Comment 22 2013-07-31 05:52:50 PDT

Created attachment 207844 [details] Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-16 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.4

Arpita Bahuguna

Comment 23 2013-07-31 06:28:43 PDT

(In reply to comment #21) > (From update of attachment 207820 [details]) > Attachment 207820 [details] did not pass mac-wk2-ews (mac-wk2): > Output: http://webkit-queues.appspot.com/results/1299642 > > New failing tests: > editing/selection/3690703-2.html > editing/selection/replaced-boundaries-3.html These two test cases run perfectly well for me on my Qt port :-\

Build Bot

Comment 24 2013-07-31 08:21:38 PDT

Comment on attachment 207820 [details] Patch Attachment 207820 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1294735 New failing tests: editing/selection/3690703-2.html editing/selection/replaced-boundaries-3.html

Build Bot

Comment 25 2013-07-31 08:21:41 PDT

Created attachment 207852 [details] Archive of layout-test-results from webkit-ews-04 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-04 Port: mac-mountainlion Platform: Mac OS X 10.8.4

Ryosuke Niwa

Comment 26 2013-07-31 16:03:40 PDT

(In reply to comment #20) > I understand that the renderer's node and the position's anchorNode shall not be the same but I couldn't find a way to make this work by just using the anchorNode. That seems to indicate we're doing something wrong here. You're checking position's anchor type but checking the atomicity of some other node. That doesn't make much sense. We need to dig deeper and understand what the problem is in each case.

Arpita Bahuguna

Comment 27 2013-08-01 03:32:07 PDT

(In reply to comment #26) > (In reply to comment #20) > > I understand that the renderer's node and the position's anchorNode shall not be the same but I couldn't find a way to make this work by just using the anchorNode. > > That seems to indicate we're doing something wrong here. You're checking position's anchor type but checking the atomicity of some other node. That doesn't make much sense. We need to dig deeper and understand what the problem is in each case. I did try to investigate further and found that for our case involving an inline image the position returned for renderer->positionForPoint(pointInLine) does point to the anchorNode as the image element with offset of 0 and type as PositionIsBeforeAnchor. However, when we convert this to a VisiblePosition this same then points to the previous text node's end point. This is due to our canonicalPosition()'s upstream() computation wherein endsOfNodeAreVisuallyDistinctPositions() does not return true for the image element due to the check: return node->renderer()->isReplaced() && canHaveChildrenForEditing(node) && toRenderBox(node->renderer())->height() != 0 && !node->firstChild(); Since the image element doesn't have a child, endsOfNodeAreVisuallyDistinctPositions() always returns false even though I suppose that even for an image element endsOfNodeAreVisuallyDistinctPositions() should return true. (??) Shouldn't it be possible to have a position before (PositionIsBeforeAnchor) an Image element? For testing purpose I added some temp code in upstream(): if (editingIgnoresContent(currentNode) && renderer->isImage() && currentPos.atStartOfNode() && atFirstEditingPositionForNode()) return lastVisible; which makes our behavior for the given testcase work exactly as FireFox's. That is to say that when moving down from a text line, if the text content is less than half the width of the node on the next line, the caret moves before that node (image in our case) and if the text content is greater than half the logical width of the next line node, the caret moves to after the node. This change made our up/down movement between lines work well and just like Firefox's however, as expected, it fails several other cases. I suppose this is coz our editing code is based on the premise that for a non-editable element we cannot have a VisiblePosition with the anchorNode as that element and type as PositionIsBeforeAnchor. (??) I could be wrong in this assumption but throughout we check for this anomaly. Such as in startOfParagraph() where we have a check as: else if (editingIgnoresContent(n) || isTableElement(n)) { node = n; type = Position::PositionIsBeforeAnchor; n = n->previousSibling() ? n->previousSibling() : n->traversePreviousNodePostOrder(startBlock); So is it possible to have a VisiblePosition that's anchored in an image and is of type PositionIsBeforeAnchor? If so then why when converting our position to a VisiblePosition does the upstream() return a position which is anchored in the previous editable text.? Should endsOfNodeAreVisuallyDistinctPositions() not consider image elements as a special case since I believe the ends of node for an image should be considered as visually distinct positions. If the same position were to be passed through isCandidate() it returns true. Would really appreciate your thoughts on this and some guidance on how to proceed further with this.

Ryosuke Niwa

Comment 28 2013-08-01 10:42:38 PDT

(In reply to comment #27) > > I did try to investigate further and found that for our case involving an inline image the position returned for renderer->positionForPoint(pointInLine) does point to the anchorNode as the image element with offset of 0 and type as PositionIsBeforeAnchor. > However, when we convert this to a VisiblePosition this same then points to the previous text node's end point. > > This is due to our canonicalPosition()'s upstream() computation wherein endsOfNodeAreVisuallyDistinctPositions() does not return true for the image element due to the check: > return node->renderer()->isReplaced() && canHaveChildrenForEditing(node) && toRenderBox(node->renderer())->height() != 0 && !node->firstChild(); > > Since the image element doesn't have a child, endsOfNodeAreVisuallyDistinctPositions() always returns false even though I suppose that even for an image element endsOfNodeAreVisuallyDistinctPositions() should return true. (??) Yeah, this check is strange in that all replaced elements shouldn't have children for editing so I'm not quite sure what it's trying to do... > I suppose this is coz our editing code is based on the premise that for a non-editable element we cannot have a VisiblePosition with the anchorNode as that element and type as PositionIsBeforeAnchor. (??) I don't think so. > I could be wrong in this assumption but throughout we check for this anomaly. > Such as in startOfParagraph() where we have a check as: > > else if (editingIgnoresContent(n) || isTableElement(n)) { > node = n; > type = Position::PositionIsBeforeAnchor; > n = n->previousSibling() ? n->previousSibling() : n->traversePreviousNodePostOrder(startBlock); > > So is it possible to have a VisiblePosition that's anchored in an image and is of type PositionIsBeforeAnchor? If so then why when converting our position to a VisiblePosition does the upstream() return a position which is anchored in the previous editable text.? > > Should endsOfNodeAreVisuallyDistinctPositions() not consider image elements as a special case since I believe the ends of node for an image should be considered as visually distinct positions. If the same position were to be passed through isCandidate() it returns true. That seems reasonable but I don't think that's a special case for an image. All replaced elements such as input element and other atomic elements (e.g. non-editable elements) should probably be treated the same.

Darin Adler

Comment 29 2016-03-09 09:30:16 PST

Comment on attachment 207820 [details] Patch Setting review- because we have to fix what Ryosuke pointed out.

Note You need to log in before you can comment on or make changes to this bug.

Status NEW

Resolution

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Mac

OS OS X 10.7

Product WebKit

Component New Bugs

Assignee

Arpita Bahuguna

Reported

2013-01-14 14:13 PST

Modified

2016-03-09 09:30 PST History

CC List

7 users Show

URL

Keywords InRadar

Depends on

Blocks

118786

Dependencies

tree graph