[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 #22 from MORITA Hajime <morrita at google.com>  2011-01-14 06:02:32 PST ---
Hi Ryosuke, thank you for reviewing!
I updated the patch again for addressing the feedback.

(In reply to comment #20)
> (From update of attachment 78794 [details])
> 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?
Honestly, I have no idea. This is the default behavior.

> 
> > 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.
> 
Removed the redundant part.

> > 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?
> 
I have no idea here...
But I tried to sweep out one-by-one tricks around here and tests passed.

> > 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?
Purged one-by-one.

> 
> > 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?
My guess is that this is for handling special case for typing the space key.
Updated patch generalized this though.

> 
> > Source/WebCore/editing/InsertTextCommand.h:37
> > +        WhitespaceRebalanceSurrounding,
> > +        WhitespaceRebalanceAll,
> 
> Mn... I think these should be RebalanceLeadingAndTrailingWhitespaces and RebalanceAllWhitespaces respectively.
Sure. Renamed.

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