303781 - Bidi: In RTL text in a textarea, deleting a blank line requires pressing "Backspace" twice

Status: RESOLVED FIXED

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

Description

[Uri Bernstein]

In the following testcase: https://bugzilla.mozilla.org/attachment.cgi?id=128681
, use the arrow keys to place the caret on any of the blank lines (in either of
the textareas). Press "Backspace" (or "Delete") once. The caret will move (until
bug 203399 is fixed), but the empty line will not be deleted. Pressing
"Backspace" (or "Delete") again will delete the blank line.

Comment 1

[Uri Bernstein]

Attachment: patch (1.8 branch)

When the caret is next to a BRFrame, aSelNode contains the textarea itself (not
a text node), and aSeloffset is the offset of the BRFrame inside the textarea
(not the offset of the caret inside the text node). In this case, looking at
the bidi properties of aSelNode's content and its neighbours is useless - we
need to drill down to the BRFrame itself.

The patch was inspired by nsSelection::GetFrameForNodeOffset(), which basically
does the same thing before calling GetChildFrameContainingOffset().

Comment 2

[Uri Bernstein]

In comment #0, I meant to say "until bug 303399 is fixed".

Comment 3

[Uri Bernstein]

I did some testing in designmode (HTML editor), and I found that my patch works,
without having to do recursive drilling-down into the node, because the node
passed to CheckBidiLevelForDeletion() (as aSelNode) is always either a text
node, or the immediate container of the non-text node to be deleted.

e.g, for: "a<br/><br/>b", when deleting the empty line, aSelNode is the
nsHTMLBodyElement (and aSelOffset is 2).
But for "a<i>b<br/><br/>c</i>d", aSelNode is the nsHTMLSpanElement (for the
<i>), and aSelOffset is again 2. 

Comment 4

[Uri Bernstein]

Attachment: patch, updated

Same patch, updated for changes made in the meantime.

Comment 5

[Simon Montagu :smontagu]

Comment on attachment 194088 [details] [diff] [review]
patch, updated

r = me since this patch improves behaviour (though the more I look at this code
the more fundamental problems I see with it, but we already have bugs about
that)

Comment 6

[Uri Bernstein]

Comment on attachment 194088 [details] [diff] [review]
patch, updated

Thnaks, Simon. Would you mind pointing out the bug numbers for the fundamental
problems you mention?

Comment 7

[Uri Bernstein]

Comment on attachment 194088 [details] [diff] [review]
patch, updated

Requesting approval1.8b5, as this is a simple fix to an annoying problem.

Comment 8

[Asaf Romano (gone)]

Checking in editor/libeditor/text/nsTextEditRulesBidi.cpp;
/cvsroot/mozilla/editor/libeditor/text/nsTextEditRulesBidi.cpp,v  <-- 
nsTextEditRulesBidi.cpp
new revision: 1.15; previous revision: 1.14
done

Comment 9

[Mike Schroepfer]

Comment on attachment 194088 [details] [diff] [review]
patch, updated

Approved for 1.8b5 per bug meeting

Comment 10

[Asaf Romano (gone)]

Comment on attachment 191887 [details] [diff] [review]
patch (1.8 branch)

1.8 Branch:
Checking in nsTextEditRulesBidi.cpp;
/cvsroot/mozilla/editor/libeditor/text/nsTextEditRulesBidi.cpp,v  <-- 
nsTextEditRulesBidi.cpp
new revision: 1.13.20.1; previous revision: 1.13
done

Comment 11

[(no longer active)]

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