720659 - "Auto-completing" address bar doesn't play nice with IME

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect)

Description

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120123 Firefox/12.0a1
Build ID: 20120123095741

Steps to reproduce:

My enviroment:
・Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120123 Firefox/12.0a1( http://hg.mozilla.org/mozilla-central/rev/9b069a37f58f )
・Microsoft Office IME 2010


Actual results:

Stpe to Reproduce:
1. Typing "google" with IME in address bar.（strings are NOT decided）
2. Show "google.com" in address bar. (".com" is selected)
3. Decide typed strings.
4. Input only ".com" in address bar.


Expected results:

So it inputs "google.com" in address bar.

Comment 1

[Alice0775 White]

On Ubuntu10.40+GNOME2.30.2+Ibus Anthy

1. Start Nightly with new profile
2. Ibus On , input mode switch to Latin
3. type m in urlbar

Actual
ozilla.com/  (zilla.com/ selected)

Expected

mozilla.com/  (ozilla.com/ selected)

Comment 2

[Marco Bonardo [:mak]]

was the previous autoFill implementation working correctly?

Comment 3

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Marco Bonardo [:mak] from comment #2)
> was the previous autoFill implementation working correctly?

Yes.
On previous versions (e.g. Firefox 9), bookmark&history entries show only after the user defined typed strings. This bug does not occur.
This bug is related to inline auto-complete.

Comment 4

[Alice0775 White]

Step to Reproduce:
Microsoft Office IME 2010 + force autofill enabled ( Set  browser.urlbar.autoFill to true )

1. Start Nightly with new profile
2. IME on あ ローマ字入力
3. Type www.mo and commite(enter)

Actual Result:
 Prior landing cset 953bde82b7a7 : 
 www.mozilla.com/en-US/about  ( zilla.com/en-US/abou selected )

 After landing cset 953bde82b7a7 and recent Nightly :
 zilla.com/  ( .com/ selected )

Expected Result:
 www.mozilla.com/en-US/about  ( zilla.com/en-US/abou selected )
 OR
 www.mozilla.com/ ( zilla.com/ selected )


Regression window(m-i)
Works:
http://hg.mozilla.org/integration/mozilla-inbound/rev/eea95e86541f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120119 Firefox/12.0a1 ID:20120119024012
Fails:
http://hg.mozilla.org/integration/mozilla-inbound/rev/19a5e75b8ed8
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120119 Firefox/12.0a1 ID:20120119033312
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=eea95e86541f&tochange=19a5e75b8ed8

Triggered by (confirmen on local build):
953bde82b7a7	Michael Ventnor — Bug 566489 - Enable inline autocomplete again, but make it smarter. Original patch by Michael Ventnor, further improved by David Dahl <[e-mail]>. r=mak

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I'm not sure what happens.

During composition, the nsAutoCompleteController stops modifying mSearchString, starting search and listing up the suggestions.

I guess some code in places don't know this behavior and assume mSearchString is always same as the editor's value. If so, it may cause this behavior.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Or, currently, nsEditor doesn't support normal selection during composition. It might cause this bug too.

Comment 7

[Alice0775 White]

FYI
In my PC Core2Quad.
If set browser.urlbar.delay to 50, I cannot reproduce on Windows and Ubuntu both.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Alice0775 White from comment #7)
> FYI
> In my PC Core2Quad.
> If set browser.urlbar.delay to 50, I cannot reproduce on Windows and Ubuntu
> both.

Great, if so, we can fix this bug by backing out the change of nsAutoCompleteController.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> 1101   if (timeout == 0) {
> 1102     // The consumer wants to execute the search synchronously
> 1103     StartSearch();
> 1104     return NS_OK;
> 1105   }

I don't understand the reason since I don't familiar with places side. The main cause of this bug is this. When an event target receives compositionend event, the composition in editor hasn't finished yet actually. During "committing", our editor still has some limitations. Before bug 566489, we avoided the limitation by async trick (I guess it's not intentional) but the patch broke it.

