Closed Bug 549674 Opened 14 years ago Closed 9 years ago

Uncommitted IME means you can't set value of an input

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla41
Tracking Flags:
Tracking Status
p11 + ---
firefox41 --- fixed

People

(Reporter: mdavids, Assigned: masayuki)

References

(Depends on 1 open bug,
URL
)

Details

(Keywords: dev-doc-needed, inputmethod, site-compat, Whiteboard: [evang-wanted][compat])

Attachments

(2 files, 1 obsolete file)

part.2 SearchSuggestionUIController shouldn't set input.value from input event handler and ignore input events which is caused by commiting composition at setting value
2.50 KB, patch
adw
: review+
Details | Diff | Splinter Review
part.1 Commit composition string at setting value of <input> or <textarea>
47.65 KB, patch
smaug
: review+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.8) Gecko/20100202 Firefox/3.5.8 GTB6
Build Identifier: Firefox 3.5.8

See repro steps below. 

Reproducible: Always

Steps to Reproduce:
1. Open Gmail and start to compose 

2. Using Japanese IME, start to type a name that will autocomplete, but do not commit the IME selection.

For example, if you have a contact that starts with カ, switch to Katakana, type 'ka', but do not press enter.

3. Click on the autocomplete menu with the contact.
Actual Results:  
Everything in the compose field is cleared.

Expected Results:  
The autocomplete successfully fills in the To: field.

