+++ This bug was initially created as a clone of Bug #1240170 +++ I found an incompatible point with the other browsers. When changing the value of <textarea> or <input>, browsers including Gecko commits composition forcibly. However, when not changing the value, i.e., doing <textarea>.value = <textarea>.value;, the other browsers don't commit composition but Gecko does it.

testcase: changing <textarea>.value: https://jsfiddle.net/d_toybox/27knkt43/1/ not changing <textarea>.value; https://jsfiddle.net/d_toybox/27knkt43/

[Tracking Requested - why for this release]: bug 1240170 is caused by TweetDeck's update. However, I found a workaround for that. If TweetDeck won't fix the bug until 44, we can save our users by ourselves if this will be fixed in 44.

Comment on attachment 8708746 [details] [diff] [review] Setting same value to either <input> or <textarea> shouldn't cause committing existing composition Ehsan, could you check this as soon as possible? nsTextEditorState::SetValue() does do nothing if setting value doesn't change the value actually: http://mxr.mozilla.org/mozilla-central/source/dom/html/nsTextEditorState.cpp?rev=f6e63dd4fd9c&mark=1992-1993,1998-1998#1977 However, the change of bug 549674 doesn't check this fact before committing composition. So, let's check that and quit from it if there is composition and setting value won't change the value. This behavior is same as the other browsers' behavior. I think that we can sort out the code better but this patch is the safest change for uplifting. So, I'd like to just add this code for this particular case.

Given that we don't have a fix ready and we are into RC mode, it's too late and this is now a wontfix for Fx44.

Comment on attachment 8708746 [details] [diff] [review] Setting same value to either <input> or <textarea> shouldn't cause committing existing composition Review of attachment 8708746 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/html/nsTextEditorState.cpp @@ +1936,5 @@ > } > + // If setting value won't change current value, we shouldn't commit > + // composition for compatibility with the other browsers. > + nsAutoString currentValue; > + GetValue(currentValue, true); This doesn't mirror the condition below (the one referenced in comment 6) correctly. For one, the GetValue() call here may return the default value if the control is not yet initialized or if it doesn't have a frame. Also, the condition below compares against |newValue| which may be different than aValue in case mIsCommittingComposition is true. (That being said, we return before this point if mIsCommittingComposition is true, so I think using aValue here may actually be OK, but please double check. I think at the very least, you need to see whether mEditor and mBoundFrame are non-null here and in that case use GetText(), otherwise skip this check. ::: widget/tests/window_composition_text_querycontent.xul @@ +4102,5 @@ > "runForceCommitTest: the input still has composition #14"); > is(input.value, "\u306E appended value", > "runForceCommitTest: the input should have both composed text and appended text #14"); > > + input.focus(); It may also be a good idea to try to create a test case which would capture the behavior where we get the default value (for example by making the text control display:none) in order to cover more cases here.

