[Webkit-unassigned] [Bug 5241] Space and tab characters "sent" by an input method give totally different results than typing them directly

[bugzilla-daemon at webkit.org]

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





--- Comment #20 from Ryosuke Niwa <rniwa at webkit.org>  2011-01-13 09:49:37 PST ---
(From update of attachment 78794)
View in context: https://bugs.webkit.org/attachment.cgi?id=78794&action=review

The change looks much saner but I still need answers to the following questions.

> LayoutTests/editing/inserting/insert-composition-whitespace.html:52
> +test("div", " AB", "\xA0AB");
> +test("div", "  AB", "\xA0\xA0AB");
> +test("div", "   AB", "\xA0\xA0 AB");
> +test("div", "    AB", "\xA0\xA0 \xA0AB");
> +test("div", "     AB", "\xA0\xA0 \xA0 AB");
> +test("div", "      AB", "\xA0\xA0 \xA0 \xA0AB");
> +test("div", "       AB", "\xA0\xA0 \xA0 \xA0 AB");

Huh. It's kind of odd that these expected results have two consecutive non-breaking spaces.  Is this really expected?

> Source/WebCore/dom/TextEvent.h:39
> +            InputTypeComposition, // input is done by IME, so whitespace characters should be preserved.

I'm not sure if we should mention about whitespace preservation at all here. That sounds rather arbitrary.

> Source/WebCore/editing/CompositeEditCommand.cpp:427
> +        int newOffset = startOffset - 1;
> +        if (newOffset < 0 || !isWhitespace(text[newOffset]))
>              return;
> +        startOffset = endOffset = newOffset;

Why do we do this off-by-one adjustment here?

> Source/WebCore/editing/CompositeEditCommand.cpp:435
> +    int downstream = std::max(startOffset, endOffset - 1);

I don't get this -1 either. Could you explain what they are doing?

> Source/WebCore/editing/InsertTextCommand.cpp:184
> +        if (whitespaceRebalance == WhitespaceRebalanceSurrounding) {
> +            // The insertion may require adjusting adjacent whitespace, if it is present.
> +            rebalanceWhitespaceAt(endPosition);
> +            // Rebalancing on both sides isn't necessary if we've inserted a space.
> +            if (text != " ") 
> +                rebalanceWhitespaceAt(startPosition);
> +        } else {
> +            ASSERT(whitespaceRebalance == WhitespaceRebalanceAll);
> +            rebalanceWhitespaceBetween(startPosition, endPosition);
> +        }

You could use switch but I guess that's an overkill for just two values. But I'm quite surprised about 'text != " "'.  Shouldn't we also be checking for the case where text had 2, 3, 4, ... whitespaces?

> Source/WebCore/editing/InsertTextCommand.h:37
> +        WhitespaceRebalanceSurrounding,
> +        WhitespaceRebalanceAll,

Mn... I think these should be RebalanceLeadingAndTrailingWhitespaces and RebalanceAllWhitespaces respectively.

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