it only works within one line and could not go beyond line boundary.
Created attachment 94896 [details] work in progress
Created attachment 95241 [details] work in progress needs layout test.
Created attachment 95693 [details] patch w/ layout test
Comment on attachment 95693 [details] patch w/ layout test View in context: https://bugs.webkit.org/attachment.cgi?id=95693&action=review > Source/WebCore/editing/visible_units.cpp:1469 > +static const RootInlineBox* previousRootInlineBox(const InlineBox* box) This seems like a very expensive function. Why do we need to do this?
Comment on attachment 95693 [details] patch w/ layout test View in context: https://bugs.webkit.org/attachment.cgi?id=95693&action=review >> Source/WebCore/editing/visible_units.cpp:1469 >> +static const RootInlineBox* previousRootInlineBox(const InlineBox* box) > > This seems like a very expensive function. Why do we need to do this? RootInlineBox->prevLineBox() and nextLineBox() only catches the multiple lines caused by wrapping or by <br> in <div>abc def<br>hij opq</div>. For the <br> in <div>abc def <div><br></div>hij opq</div> (or the newline entered by user in contenteditable div), we need to find the previousRootInlineBox in order to go from "hij opq" to "abc def".
Comment on attachment 95693 [details] patch w/ layout test View in context: https://bugs.webkit.org/attachment.cgi?id=95693&action=review >>> Source/WebCore/editing/visible_units.cpp:1469 >>> +static const RootInlineBox* previousRootInlineBox(const InlineBox* box) >> >> This seems like a very expensive function. Why do we need to do this? > > RootInlineBox->prevLineBox() and nextLineBox() only catches the multiple lines caused by wrapping or by <br> in <div>abc def<br>hij opq</div>. > For the <br> in <div>abc def <div><br></div>hij opq</div> (or the newline entered by user in contenteditable div), we need to find the previousRootInlineBox in order to go from "hij opq" to "abc def". Can't we just walk on render tree?
Created attachment 96341 [details] patch w/ layout test Changed previousRootInlineBox/nextRootInlineBox to walk RenderTree instead of DOM node. I will add more tests.
Comment on attachment 96341 [details] patch w/ layout test View in context: https://bugs.webkit.org/attachment.cgi?id=96341&action=review Great! New patch looks much better. But it'll be nice if rendering experts like mitz or dhyatt could give us some feedback. > Source/WebCore/editing/htmlediting.h:144 > +inline Position createPosition(Node* node, int offset) This function name sounds too general. How about createPosiitonAvoidingIgnoredNode? > Source/WebCore/editing/visible_units.cpp:1506 > + RenderObject* previousBlockRenderer = renderer->previousSibling(); > + while (previousBlockRenderer) { > + const RenderBlock* blockWithLineBoxes = lastChildBlockWithLineBoxes(toRenderBlock(previousBlockRenderer)); > + if (blockWithLineBoxes) > + return blockWithLineBoxes->lastRootBox(); > + > + previousBlockRenderer = previousBlockRenderer->previousSibling(); > + } It seems like this loop is almost identical to the one in lastChildBlockWithLineBoxes. Can we combine these two loops? > Source/WebCore/editing/visible_units.cpp:1564 > + const InlineFlowBox* leftLineBox = (blockDirection == LTR) ? rootBox->prevLineBox() : > + rootBox->nextLineBox(); I wonder it makes a sense to add a helper function on InlineTextBox.
mitz, hyatt, & eric, Could you take a look at the code that walks render tree and see if there are any problems?
Comment on attachment 96341 [details] patch w/ layout test View in context: https://bugs.webkit.org/attachment.cgi?id=96341&action=review >> Source/WebCore/editing/visible_units.cpp:1564 >> + rootBox->nextLineBox(); > > I wonder it makes a sense to add a helper function on InlineTextBox. Responding to rniwa, I think we should just leave the code here.
Created attachment 100589 [details] patch w/ layout test Thanks for the review! (In reply to comment #8) > (From update of attachment 96341 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=96341&action=review > > Great! New patch looks much better. But it'll be nice if rendering experts like mitz or dhyatt could give us some feedback. > > > Source/WebCore/editing/htmlediting.h:144 > > +inline Position createPosition(Node* node, int offset) > > This function name sounds too general. How about createPosiitonAvoidingIgnoredNode? done. > > > Source/WebCore/editing/visible_units.cpp:1506 > > + RenderObject* previousBlockRenderer = renderer->previousSibling(); > > + while (previousBlockRenderer) { > > + const RenderBlock* blockWithLineBoxes = lastChildBlockWithLineBoxes(toRenderBlock(previousBlockRenderer)); > > + if (blockWithLineBoxes) > > + return blockWithLineBoxes->lastRootBox(); > > + > > + previousBlockRenderer = previousBlockRenderer->previousSibling(); > > + } > > It seems like this loop is almost identical to the one in lastChildBlockWithLineBoxes. Can we combine these two loops? Thanks for the code snippet. Done. > > > Source/WebCore/editing/visible_units.cpp:1564 > > + const InlineFlowBox* leftLineBox = (blockDirection == LTR) ? rootBox->prevLineBox() : > > + rootBox->nextLineBox(); > > I wonder it makes a sense to add a helper function on InlineTextBox. no change per hyatt's feedback.
Comment on attachment 100589 [details] patch w/ layout test Attachment 100589 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9017313 New failing tests: editing/selection/move-by-word-visually-others.html
Created attachment 100591 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 100723 [details] patch w/ layout test make layout test cross-platform
Comment on attachment 100723 [details] patch w/ layout test View in context: https://bugs.webkit.org/attachment.cgi?id=100723&action=review I've got a feeling that previousBlockWithLineBoxes/nextBlockWithLineBoxes are broken for flex boxes. Please look into it. > Source/WebCore/ChangeLog:15 > + Reviewed by NOBODY (OOPS!). This should appear before the long description. > Source/WebCore/editing/htmlediting.h:150 > + return Position(node, offset, Position::PositionIsOffsetInAnchor); Can this node be non-text? If so, we'll be introducing new blocker for https://bugs.webkit.org/show_bug.cgi?id=63040. If not , you should use the one that doesn't take the anchor type. r- due to this change. To give you some background, we're trying to avoid instantiating a position with (node, offset) pair when the node is not a text. Instead, we use positions before/after node or positions before/after children. > Source/WebCore/editing/visible_units.cpp:1325 > + createPositionAvoidingIgnoredNode(box->renderer()->node(), box->caretMinOffset()); Nit: should be intended by 4 spaces. > Source/WebCore/editing/visible_units.cpp:1481 > +static const RenderBlock* previousBlockWithLineBoxes(const RenderBlock* root) I'm not sure if root is a good name for this. I'd call it startingBlock. > Source/WebCore/editing/visible_units.cpp:1483 > + for (const RenderBlock* previous = root; previous; previous = toRenderBlock(previous->previousSibling())) { "previous" is a relative term. I get confused by its use in this function. Can we just call it block or currentBlock? > Source/WebCore/editing/visible_units.cpp:1485 > + // Renderer having inline children with contents has root line boxes. > + if (previous->childrenInline() && previous->firstRootBox()) I don't think this comment adds any new information because you're indeed checking that the first root box isn't null. Please remove it. > Source/WebCore/editing/visible_units.cpp:1509 > + while (renderer) { > + while (renderer && !renderer->isRenderBlock()) > + renderer = renderer->parent(); > + > + if (!renderer) > + return 0; > + > + if (const RenderBlock* blockWithLineBoxes = previousBlockWithLineBoxes(toRenderBlock(renderer->previousSibling()))) > + return blockWithLineBoxes->lastRootBox(); > + > + renderer = renderer->parent(); > + } I started to think that these nested loops can be flattened as follows: for (RenderObject* renderer = node->renderer(); renderer; renderer = renderer->parent()) { if (renderer->isRenderBlock()) { if (const RenderBlock* blockWithLineBoxes = previousBlockWithLineBoxes(toRenderBlock(renderer->previousSibling())) return blockWithLineBoxes->lastRootBox(); } } no? > Source/WebCore/editing/visible_units.cpp:1516 > + // Renderer having inline children with contents has root line boxes. Ditto about the comment. > Source/WebCore/editing/visible_units.cpp:1532 > + while (renderer) { > + while (renderer && !renderer->isRenderBlock()) > + renderer = renderer->parent(); Ditto about flattening the nested loop. > Source/WebCore/editing/visible_units.cpp:1552 > + rootBox->nextLineBox(); Nit: indentation. > Source/WebCore/editing/visible_units.cpp:1564 > + if (leftRootInlineBox) > + return leftRootInlineBox->lastLeafChild(); > + return 0; I'd use tertiary here. > Source/WebCore/editing/visible_units.cpp:1574 > + rootBox->prevLineBox(); Nit: indentation. > Source/WebCore/editing/visible_units.cpp:1586 > + if (rightRootInlineBox) > + return rightRootInlineBox->firstLeafChild(); > + return 0; Nit: tertiary. > Source/WebCore/editing/visible_units.cpp:1704 > + VisiblePosition rightWordBreak = rightWordPositionAcrossBoundary(visiblePosition); You should check the nullity of visiblePosition here. > LayoutTests/editing/selection/move-by-word-visually-others.html:94 > +<div contenteditable dir=ltr id="multi_line_1" class="test_move_by_word fix_width" style="width:50px" title="[multi_line_1, 0][multi_line_1, 4][multi_line_1, 8][multi_line_1, 12][multi_line_1, 16][multi_line_1, 0, 5][multi_line_1, 4, 5][multi_line_1, 8, 5][multi_line_1, 12, 5]|[multi_line_1, 15, 5][multi_line_1, 12, 5][multi_line_1, 8, 5][multi_line_1, 4, 5][multi_line_1, 0, 5][multi_line_1, 16][multi_line_1, 12][multi_line_1, 8][multi_line_1, 4][multi_line_1, 0]">abc def ghi jkl mn <br/><br/><br/>opq rst uvw xyz</div> Can we give a shorter id to make the expected result shorter?
(In reply to comment #15) > (From update of attachment 100723 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=100723&action=review > > I've got a feeling that previousBlockWithLineBoxes/nextBlockWithLineBoxes are broken for flex boxes. Please look into it. I talked with Tony, who is implementing new flex box, and confirmed that these function work as expected with flex boxes.
Comment on attachment 100723 [details] patch w/ layout test View in context: https://bugs.webkit.org/attachment.cgi?id=100723&action=review >> Source/WebCore/editing/visible_units.cpp:1509 >> + } > > I started to think that these nested loops can be flattened as follows: > > for (RenderObject* renderer = node->renderer(); renderer; renderer = renderer->parent()) { > if (renderer->isRenderBlock()) { > if (const RenderBlock* blockWithLineBoxes = previousBlockWithLineBoxes(toRenderBlock(renderer->previousSibling())) > return blockWithLineBoxes->lastRootBox(); > } > } > > no? Yes, and Thanks!
Created attachment 101358 [details] patch w/ layout test updated per comment #15
Comment on attachment 101358 [details] patch w/ layout test Attachment 101358 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9158214
Comment on attachment 101358 [details] patch w/ layout test Attachment 101358 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9159225
Comment on attachment 101358 [details] patch w/ layout test Attachment 101358 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9158219
Created attachment 101368 [details] patch w/ layout test fix compilation error.
Comment on attachment 101368 [details] patch w/ layout test View in context: https://bugs.webkit.org/attachment.cgi?id=101368&action=review > Source/WebCore/editing/htmlediting.cpp:1144 > + if (editingIgnoresContent(node) || !node->isTextNode()) { It appears that all we need to check is !node->isTextNode(). Since editingIgnoresContent calls a virtual function, we should get rid of this function call if possible. > Source/WebCore/editing/visible_units.cpp:1539 > + Nit: Please get rid of this blank line. > Source/WebCore/editing/visible_units.cpp:1541 > + const InlineFlowBox* leftLineBox = (blockDirection == LTR) ? rootBox->prevLineBox() : > + rootBox->nextLineBox(); Nit: we can fit this in one line if we had added a local variable like "const bool isBlockLTR = blockDirection == LTR;" > Source/WebCore/editing/visible_units.cpp:1550 > + const RootInlineBox* leftRootInlineBox; > + if (blockDirection == LTR) > + leftRootInlineBox = previousRootInlineBox(box); > + else > + leftRootInlineBox = nextRootInlineBox(box); Nit: I would have used tertiary. > Source/WebCore/editing/visible_units.cpp:1563 > + > + const InlineFlowBox* rightLineBox = (blockDirection == LTR) ? rootBox->nextLineBox() : > + rootBox->prevLineBox(); Ditto. > Source/WebCore/editing/visible_units.cpp:1572 > + const RootInlineBox* rightRootInlineBox; > + if (blockDirection == LTR) > + rightRootInlineBox = nextRootInlineBox(box); > + else > + rightRootInlineBox = previousRootInlineBox(box); Ditto. > LayoutTests/editing/selection/move-by-word-visually-others.html:93 > +<div contenteditable dir=ltr id="ml_1" class="test_move_by_word fix_width" style="width:50px" title="[ml_1, 0][ml_1, 4][ml_1, 8][ml_1, 12][ml_1, 16][ml_1, 0, 5][ml_1, 4, 5][ml_1, 8, 5][ml_1, 12, 5]|[ml_1, 15, 5][ml_1, 12, 5][ml_1, 8, 5][ml_1, 4, 5][ml_1, 0, 5][ml_1, 16][ml_1, 12][ml_1, 8][ml_1, 4][ml_1, 0]">abc def ghi jkl mn <br/><br/><br/>opq rst uvw xyz</div> I'm afraid that specifying "50px" will make this test platform-dependent. I'd at least use "ex" or "em" and only use "x" or some other letter since mixing letters will make the text width more dependent on font metrics. > LayoutTests/editing/selection/move-by-word-visually-others.html:108 > +<div contenteditable dir=rtl id="ml_7" class="test_move_by_word fix_width" style="width:50px" title="[ml_7, 15, 5][ml_7, 11, 5][ml_7, 8, 5][ml_7, 3, 5][ml_7, 0, 5][ml_7, 16][ml_7, 11][ml_7, 8][ml_7, 3][ml_7, 0]|[ml_7, 0][ml_7, 3][ml_7, 8][ml_7, 11][ml_7, 16][ml_7, 0, 5][ml_7, 3, 5][ml_7, 8, 5][ml_7, 11, 5]">abc def ghi jkl mn <br/><br/><br/>opq rst uvw xyz</div> > + > +<div contenteditable dir=rtl id="ml_8" class="test_move_by_word fix_width" style="width:50px" title="[ml_8, 15, 5][ml_8, 11, 5][ml_8, 8, 5][ml_8, 3, 5][ml_8, 0, 5][ml_8, 18][ml_8, 16][ml_8, 11][ml_8, 8][ml_8, 3][ml_8, 0]|[ml_8, 0][ml_8, 3][ml_8, 8][ml_8, 11][ml_8, 16][ml_8, 18][ml_8, 0, 5][ml_8, 3, 5][ml_8, 8, 5][ml_8, 11, 5]">abc def ghi jkl mn <div><br/></div><div><br/></div><div><br/></div>opq rst uvw xyz</div> Ditto about width. > LayoutTests/editing/selection/move-by-word-visually-single-space-sigle-line.html:68 > -<div dir=ltr id="div_1" class="test_move_by_word" title="[div_1, 0][div_1, 3]|[span_1, 2][div_1, 3][div_1,0]" contenteditable>××× <span id="span_1">××</span></div> > -<div dir=rtl id="div_2" class="test_move_by_word" title="[span_2, 2][div_2, 4][div_2, 0]|[div_2, 0][div_2, 4]" contenteditable>××× <span id="span_2">××</span></div> > +<div dir=ltr id="d_1" class="test_move_by_word" title="[d_1, 0][d_1, 3]|[s_1, 2][d_1, 3][d_1,0]" contenteditable>××× <span id="s_1">××</span></div> > +<div dir=rtl id="d_2" class="test_move_by_word" title="[s_2, 2][d_2, 4][d_2, 0]|[d_2, 0][d_2, 4]" contenteditable>××× <span id="s_2">××</span></div> Why don't I see these changes in expected.txt? > LayoutTests/editing/selection/resources/move-by-word-visually.js:247 > + var span = document.getElementById("span_size"); > + var length = span.offsetWidth; > + test.style.width = length + "px"; Mn... given this code maybe you didn't need to set width property in above test cases anyways?
Comment on attachment 101368 [details] patch w/ layout test View in context: https://bugs.webkit.org/attachment.cgi?id=101368&action=review >> LayoutTests/editing/selection/move-by-word-visually-others.html:93 >> +<div contenteditable dir=ltr id="ml_1" class="test_move_by_word fix_width" style="width:50px" title="[ml_1, 0][ml_1, 4][ml_1, 8][ml_1, 12][ml_1, 16][ml_1, 0, 5][ml_1, 4, 5][ml_1, 8, 5][ml_1, 12, 5]|[ml_1, 15, 5][ml_1, 12, 5][ml_1, 8, 5][ml_1, 4, 5][ml_1, 0, 5][ml_1, 16][ml_1, 12][ml_1, 8][ml_1, 4][ml_1, 0]">abc def ghi jkl mn <br/><br/><br/>opq rst uvw xyz</div> > > I'm afraid that specifying "50px" will make this test platform-dependent. I'd at least use "ex" or "em" and only use "x" or some other letter since mixing letters will make the text width more dependent on font metrics. Looks like for pure ascii, the result is cross-platform if font-size and font-family are fixed. But you are probably right that I do not need to set width here since I am setting width in JS. BTW, if width is set here and by JS, which one take precedence? >> LayoutTests/editing/selection/move-by-word-visually-single-space-sigle-line.html:68 >> +<div dir=rtl id="d_2" class="test_move_by_word" title="[s_2, 2][d_2, 4][d_2, 0]|[d_2, 0][d_2, 4]" contenteditable>××× <span id="s_2">××</span></div> > > Why don't I see these changes in expected.txt? id does not appear in expected file. instead of element id, the real content appears in expected file, which should make the expected file easier to read.
Comment on attachment 101368 [details] patch w/ layout test View in context: https://bugs.webkit.org/attachment.cgi?id=101368&action=review >>> LayoutTests/editing/selection/move-by-word-visually-others.html:93 >>> +<div contenteditable dir=ltr id="ml_1" class="test_move_by_word fix_width" style="width:50px" title="[ml_1, 0][ml_1, 4][ml_1, 8][ml_1, 12][ml_1, 16][ml_1, 0, 5][ml_1, 4, 5][ml_1, 8, 5][ml_1, 12, 5]|[ml_1, 15, 5][ml_1, 12, 5][ml_1, 8, 5][ml_1, 4, 5][ml_1, 0, 5][ml_1, 16][ml_1, 12][ml_1, 8][ml_1, 4][ml_1, 0]">abc def ghi jkl mn <br/><br/><br/>opq rst uvw xyz</div> >> >> I'm afraid that specifying "50px" will make this test platform-dependent. I'd at least use "ex" or "em" and only use "x" or some other letter since mixing letters will make the text width more dependent on font metrics. > > Looks like for pure ascii, the result is cross-platform if font-size and font-family are fixed. But you are probably right that I do not need to set width here since I am setting width in JS. BTW, if width is set here and by JS, which one take precedence? will remove the width style since it is reset in JS.
Created attachment 102043 [details] for EWS
Committed r91788: <http://trac.webkit.org/changeset/91788>
Comment on attachment 101368 [details] patch w/ layout test clear flag on landing patch.
the fix was rolled back in http://trac.webkit.org/changeset/93935
Created attachment 107738 [details] patch w/ layout test These functions are only used in moving cursor by word, more specifically, the 2 newly added functions only triggered when finding word break on next or previous line. Considering the speed of key press in real use case, where user press ctrl (alt) + arrow to move cursor by word, and considering the functions are only triggered when searching words across line boundary ("\n" in text field, not wrapped lines), I do not think performance is a concern here. Traversing DOM node is a much cleaner/safer solution.
Comment on attachment 107738 [details] patch w/ layout test View in context: https://bugs.webkit.org/attachment.cgi?id=107738&action=review > Source/WebCore/ChangeLog:10 > + DOM node. Nit: it seems like DOM node fits in the previous line. > Source/WebCore/editing/visible_units.cpp:1485 > + Position pos; > + pos = createLegacyEditingPosition(previousNode, caretMaxOffset(previousNode)); Nit: this should be don in one line. > Source/WebCore/editing/visible_units.cpp:1487 > + if (pos.isCandidate()) { Why do we need to check that this is a candidate? > Source/WebCore/editing/visible_units.cpp:1493 > + InlineBox* boxInPreviousNode; > + int ignoredCaretOffset; > + pos.getInlineBoxAndOffset(DOWNSTREAM, boxInPreviousNode, ignoredCaretOffset); > + > + if (boxInPreviousNode) > + return boxInPreviousNode->root(); Please use RenderedPosition. > Source/WebCore/editing/visible_units.cpp:1539 > + nextRootInlineBox(box); Nit: wrong indentation.
Created attachment 110748 [details] WIP Comparing with previously reviewed patch (107738), this WIP extracts the getting next root inline box functionality from nextLinePosition() and shares it with nextRootInlineBox(). I have not extract the getting previous root inline box functionality from previousLinePosition() and share with previousRootInlineBox() yet, want to get the idea that this is in the right track.
Attachment 110748 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1 Source/WebCore/editing/visible_units.cpp:641: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Total errors found: 1 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 110748 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=110748&action=review > Source/WebCore/editing/visible_units.cpp:606 > +struct RootInlineBoxAndPositionInfo { > + RootInlineBoxAndPositionInfo(EditingBoundaryCrossingRule rule, int offset, Node* root, bool returnPos) > + : rule(rule) > + , offset(offset) > + , highestRoot(root) > + , returnCandidatePosition(returnPos) > + { > + } Why do we need this struct? Can't we just pass arguments around? > Source/WebCore/editing/visible_units.cpp:629 > +static void nextRootInlineBoxOrCandidatePos(Node* node, const RootInlineBoxAndPositionInfo& info, > + RootInlineBox*& rootInlineBox, Position& candidatePos) { Maybe this needs to moved to RenderedPosition. > Source/WebCore/editing/visible_units.cpp:694 > + highestEditableRoot(p), true); Nit: wrong indentation. > Source/WebCore/editing/visible_units.cpp:1523 > +static const RootInlineBox* previousRootInlineBox(const InlineBox* box) Why is this function different from nextRootInlineBoxOrCandidatePos? These two functions should be symmetric to each other.
Comment on attachment 110748 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=110748&action=review > Source/WebCore/editing/visible_units.cpp:656 > + // FIXME: how to handle it if we only want to return next root inline box. I am not sure how to handle the case if we'd like to get next root inline box only which is the case in nextRootInlineBox(). Ignoring this position might miss some visible position candidates, but I have not thought of a test case yet. I am thinking maybe we could handle this iteratively.
Comment on attachment 110748 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=110748&action=review >> Source/WebCore/editing/visible_units.cpp:606 >> + } > > Why do we need this struct? Can't we just pass arguments around? yes you are right that we do not need this struct. instead, we can just pass arguments around. then, instead of a struct, we will need to pass 4 arguments around. Also, the |offset| and |root| are only needed if the |rule| is CanNotCrossEditingBoundary. >> Source/WebCore/editing/visible_units.cpp:629 >> + RootInlineBox*& rootInlineBox, Position& candidatePos) { > > Maybe this needs to moved to RenderedPosition. what is the rule-of-thumb here? >> Source/WebCore/editing/visible_units.cpp:1523 >> +static const RootInlineBox* previousRootInlineBox(const InlineBox* box) > > Why is this function different from nextRootInlineBoxOrCandidatePos? These two functions should be symmetric to each other. Yes, you are right. I have not make that change yet. would like to check with you that this is in the right track first.
The general approach of extracting code from the existing visible_units functions is good but I'm concerned about new struct and the fact next/previous functions seem to have completely different interface and behavior.
(In reply to comment #36) > (From update of attachment 110748 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=110748&action=review > > >> Source/WebCore/editing/visible_units.cpp:606 > >> + } > > > > Why do we need this struct? Can't we just pass arguments around? > > yes you are right that we do not need this struct. instead, we can just pass arguments around. then, instead of a struct, we will need to pass 4 arguments around. Also, the |offset| and |root| are only needed if the |rule| is CanNotCrossEditingBoundary. Could you elaborate on why we need those 4 arguments? > >> Source/WebCore/editing/visible_units.cpp:629 > >> + RootInlineBox*& rootInlineBox, Position& candidatePos) { > > > > Maybe this needs to moved to RenderedPosition. > > what is the rule-of-thumb here? RenderedPosition is meant to provide abstraction around renderers and inline boxes in editing code. If the code touches inline boxes and/or talks on render tree, then it's a strong indication that it should belong in RenderedPosition.
Comment on attachment 110748 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=110748&action=review >>>> Source/WebCore/editing/visible_units.cpp:606 >>>> + } >>> >>> Why do we need this struct? Can't we just pass arguments around? >> >> yes you are right that we do not need this struct. instead, we can just pass arguments around. then, instead of a struct, we will need to pass 4 arguments around. Also, the |offset| and |root| are only needed if the |rule| is CanNotCrossEditingBoundary. > > Could you elaborate on why we need those 4 arguments? this function is extracted to be shared by nextLinePosition() and nextRootInlineBox(). nextLinePosition() honor editing boundary while nextRootInlineBox() does not. This is from the 1st data member. When it is honor editing boundary, in original code inside nextLinePosition(), the first next leaf node it gets is through nextLeafWithSameEditability(), and it accepts 2 parameters -- the node, and the original visible position's Position's offset which is the 2nd data member here. When it is honor editing boundary, the code exits earlier when the next node's highest editable root is not the same as original node's highest editable root. original node's highest editable root is the 3rd data member. In original code inside nextLinePosition(), when position is a candidate but without root inline box, this position is returned as next line position. I am not clear on what to do when we only want to get next root inline box in this case (pls. refer comment at line 656), and I ignored such case for nextRootInlineBox(), hence the 4th parameter to differentiate.
Comment on attachment 110748 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=110748&action=review >>>>> Source/WebCore/editing/visible_units.cpp:606 >>>>> + } >>>> >>>> Why do we need this struct? Can't we just pass arguments around? >>> >>> yes you are right that we do not need this struct. instead, we can just pass arguments around. then, instead of a struct, we will need to pass 4 arguments around. Also, the |offset| and |root| are only needed if the |rule| is CanNotCrossEditingBoundary. >> >> Could you elaborate on why we need those 4 arguments? > > this function is extracted to be shared by nextLinePosition() and nextRootInlineBox(). > nextLinePosition() honor editing boundary while nextRootInlineBox() does not. This is from the 1st data member. > > When it is honor editing boundary, in original code inside nextLinePosition(), the first next leaf node it gets is through nextLeafWithSameEditability(), and it accepts 2 parameters -- the node, and the original visible position's Position's offset which is the 2nd data member here. > > When it is honor editing boundary, the code exits earlier when the next node's highest editable root is not the same as original node's highest editable root. original node's highest editable root is the 3rd data member. > > In original code inside nextLinePosition(), when position is a candidate but without root inline box, this position is returned as next line position. I am not clear on what to do when we only want to get next root inline box in this case (pls. refer comment at line 656), and I ignored such case for nextRootInlineBox(), hence the 4th parameter to differentiate. It seems to me we need to decide to do either and refactor the code first accordingly. It's not great to support both behaviors.
Comment on attachment 110748 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=110748&action=review >>>>>> Source/WebCore/editing/visible_units.cpp:606 >>>>>> + } >>>>> >>>>> Why do we need this struct? Can't we just pass arguments around? >>>> >>>> yes you are right that we do not need this struct. instead, we can just pass arguments around. then, instead of a struct, we will need to pass 4 arguments around. Also, the |offset| and |root| are only needed if the |rule| is CanNotCrossEditingBoundary. >>> >>> Could you elaborate on why we need those 4 arguments? >> >> this function is extracted to be shared by nextLinePosition() and nextRootInlineBox(). >> nextLinePosition() honor editing boundary while nextRootInlineBox() does not. This is from the 1st data member. >> >> When it is honor editing boundary, in original code inside nextLinePosition(), the first next leaf node it gets is through nextLeafWithSameEditability(), and it accepts 2 parameters -- the node, and the original visible position's Position's offset which is the 2nd data member here. >> >> When it is honor editing boundary, the code exits earlier when the next node's highest editable root is not the same as original node's highest editable root. original node's highest editable root is the 3rd data member. >> >> In original code inside nextLinePosition(), when position is a candidate but without root inline box, this position is returned as next line position. I am not clear on what to do when we only want to get next root inline box in this case (pls. refer comment at line 656), and I ignored such case for nextRootInlineBox(), hence the 4th parameter to differentiate. > > It seems to me we need to decide to do either and refactor the code first accordingly. It's not great to support both behaviors. nextRootInlinBox() can do honor editing boundary. It is just that honor editing boundary is the last step in those editing operations, so that we do that as the last step in rightWordPosition()/leftWordPosition. If we honor editing boundary in nextRootInlineBox, the functionality should be the same, but the code will exit earlier.
Created attachment 111092 [details] WIP
Comment on attachment 111092 [details] WIP Attachment 111092 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10068479
Comment on attachment 111092 [details] WIP Attachment 111092 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10056710
Created attachment 111103 [details] WIP fix EWS error.
Comment on attachment 101368 [details] patch w/ layout test Cleared Ryosuke Niwa's review+ from obsolete attachment 101368 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Created attachment 112036 [details] patch w/ layout test talked with rniwa offline and upload this patch which only fixes the nits in attachement 107738
Comment on attachment 112036 [details] patch w/ layout test View in context: https://bugs.webkit.org/attachment.cgi?id=112036&action=review > Source/WebCore/editing/visible_units.cpp:1492 > + InlineBox* boxInPreviousNode; > + int ignoredCaretOffset; > + pos.getInlineBoxAndOffset(DOWNSTREAM, boxInPreviousNode, ignoredCaretOffset); > + > + if (boxInPreviousNode) > + return boxInPreviousNode->root(); Please use RenderedPosition to obtain the root inline box. > Source/WebCore/editing/visible_units.cpp:1518 > + InlineBox* boxInNextNode; > + int ignoredCaretOffset; > + pos.getInlineBoxAndOffset(DOWNSTREAM, boxInNextNode, ignoredCaretOffset); > + > + if (boxInNextNode) > + return boxInNextNode->root(); Ditto.
Committed r98428: <http://trac.webkit.org/changeset/98428>