I have been unable to generate a smaller repro. (I'm a Gmail developer working on this bug.)

The problem is that we are getting the input element and calling

element.value = newVal;

I have verified that newVal contains the expected string, but the value after this line is empty. Setting the value in a timeout doesn't work. I'm a bit stuck.
cc'ing some people that know our i18n editor code.
Whiteboard: [evang-wanted]
This problem can be seen in 1.0x, 1.5x, 2,0x code base as well. It's been there all along to be discovered -- via something like auto completion before words are committed.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Probably, we should call nsIWidget::ResetInputState() when js sets value. However, that's not work fine on Linux (GTK2) because we cannot commit forcedly with uim or iBus... See bug 520732 comment 29.
Is there a workaround that I can put in the Gmail code? Setting the value in a timeout didn't work.
When you blur the focus, cannot you commit forcedly?
How do I commit the IME in JS?
For example, there is <input id="i">,

document.getElementById("i").blur(); can commit the composition.
# It should work fine on Win and Mac.

testcase:
> data:text/html,<body onload="document.getElementById('i').focus(); setTimeout(function() { document.getElementById('i').blur() }, 10000);"><input id="i"></body>
That does appear to work. I'll make the change in Gmail.
will be fixed by bug 632744 soon.
Assignee: nobody → m_kato
Depends on: 632744
(In reply to comment #9)
> will be fixed by bug 632744 soon.

Actually, bug 632744 ended up taking a Fennec specific workaround...
No longer depends on: 632744
This should be a bug of editor.

I think that after I fix bug 258291, we can avoid forceCompositionEnd() call from setting value handler.
Assignee: m_kato → masayuki
Component: DOM: Other → Editor
OS: Windows XP → All
Hardware: x86 → All
Version: unspecified → Trunk
According to bug 1170603, now, we should fix this bug in Mozilla 41.

Although, we still have bug 258291, we should commit composition at setting value. The iBus issue has already gone because TextComposition guarantees composition is committed synchronously and ignores delayed commit events coming from iBus automatically.
Status: NEW → ASSIGNED
tracking-p11: --- → +
Whiteboard: [evang-wanted] → [evang-wanted][compat]
Kato-san, do you have something to comment here about making Gecko commit composition at setting value?
Flags: needinfo?(m_kato)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #14)
> Kato-san, do you have something to comment here about making Gecko commit
> composition at setting value?

At least, composition should be commit or cancel, then it should set value by JS.

Also, I test on IE11 and Chrome on Windows 8.1,

IE11 ... composition is committed, then set value
Chrome ... composition is cancel, then set value

As result, value by JS is set.

You know, Firefox doesn't commit or cancel composition.  So Although composition is removed, OS still keep composition.
Flags: needinfo?(m_kato)
Thank you, Kato-san.

Chrome sometimes doesn't work well in my environment, e.g., remaining composition in IME.

I like IE11's behavior which is more natural. If some event handlers set .value during committing composition, the latest set value is set.
http://jsfiddle.net/d_toybox/h07w83kj/2/
Arai-san, with this patch which makes composition committed at setting value, browser_searchSuggestionUI.js causes random failure in composition test. I cannot reproduce this orange locally when I run the test alone. And also, when I run the test with --repeat-until-failure, the test won't work in the second time. So, I guess that the test or searchSuggestionUI.js has some bugs. Do you have any ideas?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5edf17c0c830
> 21:48:54     INFO -  601 INFO Entering test composition
> 21:48:54     INFO -  602 INFO TEST-PASS | browser/base/content/test/general/browser_searchSuggestionUI.js | State
> 21:48:54     INFO -  603 INFO TEST-PASS | browser/base/content/test/general/browser_searchSuggestionUI.js | State
> 21:48:54     INFO -  604 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_searchSuggestionUI.js | Test timed out - expected PASS
Attachment #8623477 - Flags: feedback?(arai.unmht)
Comment on attachment 8623477 [details] [diff] [review]
part.1 Commit composition string at setting value of <input> or <textarea>

Anyway, we need to land this for fixing new site-compat issue on Firefox for Android. So, let's start review of this. I might post follow up patches later, though.

First, nsTextEditorState::SetValue(), HTMLInputElement::SetValueInternal() and HTMLTextAreaElement::SetValueInternal() already has bool arguments. Therefore, I don't like to add new bool argument for distinguishing SetValue() is called by content or not.

Therefore, I make them use flags which is defined in nsTextEditorState. I believe that this makes the code easier to read.

And the main fix in this patch is changes in nsTextEditorState::SetValue(). If it's called not for internal processing (e.g., restoring the editor content after reframe), it should request to commit composition. Then, at least a text event, a compositionend event and an input event will be fired. In such event handlers, .value may be set again. Even when .value is set again during committing composition, nsIEditorIMESupport.composing should return false. However, for safety, this makes a wallpaper with mIsCommittingComposition and mSettingValue. While mIsCommittingComposition is true, mSettingValue is modified by each SetValue() call and which is used by GetValue() for returning temporary value even if the editor's value hasn't been modified actually.

After you review this, I'll request review to ehsan too if you think he should do it.
Attachment #8623477 - Flags: review?(bugs)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #17)
> Created attachment 8623477 [details] [diff] [review]
> part.1 Commit composition string at setting value of <input> or <textarea>
> 
> Arai-san, with this patch which makes composition committed at setting
> value, browser_searchSuggestionUI.js causes random failure in composition
> test. I cannot reproduce this orange locally when I run the test alone. And
> also, when I run the test with --repeat-until-failure, the test won't work
> in the second time. So, I guess that the test or searchSuggestionUI.js has
> some bugs. Do you have any ideas?
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=5edf17c0c830
> > 21:48:54     INFO -  601 INFO Entering test composition
> > 21:48:54     INFO -  602 INFO TEST-PASS | browser/base/content/test/general/browser_searchSuggestionUI.js | State
> > 21:48:54     INFO -  603 INFO TEST-PASS | browser/base/content/test/general/browser_searchSuggestionUI.js | State
> > 21:48:54     INFO -  604 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_searchSuggestionUI.js | Test timed out - expected PASS

The timeout is reproducible for me locally, on OS X. it's randomly waiting forever for following yields in composition test.
> state = yield msg("mousemove", 0);
or
> state = yield msg("mousemove", 1);

Then, with 2nd run, it waits forever for following yield in emptyInput test:
>  let state = yield msg("key", { key: "x", waitForSuggestions: true });

I'll look into them tonight.
> I'll look into them tonight.

Thanks in advance!

I have a question. If we cannot fix the failure, can I disable the part of composition in the test temporarily? If we miss to fix this bug in 41, some web sites which suggest search keywords do not work on Firefox for Android (bug 1170603). Perhaps, that is a regression of bug 1162818. However, we cannot back out the patches because the patches fixed serious web site compatibility and crash in TSF, additionally a lot of my patches depend on them.
Yeah, it can be disabled for a while, as long as it works with manual test (I will do later).
for now, I guess that it's a timing issue, around mouse event dispatch and suggestion list arrival, so, automated test adds event handler to suggestion list row which will be removed and rebuilt before the mousemove event. that won't matter outside of automated test.
oops, it's not good.  it doesn't work with manual test.  composition is committed for each input event in about:newtab. because of the selectAndUpdateInput call inside _onInput handler, which sets this.input.value.
And it results in calling _getSuggestions twice, and that's the reason of the random failure, so, the second response rebuilds the suggestion list while testing.
Wow, thanks for your investigation. Sounds like that _onInput should ignore input event if its isComposing is true.
https://developer.mozilla.org/en-US/docs/Web/API/InputEvent
if we ignore the input event while composing, suggestion list won't be updated until commit.

I think we shouldn't set this.input.value in _onInput, it's always same this._stickyInputValue.
(actually I don't see any reason to set it for input event)
https://dxr.mozilla.org/mozilla-central/source/browser/base/content/searchSuggestionUI.js#116
>  selectAndUpdateInput: function (idx) {
>    this.selectedIndex = idx;
>    this.input.value = idx >= 0 ? this.suggestionAtIndex(idx) :
>                       this._stickyInputValue;
>  },
Yeah, that makes sense.
So should the patch be reviewed or will there be some changes to it?
(In reply to Olli Pettay [:smaug] from comment #26)
> So should the patch be reviewed or will there be some changes to it?

Could you review it? We need a patch for browser/, but it's not related to the patch.
Comment on attachment 8623477 [details] [diff] [review]
part.1 Commit composition string at setting value of <input> or <textarea>

Review of attachment 8623477 [details] [diff] [review]:
-----------------------------------------------------------------

As the random failure was the issue inside searchSuggestionUI.js, I don't see any other issue on this patch's side :)

::: dom/html/nsTextEditorState.cpp
@@ +1934,4 @@
>  {
> +  nsAutoString newValue(aValue);
> +
> +  // While mIsCommittingComposition is true, GetValue() users mSettingValue for

GetValue() *uses* mSettingValue
Attachment #8623477 - Flags: feedback?(arai.unmht) → feedback+
adw: Could you review this? I'm trying to commit IME composition when JS sets value of <input> or <textarea> element. Then, the suggest code in browser is broken. It sets sticky value which is set from input.value to input.value in input event handler. So, that doesn't make sense and it causes not allowing to compose string with IME. So, in _onInput(), it shouldn't set input.value.
Attachment #8623728 - Flags: review?(adw)
Comment on attachment 8623728 [details] [diff] [review]
part.2 SearchSuggestionUIController shouldn't set input.value from input event handler and ignore input events which is caused by commiting composition at setting value

Review of attachment 8623728 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, makes sense.
Attachment #8623728 - Flags: review?(adw) → review+
Comment on attachment 8623477 [details] [diff] [review]
part.1 Commit composition string at setting value of <input> or <textarea>

>+      // If there is composition, needs to commit composition first because
s/needs/need/

>+  // While mIsCommittingComposition is true, GetValue() users mSettingValue for
>+  // its result.  So, when it's true, we need to modify mSettingValue.
>+  if (mIsCommittingComposition) {
>+    mSettingValue = aValue;
>+  }
I don't see anything guaranteeing we clear mSettingValue ever.

>+  // mSettingValue is available only during SetValue() is requesting to commit
Do you mean s/during/while/

>+  // composition.  I.e., this is valid only while mIsCommittingComposition is
>+  // true.  While active composition is being committed, GetValue() needs to
>+  // the latest value which is set by SetValue().
needs to the? I think you mean s/to//

 So, this is cache for that.
>+  nsString mSettingValue;
But I think the name of the variable is a bit odd.
Perhaps mValueBeingSet?
Attachment #8623477 - Flags: review?(bugs) → review-
>>+  // While mIsCommittingComposition is true, GetValue() users mSettingValue for
>>+  // its result.  So, when it's true, we need to modify mSettingValue.
>>+  if (mIsCommittingComposition) {
>>+    mSettingValue = aValue;
>>+  }
> I don't see anything guaranteeing we clear mSettingValue ever.

It's used for guaranteeing that input.value returns the latest value to be set during committing composition since the first setting value has not been completed at requesting committing composition (i.e., cannot retrieve the latest value from editor when GetValue() is called). When mIsCommittingComposition becomes false, it's truncated. See runNestedSettingValue() in the test, that is the reason why this is necessary.
Attachment #8623477 - Attachment is obsolete: true
I see mSettingValue.Truncate(); called only once, and specifically only when it is not safe to run scripts.


We set the value of mSettingValue in two places, and the latter of those is truncated later, but I don't see what truncates in case of
+nsTextEditorState::SetValue(const nsAString& aValue, uint32_t aFlags)
 {
+  nsAutoString newValue(aValue);
+
+  // While mIsCommittingComposition is true, GetValue() users mSettingValue for
+  // its result.  So, when it's true, we need to modify mSettingValue.
+  if (mIsCommittingComposition) {
+    mSettingValue = aValue;
+  }
(In reply to Olli Pettay [:smaug] from comment #33)
> I see mSettingValue.Truncate(); called only once, and specifically only when
> it is not safe to run scripts.

Because mIsCommittingComposition is true only when it's safe to run script and tries to commit composition. So, it's always empty when mIsCommittingComposition is false.

> We set the value of mSettingValue in two places, and the latter of those is
> truncated later, but I don't see what truncates in case of
> +nsTextEditorState::SetValue(const nsAString& aValue, uint32_t aFlags)
>  {
> +  nsAutoString newValue(aValue);
> +
> +  // While mIsCommittingComposition is true, GetValue() users mSettingValue
> for
> +  // its result.  So, when it's true, we need to modify mSettingValue.
> +  if (mIsCommittingComposition) {
> +    mSettingValue = aValue;
> +  }

If mIsCommittingComposition is true here, that means that some event handlers which are fired during committing composition are the caller of this method. So, after that, we must go back to the call of nsIEditorIMESupport::ForceCompositionEnd(). Then, mSettingValue is always truncated later than here.
Flags: needinfo?(bugs)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #34)
> Because mIsCommittingComposition is true only when it's safe to run script
> and tries to commit composition. 
That isn't actually quite accurate, since some nested call might use script blockers

>So, it's always empty when
> mIsCommittingComposition is false.
But yes, I see this now.


> If mIsCommittingComposition is true here, that means that some event
> handlers which are fired during committing composition are the caller of
> this method. So, after that, we must go back to the call of
> nsIEditorIMESupport::ForceCompositionEnd(). Then, mSettingValue is always
> truncated later than here.
So, there will be another SetValue call in that case? Please add a comment.



Could you please change if (!NS_WARN_IF(!nsContentUtils::IsSafeToRunScript())) {
That is hard to read. If you want the warning in some cases, put it to 'else'
Flags: needinfo?(bugs)
https://hg.mozilla.org/mozilla-central/rev/2a8ff86a5392
https://hg.mozilla.org/mozilla-central/rev/541e2c3f465f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
What impact does this have on developers? I'm trying to be sure I understand the documentation issues involved. I'm reviewing the patches too, but human insight is also helpful.
(In reply to Eric Shepherd [:sheppy] from comment #38)
> What impact does this have on developers? I'm trying to be sure I understand
> the documentation issues involved. I'm reviewing the patches too, but human
> insight is also helpful.

Positive impact: web developers can remove hack for Gecko to commit composition forcibly, that is calls of .blur() and .focus().

Negative impact: composition may be committed unexpectedly at setting .value attribute. The part.2 patch is a fix for this issue. However, this must not be problem for public web apps because we make the behavior same as other browsers.
See Also: → 1199658
See Also: → 1231162
Depends on: 1240170
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: