Closed Bug 1162818 Opened 10 years ago Closed 10 years ago

[IME] Typing a name in Japanese in facebook message compose produces junk (or crashes)

Categories

(Core :: DOM: UI Events & Focus Handling, defect)

Product:
Core
Component:
DOM: UI Events & Focus Handling
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla41
Tracking Flags:
Tracking Status
firefox40 --- wontfix
firefox41 --- fixed

People

(Reporter: birtles, Assigned: masayuki)

References

(
URL
)

Details

(Keywords: inputmethod)

Attachments

(9 files, 14 obsolete files)

Screenshot during typing
6.98 KB, image/png
Details
Screenshot after changing window and back
2.79 KB, image/png
Details
part.7 Add test for reframing focused editor when it has composition (r=smaug)
13.45 KB, patch
Details | Diff | Splinter Review
part.1 nsEditor shouldn't release/forget mComposition becuase it should be handled by it after reframing
8.47 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
part.2 Make it possible to set IME selection from outside of IMETextTxn (r=ehsan)
7.48 KB, patch
Details | Diff | Splinter Review
part.3 nsEditor should store actual composition string length in it (r=ehsan)
3.92 KB, patch
Details | Diff | Splinter Review
part.4 Restore IME selection at initializing selection of the editor (r=ehsan)
2.87 KB, patch
Details | Diff | Splinter Review
part.5 The offset of nsEditor::InsertTextImpl() should be minimum offset of IME selections if there is
17.55 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
part.6 Don't notify IME of anything during reframing the editor (r=smaug)
7.13 KB, patch
Details | Diff | Splinter Review
STR: 1. Go to https://www.facebook.com/messages/new 2. With IME active, type something in the 宛先/To field, e.g. n-a-k-a-n-o Expected: なかの is displayed and the candidate window pops up. Actual: Something like 'nなな' is display but appears to be clipped. If I change windows and come back I see 'ｎななｋなかなかｎなかのなかの'. I'm seeing this with Google IME on Windows 8.1, but Yoshinaga-san confirmed the same behavior with the Microsoft IME. On a different computer running Aurora with Microsoft IME I get a crash on the first keystroke. Unfortunately I don't have the crashlog handy but it is crashing in imjpapi.dll.
(In reply to Brian Birtles (:birtles) from comment #0) > On a different computer running Aurora with Microsoft IME I get a crash on > the first keystroke. Unfortunately I don't have the crashlog handy but it is > crashing in imjpapi.dll. Here is the crash report, same STR: https://crash-stats.mozilla.com/signature/?product=Firefox&user_comments=~facebook&signature=imjpapi.dll%400x1d083&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&page=1
(In reply to Brian Birtles (:birtles) from comment #2) > (In reply to Brian Birtles (:birtles) from comment #0) > > On a different computer running Aurora with Microsoft IME I get a crash on > > the first keystroke. Unfortunately I don't have the crashlog handy but it is > > crashing in imjpapi.dll. > > Here is the crash report, same STR: > https://crash-stats.mozilla.com/signature/ > ?product=Firefox&user_comments=~facebook&signature=imjpapi. > dll%400x1d083&_columns=date&_columns=product&_columns=version&_columns=build_ > id&_columns=platform&_columns=reason&_columns=address&page=1 Sorry, the following URL gives the full set: https://crash-stats.mozilla.com/signature/?product=Firefox&signature=imjpapi.dll%400x1d083&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&page=1
Odd... I guess that we may behave unexpectedly for MS-IME.
Perhaps, the crash is TSF only but the broken behavior isn't recent regression since I can reproduce this bug with 31 ESR too.
I can reproduce this bug on Linux too. Perhaps, on Mac too.
OS: Windows 8.1 → All
Hardware: x86_64 → All
Hmm, we need a simple testcase for this. According to the log of IMEStateManager, the editor is recreated continuously at every input since the <input> element's width are adjusted for its content.　However, I cannot reproduce this bug with following testcase: http://jsfiddle.net/d_toybox/xhzmva34/
Perhaps, this might be a regression of bug 806996. When we reframe an editor, we notify IME of focus change. If it's the cause, we should just reinitialize IMEContentObserver.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Component: DOM: Events → Event Handling
Okay, http://jsfiddle.net/xhzmva34/1/ is the simplest testcase, although, it cannot cause the crash.
Input text corruption using testcase of comment #9 with IME on: keypres: nakano Results: ｎななｋなかなかｎなかのなかの Regressed Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=196f5b34b6e3&tochange=eabdc8f1c344 Triggered by: 7fc0a1ede6a9 Masayuki Nakano — Bug 713502 Fire input event even during composition r=smaug+ehsan
I can reproduce the crash using testcase of comment #9 on Aurora49.0a2 and Nightly40.0a1+Windows7+IME2010 with e10s disabled. But not crash on 38.0Build3. Disable TSF fixes the crash. STR 1. Open testcase 2. click the input area (not need ime on) 3. click empty area, then repeat step 2 if not crash Pushlog(force enable TSF):
With these patches, editor side's bugs are probably fixed. However, in TSF mode, TIP is confused sometimes. I guess that we can fix it with preventing to notify IME of all notifications during reframing the editor.
We need to restore composition after focused editor is reframed. For that, nsEditor shouldn't forget mComposition at being destroyed. And then, mComposition is not nullptr even during restoring the old value with InsertText(). In this time, nsEditor shouldn't insert text as a part of composition. Therefore, IsPossibleToHandleAsIMEComposition() is useful since it checks mDidPostCreate. (The value is restored before PostCreate() is called.)
Attachment #8609753 - Attachment is obsolete: true
Attachment #8612138 - Flags: review?(ehsan)
IME selections are now created in IMETextTxn. On the other hand, the composition string is restored without IME selection at reframing. So, the part of creating IME selections in IMETextTxn should be accessible from nsEditor.
Attachment #8612139 - Flags: review?(ehsan)
And also, nsEditor should store the last composition string length which is handled by the editor since the restoring composition string might be different from the composition string stored in mComposition.
Attachment #8609754 - Attachment is obsolete: true
Attachment #8609755 - Attachment is obsolete: true
Attachment #8612140 - Flags: review?(ehsan)
Now, nsEditor can restore IME selection when the editor is reframed. This patch makes nsEditor tries to create IME selections from InitializeSelection(). Note that this doesn't work fine if the editor has composition but the window is deactive. It's possible on Linux and Mac because their IME context is created per window. But I've not found a good way to fix this. Anyway, it must be very rare case and when the window becomes active, InitializeSelection() is called. So, it's not serious problem.
Attachment #8609756 - Attachment is obsolete: true
Attachment #8612146 - Flags: review?(ehsan)
This is a bug since Netscape maintained editor! InsertTextImpl() is called with caret offset (i.e., normal selection's offset) and its parent node. However, during a composition, it's ignored because the caret offset at starting composition is stored in mIMETextOffset. However, we need to call it with the first offset of IME selections when the editor handles the first composition change after reframed because if mIMETextNode is replaced, mIMETextOffset is overwritten by aOffset. So, the callers should compute the start offset of IME selections (with GetIMESelectionStartOffsetIn()) and find a text to be inserted the new composition string (with FindBetterInsertionPoint()). The latter information is necessary when the editor is <textarea>.
Attachment #8609757 - Attachment is obsolete: true
Attachment #8612149 - Flags: review?(ehsan)
While focused editor is being reframed, we shouldn't notify IME of selection change, text change nor layout change since if IME tries to query content synchronously, we may return information before we finish restoring all information (e.g., before restoring old text value, we may return empty text). So, we should notify IMEStateManager of editor state and IMEStateManager should notify active IMEContentObserver of the state. Then, IMEContentObserver can put off to notify IME of the changes.
Attachment #8612152 - Flags: review?(bugs)
This tests editor reframing before (compositionupdate) and after (input) the DOM is modified.
Attachment #8612153 - Flags: review?(bugs)
Comment on attachment 8612152 [details] [diff] [review] part.6 Don't notify IME of anything during reframing the editor I would perhaps call it mSuppressNotifications and then SuppressNotifyingIME() and UnsuppressNotifyingIME()
Attachment #8612152 - Flags: review?(bugs) → review+
Comment on attachment 8612153 [details] [diff] [review] part.7 Add test for reframing focused editor when it has composition >+ function doNext() >+ { >+ if (tests.length <= index) { >+ aEditor.style.overflow = "auto"; >+ aEditor.removeEventListener(aEventType, doReframe); >+ hitEventLoop(aNextTest, 20); >+ return; >+ } Is hitEventLoop actually safe here. Should we perhaps use do something like requestAnimationFrame(function() { hitEventLoop(aNextTest, 20)}); or perhaps just requestAnimationFrame(function() { setTimeout(aNextTest); }); to ensure layout has been flushed and all. With that, r+
Attachment #8612153 - Flags: review?(bugs) → review+
ehsan: ping (for checking if the requests of review are still in your queue)
Flags: needinfo?(ehsan)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #30) > ehsan: ping (for checking if the requests of review are still in your queue) Yep, sorry, I've just been too busy... Reviewing the patches right now.
Flags: needinfo?(ehsan)
Comment on attachment 8612138 [details] [diff] [review] part.1 nsEditor shouldn't release/forget mComposition becuase it should be handled by it after reframing Review of attachment 8612138 [details] [diff] [review]: ----------------------------------------------------------------- I'm pretty sure that you don't want to do this work in the destructor of nsEditor, minusing for now. If I have misunderstood what you are trying to do, please ask for review again! ::: editor/libeditor/nsEditor.cpp @@ +151,5 @@ > nsEditor::~nsEditor() > { > NS_ASSERTION(!mDocWeak || mDidPreDestroy, "Why PreDestroy hasn't been called?"); > > + if (mComposition) { Since this bug is about reframing the text control, let me double check this with you. This code will not run when a text editor object is unbound from its frame[1]. Did you expect that? [1] (Please forgive me for possibly repeating what you already know.) The nsTextEditorState object belonging to the text control will hold the editor object alive in its mEditor member, and will only actually destroy the editor object when a) the text box DOM node gets destroyed, or b) when the type of an input element changes in a way that it is not a text box any more (such as changing the type from "text" to "radio" -- changing the type from "text" to "email" for example would preserve the editor.) @@ +154,5 @@ > > + if (mComposition) { > + // TODO: We need to investigate if we can cancel composition here actually. > + nsCOMPtr<nsIWidget> widget = GetWidget(); > + if (widget) { We get the widget from the presshell, which is kind of unlikely to exist at this point... Should you do this work in PreDestroy() instead? (Note that PreDestroy runs every time a text box object is unbound from its frame.) @@ +4039,5 @@ > return mComposition && mComposition->IsComposing(); > } > > +bool > +nsEditor::IsPossibleToHandleAsIMEComposition() const Nit: is it better to call this ShouldHandleIMEComposition()? It seems like that is essentially how it's used above.
Attachment #8612138 - Flags: review?(ehsan) → review-
Attachment #8612139 - Flags: review?(ehsan) → review+
Comment on attachment 8612140 [details] [diff] [review] part.3 nsEditor should store actual composition string length in it Review of attachment 8612140 [details] [diff] [review]: ----------------------------------------------------------------- Can you please add a comment in nsEditor::Init() near the "/* initialize IME stuff */" comment saying that this new member may come from the nsEditor before reframing it? Or something that explains why you're not handling this member there. ::: editor/libeditor/nsEditor.cpp @@ +4197,5 @@ > nsEditor::CreateTxnForIMEText(const nsAString& aStringToInsert) > { > // During handling IME composition, mComposition must have been initialized. > // TODO: We can simplify IMETextTxn::Init() with TextComposition class. > nsRefPtr<IMETextTxn> txn = new IMETextTxn(*mIMETextNode, mIMETextOffset, Nit: while you're here, can you please MOZ_ASSERT(mIMETextNode)? Thanks!
Attachment #8612140 - Flags: review?(ehsan) → review+
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #22) > Note that this doesn't work fine if the editor has composition but the > window is deactive. It's possible on Linux and Mac because their IME context > is created per window. But I've not found a good way to fix this. Anyway, it > must be very rare case and when the window becomes active, > InitializeSelection() is called. So, it's not serious problem. We also call InitializeSelection() from nsEditor::OnFocus(), which I think should be called when the current window is active. Doesn't that handle the case you're worrying about?
Comment on attachment 8612146 [details] [diff] [review] part.4 Restore IME selection at initializing selection of the editor Review of attachment 8612146 [details] [diff] [review]: ----------------------------------------------------------------- ::: editor/libeditor/nsEditor.cpp @@ -251,5 @@ > mRootElement = do_QueryInterface(aRoot); > > mUpdateCount=0; > > - /* initialize IME stuff */ I guess nevermind my previous comment. ;-) @@ +254,5 @@ > > + // At reframing the editor, mIMETextNode may be replaced with new one. > + // So, only when it's out of document, we should clear it. > + if (mIMETextNode && !mIMETextNode->IsInComposedDoc()) { > + mIMETextNode = nullptr; I think you should reset mIMETextOffset and mIMETextLength in this case too. @@ +4709,5 @@ > + // selection because if the editor is reframed, this already forgot IME > + // selection and the transaction. > + if (mComposition && !mIMETextNode && mIMETextLength) { > + // We need to look for the new mIMETextNode from current selection. > + // XXX If selection is changed during reframe, this doesn't work well! Can't we save everything that we need from the IME selection in PreDestroy() and use it here? This seems pretty fragile. (If this fixes the current bug on Facebook in your testing, I'm fine with deferring this to a follow-up.)
Attachment #8612146 - Flags: review?(ehsan) → review+
Comment on attachment 8612149 [details] [diff] [review] part.5 The offset of nsEditor::InsertTextImpl() should be minimum offset of IME selections if there is Review of attachment 8612149 [details] [diff] [review]: ----------------------------------------------------------------- Minusing because of the way you handle overflows, and also because of the usage of the external APIs in nsEditor::GetIMESelectionStartOffsetIn(). ::: editor/libeditor/nsEditor.cpp @@ +2276,5 @@ > > return NS_OK; > } > > +bool This return value is never used. Just return void. @@ +2278,5 @@ > } > > +bool > +nsEditor::FindBetterInsertionPoint(nsCOMPtr<nsINode>& aNode, > + int32_t& aOffset) Nit: I think this can be const. @@ +2382,5 @@ > res = InsertTextIntoTextNodeImpl(aStringToInsert, *node->GetAsText(), > offset); > NS_ENSURE_SUCCESS(res, res); > + NS_ASSERTION(aStringToInsert.Length() <= INT32_MAX, > + "aStringToInsert is too long"); You can't just assert this, you need to actually check this, since aStringToInsert can come from untrusted script, in that case offset will overflow. @@ +2409,2 @@ > offset = aStringToInsert.Length(); > + NS_ASSERTION(offset >= 0, "offset overflowed"); Same in these two places. You may want to use CheckedInt and just check for overflows. @@ +5197,5 @@ > + }; > + for (auto selectionType : kIMESelectionTypes) { > + nsCOMPtr<nsISelection> selection; > + rv = selectionController->GetSelection(selectionType, > + getter_AddRefs(selection)); Instead of getting this through the selection controller, just call GetSelection() <https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/nsEditor.h?from=nsEditor.h#621>. Then you can use the internal APIs to deal with the selections and ranges. @@ +5206,5 @@ > + rv = selection->GetRangeCount(&rangeCount); > + if (NS_WARN_IF(NS_FAILED(rv))) { > + continue; > + } > + for (auto i = 0; i < rangeCount; i++) { Nit: technically auto is wrong, since it can be a 64-bit int. Use int32_t instead. ::: editor/libeditor/nsHTMLEditRules.cpp @@ +1332,5 @@ > // Right now the nsWSRunObject code bails on empty strings, but IME needs > // the InsertTextImpl() call to still happen since empty strings are meaningful there. > + NS_ENSURE_STATE(mHTMLEditor); > + // Find better insertion point to insert text. > + mHTMLEditor->FindBetterInsertionPoint(selNode, selOffset); Hmm, wouldn't this always return early? IINM this cannot be called on a plaintext editor. If that is correct, you may want to not bother calling FindBetterInsertionPoint here. @@ +1347,5 @@ > &selOffset, doc); > } > else > { > NS_ENSURE_STATE(mHTMLEditor); Nit: you should be able to remove this line.
Attachment #8612149 - Flags: review?(ehsan) → review-
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #31) > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #30) > > ehsan: ping (for checking if the requests of review are still in your queue) > > Yep, sorry, I've just been too busy... Reviewing the patches right now. No problem. And thank you. (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #32) > ::: editor/libeditor/nsEditor.cpp > @@ +151,5 @@ > > nsEditor::~nsEditor() > > { > > NS_ASSERTION(!mDocWeak || mDidPreDestroy, "Why PreDestroy hasn't been called?"); > > > > + if (mComposition) { > > Since this bug is about reframing the text control, let me double check this > with you. This code will not run when a text editor object is unbound from > its frame[1]. Did you expect that? Yes. > [1] (Please forgive me for possibly repeating what you already know.) > The nsTextEditorState object belonging to the text control will hold the > editor object alive in its mEditor member, and will only actually destroy > the editor object when a) the text box DOM node gets destroyed, or b) when > the type of an input element changes in a way that it is not a text box any > more (such as changing the type from "text" to "radio" -- changing the type > from "text" to "email" for example would preserve the editor.) It's helpful information to me since the code is complicated, so, honestly, I'm not 100% sure the fact but I'm now clear! > @@ +154,5 @@ > > > > + if (mComposition) { > > + // TODO: We need to investigate if we can cancel composition here actually. > > + nsCOMPtr<nsIWidget> widget = GetWidget(); > > + if (widget) { > > We get the widget from the presshell, which is kind of unlikely to exist at > this point... Ah... right... > Should you do this work in PreDestroy() instead? > (Note that PreDestroy runs every time a text box object is unbound from its > frame.) No, at PreDestroy(), we need to keep the composition. So, I guess that when TextComposition is being destroyed, it should clean up the composition itself. > > +bool > > +nsEditor::IsPossibleToHandleAsIMEComposition() const > > Nit: is it better to call this ShouldHandleIMEComposition()? It seems like > that is essentially how it's used above. OK.
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #33) > part.3 nsEditor should store actual composition string length in it > > Can you please add a comment in nsEditor::Init() near the "/* initialize IME > stuff */" comment saying that this new member may come from the nsEditor > before reframing it? Or something that explains why you're not handling > this member there. Sure. > ::: editor/libeditor/nsEditor.cpp > @@ +4197,5 @@ > > nsEditor::CreateTxnForIMEText(const nsAString& aStringToInsert) > > { > > // During handling IME composition, mComposition must have been initialized. > > // TODO: We can simplify IMETextTxn::Init() with TextComposition class. > > nsRefPtr<IMETextTxn> txn = new IMETextTxn(*mIMETextNode, mIMETextOffset, > > Nit: while you're here, can you please MOZ_ASSERT(mIMETextNode)? Thanks! OK. (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #34) > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #22) > > Note that this doesn't work fine if the editor has composition but the > > window is deactive. It's possible on Linux and Mac because their IME context > > is created per window. But I've not found a good way to fix this. Anyway, it > > must be very rare case and when the window becomes active, > > InitializeSelection() is called. So, it's not serious problem. > > We also call InitializeSelection() from nsEditor::OnFocus(), which I think > should be called when the current window is active. Doesn't that handle the > case you're worrying about? Yes. But while the window is deactive, IME selections gone, but they will be restored when the window becomes active. So, it must look like odd. Anyway, this is very rare case. (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #35) > Comment on attachment 8612146 [details] [diff] [review] > part.4 Restore IME selection at initializing selection of the editor > > Review of attachment 8612146 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: editor/libeditor/nsEditor.cpp > @@ -251,5 @@ > > mRootElement = do_QueryInterface(aRoot); > > > > mUpdateCount=0; > > > > - /* initialize IME stuff */ > > I guess nevermind my previous comment. ;-) Oh, okay. > > + // At reframing the editor, mIMETextNode may be replaced with new one. > > + // So, only when it's out of document, we should clear it. > > + if (mIMETextNode && !mIMETextNode->IsInComposedDoc()) { > > + mIMETextNode = nullptr; > > I think you should reset mIMETextOffset and mIMETextLength in this case too. No, mIMETextNode is removed from document if the editor is <input> or <textarea> when it's reframed. However, new text node whose text is same as mIMETextNode is recreated. So, they are necessary to restore IME selections and replacing range with next composition update. I'll add comment about this. > @@ +4709,5 @@ > > + // selection because if the editor is reframed, this already forgot IME > > + // selection and the transaction. > > + if (mComposition && !mIMETextNode && mIMETextLength) { > > + // We need to look for the new mIMETextNode from current selection. > > + // XXX If selection is changed during reframe, this doesn't work well! > > Can't we save everything that we need from the IME selection in PreDestroy() > and use it here? This seems pretty fragile. > > (If this fixes the current bug on Facebook in your testing, I'm fine with > deferring this to a follow-up.) Hmm, I'll file a followup bug because some patches of this bug is needed by bug 1166436 too. So, I'd like to fix this bug ASAP. But I think it's difficult since addons can do everything during reframe, although, nobody does it actually.
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #36) > part.5 The offset of nsEditor::InsertTextImpl() should be minimum offset of > IME selections if there is > > ::: editor/libeditor/nsHTMLEditRules.cpp > @@ +1332,5 @@ > > // Right now the nsWSRunObject code bails on empty strings, but IME needs > > // the InsertTextImpl() call to still happen since empty strings are meaningful there. > > + NS_ENSURE_STATE(mHTMLEditor); > > + // Find better insertion point to insert text. > > + mHTMLEditor->FindBetterInsertionPoint(selNode, selOffset); > > Hmm, wouldn't this always return early? IINM this cannot be called on a > plaintext editor. > > If that is correct, you may want to not bother calling > FindBetterInsertionPoint here. Yes, it is. But I wonder, if somebody will change FindBetterInsertionPoint(), they must not check here. How do you think about this issue?
Flags: needinfo?(ehsan)
Attachment #8614770 - Flags: review?(ehsan)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #39) > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from > comment #36) > > part.5 The offset of nsEditor::InsertTextImpl() should be minimum offset of > > IME selections if there is > > > > ::: editor/libeditor/nsHTMLEditRules.cpp > > @@ +1332,5 @@ > > > // Right now the nsWSRunObject code bails on empty strings, but IME needs > > > // the InsertTextImpl() call to still happen since empty strings are meaningful there. > > > + NS_ENSURE_STATE(mHTMLEditor); > > > + // Find better insertion point to insert text. > > > + mHTMLEditor->FindBetterInsertionPoint(selNode, selOffset); > > > > Hmm, wouldn't this always return early? IINM this cannot be called on a > > plaintext editor. > > > > If that is correct, you may want to not bother calling > > FindBetterInsertionPoint here. > > Yes, it is. But I wonder, if somebody will change > FindBetterInsertionPoint(), they must not check here. How do you think about > this issue? OK that's fair. Let's keep it then.
Flags: needinfo?(ehsan)
Attachment #8614770 - Flags: review?(ehsan) → review+
Comment on attachment 8614776 [details] [diff] [review] part.5 The offset of nsEditor::InsertTextImpl() should be minimum offset of IME selections if there is Review of attachment 8614776 [details] [diff] [review]: ----------------------------------------------------------------- ::: editor/libeditor/nsEditor.cpp @@ +2287,5 @@ > + if (!IsPlaintextEditor()) { > + // We cannot find "better" insertion point in HTML editor. > + // WARNING: When you add some code to find better node in HTML editor, > + // you need to call this before calling InsertTextImpl() in > + // nsHTMLEditRules. Please just add that call site back. :-) @@ +5206,5 @@ > + continue; > + } > + for (int32_t i = 0; i < rangeCount; i++) { > + nsCOMPtr<nsIDOMRange> domRange; > + rv = selection->GetRangeAt(i, getter_AddRefs(domRange)); You're still using nsISelection. You should use mozilla::Selection instead. That way you can use functions such as: GetRangeAt: <https://dxr.mozilla.org/mozilla-central/source/layout/generic/Selection.h?from=Selection.h#117> RangeCount: <https://dxr.mozilla.org/mozilla-central/source/layout/generic/Selection.h?from=Selection.h#163> etc. ::: editor/libeditor/nsEditor.h @@ +817,5 @@ > + * FindBetterInsertionPoint() tries to look for better insertion point which > + * is typically the nearest text node and offset in it. > + */ > + void FindBetterInsertionPoint(nsCOMPtr<nsINode>& aNode, > + int32_t& aOffset); Nit: please make this const.
Attachment #8614776 - Flags: review?(ehsan) → review-
> ::: editor/libeditor/nsEditor.h > @@ +817,5 @@ >> + * FindBetterInsertionPoint() tries to look for better insertion point which >> + * is typically the nearest text node and offset in it. >> + */ >> + void FindBetterInsertionPoint(nsCOMPtr<nsINode>& aNode, >> + int32_t& aOffset); > > Nit: please make this const. Unfortunately, impossible. GetRoot() calls nsIEditor::GetRootElement(). nsHTMLEditor::GetRootElement() may modify mRootElement.
Attachment #8614776 - Attachment is obsolete: true
Attachment #8615087 - Flags: review?(ehsan)
I forgot to add logging code into new methods of IMEStateManager. (but carrying over the r+)
Attachment #8613539 - Attachment is obsolete: true
Hmm, even with these patches, when I type composition string fast, TIP may be confused sometimes. In such case, NS_QUERY_TEXT_RECT fails, perhaps, there is old text node which doesn't include the latest composition string. I'll investigate it in new bug since we probably need some patches including nsTextStore. So, it's out of scope of this bug.
Attachment #8615087 - Flags: review?(ehsan) → review+
Nakano-san, does this need uplift to aurora?
Flags: needinfo?(masayuki)
(In reply to Brian Birtles (:birtles) from comment #52) > Nakano-san, does this need uplift to aurora? I don't think so. It's too risky.
Flags: needinfo?(masayuki)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #53) > (In reply to Brian Birtles (:birtles) from comment #52) > > Nakano-san, does this need uplift to aurora? > > I don't think so. It's too risky. Currently Aurora crashes when using facebook. When this hits beta will this crash still exist? (Or will the relevant bits of TSF support be turned off there so we'll be ok?)
(In reply to Brian Birtles (:birtles) from comment #54) > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #53) > > (In reply to Brian Birtles (:birtles) from comment #52) > > > Nakano-san, does this need uplift to aurora? > > > > I don't think so. It's too risky. > > Currently Aurora crashes when using facebook. When this hits beta will this > crash still exist? (Or will the relevant bits of TSF support be turned off > there so we'll be ok?) It's okay to put off to turn on TSF mode in default settings since a couple of improvements are landed on 41. However, even we do so, the input result won't be fixed. This bug occurs Facebook, of course, it's very major web service. But this bug hasn't been reported and I don't see complaint on web (Army of Awesome, Firefox input, etc). So, this could be not so important at least in Japan. How do you think? # Anyway, I want to check feedback from Beta users who must be more than Aurora's. So, I want to back it out in middle of next Beta if we should need to disable TSF mode.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #55) > It's okay to put off to turn on TSF mode in default settings since a couple > of improvements are landed on 41. However, even we do so, the input result > won't be fixed. This bug occurs Facebook, of course, it's very major web > service. But this bug hasn't been reported and I don't see complaint on web > (Army of Awesome, Firefox input, etc). So, this could be not so important at > least in Japan. How do you think? I'm less concerned about the buggy input and more about the crash. Looking at crash data, however, I don't see too many cases of this: https://crash-stats.mozilla.com/signature/?signature=imjpapi.dll%400x2a661&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&page=1 (The first record has a number of cases from Firefox 40 which seem likely to be this bug but it's still only 7 hits.) I'm not the right person to decide if TSF is safe to ship without this fixed but I'd lean towards deferring until 41. We can monitor crash data during beta and get release driver advice at that point.
(In reply to Brian Birtles (:birtles) from comment #56) > We can monitor crash data during > beta and get release driver advice at that point. Sure, let's check it until mid of next month.
Can we think of any safer patches for aurora/beta then? Sounds like this is rather major issue when this happens (especially the possible crash).
(In reply to Olli Pettay [:smaug] (for generic DOM reviews, you may want to look for other reviewers too ;)) from comment #58) > Can we think of any safer patches for aurora/beta then? Sounds like this is > rather major issue when > this happens (especially the possible crash). It's difficult to decide what's the exactly cause. TSF/TIP works with content. However, if content is broken by the reframe, cache in the TIP becomes different from actual content. Then, when TIP does something, it may hit an accident. Therefore, the right fix is only one way, we avoid to make broken editor content. Note that in Japan, at least 3 TIPs are major. So, hack for all of them is too hard...
In other words, we need to avoid all bugs of all TIPs if we hide the crash.
Component: Event Handling → User events and focus handling
Duplicate of this bug: 1272250
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: