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

Fri Mar 27 12:15:45 PDT 2009

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

------- Comment #21 from xji at chromium.org  2009-03-27 12:15 PDT -------
Hi Mitz,

Thanks for the review!
Please see my comments inlined.

(In reply to comment #16)
> (From update of attachment 28815 [review])
> > Looks like FF does not across block boundary when extending selection.
> > But currently, Webkit across block boundary while extending selection if the
> > blocks are parent-child. It can extend selection inward or outward.
> > For example: 
> > <div> whatever <div dir="rtl"> just a test</div> end</div>
> > selection can be extended from the block "just a test" to "whatever ... end",
> > and vice versa.
> >
> > In the current fix, when checking the directionality of enclosing block 
> > (SelectionController::directionOfEnclosingBlock()), I used the starting point
> > of the selection (m_sel.base().node()), not the ending point
> > (m_sel.extend().node()).
> 
> What does Firefox do when the selection already crosses a block boundary and
> the extent is in a block that has different directionality from the base? I
> think using the directionality of the block enclosing the extent makes more
> sense, because the extent is what is being modified, although I realize that it
> may be weird when crossing into a different direction, like you said.
> 

I've changed the code to use the directionality of the block enclosing the
extent.

I finally figured out how FF does selection across block boundary.
I thought it does not. But actually it does, and it does in a way that an
implicit/control character is inserted between block boundary. 

I have attached 2 snapshots showing you the difference of FF selection and
Webkit selection (after using the direction of the enclosing block contains the
extent, not the base in the fix).

For example:
<div contenteditable >Lorem <span  style="direction: rtl">ipsum dolor<div >
what ever you want </div> sit</span> amet</div>,

Although not showing visually in selection, but from the cursor movement and
selection range, an implicit/control character is inserted between 'r' in
"dolor' and 'w' in 'what".
So, if cursor is between 'o' and 'r' in 'dolor', press shift-right-arrow
highlights 'r', press it again does not show anything visually, but it actually
selects the implicit character. 

At this state, if press shift-left-arrow, it will select 'w'. 
But if press shift-right-arrow, it un-selects the implicit character (which not
showing visually), if press shift-left-arrow then, it un-selects 'r'.

So, if the cursor is between 'o' and 'r', press shift-right-arrow once select
'r', press shift-right-arrow in odd number of times, then, press
shift-left-arrow will select 'w'. But if press shift-right-arrow in even number
of times,
 then, press shift-left-arrow will un-select 'r', because press
shift-right-arrow in even number of times actually selects and un-selects the
implicit/control character, although the selection does not show any visual
effect.

Webkit behaves similar with the only difference that it
highlights/un-highlights the implicit character selection.

For FF to selection from "what ever you want" to "sit amet" when the highlight
already covers 't' in 'want', you need to press shift-left-arrow twice, then,
shift-right-arrow twice to select to 's' in 'sit. I do not now why.

In WebKit, you just need to press shift-left-arrow once to select the implicit
character, then, shift-right-arrow once to select 's' in 'sit'.

For another example, 
<div contenteditable >Lorem <div dir="rtl"> what ever you want </div>
amet</div>

For FF to continue selection from "Lorem" to 'w', it needs to press shift-right
arrow twice, then shift-left-arrow once.
While Webkit needs to press shift-right arrow once, then shift-left-arrow once.

I tried in Mac, seems FF in Mac behaves the same as in Windows.

By comparing Webkit with FF, I do not think WebKit is worse.
The only weird thing in Webkit is the select/un-select of implicit character
when continuing press shift-right-arrow after selecting "Loren ipsum dolor",
while FF's selection stays the same.

But in a 2nd thought, Webkit visually shows what happen. It might be even
better.

And if I copy what selected from FF or Webkit(with the selection of implicit
character) and paste them, the text pasted does not show differences.

> > +TextDirection SelectionController::directionOfEnclosingBlock() {
> > +    Node *n = m_sel.base().node();
> 
> The * should go next to "Node".

Done.

> 
> > +    Node* enclosingBlockNode = enclosingBlock(n);
> > +    if (enclosingBlockNode) {
> 
> You can write this as
>        if (Node* enclosingBlockNode = enclosingBlock(n))...
> 
> or better yet, you can save some indentation by reversing the condition and
> turning this into an early return:
>        if (!enclosingBlockNode)
>            return LTR;

Used the 2nd suggestion.

> 
> > +        WebCore::RenderObject* renderer = enclosingBlockNode->renderer();
> 
> Since this function is in the WebCore namespace, you shouldn't add the
> "WebCore::" prefix.

Removed.

> 
> > +        if (renderer) {
> > +            if (renderer->style()->direction() == WebCore::RTL) {
> > +                return RTL;
> > +            }
> 
> The WebKit coding style is to omit braces around single-line blocks like the
> above. But the inner if statement is unnecessary. You can simply write
>            if (renderer)
>                return renderer->style()->direction();

Done.

> 
> > +        case CharacterGranularity:
> > +            if (directionOfEnclosingBlock() == RTL) {
> > +                pos = pos.previous(true);
> > +            } else {
> > +                pos = pos.next(true);
> > +            }
> 
> Please remove the braces around those single-line blocks.

Done.

> 
> In existing WebCore code, sometimes the code for the RTL case comes first and
> sometimes it comes second. In new code, I would rather make the LTR case come
> first.

Done.

> 
> > +        case WordGranularity:
> > +            if (directionOfEnclosingBlock() == RTL) {
> > +                pos = previousWordPosition(pos);
> > +            } else {
> > +                pos = nextWordPosition(pos);
> > +            }
> 
> Ditto re: braces and order.
> 

Done.

Will attach the patch for review soon.

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.