Deleting LTR text in comments on sometimes requires two keypresses per character

RESOLVED FIXED in mozilla1.9alpha1



Layout: Text
12 years ago
10 years ago


(Reporter: Uri Bernstein (Google), Assigned: Uri Bernstein (Google))




Firefox Tracking Flags

(Not tracked)




(2 attachments, 2 obsolete attachments)



12 years ago
When editing a comment on any blog on Israblog, if you have LTR text followed by RTL text, and you try deleting the last characters of the LTR text using the backspace key, deleting each character will require two presses of "backspace".

This has to do with the "comment length counter" feature, which is implemented using an onkeydown event handler.

I haven't included a direct URL to a comments page in order to avoid spamming a poor blog with test comments. I'll attach a minimal testcase soon.

Comment 1

12 years ago
Created attachment 231627 [details]

Comment 2

12 years ago
The problem seems to be that the (global) caret level gets undefined (in nsTextEditRules::AfterEdit) every time the text inside the "counter" box is set.

I'm not sure what the solution should be. Having a caret level per selection controller, instead of a global one? But then, will fixing bug 339400 break it?

Or perhaps the "undefine caret level" code shouldn't be in nsTextEditRules::AfterEdit, so that it would only be executed for user-initiated actions, but not for setting of text programmatically?

Comment 3

12 years ago
(In reply to comment #2)
> Having a caret level per selection
> controller, instead of a global one? But then, will fixing bug 339400 break it?

I actually meant to say "store the caret level on nsFrameSelection". Also, I apparently misunderstood bug 339400 - which is not calling for having a single nsFrameSelection per document.
Also, when I said "global", I really meant "one per document" (stored on the nsPresShell).
Thanks to Simon for pointing these out to me.

I'll try implementing this solution.

Comment 4

12 years ago
Created attachment 231934 [details] [diff] [review]
patch v1

This patch moves the caret bidi level, and the methods used to access it, from nsPresShell to nsFrameSelection.
I feel I might be doing something wrong here: nsFrameSelection apparently isn't meant to be "public", and accessing its methods from outside layout (specifiaclly, for the editor library), requires defining the methods "virtual".

Alternatives might be: Store the caret bidi level on nsTypedSelection, and providing access to it via nsISelectionPrivate; Store  the caret bidi level on nsFrameSelection, but only provide access to it via methods in nsTypedSelection (exposed in nsISelectionPrivate).
Assignee: nobody → uriber
Attachment #231934 - Flags: superreview?(roc)

Comment 5

12 years ago
Please ignore the changes in nsFrameSelection::GetPrevNextBidiLevels - they belong to a different bug.
This looks like the right thing to me.

+#ifdef IBMBIDI
+  mBidiKeyboard = do_GetService(";1");

How about removing mBidiKeyboard and making the bidi keyboard service one of the static services exported by nsContentUtils?

+      nsCOMPtr<nsISelectionPrivate> privateSelection(do_QueryReferent(mDomSelectionWeak));
+      if (!privateSelection)
+        return NS_ERROR_FAILURE;
+      nsCOMPtr<nsFrameSelection> frameSelection;
+      privateSelection->GetFrameSelection(getter_AddRefs(frameSelection));

Make this into a helper function that returns an unref'ed nsFrameSelection*, that you can call from the three places you're adding this code?

Comment 7

12 years ago
Created attachment 232572 [details] [diff] [review]
patch v2

Implemented the two requests from comment 6.

I used a protected method, rather than a static function, since I didn't want to have to pass mDomSelectionWeak into it. I hope that's OK.
Also, I hope I got the "unref'ed" part OK.
Attachment #231934 - Attachment is obsolete: true
Attachment #232572 - Flags: review?(roc)
Attachment #231934 - Flags: superreview?(roc)

Comment 8

12 years ago
Created attachment 232577 [details] [diff] [review]
patch v2a

Oops. I forgot to get rid of nsCaret::mBidiKeyboard in the previous patch.
Attachment #232572 - Attachment is obsolete: true
Attachment #232577 - Flags: review?(roc)
Attachment #232572 - Flags: review?(roc)
Comment on attachment 232577 [details] [diff] [review]
patch v2a

Yeah, that's exactly what I wanted
Attachment #232577 - Flags: superreview+
Attachment #232577 - Flags: review?(roc)
Attachment #232577 - Flags: review+

Comment 10

12 years ago
Checked in:

Checking in layout/generic/nsFrameSelection.h;
/cvsroot/mozilla/layout/generic/nsFrameSelection.h,v  <--  nsFrameSelection.h
new revision: 1.8; previous revision: 1.7
Checking in layout/generic/nsSelection.cpp;
/cvsroot/mozilla/layout/generic/nsSelection.cpp,v  <--  nsSelection.cpp
new revision: 3.257; previous revision: 3.256
Checking in layout/base/nsIPresShell.h;
/cvsroot/mozilla/layout/base/nsIPresShell.h,v  <--  nsIPresShell.h
new revision: 3.178; previous revision: 3.177
Checking in layout/base/nsPresShell.cpp;
/cvsroot/mozilla/layout/base/nsPresShell.cpp,v  <--  nsPresShell.cpp
new revision: 3.930; previous revision: 3.929
Checking in layout/base/nsCaret.h;
/cvsroot/mozilla/layout/base/nsCaret.h,v  <--  nsCaret.h
new revision: 1.52; previous revision: 1.51
Checking in layout/base/nsCaret.cpp;
/cvsroot/mozilla/layout/base/nsCaret.cpp,v  <--  nsCaret.cpp
new revision: 1.163; previous revision: 1.162
Checking in editor/libeditor/text/nsTextEditRules.cpp;
/cvsroot/mozilla/editor/libeditor/text/nsTextEditRules.cpp,v  <--  nsTextEditRules.cpp
new revision: 1.204; previous revision: 1.203
Checking in editor/libeditor/text/nsTextEditRulesBidi.cpp;
/cvsroot/mozilla/editor/libeditor/text/nsTextEditRulesBidi.cpp,v  <--  nsTextEditRulesBidi.cpp
new revision: 1.21; previous revision: 1.20
Checking in editor/libeditor/html/nsHTMLEditRules.cpp;
/cvsroot/mozilla/editor/libeditor/html/nsHTMLEditRules.cpp,v  <--  nsHTMLEditRules.cpp
new revision: 1.325; previous revision: 1.324
Checking in content/base/public/nsContentUtils.h;
/cvsroot/mozilla/content/base/public/nsContentUtils.h,v  <--  nsContentUtils.h
new revision: 1.104; previous revision: 1.103
Checking in content/base/src/nsContentUtils.cpp;
/cvsroot/mozilla/content/base/src/nsContentUtils.cpp,v  <--  nsContentUtils.cpp
new revision: 1.175; previous revision: 1.174
Last Resolved: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha


10 years ago
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: layout.bidi → layout.fonts-and-text
You need to log in before you can comment on or make changes to this bug.