Closed Bug 303781 Opened 15 years ago Closed 15 years ago

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


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

Not set





(Reporter: uriber, Assigned: uriber)




(Keywords: rtl, verified1.8)


(2 files)

In the following testcase:
, 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.
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().
Attachment #191887 - Flags: review?(smontagu)
In comment #0, I meant to say "until bug 303399 is fixed".
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. 
Attached patch patch, updatedSplinter Review
Same patch, updated for changes made in the meantime.
Attachment #191887 - Attachment is obsolete: true
Attachment #194088 - Flags: review?(smontagu)
Attachment #191887 - Flags: review?(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
Attachment #194088 - Flags: review?(smontagu) → review+
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?
Attachment #194088 - Flags: superreview?(roc)
Attachment #194088 - Flags: superreview?(roc) → superreview+
Comment on attachment 194088 [details] [diff] [review]
patch, updated

Requesting approval1.8b5, as this is a simple fix to an annoying problem.
Attachment #194088 - Flags: approval1.8b5?
Checking in editor/libeditor/text/nsTextEditRulesBidi.cpp;
/cvsroot/mozilla/editor/libeditor/text/nsTextEditRulesBidi.cpp,v  <-- 
new revision: 1.15; previous revision: 1.14
Closed: 15 years ago
Resolution: --- → FIXED
Flags: blocking1.8b5+
Comment on attachment 194088 [details] [diff] [review]
patch, updated

Approved for 1.8b5 per bug meeting
Attachment #194088 - Flags: approval1.8b5? → approval1.8b5+
Target Milestone: --- → mozilla1.8beta5
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  <-- 
new revision:; previous revision: 1.13
Attachment #191887 - Attachment description: patch → patch (1.8 branch)
Attachment #191887 - Attachment is obsolete: false
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: zach → layout.fonts-and-text
You need to log in before you can comment on or make changes to this bug.