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.
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?
(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.
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
Status: NEW → ASSIGNED
Attachment #231934 - Flags: superreview?(roc)
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("@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?
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.
Created attachment 232577 [details] [diff] [review] patch v2a Oops. I forgot to get rid of nsCaret::mBidiKeyboard in the previous patch.
Comment on attachment 232577 [details] [diff] [review] patch v2a Yeah, that's exactly what I wanted
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
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha
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.