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

RESOLVED FIXED in Firefox 51

Status

()

Core
Editor
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Fubuki Hoshino, Assigned: m_kato)

Tracking

({inputmethod, regression})

47 Branch
mozilla51
inputmethod, regression
Points:
---

Firefox Tracking Flags

(firefox47 wontfix, firefox48 wontfix, firefox49 wontfix, firefox-esr45 affected, firefox50 wontfix, firefox51 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(9 attachments)

(Reporter)

Description

2 years ago
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.

Updated

2 years ago
Keywords: inputmethod

Updated

2 years ago
Component: Untriaged → Internationalization
Product: Firefox → Core
(Assignee)

Comment 1

2 years ago
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)
(Reporter)

Comment 2

2 years ago
Is there a step by step guide on how to enable the NSPR_LOG_FILE flag and where to point the log file?
(Assignee)

Comment 3

2 years ago
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)
(Reporter)

Comment 4

2 years ago
Created attachment 8744781 [details]
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)

Comment 5

2 years ago
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)
Keywords: regressionwindow-wanted

Updated

2 years ago
status-firefox48: --- → affected
(Reporter)

Comment 6

2 years ago
This bug affects from release up to nightly
(Assignee)

Comment 7

2 years ago
(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)
(Assignee)

Comment 8

2 years ago
(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)?
(Reporter)

Comment 9

2 years ago
(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

Updated

2 years ago
Summary: Occasional "IME is disabled" with e10s in telegram web / whatsapp web → Occasional "IME is disabled" in telegram web / whatsapp web
(Assignee)

Updated

2 years ago
status-firefox47: --- → affected
(Reporter)

Comment 10

2 years ago
Created attachment 8762790 [details]
firefox_2016-06-15_13-14-07.png

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.
(Reporter)

Comment 12

2 years ago
(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.
(Reporter)

Comment 13

2 years ago
Created attachment 8772430 [details]
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.
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.
(Reporter)

Comment 15

2 years ago
(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:"
(Reporter)

Comment 16

2 years ago
Created attachment 8772761 [details]
firefox_2016-07-20_17-21-07.png

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.
(Assignee)

Comment 17

2 years ago
I confirmed, then I will look this.
Assignee: nobody → m_kato
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 18

2 years ago
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...

Comment 19

2 years ago
Created attachment 8772811 [details]
testcase

Updated

2 years ago
See Also: → bug 1146881
I have tried to narrow down a regression range but I couldn't due to the fact that this bug is reproduced intermittent.

Comment 21

2 years ago
Created attachment 8772872 [details]
testcase without download attribute

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

Updated

2 years ago
Blocks: 488420
status-firefox49: --- → affected
status-firefox50: --- → affected
status-firefox-esr45: --- → affected
Keywords: regressionwindow-wanted → regression

Updated

2 years ago
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)
(Assignee)

Comment 23

2 years ago
(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...?
(Assignee)

Updated

2 years ago
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.
(Assignee)

Comment 25

2 years ago
(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.
status-firefox47: affected → wontfix
status-firefox48: affected → wontfix
(Assignee)

Comment 28

2 years ago
Created attachment 8775017 [details]
Bug 1266815 - Part 1. Unnecessary DetachEditorFromWindow call from nsEditingSession.

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)
(Assignee)

Comment 29

2 years ago
Created attachment 8775018 [details]
Bug 1266815 - Part 2. Add editing session test.

Review commit: https://reviewboard.mozilla.org/r/67358/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67358/
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-
(Assignee)

Comment 32

2 years ago
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
(Assignee)

Comment 33

2 years ago
(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.
(Assignee)

Comment 34

2 years ago
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)
(Assignee)

Comment 35

2 years ago
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/
Comment on attachment 8775018 [details]
Bug 1266815 - Part 2. Add editing session test.

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

Nice!
Attachment #8775018 - Flags: review?(masayuki) → review+

Comment 37

2 years ago
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

Comment 38

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/59fa797ab438
https://hg.mozilla.org/mozilla-central/rev/10857066c347
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(Assignee)

Updated

2 years ago
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)
(Assignee)

Comment 41

2 years ago
(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)
(Assignee)

Comment 42

2 years ago
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.
status-firefox49: affected → wontfix

Updated

2 years ago
Version: 48 Branch → 47 Branch
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-

Updated

2 years ago
status-firefox50: affected → wontfix
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1278155
(Assignee)

Updated

a year ago
Duplicate of this bug: 1129372

Updated

a year ago
Duplicate of this bug: 991537
You need to log in before you can comment on or make changes to this bug.