Closed Bug 1266815 Opened 4 years ago Closed 4 years ago

Occasional "IME is disabled" in telegram web / whatsapp web

Categories

(Core :: DOM: Editor, defect)

47 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox47 --- wontfix
firefox48 --- wontfix
firefox49 --- wontfix
firefox-esr45 --- affected
firefox50 --- wontfix
firefox51 --- fixed

People

(Reporter: swstar, Assigned: m_kato)

References

Details

(Keywords: inputmethod, regression)

Attachments

(9 files)

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160422030223

Steps to reproduce:

Switching tabs forth and back, and switching input method, with e10s enabled.
OS: Windows 8.1 x64, IME: Microsoft Quick (Traditional Chinese)


Actual results:

Occasionally trigger a bug which "IME is disabled" on message input box in telegram web and whatsapp web.
Triggering method unknown, appears randomly after switching IME, even stayed on the same tab.
There is no way to re-enable it unless close tab and re-open it.
With e10s disabled, this problem occurs after download dialog box is poped-up.
To re-enable IME, reload tab.
Clean profile DOES NOT help.


Expected results:

IME should not be disabled.
Keywords: inputmethod
Component: Untriaged → Internationalization
Product: Firefox → Core
Could you get the TSF log?  (see https://dev.mozilla.jp/2012/09/ime_logging_of_tsf/.  If you cannot read Japanese document, let me know).

Because there is no exactly reproduce step for this issue and I cannot reproduce this.
Component: Internationalization → Widget: Win32
Flags: needinfo?(swstar)
Is there a step by step guide on how to enable the NSPR_LOG_FILE flag and where to point the log file?
1. Open Command Prompt
2. SET NSPR_LOG_FILE=c:\users\<your name>\tsf.log <-- log file full path name
3. SET NSPR_LOG_MODULES=nsTextStoreWidgets:4
4. "C:\Program Files\Nightly\firefox.exe"

Then, firefox will be launched.  So you test until you reproduce this.

5. close Firefox.  Then, log file (2.) is generated.
6. Attach it on this bug.

And there is a few question.

Q1. Does this occurs even if non-e10s mode?  (Enabled/Disable multi process Nightly on settings)
Q2. Does this occurs on another IME?
Q3. Does this occurs even if on IMM mode (set intl.tsf.enable=false on about:preferences)
Attached file tsf.log
Q1. Does this occurs even if non-e10s mode?  (Enabled/Disable multi process
Nightly on settings)
-> yes
Q2. Does this occurs on another IME?
-> yes, microsoft japanese ime is also affected.
Q3. Does this occurs even if on IMM mode (set intl.tsf.enable=false on
about:preferences)
-> yes
Flags: needinfo?(swstar)
Makoto, do you consider this an e10s blocker? Sounds like it happens in e10s and without e10s, but may be related to some e10s work that landed recently?
Flags: needinfo?(m_kato)
This bug affects from release up to nightly
(In reply to Jim Mathies [:jimm] from comment #5)
> Makoto, do you consider this an e10s blocker? Sounds like it happens in e10s
> and without e10s, but may be related to some e10s work that landed recently?

It seems to be another regression without e10s.  So I don't think this is e10s blocker.
Flags: needinfo?(m_kato)
(In reply to Fubuki Hoshino from comment #6)
> This bug affects from release up to nightly

? One question.  Does this issue occur on Firefox 47 (beta)?
(In reply to Makoto Kato [:m_kato] from comment #8)
> (In reply to Fubuki Hoshino from comment #6)
> > This bug affects from release up to nightly
> 
> ? One question.  Does this issue occur on Firefox 47 (beta)?

yes, it still does affect 47b1
Summary: Occasional "IME is disabled" with e10s in telegram web / whatsapp web → Occasional "IME is disabled" in telegram web / whatsapp web
New findings shows downloading from "blob:" will cause the problem.
Normal pages seems not to be affected.

Besides downloading, the random block out is still unknown.
Can you provide more exact steps to reproduce this? I have tried to test this on Windows 8.1 x64 and I can't with FF 50.0a1.
(In reply to ovidiu boca[:Ovidiu] from comment #11)
> Can you provide more exact steps to reproduce this? I have tried to test
> this on Windows 8.1 x64 and I can't with FF 50.0a1.

1. Go to telegram web / whatsapp web, login
2. Get into a chat / group chat
3. Let someone send you a file (must be a file for download, not photo)
4. Click download, the download dialogue pops up
5. Your IME (NON-ENGLISH, MOST LIKELY ASIAN IMES) will be disabled, you have no way to re-enable and type non-English characters, unless reload page in non-e10s or close tab and re-open in e10s.
Attached image 2016-07-19_23-21-23.png
Your IME, will look like this, the circle cross means disabled and you can not type non-English characters.
Attached image imgpsh_fullsize.jpg
Thanks for clarification, I have tested on Win 8.1 x64 with FF Nightly 50.0a1 but I couldn't reproduce it. I attached a file where you can see my result. maybe I am doing something wrong.
(In reply to ovidiu boca[:Ovidiu] from comment #14)
> Created attachment 8772736 [details]
> imgpsh_fullsize.jpg
> 
> Thanks for clarification, I have tested on Win 8.1 x64 with FF Nightly
> 50.0a1 but I couldn't reproduce it. I attached a file where you can see my
> result. maybe I am doing something wrong.

Download from pdf.js works fine, the bug is triggered by download from "blob:"
If testing from Whatsapp web, try to download a picture, by clicking to view the picture, the download icon shown on the top right conner, then it will give a download from "blob:", and trigger the bug.
I confirmed, then I will look this.
Assignee: nobody → m_kato
Status: UNCONFIRMED → NEW
Ever confirmed: true
Humm, IMEStateManager sets input context to disable state when this occurs.  Also, this occurs on content editable only....

nsContentUtils::GetHTMLEditor in nsIContent::GetDesiredIMEState returns nullptr.  I need more investigating...
Attached file testcase
See Also: → 1146881
I have tried to narrow down a regression range but I couldn't due to the fact that this bug is reproduced intermittent.
Regression window:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=21d669bd40c5&tochange=cbb8d8caed8a

Regressed by:
cbb8d8caed8a	Masayuki Nakano — Bug 488420 IME enabled state is not modified when a focused editor's readonly attribute is changed r=smaug
Flags: needinfo?(masayuki)
Flags: needinfo?(bugs)
(In reply to Makoto Kato [:m_kato] from comment #18)
> Humm, IMEStateManager sets input context to disable state when this occurs. 
> Also, this occurs on content editable only....
> 
> nsContentUtils::GetHTMLEditor in nsIContent::GetDesiredIMEState returns
> nullptr.  I need more investigating...

Although, I don't remember how focus is managed with modal dialog, it sounds like HTML editor (and/or its document) still doesn't have DOM focus when IMEStateManger retrieves the desired IME state?

Alice-san:

You shouldn't request ni to reviewer of the regression changeset except me since I'm the only person who understand whole code of IME. So, ni? to me is enough. If there is a better person than me about the regression cause, I'll request ni? to proper person instead of you ;-)
Flags: needinfo?(masayuki)
Flags: needinfo?(bugs)
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #22)
> (In reply to Makoto Kato [:m_kato] from comment #18)
> > Humm, IMEStateManager sets input context to disable state when this occurs. 
> > Also, this occurs on content editable only....
> > 
> > nsContentUtils::GetHTMLEditor in nsIContent::GetDesiredIMEState returns
> > nullptr.  I need more investigating...
> 
> Although, I don't remember how focus is managed with modal dialog, it sounds
> like HTML editor (and/or its document) still doesn't have DOM focus when
> IMEStateManger retrieves the desired IME state?

GetDesiredIMEState wants HTMLEditor, but nsEditingSession will be the detached state by download dialog, so we don't get HTMLEditor.

Child-SP          RetAddr           Call Site
00000071`005fd8a8 00007ffd`d0113b37 xul!nsSHEntry::SetEditorData
00000071`005fd8b0 00007ffd`d12909e9 xul!nsDocShell::DetachEditorFromWindow+0x646a5f
00000071`005fd8e0 00007ffd`d128e2c1 xul!nsEditingSession::StartDocumentLoad+0x71
00000071`005fd910 00007ffd`cfa9c38c xul!nsEditingSession::OnStateChange+0x1a5
00000071`005fda40 00007ffd`cfa9c232 xul!nsDocLoader::DoFireOnStateChange+0xe4
00000071`005fdad0 00007ffd`cfa9bc80 xul!nsDocLoader::FireOnStateChange+0x72
(Inline Function) --------`-------- xul!nsDocLoader::doStartURLLoad+0x20
00000071`005fdb70 00007ffd`cfa9baf8 xul!nsDocLoader::OnStartRequest+0xb8
00000071`005fdc60 00007ffd`cfaaebca xul!mozilla::net::nsLoadGroup::AddRequest+0x130
00000071`005fdd80 00007ffd`cf9129bd xul!mozilla::net::nsHttpChannel::AsyncOpen+0x1aa
00000071`005fdee0 00007ffd`cf9128bb xul!nsURILoader::OpenURI+0xa5

So we need reattach previous edit or don't detach on StartDocumentLoad...?
Component: Widget: Win32 → Editor
I *guess* that we would need to create events to notify contents of entering/exiting to/from native modal mode if we fixed this bug strictly. It seems that we have same bug with printer's dialog.
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #24)
> I *guess* that we would need to create events to notify contents of
> entering/exiting to/from native modal mode if we fixed this bug strictly. It
> seems that we have same bug with printer's dialog.

This doesn't depends on native dialog.  When loading new content (for both internal viewer by docshell and external viewer), OnStateChange is always called with START state.  But we don't know whether its MIME type is internal viewer or external viewer at this time.  Even if MIME is external viewer (it is handled by uriloader and apphelper), editor detaches session data.

nsEditingSession doesn't consider that it is external viewer.  And So this will only occur that OnLocationChange event by nsIWebProgressListener is fired.
nsIContent::GetDesiredIMEState sets IMEState::DISABLED, so IME doesn't work well.

Actually, DetachEditorFromWindow is called twice.  One is from nsEditingSession::StartDocumentLoad, and another is from nsDocShell::FirePageHideNotification.

When navigating to next, OnStateChange(..., STATE_START) is called.  Even if next page is external viewer such as launching another application, STAT_START is called. So even if launching external application, DetachEditorFromWindow is called.  So it means that editor is detached from docshell.

nsIContent::GetDesiredIMEState uses nsIEditor, but editor is detached state, so it cannot get nsIEditor.

When navigating to next content on internel viewer such HTML, nsDocShell::CreateContentViewer is called, then FirePageHideNotification is called.

So it is unnecessary to call DetachEditorFromWindow from nsEditorSession.  And its call causes this issue.

Review commit: https://reviewboard.mozilla.org/r/67356/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67356/
Attachment #8775017 - Flags: review?(masayuki)
Attachment #8775018 - Flags: review?(masayuki)
Comment on attachment 8775017 [details]
Bug 1266815 - Part 1. Unnecessary DetachEditorFromWindow call from nsEditingSession.

https://reviewboard.mozilla.org/r/67356/#review64736

::: editor/composer/nsEditingSession.cpp
(Diff revision 1)
> -  // If we have an editor here, then we got a reload after making the editor.
> -  // We need to blow it away and make a new one at the end of the load.

According to this comment, this is a hack for reload. So, please check if there is no problem after reload.
Attachment #8775017 - Flags: review?(masayuki) → review+
Comment on attachment 8775018 [details]
Bug 1266815 - Part 2. Add editing session test.

https://reviewboard.mozilla.org/r/67358/#review64738

::: editor/composer/test/chrome.ini:3
(Diff revision 1)
> +support-files =
> +  file_bug1266815.html

Why don't you add this to [test_bug1266815.html] section? (Although, I don't know the exact rules how to define support files...)

::: editor/composer/test/test_bug1266815.html:65
(Diff revision 1)
> +      is(iframe.contentDocument.getElementById("edit").innerText, "abc",
> +         "load iframe source");
> +      resolve();
> +    };
> +    iframe.id = "testframe";
> +    iframe.src = "file_bug1266815.html";

Well, I wonder, cannot this test work with data URI instead of a support file? If so, I like to use data URI better because the contents are enough simple.

::: editor/composer/test/test_bug1266815.html:84
(Diff revision 1)
> +    function waitDialog() {
> +      if (gShowCount > 0) {
> +        is(gShowCount, 1, "helperapp dialog should be shown");
> +        resolve();
> +      } else {
> +        setTimeout(waitDialog, 100);
> +      }
> +    }
> +    setTimeout(waitDialog, 100);

Instead of using gShowCount, how about to call it as a callback from show()? I'm not familiar with scope of JS, but I guess that it's possible with using global variable which stores the callback function.

Then, you can avoid setTimeout() hack which may cause random orange.
Attachment #8775018 - Flags: review?(masayuki) → review-
https://reviewboard.mozilla.org/r/67358/#review64738

> Well, I wonder, cannot this test work with data URI instead of a support file? If so, I like to use data URI better because the contents are enough simple.

When starting test, data url doesn't work.  But when I rewrite this today, it works.  Previous seems to be that my code is wrong...  OK, I use promise + data: url.

> Instead of using gShowCount, how about to call it as a callback from show()? I'm not familiar with scope of JS, but I guess that it's possible with using global variable which stores the callback function.
> 
> Then, you can avoid setTimeout() hack which may cause random orange.

I will use promise instead
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #30)
> Comment on attachment 8775017 [details]
> Bug 1266815 - Part 1. Unnecessary DetachEditorFromWindow call from
> nsEditingSession.
> 
> https://reviewboard.mozilla.org/r/67356/#review64736
> 
> ::: editor/composer/nsEditingSession.cpp
> (Diff revision 1)
> > -  // If we have an editor here, then we got a reload after making the editor.
> > -  // We need to blow it away and make a new one at the end of the load.
> 
> According to this comment, this is a hack for reload. So, please check if
> there is no problem after reload.

Both case calls nsDocShell::PageHideNotification by CreateCotnentViewer.  So DetechEditorFromWindow is alwasy called even if reload case.
Comment on attachment 8775017 [details]
Bug 1266815 - Part 1. Unnecessary DetachEditorFromWindow call from nsEditingSession.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67356/diff/1-2/
Attachment #8775018 - Flags: review- → review?(masayuki)
Comment on attachment 8775018 [details]
Bug 1266815 - Part 2. Add editing session test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67358/diff/1-2/
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/59fa797ab438
Part 1. Unnecessary DetachEditorFromWindow call from nsEditingSession. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/10857066c347
Part 2. Add editing session test. r=masayuki
https://hg.mozilla.org/mozilla-central/rev/59fa797ab438
https://hg.mozilla.org/mozilla-central/rev/10857066c347
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Duplicate of this bug: 1274490
Hi :m_kato,
Since this bug is a regression and also affects 49/50, do you consider to uplift this for 49/50 if this patch is not too risky?
Flags: needinfo?(m_kato)
(In reply to Gerry Chang [:gchang] from comment #40)
> Hi :m_kato,
> Since this bug is a regression and also affects 49/50, do you consider to
> uplift this for 49/50 if this patch is not too risky?

For beta, this is too risky because this changes editor session's behavior.  Editor session is a persist storage into history to save inputted data into edit box.

But I will request this for aurora because we have a time to fix new regression if we find it.
Flags: needinfo?(m_kato)
Comment on attachment 8775017 [details]
Bug 1266815 - Part 1. Unnecessary DetachEditorFromWindow call from nsEditingSession.

Approval Request Comment
[Feature/regressing bug #]:
Bug 488420

[User impact if declined]:
User cannot turn on IME on contenteditable's area for CJK after the following scenario.

1. User clicks download link
2. Firefox shows download dialog.
3. User saves or cancel this.
4. On contenteditable's element, user cannot turn on IME.  If not using IME, it can input any text.

Also, this issue is contenteditable's element only.  If it is input/textarea element, this doesn't occur.  Also, When refreshing page after this occurs, IME will work again.

[Describe test coverage new/current, TreeHerder]:
Landed in m-c with unit test.

[Risks and why]: 
Middle.  This fix changes editor session's behavior.  Editor session is a persist storage into history to save inputted data into edit box.


[String/UUID change made/needed]:
None
Attachment #8775017 - Flags: approval-mozilla-aurora?
Due to the risk of changing editor session's behavior, let it ride the train on 50 and mark 49 as won't fix.
Comment on attachment 8775017 [details]
Bug 1266815 - Part 1. Unnecessary DetachEditorFromWindow call from nsEditingSession.

Given the medium risk associated with the fix and the fact that this has been a regression since 47 with a workaround of closing and reopening the tab, I would prefer to let this one ride the 51 train and not uplift to Aurora.
Attachment #8775017 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Duplicate of this bug: 1278155
Duplicate of this bug: 1129372
Duplicate of this bug: 991537
You need to log in before you can comment on or make changes to this bug.