346891 - Deleting LTR text in comments on israblog.co.il sometimes requires two keypresses per character

Status: RESOLVED FIXED

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

Description

[Uri Bernstein]

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

[Uri Bernstein]

Attachment: testcase

Comment 2

[Uri Bernstein]

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

[Uri Bernstein]

(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

[Uri Bernstein]

Attachment: 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).

Comment 5

[Uri Bernstein]

Please ignore the changes in nsFrameSelection::GetPrevNextBidiLevels - they belong to a different bug.

Comment 6

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

This looks like the right thing to me.

+#ifdef IBMBIDI
+  mBidiKeyboard = do_GetService("@mozilla.org/widget/bidikeyboard;1");
+#endif

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

[Uri Bernstein]

Attachment: 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.

Comment 8

[Uri Bernstein]

Attachment: patch v2a

Oops. I forgot to get rid of nsCaret::mBidiKeyboard in the previous patch.

Comment 9

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

Comment on attachment 232577 [details] [diff] [review]
patch v2a

Yeah, that's exactly what I wanted

Comment 10

[Uri Bernstein]

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
done
Checking in layout/generic/nsSelection.cpp;
/cvsroot/mozilla/layout/generic/nsSelection.cpp,v  <--  nsSelection.cpp
new revision: 3.257; previous revision: 3.256
done
Checking in layout/base/nsIPresShell.h;
/cvsroot/mozilla/layout/base/nsIPresShell.h,v  <--  nsIPresShell.h
new revision: 3.178; previous revision: 3.177
done
Checking in layout/base/nsPresShell.cpp;
/cvsroot/mozilla/layout/base/nsPresShell.cpp,v  <--  nsPresShell.cpp
new revision: 3.930; previous revision: 3.929
done
Checking in layout/base/nsCaret.h;
/cvsroot/mozilla/layout/base/nsCaret.h,v  <--  nsCaret.h
new revision: 1.52; previous revision: 1.51
done
Checking in layout/base/nsCaret.cpp;
/cvsroot/mozilla/layout/base/nsCaret.cpp,v  <--  nsCaret.cpp
new revision: 1.163; previous revision: 1.162
done
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
done
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
done
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
done
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
done
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
done