Closed Bug 720659 Opened 12 years ago Closed 12 years ago

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

Categories

(Firefox :: Address Bar, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 13

People

(Reporter: tetsuharu, Assigned: masayuki)

References

Details

(Keywords: inputmethod, Whiteboard: [inline-autocomplete:blocker])

Attachments

(1 file, 4 obsolete files)

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.
Blocks: 566489
Component: Untriaged → Location Bar
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)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: inputmethod
OS: Windows 7 → All
Hardware: x86_64 → x86
was the previous autoFill implementation working correctly?
(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.
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 <ddahl@mozilla.com>. r=mak
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.
Or, currently, nsEditor doesn't support normal selection during composition. It might cause this bug too.
FYI
In my PC Core2Quad.
If set browser.urlbar.delay to 50, I cannot reproduce on Windows and Ubuntu both.
(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.
> 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
Attached patch Patch (obsolete) — Splinter Review
See comment 9 for the detail.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Attachment #591372 - Flags: review?(mak77)
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.
(In reply to Gavin Sharp (use gavin@gavinsharp.com 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.
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.
Depends on: 720792
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.
(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.
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #591372 - Attachment is obsolete: true
Attachment #591372 - Flags: review?(mak77)
Attachment #591751 - Flags: review?(mak77)
Whiteboard: [inline-autocomplete:blocker]
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"
Attachment #591751 - Flags: review?(mak77)
Attachment #591751 - Flags: review?(gavin.sharp)
Attachment #591751 - Flags: feedback+
Attached patch Patch (obsolete) — Splinter Review
Thank you, mak.
Attachment #591751 - Attachment is obsolete: true
Attachment #591751 - Flags: review?(gavin.sharp)
Attachment #592013 - Flags: review?(gavin.sharp)
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?
(In reply to Gavin Sharp (use gavin@gavinsharp.com 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.
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?
(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.
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.
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.
Attached patch Patch (obsolete) — Splinter Review
I confirmed this patch can fix this bug (updated for latest m-i).
Attachment #592013 - Attachment is obsolete: true
Attachment #592013 - Flags: review?(gavin.sharp)
Attachment #602175 - Flags: review?(gavin.sharp)
Attachment #602175 - Attachment is patch: true
(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).
(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.
Btw, completeDefaultIndex goes through nsAutoCompleteController::CompleteValue
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.
Attachment #602175 - Flags: feedback?(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?
Attachment #602175 - Flags: feedback?(m_kato) → feedback+
(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
Attached patch PatchSplinter Review
Attachment #602175 - Attachment is obsolete: true
Attachment #602175 - Flags: review?(gavin.sharp)
Attachment #603153 - Flags: review?(gavin.sharp)
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 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.
Attachment #603153 - Flags: review?(gavin.sharp) → review+
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.
Whiteboard: [inline-autocomplete:blocker] → [inline-autocomplete:blocker][inbound]
Target Milestone: --- → Firefox 13
Bug 720792 is also fixed. When will inline autocomplete be re-enabled?
(In reply to Siddhartha Dugar (sdrocking) from comment #36)
> Bug 720792 is also fixed. When will inline autocomplete be re-enabled?

I think so.
(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?
I will re-enable it after merging inbound
https://hg.mozilla.org/mozilla-central/rev/d4496bf4bb64

thank you for your help and responsiveness Masayuki-san and Makoto-san.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [inline-autocomplete:blocker][inbound] → [inline-autocomplete:blocker]
QA Contact: untriaged → location.bar
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: