[Webkit-unassigned] [Bug 25321] RLE/LRE characters are removed from any text entered into a text box (either using a keyboard or by pasting)

[bugzilla-daemon at webkit.org]

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





--- Comment #14 from Ryosuke Niwa <rniwa at webkit.org>  2010-10-12 16:18:11 PST ---
(From update of attachment 67049)
Thanks for tackling this bug!  Here are some preliminary feedbacks.

> WebCore/dom/Position.cpp:556
> +        if (renderer->isText() && mode == PreserveUnicodeBidiControlCharacter) {

Can we do:
if (!renderer->isText())
  continue;
instead of checking renderer->isText() here

> WebCore/dom/Position.cpp:557
> +            unsigned textOffset = currentPos.offsetInLeafNode();

There's a code below that does the exact same thing.  Only difference being checking whether or not there's rendered text.  Can we somehow combine these two code?

> WebCore/dom/Position.cpp:567
>          if (renderer->isText() && toRenderText(renderer)->firstTextBox()) {

and here?  You can file a separate bug to do this cleanup first to make the patch smaller.

> WebCore/dom/Position.cpp:693
> +        if (renderer->isText() &&  mode == PreserveUnicodeBidiControlCharacter) {

Same comment goes here.

> WebCore/dom/Position.cpp:792
> +        if (!nodeIsUserSelectNone(node())) {
> +            bool rendered = inRenderedText();
> +            // In the case of PreserveUnicodeBidiControlCharacter, consider a position begins with unicode bidi control
> +            // character as a candidate as well.
> +            return mode == PreserveUnicodeBidiControlCharacter ? rendered || startWithUnicodeBidiControlCharacter() : rendered;
> +        }
> +        return false;

(I think) you can simplify this as:
return !nodeIsUserSelectNone(node()) && (inRenderedText()
    || (mode == PreserveUnicodeBidiControlCharacter ? startWithUnicodeBidiControlCharacter() : true))

> WebCore/dom/Position.cpp:813
> +bool Position::startWithUnicodeBidiControlCharacter() const

This function name does not seem to match what the function does.

> WebCore/dom/Position.cpp:830
> +        for (int i = m_offset; i < static_cast<int>(static_cast<Text*>(node())->length()); ++i) {
> +            UChar ch = static_cast<Text*>(node())->data()[i];
> +            if (!isBidiControlCharacter(ch))
> +                return false;
> +        }
> +        return true;

e.g. this code seems to return true only if everything in the text box was bidi control character, which doesn't match what the function name implies.  Could you clarify what you're trying to do in this function?

> WebCore/dom/Position.cpp:858
> +        // Check against unicode bidi control characters after text box end.
> +        if (m_offset > static_cast<int>(box->start() + box->len())) {
> +            int i = static_cast<int>(box->start() + box->len());
> +            for (; i < m_offset; i++) {
> +                UChar ch = static_cast<Text*>(node())->data()[i];
> +                if (!isBidiControlCharacter(ch))
> +                    break;
> +            }
> +            if (i == m_offset)
> +                return true;
> +        }

I don't understand why we need to check the control characters after the text box.  Is that because control characters after the text box influence whatever proceeds it?  Could you clarify?

> WebCore/dom/Position.h:70
> +    enum EditingMode {

I think EditingMode is very general name, and can easily be confused with editing behavior and other things.  Please rename it to something more specific like UnicodeBidiControlCharacterPreservationMode or BidiControlCharacterMode if you think the formar is too long.

> WebCore/dom/Position.h:164
> +    Position upstream(EditingBoundaryCrossingRule = CannotCrossEditingBoundary, EditingMode = NotPreserveUnicodeBidiControlCharacter) const;
> +    Position downstream(EditingBoundaryCrossingRule = CannotCrossEditingBoundary, EditingMode = NotPreserveUnicodeBidiControlCharacter) const;

Is there a case where we don't want to preserve the bidi control character?  i.e. are there cases where we intentionally remove them?

> WebCore/editing/VisiblePosition.cpp:1
> +

You probably didn't intend to insert a blank line here, did you?

> WebCore/editing/VisiblePosition.cpp:48
> +VisiblePosition::VisiblePosition(const Position &pos, EAffinity affinity, Position::EditingMode editMode)
> +    : m_editMode(editMode)    

Again, editMode sounds too general.

> WebCore/editing/VisibleSelection.cpp:171
> +        s = m_start.downstream(Position::CannotCrossEditingBoundary, m_editMode);
> +        e = m_end.upstream(Position::CannotCrossEditingBoundary, m_editMode);

You may consider changing the order of EditingBoundaryCrossingRule and EditingMode (or whatever you rename to) so that you don't have to keep repeating CannotCrossEditingBoundary here and there.  As far as I searched, only RenderObject::createVisiblePosition calls upstream / downstream with CanCrossEditingBoundary.

> LayoutTests/editing/inserting/insert-paste-bidi-control.html:89
> +runEditingTest();

Do we need editing delegates dump & rendering results for this test?  If we're only concerned about bidi control characters being preserved, then we can use dump-as-text test.  i.e. you call runDumpAsTextEditingTest() here instead of the regular runEditingTest.  If that's not sufficient, then you can add a little script above to check the existence of bidi control characters and print PASS/FAIL.

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