If we don't need to search it synchronously, we should just remove this block.  However, otherwise, we can use following patch which I wrote and being tested on tryserver:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=15c47bc0b2d7

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

See comment 9 for the detail.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

FWIW, some portion of the timeout may be re-instated by bug 720792. It seems like we might want a fix for this anyways though.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Gavin Sharp (use [e-mail] for email) from comment #11)
> FWIW, some portion of the timeout may be re-instated by bug 720792. It seems
> like we might want a fix for this anyways though.

Thanks, I'll test the patch later.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Gavin and mak:

Those patches work fine with IME. If they won't go for Fx12 but the feature won't disabled, use my patch.

Comment 14

[Marco Bonardo [:mak]]

yes, we will still need to search sinchronously, though the timeout code will change. I suspect that won't fix this bug though.
I can't still say if the feature will stay enabled, if this bug is not fix it will likely stay disabled. Regardless, bug 720792 is needed before we can safely disable the feature.

From what I see in your patch there is absolutely no way to have this working with a synchronous search? Since inline needs to be synchronous otherwise it's going to replace chars while they are being typed.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Marco Bonardo [:mak] from comment #14)
> From what I see in your patch there is absolutely no way to have this
> working with a synchronous search? Since inline needs to be synchronous
> otherwise it's going to replace chars while they are being typed.

I think that we should handle an input event which is fired immediately after compositionend as compositionend. At the input event, editor must be free from the limitations.
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/base/nsEditor.cpp#1939

But it might need bigger patch than this. I worry about the risk.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

This patch moves some behaviors which are executed by compositionevent handler (HandleCompositionEnd()) to input event handler (HandleText()).

When HandleText() is called during eCompositionState_Committing, the composition is actually finished. I.e., nsEditor exits the composition mode. So, any code can access all features of editor.

I'd like to add a lot of tests for IME composition and autocomplete. But I don't have much time for now.

Comment 17

[Marco Bonardo [:mak]]

Comment on attachment 591751 [details] [diff] [review]
Patch

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

I honestly understand little related to IME (promise to document better on the argument), though the changes look sane to me.
I'm forwarding for the actual review to Gavin, who may have some deeper knowledge on the argument and is tracking this autocomplete-bugfixing effort.

::: toolkit/components/autocomplete/nsAutoCompleteController.cpp
@@ +216,5 @@
> +      }
> +    }
> +  private:
> +    nsRefPtr<nsAutoCompleteController> mController;
> +  };

I'd suggest moving this class to an anonymous namespace at the top of the file, and add a javadoc before it, explaining its scope and usage.

nit: Maybe I'd also replace the "Manager" naming with Scoper or Handler... manager to me sounds like a service or something similar.

@@ +375,5 @@
> +  // Now, the composition is just committing, not committed yet.
> +  // That means our editor still have some limitations.  We should touch
> +  // our editors until actually committed in them.
> +  // Fortunately, an input event is fired after compositionend event.
> +  // At that time, composition iis committed already.

typo: iis

