[Webkit-unassigned] [Bug 57543] -webkit-visual-word does not work on words separated by multiple spaces

[bugzilla-daemon at webkit.org]

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


Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #93675|review?                     |review-
               Flag|                            |




--- Comment #22 from Ryosuke Niwa <rniwa at webkit.org>  2011-05-16 15:59:28 PST ---
(From update of attachment 93675)
View in context: https://bugs.webkit.org/attachment.cgi?id=93675&action=review

> Source/WebCore/editing/visible_units.cpp:1182
> +        // and the "hij opq" (starts at 12 and length is 7). The word breaks are 
> +        // "abc |def |    hij |opq". We are traversing these boxes from right to left. When
> +        // traversing box "abc def", the word break between "def" and "hij" (which is position 8)

You need to clarify why we need to special-case this and which one of "abc def" or "hij opq" is current box.

How about something like this?

We normally catch the line break between "def" and "hij" when we visit the box that contains "hij opq"
but the line break doesn't exist in the box that contains "hij opq" when there are multiple spaces.
So we detect it when we're traversing the box that contains "abc def " instead.

> Source/WebCore/editing/visible_units.cpp:1190
> +            positionBeforeWord.getInlineBoxAndOffset(boxContainingPreviousWordBreak, offsetOfWordBreak);

To address the issue below, you should declare boxContainingPreviousWordBreak here.

> Source/WebCore/editing/visible_units.cpp:1201
> -    InlineBox* boxContainingPreviousWordBreak;
>      wordBreak.getInlineBoxAndOffset(boxContainingPreviousWordBreak, offsetOfWordBreak);

I don't think we should be reusing variable like this.  Please declare boxContainingPreviousWordBreak here.

> Source/WebCore/editing/visible_units.cpp:1321
> +        InlineBox* boxContainingWordBreak;
> +        int offset;
> +        positionAfterWord.getInlineBoxAndOffset(boxContainingWordBreak, offset);
> +    
> +        if (boxContainingWordBreak == box) {

Can we extract this as a function?  e.g. positionIsInBox(const VisiblePosition&, InlineBox*)?

> Source/WebCore/editing/visible_units.cpp:1330
> +    previousWordBreakWasAtBoxStart = false;
> +
> +    VisiblePosition wordBreak = (hasSeenWordBreakInThisBox && !previousWordBreakWasAtBoxStart) ? previousWordBreak : Position(box->renderer()->node(), box->caretMinOffset(), Position::PositionIsOffsetInAnchor);

Huh? Isn't previousWordBreakWasAtBoxStart always false here?  r- because of this.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.