[Webkit-unassigned] [Bug 57336] experiment with moving caret by word in visual order
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Mar 29 09:27:22 PDT 2011
https://bugs.webkit.org/show_bug.cgi?id=57336
Ryosuke Niwa <rniwa at webkit.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #87322|review? |review-
Flag| |
--- Comment #2 from Ryosuke Niwa <rniwa at webkit.org> 2011-03-29 09:27:22 PST ---
(From update of attachment 87322)
View in context: https://bugs.webkit.org/attachment.cgi?id=87322&action=review
Great! This patch is much more reviewable.
> Source/WebCore/editing/TextGranularity.h:44
> + // caret by wordGranularity. After all patches in, it should be removed.
Nit: Once all patches are landed
> Source/WebCore/editing/visible_units.cpp:1237
> + bool first = previousWordBreak.isNull(); // Whether this is to collect the first word break in the box.
Instead of adding a comment here, let us instead have:
bool hasSeenWordBreakInThisBox = previousWordBreak.isNotNull();
> Source/WebCore/editing/visible_units.cpp:1238
> + InlineBox* boxOfWordBreak;
Nit: should this be named as boxContainingPreviousWordBreak?
> Source/WebCore/editing/visible_units.cpp:1240
> + if (box->direction() == blockDirection) {
This function should probably named as nextWordBreakInBox or previousWordBreakInBoxInVisualOrder.
It also seems like we should split this function into two versions the one for box->direction() == blockDirection and (box->direction() != blockDirection.
They can be named as:
previousWordBreakInBoxInsideBlockWithSameDirectionality
nextWordBreakInBoxInsideBlockWithOppositeDirectionality
> Source/WebCore/editing/visible_units.cpp:1243
> + wordBreak = first ? VisiblePosition(Position(box->renderer()->node(), box->caretMaxOffset(), Position::PositionIsOffsetInAnchor), VP_DEFAULT_AFFINITY) : previousWordBreak;
You don't need to explicitly call VisiblePosition's constructor. it'll automatically do it. So you can do:
wordBreak = first ? Position(box->renderer()->node(), box->caretMaxOffset(), Position::PositionIsOffsetInAnchor) : previousWordBreak;
> Source/WebCore/editing/visible_units.cpp:1245
> + if (first && box->nextLeafChild()) {
It seems that this condition should be dependent on the value of directionality. It doesn't make sense that we check the same box for both LTR and RTL cases.
> Source/WebCore/editing/visible_units.cpp:1248
> + // when cursor is at the right of "opq", its previous word boundary's offset returned from previousWordPosition() is 15, which is not in the same box as "opq",
Nit: immediately to the right of "opq".
Nit: offset from previousWordPosition() is immediately to the right of "hij ".
> Source/WebCore/editing/visible_units.cpp:1262
> + last = true;
> + return VisiblePosition();
For this version at least, it seems simpler to get rid of "last" argument and use the null visible position to mean the last.
> Source/WebCore/editing/visible_units.cpp:1264
> + // FIXME in next version.
FIXME implies "next version". We should explain what need to be implemented. I guess:
FIXME: Implement the case when the box direction is not equal to the block direction
would work here.
> Source/WebCore/editing/visible_units.cpp:1277
> +// Get the leftmost or rightmost word bounary in a box.
> +// 'box' is the box to start search.
> +// 'offset' is the node offset of the current caret.
> +// When 'offset' is valid (not equal to -1), the word boundary returned should be different from the word boundary starts at 'offset'.
> +// 'boundaryDirection' indicates the visual direction (left or right) to search for
> +// word boundary.
> +// 'boundaryDirection' == DirectionRight means search rightmost word boundary in box.
> +// 'boundaryDirection' == DirectionLeft means search leftmost word boundary in box.
> +// 'blockDirection' indicates the directionality of the current block.
No need for these comments.
> Source/WebCore/editing/visible_units.cpp:1291
> + VisiblePosition wordBreak = collectOneWordBreakInBox(box, blockDirection, VisiblePosition(), offsetOfWordBreak, last);
> + while (!last) {
> + if (offset == -1 || offsetOfWordBreak != offset)
> + return wordBreak;
> + wordBreak = collectOneWordBreakInBox(box, blockDirection, wordBreak, offsetOfWordBreak, last);
> + }
This can be written as a do-while as in:
VisiblePosition wordBreak;
do {
wordBreak = collectOneWordBreakInBox(box, blockDirection, wordBreak, offsetOfWordBreak, last);
if (offset == -1 || offsetOfWordBreak != offset)
return wordBreak;
} while (!last);
> Source/WebCore/editing/visible_units.cpp:1293
> + if (!wordBreak.isNull() && (offset == -1 || offsetOfWordBreak != offset))
> + return wordBreak;
This is dead code for this version. Please remove.
> Source/WebCore/editing/visible_units.cpp:1299
> +// Search word boundary in visually adjacent (previous or next) boxes.
This repeats the function name. Please remove it.
> Source/WebCore/editing/visible_units.cpp:1301
> +// 'box' is the box to start search.
> +// 'offset' is the node offset of the current caret.
No need. Please get rid of it.
> Source/WebCore/editing/visible_units.cpp:1302
> +// When 'offset' is valid (not equal to -1), the word boundary returned should be different from the word boundary starts at 'offset'.
We should define a value as in #define INVALID_OFFSET -1 so that we don't need to add this comment.
> Source/WebCore/editing/visible_units.cpp:1308
> +// 'boundaryDirection' indicates the visual direction (left or right) to search for
> +// word boundary.
> +// 'boundaryDirection' == DirectionRight means search rightmostWordBoundaryInPreviousBoxes.
> +// 'boundaryDirection' == DirectionLeft means search leftmostWordBoundaryInNextBoxes.
> +// 'blockDirection' indicates the directionality of the current block.
> +// 'visiblePosition' is the visible position of current caret.
I don't think these comments are necessary.
> Source/WebCore/editing/visible_units.cpp:1309
> +static VisiblePosition wordBoundaryInAdjacentBoxes(const InlineBox* box, int offset, WebCore::SelectionDirection boundaryDirection, TextDirection blockDirection, const VisiblePosition& visiblePosition)
boundaryDirection might be a bit confusing? Maybe directionToSearch?
> Source/WebCore/editing/visible_units.cpp:1322
> +static VisiblePosition rightmostWordBoundaryInPreviousBoxes(const InlineBox* box, int offset, TextDirection blockDirection, const VisiblePosition& visiblePosition)
This function name is confusing because I get an impression that we start looking for a word boundary from box->prevLeafChild().
maybe leftWordBoundary or rightmostWordBoundaryInBoxOrPreviousBoxes?
> Source/WebCore/editing/visible_units.cpp:1348
> + // FIXME: If the box's directionality is the same as that of the enclosing block, when the offset is at the box boundary and the direction is towards inside the box,
> + // do I still need to make it a special case? For example, a LTR box inside a LTR block, when offset is at box's caretMinOffset and the direction is DirectionRight,
> + // should it be taken care as a general case?
These lines look too long. Please make each line a little shorter (20-30%).
> Source/WebCore/editing/visible_units.cpp:1352
> + if (wordDirection == WebCore::DirectionLeft)
> + return rightmostWordBoundaryInPreviousBoxes(isLTRBox ? box->prevLeafChild() : box, isLTRBox ? -1 : offset, blockDirection, visiblePosition);
> + return leftmostWordBoundaryInNextBoxes(isLTRBox ? box : box->nextLeafChild(), isLTRBox ? offset : -1, blockDirection, visiblePosition);
Maybe it's better to have these logic directly in leftWordPosition and rightWordPosition.
e.g.
VisiblePosition leftWordPosition(const VisiblePosition& visiblePosition) {
InlineBox* box;
int offset;
visiblePosition.getInlineBoxAndOffset(box, offset);
TextDirection blockDirection = directionOfEnclosingBlock(visiblePosition.deepEquivalent());
if (offset == box->leftmostCaretOffset())
return rightmostWordBoundaryInPreviousBoxes(box->prevLeafChild(), -1, blockDirection, visiblePosition);
else if (offset == box->rightmostCaretoffset())
return rightmostWordBoundaryInPreviousBoxes(box, offset, blockDirection, visiblePosition);
// FIXME: Not implemented.
return VisiblePosition();
}
> LayoutTests/editing/selection/move-by-word-visually-expected.txt:1
> +CONSOLE MESSAGE: line 180: TypeError: 'null' is not an object (evaluating 'document.getElementById("testGroup").style')
Do we expect to see this error?
> LayoutTests/editing/selection/move-by-word-visually-expected.txt:37
> +"abc def hij opq rst"[0, 0]
> +"abc def hij opq rst"[1, 1]
> +"abc def hij opq rst"[2, 2]
> +"abc def hij opq rst"[3, 3]
> +"abc def hij opq rst"[4, 4]
> +"abc def hij opq rst"[5, 5]
> +"abc def hij opq rst"[6, 6]
> +"abc def hij opq rst"[7, 7]
> +"abc def hij opq rst"[8, 8]
> +"abc def hij opq rst"[9, 9]
> +"abc def hij opq rst"[10, 10]
> +"abc def hij opq rst"[11, 11]
> +"abc def hij opq rst"[12, 12]
> +"abc def hij opq rst"[13, 13]
> +"abc def hij opq rst"[14, 14]
> +"abc def hij opq rst"[15, 15]
> +"abc def hij opq rst"[16, 16]
> +"abc def hij opq rst"[17, 17]
> +"abc def hij opq rst"[18, 18]
> +"abc def hij opq rst"[19, 19]
How do we know these results are correct? it seems like we want to print either PASS or FAIL. Otherwise, other contributors will have no idea whether this test is passing or failing.
> LayoutTests/editing/selection/move-by-word-visually.html:1
> +<head>
Missing DOCTYPE
> LayoutTests/editing/selection/move-by-word-visually.html:3
> + <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
> + <style>
Let's not indent these elements. Outdenting everything will make code simpler.
> LayoutTests/editing/selection/move-by-word-visually.html:163
> + while (true) {
> + sel.setPosition(prevNode, prevOffset);
> + var positions = [];
> + positions.push({ node: sel.anchorNode, offset: sel.anchorOffset, point: caretCoordinates() });
> + sel.modify("move", direction, "-webkit-visual-word");
> + positions.push({ node: sel.anchorNode, offset: sel.anchorOffset, point: caretCoordinates() });
> + if (direction == "right")
> + checkCoordinatesMovingRightDown(positions);
> + else
> + checkCoordinatesMovingLeftUp(positions);
> + logPositions(positions);
> + log("\n");
> + sel.setPosition(prevNode, prevOffset);
> + sel.modify("move", direction, "character");
> + if (sel.anchorNode == prevNode && sel.anchorOffset == prevOffset) {
> + break;
> + }
> + prevNode = sel.anchorNode;
> + prevOffset = sel.anchorOffset;
> + }
This function seems to be a duplicate of the code above. We should make a function and call it twice instead of duplicating the code.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list