[Webkit-unassigned] [Bug 24303] Using keyboard select RTL text, Highlight goes to opposite direction from FF&IE

[bugzilla-daemon at webkit.org]

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





------- Comment #13 from xji at chromium.org  2009-03-19 13:05 PDT -------
Hi Mitz,

Thanks for your quick review!
Please see my comments inlined.


(In reply to comment #12)
> (From update of attachment 28747 [review])
> 
> > +2009-03-18  Xiaomei Ji  <xji at chromium.org>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        Tests: editing/selection/extend-selection-bidi-start-with-english-in-rtl.html
> > +               editing/selection/extend-selection-bidi-start-with-hebrew-in-rtl.html
> > +               editing/selection/extend-selection-english-in-rtl.html
> > +               editing/selection/extend-selection-hebrew-in-rtl.html
> > +
> > +        * editing/SelectionController.cpp:
> > +        (WebCore::SelectionController::modifyExtendingRightForward):
> > +        (WebCore::SelectionController::modifyExtendingForward):
> > +        (WebCore::SelectionController::modifyExtendingLeftBackward):
> > +        (WebCore::SelectionController::modifyExtendingBackward):
> > +        * editing/SelectionController.h:
> > +
> 
> The change log entry needs to include the bug's title and a link to it. It
> should also list the changes to each function.

Will do.

> 
> >  VisiblePosition SelectionController::modifyExtendingRightForward(TextGranularity granularity)
> >  {
> >      VisiblePosition pos(m_sel.extent(), m_sel.affinity());
> > +
> > +#if PLATFORM(WIN_OS)
> > +    WebCore::Node *n = pos.deepEquivalent().node();
> > +    if (n) {
> > +        WebCore::RenderObject* renderer = n->renderer();
> > +        if (renderer) {
> > +            if (renderer->style()->direction() == WebCore::RTL) {
> > +                return modifyExtendingBackward(granularity, pos);
> > +            }
> > +        }  
> > +    }
> > +#endif
> 
> modifyExtendingRightForward() is called from both the RIGHT and FORWARD cases
> of modify(), but this new behavior is the "extend right" behavior, so it is
> wrong to do it in the FORWARD case. Like I said before, I think you need to
> define both modifyExtendingRight() and modifyExtendinForward(), implementing
> the new behavior in the former (and leaving existing behavior in the latter),
> and then change modify() to call the appropriate function in every case.

I did not split into modifyExtendingRight() and modifyExtendingForward()
because I thought "Right" implies visual.
I will do as what you suggested.

> 
> I don't think these changes should be limited to WIN_OS or to any specific
> platform. For now, this is supposed to be an improvement over the existing
> behavior for all platforms.

Will do that.


> 
> You are checking the direction of the node's renderer, but is this what Firefox
> does? I was under the impression that it uses the direction of the enclosing
> block. You can see if your patch behaves like Firefox in these cases:
> <div>Lorem <span style="direction: rtl">ipsum dolor sit</span> amet</div>
> <div>Lorem <span dir="rtl">ipsum dolor sit</span> amet</div>

You are right.
I will make the change.

> 
> > Index: LayoutTests/ChangeLog
> > ===================================================================
> > --- LayoutTests/ChangeLog	(revision 41823)
> > +++ LayoutTests/ChangeLog	(working copy)
> > @@ -1,3 +1,32 @@
> > +2009-03-18  Xiaomei Ji  <xji at chromium.org>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> 
> This should also reference the bug.

Will do.

> 
> > +        * editing/selection/extend-selection-bidi-start-with-english-in-rtl.html: Added.
> > +        * editing/selection/extend-selection-bidi-start-with-hebrew-in-rtl.html: Added.
> > +        * editing/selection/extend-selection-english-in-rtl.html: Added.
> > +        * editing/selection/extend-selection-hebrew-in-rtl.html: Added.
> 
> Can these be combined into a single test?

I will try.

> 
> > +        * platform/mac/editing/selection/extend-selection-bidi-start-with-english-in-rtl-expected.checksum: Added.
> > +        * platform/mac/editing/selection/extend-selection-bidi-start-with-english-in-rtl-expected.png: Added.
> > +        * platform/mac/editing/selection/extend-selection-bidi-start-with-english-in-rtl-expected.txt: Added.
> 
> These should be text-only tests (using layoutTestController.dumpAsText). And
> since the change should not be platform-specific, there shouldn't be
> platform-specific results.

I was following the extend-selection-bidi.html test.
Will try to use layoutTestController.dumpAsText.
Do you mean after which, the png and checksum files are not necessary (if the
text files show difference)?

> 
> > +        extendSelectionForwardByCharacterCommand();
> 
> As mentioned above, the behavior of "forward" should not change. You should be
> testing the behavior of extending "right" and "left". You are not testing word,
> sentence, line and paragraph granularities. If you change them (like this patch
> does) then you should test all of them. But are you sure you want to change the
> behavior for all granularities?
> 

You are right.
I think it would be better to restrict such change to only character and word 
for now. And I will introduce the 2 new functionalities and test them out on 2
granularities.

Thanks,
Xiaomei


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