Thank you for your review. (In reply to :Ehsan Akhgari from comment #10) > For one, the GetValue() call here may return the default value > if the control is not yet initialized It's impossible because EditorHasComposition() checks if mEditor is valid. > or if it doesn't have a frame. I have no idea what case this is. Can we create the case mEditor is non-null but mBoundFrame is null and the editor has focus? Note that when mEditor has IME composition, it means that the editor has (or had) focus. I'll rewrite the patch as you like, but I have no idea to write the testcase in this case. > Also, the condition below compares against |newValue| which may be different than > aValue in case mIsCommittingComposition is true. (That being said, we > return before this point if mIsCommittingComposition is true, so I think > using aValue here may actually be OK, but please double check. Yes, it never occurs because newValue is modified next block. But of course, it's safer to use it. > I think at the very least, you need to see whether mEditor and mBoundFrame > are non-null here and in that case use GetText(), otherwise skip this check. I agree. > ::: widget/tests/window_composition_text_querycontent.xul > @@ +4102,5 @@ > > "runForceCommitTest: the input still has composition #14"); > > is(input.value, "\u306E appended value", > > "runForceCommitTest: the input should have both composed text and appended text #14"); > > > > + input.focus(); > > It may also be a good idea to try to create a test case which would capture > the behavior where we get the default value (for example by making the text > control display:none) in order to cover more cases here. How can such display:none editor have focus? Without focus, editor can never has IME composition because composition events are fired only on focused element and if nobody has focus, IME is completely disabled by widget.

Like this testcase, I cannot use IME on display:none editor... https://jsfiddle.net/d_toybox/uqdaqo6f/2/

As I said above, I have no idea to add more tests which are related to IME. So, I just updated the SetValue()'s code.

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #12) > Thank you for your review. > > (In reply to :Ehsan Akhgari from comment #10) > > For one, the GetValue() call here may return the default value > > if the control is not yet initialized > > It's impossible because EditorHasComposition() checks if mEditor is valid. Yes, but what about the check for mBoundFrame? > > or if it doesn't have a frame. > > I have no idea what case this is. Can we create the case mEditor is non-null > but mBoundFrame is null and the editor has focus? Note that when mEditor has > IME composition, it means that the editor has (or had) focus. Hmm. It's certainly possible for mEditor to be non-null and mBoundFrame to be null. That being said, now that I think about this more, it should not be possible for the editor to have the focus in that case unless if something is very wrong. (We used to have bugs where the editor would still think it's focused after its frame was destroyed but I think they should all be fixed by now...) > I'll rewrite the patch as you like, but I have no idea to write the testcase > in this case. What I had in mind was setting the display to none and back to inline so that the frame gets recreated, but if this path won't be taken when the editor is not focused then perhaps my worries were unwarranted. > > Also, the condition below compares against |newValue| which may be different than > > aValue in case mIsCommittingComposition is true. (That being said, we > > return before this point if mIsCommittingComposition is true, so I think > > using aValue here may actually be OK, but please double check. > > Yes, it never occurs because newValue is modified next block. But of course, > it's safer to use it. Good! > > ::: widget/tests/window_composition_text_querycontent.xul > > @@ +4102,5 @@ > > > "runForceCommitTest: the input still has composition #14"); > > > is(input.value, "\u306E appended value", > > > "runForceCommitTest: the input should have both composed text and appended text #14"); > > > > > > + input.focus(); > > > > It may also be a good idea to try to create a test case which would capture > > the behavior where we get the default value (for example by making the text > > control display:none) in order to cover more cases here. > > How can such display:none editor have focus? Without focus, editor can never > has IME composition because composition events are fired only on focused > element and if nobody has focus, IME is completely disabled by widget. See above. :-)

Thank you, Ehsan. Fortunately, TweetDeck staff fixed the bug yesterday. So, we don't need to uplift the patch for 44!

https://hg.mozilla.org/integration/mozilla-inbound/rev/b30c8c5d62ba5dddcf42d29281b8fcded0b24a73 Bug 1240336 Setting same value to either <input> or <textarea> shouldn't cause committing existing composition r=ehsan

Comment on attachment 8709932 [details] [diff] [review] Setting same value to either <input> or <textarea> shouldn't cause committing existing composition Approval Request Comment [Feature/regressing bug #]: bug 549674 (but this is an incompatible issue with the other browsers of unspecced behavior). [User impact if declined]: Some web service would have same bug as bug 1240170, IME users couldn't use IME on such web apps. This is just a possible scenario, but if this occurs, the user impact is very very serious. [Describe test coverage new/current, TreeHerder]: Landed onto m-c and tested by automated tests. [Risks and why]: Low because adding additional check into the change of bug 549674. So, if there is no composition, this patch's new check won't be run. [String/UUID change made/needed]: Nothing.

Comment on attachment 8709932 [details] [diff] [review] Setting same value to either <input> or <textarea> shouldn't cause committing existing composition OK, let's take it. Thanks for the test!