243931 - GTK2 build on AIX: Caret misplaced when inserting RTL input into LTR text

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Lina Kemmel]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0)
Build Identifier: 

In mozilla text widgets, caret is misplaced when inserting RTL input into LTR 
text.
The problem is not reproducible on Windows.

Reproducible: Always
Steps to Reproduce:
1. Switch language layer to Hebrew or Arabic.
2. Locate the caret in the Address bar with some LTR text (note that gtk2 
doesn't provide contextual language switching, so the keyboard language remains 
RTL).
3. Start inputting RTL characters.

Actual Results:  
The caret is displayed at the leading edge of a typed character.

Expected Results:  
The caret is displayed at the trailing edge of a typed character.

Comment 1

[Lina Kemmel]

Simon, it seems I have an explanation to what's going on.

After repositioning, caret level is derived from the level of the content it's 
related to. So in Step 2 above it gets level 0.

After new text is inserted, caret level is reset to BIDI_LEVEL_UNDEFINED - to 
be recalculated later, according to the level of the inserted text. - That's 
was supposed to take place in Step 3.

However, on IME composing (which in our case takes place when RTL text is 
inserted), gtk emits the "commit" signal, as opposed to the "key_press_event" 
signal in the absence of IME composing. Event handlers will finally translate 
the former into |kOpInsertText| Editor operation, and the later - into 
|kOpInsertIMEText|.
The only problem is, that |nsTextEditRules::AfterEdit|, when deciding about 
undefying the level, disregards |kOpInsertIMEText|, and checks only 
for "simple" text insertion.

Once I added the missing condition, the problem was resolved.
I'll submit a patch shortly.

Comment 2

[Lina Kemmel]

Sorry, it should have been vice versa:

"...the former ["commit] - into |kOpInsertIMEText| Editor operation, and the 
latter ["key_press_event"] - into |kOpInsertText|".

Comment 3

[Lina Kemmel]

Attachment: Proposed patch

Comment 4

[Lina Kemmel]

Simon, can you review attachment 148756 [details] [diff] [review]? or help in determining r/sr? Thanks!

Comment 5

[Simon Montagu :smontagu]

Comment on attachment 148756 [details] [diff] [review]
Proposed patch

r=smontagu

Comment 6

[Simon Montagu :smontagu]

Comment on attachment 148756 [details] [diff] [review]
Proposed patch

Lina, when setting the review request flags you should insert the reviewer's
name in the "Requestee" field.

Comment 7

[Lina Kemmel]

OK, Simon. And if I don't know reviewer's name - I shouldn't set the review 
request flags at all? I think I've seen "review ?" without a name.

Comment 8

[Simon Montagu :smontagu]

Patch checked in.

(In reply to comment #7)
> OK, Simon. And if I don't know reviewer's name - I shouldn't set the review 
> request flags at all? I think I've seen "review ?" without a name.

I don't think there's any rule against it, but there is much more chance of
getting a prompt review if you request by name.

Comment 9

[Lina Kemmel]

The bug is fixed for nsPlaintextEditor, but still occurs in nsHTMLEditor.

Comment 10

[Lina Kemmel]

Attachment: Patch for the HTML editor

Similarly to a plain text editor, consider |action ==
nsEditor::kOpInsertIMEText| also in an HTML editor.

Comment 11

[Simon Montagu :smontagu]

Comment on attachment 150647 [details] [diff] [review]
Patch for the HTML editor

Sorry, I should have spotted that when reviewing the first patch. r=smontagu.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> there is much more chance of getting a prompt review if you request by name.

IN fact there is really no chance of ever getting a review if you don't put in a
name.

Comment 13

[Lina Kemmel]

> IN fact there is really no chance of ever getting a review if you don't put 
> in a name.

I see, thanks.


Can anyone check in the fix please?

Comment 14

[Philip K. Warren]

I'll check this in today.

Comment 15

[Philip K. Warren]

Fixed.

Checking in nsHTMLEditRules.cpp;
/cvsroot/mozilla/editor/libeditor/html/nsHTMLEditRules.cpp,v  <-- 
nsHTMLEditRules.cpp
new revision: 1.312; previous revision: 1.311
done

Comment 16

[(no longer active)]

Mass-assigning the new rtl keyword to RTL-related (see bug 349193).