::: toolkit/components/autocomplete/nsIAutoCompleteController.idl
@@ +79,5 @@
>  
>    /*
>     * Notify the controller that the user has changed text in the textbox.  This includes all
> +   * means of changing the text value, including typing a character, backspacing, deleting,
> +   * pasting, committing composition or canceling composition in an entirely new value.

nit: while here, could you please limit width of this to 80 chars?

@@ +109,5 @@
>    /*
>     * Notify the controller that the user wishes to end composition
> +   *
> +   * NOTE: handleText() must be called after composition actually ends even if the
> +   *       composition is canceled and the textbox value isn't changed.

nit: comma after "ends"

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

Thank you, mak.

Comment 19

[:Gavin Sharp [email: [e-mail]]]

I'm not sure I understand comment 13 - does it mean that we don't need the patch in this bug, as long as we fix bug 720792? Do I still need to review this patch?

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Gavin Sharp (use [e-mail] for email) from comment #19)
> I'm not sure I understand comment 13 - does it mean that we don't need the
> patch in this bug, as long as we fix bug 720792? Do I still need to review
> this patch?

If the patch for bug 720792 makes pass the all tests in bug 722961, we don't need the patch in this bug. Otherwise, we need it. I'm not sure what's changing in bug 720792.

If current compositionend event handler will work synchronously even with bug 720792 fix, we need this patch.

Comment 21

[Marco Bonardo [:mak]]

so, in the end, this is blocked by the review in bug 720792. Though you may already test the patch from that bug with your automated test?

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Marco Bonardo [:mak] from comment #21)
> so, in the end, this is blocked by the review in bug 720792. Though you may
> already test the patch from that bug with your automated test?

Yes, the tests assume the new behavior by this bug's patch is better.

Comment 23

[Marco Bonardo [:mak]]

Masayuki-san, could you please pull the current code from mozilla-inbound branch and update the patches accordingly?.  Please let us know what we must do still, to be able to re-enable browser.urlbar.autoFill by default without breaking IME users, or if the current behavior is already good enough.

Your tests in bug 722961 are passing for me, though doesn't look like those are actually testing the autoFill feature. So I'm a bit confused on the expected behavior.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I built with http://hg.mozilla.org/integration/mozilla-inbound/rev/7c612d36d3fa, I still see this bug. So, we need this patch.

On the other hand, bug 722961's test shouldn't pass without this patch. I'll look the tests today.

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

I confirmed this patch can fix this bug (updated for latest m-i).

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Marco Bonardo [:mak] from comment #23)
> Your tests in bug 722961 are passing for me, though doesn't look like those
> are actually testing the autoFill feature. So I'm a bit confused on the
> expected behavior.

Indeed... The tests test only the behavior of nsAutoCompleteController. So, I guess that I should add some tests places doing.

I have a question. When places sets the textbox's value and selection? I mean, I want to know what event is handled or what method is called from nsAutoCompleteController (or something).

Comment 27

[Marco Bonardo [:mak]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #26)
> I have a question. When places sets the textbox's value and selection? I
> mean, I want to know what event is handled or what method is called from
> nsAutoCompleteController (or something).

I don't think you need Places, you just need a generic controller test like test_immediate_search() in http://mxr.mozilla.org/mozilla-central/source/toolkit/components/autocomplete/tests/unit/test_immediate_search.js

Places uses an immediate search and its first result is used for completeDefaultIndex, nothing special about it. Immediate searches don't have any delay.

Comment 28

[Marco Bonardo [:mak]]

Btw, completeDefaultIndex goes through nsAutoCompleteController::CompleteValue

Comment 29

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 602175 [details] [diff] [review]
Patch

Makoto-san, would you be able to provide feedback on this patch? I have very little knowledge of IME/editing code.

Comment 30

[Makoto Kato [:m_kato]]

Comment on attachment 602175 [details] [diff] [review]
Patch

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

good, but some comments.

::: toolkit/components/autocomplete/nsAutoCompleteController.cpp
@@ +83,5 @@
> +  }
> +private:
> +  nsRefPtr<nsAutoCompleteController> mController;
> +};
> +

AutoCompositoinStateHandler isn't good name.  (e.g. AutoCompositionResetHandler?)

@@ +241,5 @@
>      return NS_OK;
>    }
>  
> +  AutoCompositionStateHandler compositionStateHandler(this);
> +

Before this, we should add a comment.  (After HandleText(), we need reset IME flags)

@@ +396,1 @@
>  

If mCompositionState isn't composing, should this flag be reset to committing?

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Makoto Kato from comment #30)
> @@ +396,1 @@
> >  
> 
> If mCompositionState isn't composing, should this flag be reset to
> committing?

# I assume that you were taking about HandleEndComposition()

No, therefore, if the state isn't composing, return from it by NS_ENSURE_TRUE().
# It shouldn't be happened, therefore, using it instead of normal early return

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

Comment 33

[Marco B.]

This is the last blocker to enable url autocomplete by default which is targeted for Fx 13, right? I guess this will miss the Aurora merge... Are there any plans for an Aurora 13 uplift of inline autocomplete?

Comment 34

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 603153 [details] [diff] [review]
Patch

>diff --git a/toolkit/components/autocomplete/nsAutoCompleteController.cpp b/toolkit/components/autocomplete/nsAutoCompleteController.cpp

> nsAutoCompleteController::HandleText()

The rest of this method only seems to care about checking mCompositionState == eCompositionState_Committing, and I don't see any users of mCompositionState or mPopupClosedByCompositionStart being called by this method. Can't you just use a couple of bools here to store state, and then reset mCompositionState/mPopupClosedByCompositionStart directly, rather than using AutoCompositionResetHandler?

bool handlingCompositionCommit = mCompositionState == eCompositionState_Committing;
bool popupClosedByCompositionStart = mPopupClosedByCompositionStart;
if (handlingCompositionCommit) {
  mCompositionState = eCompositionState_None;
  mPopupClosedByCompositionStart = false;
}

and then use handlingCompositionCommit/popupClosedByCompositionStart later in the function. That seems clearer to me, but let me know if I'm missing something...

> nsAutoCompleteController::HandleEndComposition()

>+  // Now, the composition is just committing, not committed yet.
>+  // That means our editor still have some limitations, so, we shouldn't touch
>+  // our editors until actually committed in them.
>+  // Fortunately, an input event is fired after compositionend event.
>+  // At that time, composition is committed already.

I would rephrase this as:

"We can't yet retrieve the committed value from the editor, since it isn't completely committed yet. Set mCompositionState to eCompositionState_Committing, so that when HandleText() is called (in response to the "input" event), we know that we should handle the committed text."

Should we file a followup bug on editor, to only fire compositionend events when the text has been fully committed?

>diff --git a/toolkit/components/autocomplete/nsIAutoCompleteController.idl b/toolkit/components/autocomplete/nsIAutoCompleteController.idl

>+   * Notify the controller that the user has changed text in the textbox.
>+   * This includes all means of changing the text value, including typing a
>+   * character, backspacing, deleting, pasting, committing composition or
>+   * canceling composition in an entirely new value.

Remove the "in an entirely new value" part (that was tied to "pasting", which you've now moved).

>+   * NOTE: handleText() must be called after composition actually ends, even if
>+   *       the composition is canceled and the textbox value isn't changed.
>    */
>   void handleEndComposition();

