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)
Core
DOM: UI Events & Focus Handling
Tracking
()
RESOLVED
FIXED
mozilla41
People
(Reporter: birtles, Assigned: masayuki)
References
()
Details
(Keywords: inputmethod)
Attachments
(9 files, 14 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
(deleted),
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 'nななkなかなかnなかのなかの'.
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.
Reporter | ||
Comment 1•10 years ago
|
||
Reporter | ||
Comment 2•10 years ago
|
||
(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
Reporter | ||
Comment 3•10 years ago
|
||
(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
Assignee | ||
Comment 4•10 years ago
|
||
Odd... I guess that we may behave unexpectedly for MS-IME.
Assignee | ||
Updated•10 years ago
|
Keywords: inputmethod
Assignee | ||
Comment 5•10 years ago
|
||
Perhaps, the crash is TSF only but the broken behavior isn't recent regression since I can reproduce this bug with 31 ESR too.
Assignee | ||
Comment 6•10 years ago
|
||
I can reproduce this bug on Linux too. Perhaps, on Mac too.
OS: Windows 8.1 → All
Hardware: x86_64 → All
Assignee | ||
Comment 7•10 years ago
|
||
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/
Assignee | ||
Comment 8•10 years ago
|
||
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 | ||
Updated•10 years ago
|
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Component: DOM: Events → Event Handling
Assignee | ||
Comment 9•10 years ago
|
||
Okay, http://jsfiddle.net/xhzmva34/1/ is the simplest testcase, although, it cannot cause the crash.
Comment 10•10 years ago
|
||
Input text corruption using testcase of comment #9 with IME on:
keypres: nakano
Results: nななkなかなかnなかのなかの
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
Comment 11•10 years ago
|
||
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):
Comment 12•10 years ago
|
||
Pushlog(force enable TSF):
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a07eb0e0e4ab&tochange=c8b8756142f7
suspect: Bug 808287
Assignee | ||
Comment 14•10 years ago
|
||
Assignee | ||
Comment 15•10 years ago
|
||
Assignee | ||
Comment 16•10 years ago
|
||
Assignee | ||
Comment 17•10 years ago
|
||
Assignee | ||
Comment 18•10 years ago
|
||
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.
Assignee | ||
Comment 19•10 years ago
|
||
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)
Assignee | ||
Comment 20•10 years ago
|
||
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)
Assignee | ||
Comment 21•10 years ago
|
||
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)
Assignee | ||
Comment 22•10 years ago
|
||
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)
Assignee | ||
Comment 23•10 years ago
|
||
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)
Assignee | ||
Comment 24•10 years ago
|
||
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)
Assignee | ||
Comment 25•10 years ago
|
||
This tests editor reframing before (compositionupdate) and after (input) the DOM is modified.
Attachment #8612153 -
Flags: review?(bugs)
Comment 26•10 years ago
|
||
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 27•10 years ago
|
||
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+
Assignee | ||
Comment 28•10 years ago
|
||
Attachment #8612152 -
Attachment is obsolete: true
Assignee | ||
Comment 29•10 years ago
|
||
Attachment #8612153 -
Attachment is obsolete: true
Assignee | ||
Comment 30•10 years ago
|
||
ehsan: ping (for checking if the requests of review are still in your queue)
Flags: needinfo?(ehsan)
Comment 31•10 years ago
|
||
(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 32•10 years ago
|
||
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-
Updated•10 years ago
|
Attachment #8612139 -
Flags: review?(ehsan) → review+
Comment 33•10 years ago
|
||
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+
Comment 34•10 years ago
|
||
(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 35•10 years ago
|
||
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 36•10 years ago
|
||
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-
Assignee | ||
Comment 37•10 years ago
|
||
(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.
Assignee | ||
Comment 38•10 years ago
|
||
(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.
Assignee | ||
Comment 39•10 years ago
|
||
(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)
Assignee | ||
Comment 40•10 years ago
|
||
Attachment #8612138 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8614770 -
Flags: review?(ehsan)
Assignee | ||
Comment 41•10 years ago
|
||
Attachment #8612139 -
Attachment is obsolete: true
Assignee | ||
Comment 42•10 years ago
|
||
Attachment #8612140 -
Attachment is obsolete: true
Assignee | ||
Comment 43•10 years ago
|
||
Attachment #8612146 -
Attachment is obsolete: true
Assignee | ||
Comment 44•10 years ago
|
||
Attachment #8612149 -
Attachment is obsolete: true
Attachment #8614776 -
Flags: review?(ehsan)
Comment 45•10 years ago
|
||
(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)
Updated•10 years ago
|
Attachment #8614770 -
Flags: review?(ehsan) → review+
Comment 46•10 years ago
|
||
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-
Assignee | ||
Comment 47•10 years ago
|
||
> ::: 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)
Assignee | ||
Comment 48•10 years ago
|
||
I forgot to add logging code into new methods of IMEStateManager. (but carrying over the r+)
Attachment #8613539 -
Attachment is obsolete: true
Assignee | ||
Comment 49•10 years ago
|
||
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.
Updated•10 years ago
|
Attachment #8615087 -
Flags: review?(ehsan) → review+
Comment 50•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/66bab785635f
https://hg.mozilla.org/integration/mozilla-inbound/rev/e037403d3e8c
https://hg.mozilla.org/integration/mozilla-inbound/rev/75ceb4773c0d
https://hg.mozilla.org/integration/mozilla-inbound/rev/30de3e9b5741
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9b8b65be80a
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa68553b7a71
https://hg.mozilla.org/integration/mozilla-inbound/rev/04383149db11
https://hg.mozilla.org/mozilla-central/rev/66bab785635f
https://hg.mozilla.org/mozilla-central/rev/e037403d3e8c
https://hg.mozilla.org/mozilla-central/rev/75ceb4773c0d
https://hg.mozilla.org/mozilla-central/rev/30de3e9b5741
https://hg.mozilla.org/mozilla-central/rev/b9b8b65be80a
https://hg.mozilla.org/mozilla-central/rev/aa68553b7a71
https://hg.mozilla.org/mozilla-central/rev/04383149db11
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Reporter | ||
Comment 52•10 years ago
|
||
Nakano-san, does this need uplift to aurora?
Flags: needinfo?(masayuki)
Assignee | ||
Comment 53•10 years ago
|
||
(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)
Reporter | ||
Comment 54•10 years ago
|
||
(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?)
Assignee | ||
Comment 55•10 years ago
|
||
(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.
Reporter | ||
Comment 56•10 years ago
|
||
(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.
Assignee | ||
Comment 57•10 years ago
|
||
(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.
Comment 58•10 years ago
|
||
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).
Assignee | ||
Comment 59•10 years ago
|
||
(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...
Assignee | ||
Comment 60•10 years ago
|
||
In other words, we need to avoid all bugs of all TIPs if we hide the crash.
Updated•6 years ago
|
Component: Event Handling → User events and focus handling
Updated•5 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•