What ensures that this happens in our current implementation? I guess it's the editor via the autocomplete input binding event handlers (or nsFormFillController::HandleEvent)? I assume your tests in the other bug cover this requirement?

r=me with those comments addressed; sorry for the review delay.

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4496bf4bb64

landed on m-i. If it were too late, please reland it on m-c manually.

> Should we file a followup bug on editor, to only fire compositionend events when the text has been fully committed?

I'm working on such refactoring of composition handling on editor. I'm looking for that how to handle any actions during composition, though.

> What ensures that this happens in our current implementation?
> I guess it's the editor via the autocomplete input binding event handlers
> (or nsFormFillController::HandleEvent)? I assume your tests in the other
> bug cover this requirement?

Yes. The comment is what nsAutoCompletController expects. I rewrote the comment at landing, please let me know if I need to change them again.

Comment 36

[Siddhartha Dugar [:sdrocking]]

Bug 720792 is also fixed. When will inline autocomplete be re-enabled?

Comment 37

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Siddhartha Dugar (sdrocking) from comment #36)
> Bug 720792 is also fixed. When will inline autocomplete be re-enabled?

I think so.

Comment 38

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #37)
> (In reply to Siddhartha Dugar (sdrocking) from comment #36)
> > Bug 720792 is also fixed. When will inline autocomplete be re-enabled?
> 
> I think so.

Oops, you ask "when", maybe tomorrow in PST?

Comment 39

[Marco Bonardo [:mak]]

I will re-enable it after merging inbound

Comment 40

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/d4496bf4bb64

thank you for your help and responsiveness Masayuki-san and Makoto-san.