Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8268003:
Hide autocomplete popup when IME candidate window is open (on Windows). (Closed)
|
Created:
9 years, 2 months ago by falken Modified:
9 years, 1 month ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionClose autocomplete popup when IME candidate window is open (on Windows), so they don't overlap each other.
BUG=2924
TEST=use IME in the omnibox, verify the autocomplete popup is closed whenever
the candidate window is open
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107339
Patch Set 1 #Patch Set 2 : initialize #
Total comments: 3
Patch Set 3 : actually close the popup #Patch Set 4 : non polling #
Total comments: 5
Patch Set 5 : move comment #
Total comments: 1
Patch Set 6 : expand comment #
Messages
Total messages: 14 (0 generated)
This patch implements the proposal to close the autocomplete popup whenever the IME candidate window is open (http://dev.chromium.org/developers/design-documents/omnibox-ime-coordination) It shouldn't affect Chrome Instant. It simply closes the popup.
Peter should review this too. I think this does what you want, but it's really bizarre to have the popup think it's open but not be open. In this case instant results shouldn't be pushed down but they likely are because we still think the popup is open. Maybe we just need to rename 'open' to 'running' or 'hasresults'. http://codereview.chromium.org/8268003/diff/7/chrome/browser/ui/omnibox/omnib... File chrome/browser/ui/omnibox/omnibox_view.cc (right): http://codereview.chromium.org/8268003/diff/7/chrome/browser/ui/omnibox/omnib... chrome/browser/ui/omnibox/omnibox_view.cc:23: bool OmniboxView::ShouldHideAutocompletePopup() const { Position doesn't match position in header. http://codereview.chromium.org/8268003/diff/7/chrome/browser/ui/views/autocom... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/8268003/diff/7/chrome/browser/ui/views/autocom... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:274: return (popup_ != NULL); I don't like that if ShouldHideAutocompletePopup returns true, IsOpen is still true.
http://codereview.chromium.org/8268003/diff/7/chrome/browser/ui/views/autocom... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/8268003/diff/7/chrome/browser/ui/views/autocom... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:274: return (popup_ != NULL); On 2011/10/13 15:58:02, sky wrote: > I don't like that if ShouldHideAutocompletePopup returns true, IsOpen is still > true. Yeah, this is bad. Besides the instant effect Scott mentions this is going to affect other things like how arrow keys are handled. It seems like instead the popup should actually be closed through the normal close path, not just hidden.
Thanks for the comments! I have some questions.
I guess there are two approaches for what I'm trying to do: 1) actually closing
the popup, and 2) hiding the popup.
Closing the popup almost works, but it undesirably cancels Instant during
conversion (the page reverts to what it was before editing). It seems due to
the following condition in AutocompleteEditModel::DoInstant being false:
if (user_input_in_progress() && popup_->IsOpen()) {
I'm wondering about this check. In what sort of cases do user_input_in_progress
and IsOpen differ? I think checking user_input_in_progress && (IsOpen ||
ShouldHideAutocompletePopup) would prevent the cancelling, but is kind of
hackish.
In the hide the popup approach, it seems we'd need a method like IsRunning in
addition to IsOpen, and update the IsOpen callsites to the appropriate one. For
example, use IsOpen to decide whether the push the Instant results down and
IsRunning for the above check in DoInstant. Does that sound like a reasonable
approach?
On 2011/10/13 17:53:53, Peter Kasting wrote:
>
http://codereview.chromium.org/8268003/diff/7/chrome/browser/ui/views/autocom...
> File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc
> (right):
>
>
http://codereview.chromium.org/8268003/diff/7/chrome/browser/ui/views/autocom...
> chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:274:
> return (popup_ != NULL);
> On 2011/10/13 15:58:02, sky wrote:
> > I don't like that if ShouldHideAutocompletePopup returns true, IsOpen is
still
> > true.
>
> Yeah, this is bad. Besides the instant effect Scott mentions this is going to
> affect other things like how arrow keys are handled.
>
> It seems like instead the popup should actually be closed through the normal
> close path, not just hidden.
On 2011/10/18 13:07:12, falken wrote:
> Closing the popup almost works, but it undesirably cancels Instant during
> conversion (the page reverts to what it was before editing).
That actually makes some sense to me, since I think of instant results as
inherently tied to the dropdown.
> It seems due to
> the following condition in AutocompleteEditModel::DoInstant being false:
>
> if (user_input_in_progress() && popup_->IsOpen()) {
>
> I'm wondering about this check. In what sort of cases do
user_input_in_progress
> and IsOpen differ?
Your case, and the case where a user switches tabs and then comes back (the
edits are still preserved but the dropdown is closed).
If we wanted to not cancel instant in your case we could just remove the
IsOpen() call entirely. I don't know whether we'd want to cancel instant on a
tab switch (away) or not but we could ditch this call either way. However then
you need to address Scott's question about the results getting pushed down.
Thanks for the comments. I've uploaded a new patch. I'm actually thinking now it's OK to cancel Instant during conversion, because there are already deeper issues with IME and Instant. For example, clicking on the page during IME composition cancels Instant (http://crbug.com/95397). It seems related to this check as well.. if I remove the IsOpen check, Instant is not cancelled, but things get wonky if you then switch tab out and back in. So, perhaps it's OK to commit this change for now and then work on the larger IME and instant issue? Scott, I can try bug 95397 if you don't mind. As for the pushed down results issue, it seems when the popup is closed, the popup bounds are properly updated, so the results aren't inappropriately pushed down.
This change basically uses a polling model where the contents view asks each time it updates if it should close. Instead of this you should simply instrument the message handler for your IME notification to actively close the popup. This is how we deal with other events (e.g. hitting escape, losing focus) that should result in the popup closing, and it will make the APIs simpler.
Thanks, I see. I've made a new patch with a question below. http://codereview.chromium.org/8268003/diff/13002/chrome/browser/ui/views/omn... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): http://codereview.chromium.org/8268003/diff/13002/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:699: return; I'm not sure whether this return should be above the ScopedFreeze. The comment for the early return below mentions that the ScopedFreeze was unnecessary, but it seems there is a reason for the order of freeze, set input in progress, return.
http://codereview.chromium.org/8268003/diff/13002/chrome/browser/ui/views/omn... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): http://codereview.chromium.org/8268003/diff/13002/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:698: // Don't allow the popup to overlap with the candidate window. Nit: Hoist this comment above the conditional and remove the {}. http://codereview.chromium.org/8268003/diff/13002/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:699: return; On 2011/10/20 11:06:53, falken wrote: > I'm not sure whether this return should be above the ScopedFreeze. The comment > for the early return below mentions that the ScopedFreeze was unnecessary, but > it seems there is a reason for the order of freeze, set input in progress, > return. It probably should be where you have it, but do you need this at all? Is UpdatePopup() ever called while the candidate window is open? Or can you eliminate this code and your new member variable entirely?
Thanks for the review. http://codereview.chromium.org/8268003/diff/13002/chrome/browser/ui/views/omn... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): http://codereview.chromium.org/8268003/diff/13002/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:698: // Don't allow the popup to overlap with the candidate window. On 2011/10/24 21:43:06, Peter Kasting wrote: > Nit: Hoist this comment above the conditional and remove the {}. Done. http://codereview.chromium.org/8268003/diff/13002/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:699: return; On 2011/10/24 21:43:06, Peter Kasting wrote: > On 2011/10/20 11:06:53, falken wrote: > > I'm not sure whether this return should be above the ScopedFreeze. The > comment > > for the early return below mentions that the ScopedFreeze was unnecessary, but > > it seems there is a reason for the order of freeze, set input in progress, > > return. > > It probably should be where you have it, but do you need this at all? Is > UpdatePopup() ever called while the candidate window is open? Or can you > eliminate this code and your new member variable entirely? Yes, it gets called as you change selection in the candidate window. When the selected candidate changes, the pre-edit text changes which triggers UpdatePopup.
Since you said the underlying text gets updated while the composition window is open, here's a worry I have. If I type something, then begin a composition, then switch tabs, I assume the composition window closes. Now if I switch back to the original tab, (a) Is the correct text showing -- i.e. with or without my composition window changes as appropriate? (b) If I put the cursor in the edit and hit enter without ever opening the popup, do I navigate to the right place? Or do I navigate to whatever I would have before I started my composition changes? If the above stuff all works right, LGTM. http://codereview.chromium.org/8268003/diff/16002/chrome/browser/ui/views/omn... File chrome/browser/ui/views/omnibox/omnibox_view_win.h (right): http://codereview.chromium.org/8268003/diff/16002/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_view_win.h:535: // True if the IME candidate window is open. Nit: Add "When this is true we want to avoid showing the popup."
On 2011/10/25 20:46:49, Peter Kasting wrote: > Since you said the underlying text gets updated while the composition window is > open, here's a worry I have. If I type something, then begin a composition, > then switch tabs, I assume the composition window closes. Now if I switch back > to the original tab, > (a) Is the correct text showing -- i.e. with or without my composition window > changes as appropriate? > (b) If I put the cursor in the edit and hit enter without ever opening the > popup, do I navigate to the right place? Or do I navigate to whatever I would > have before I started my composition changes? > > If the above stuff all works right, LGTM. Thanks for the worry. I tested and it's working as expected. (I think the IME or Windows itself commits the composition text upon IME losing focus, so it's effectively a commit, then switch tab.) > > http://codereview.chromium.org/8268003/diff/16002/chrome/browser/ui/views/omn... > File chrome/browser/ui/views/omnibox/omnibox_view_win.h (right): > > http://codereview.chromium.org/8268003/diff/16002/chrome/browser/ui/views/omn... > chrome/browser/ui/views/omnibox/omnibox_view_win.h:535: // True if the IME > candidate window is open. > Nit: Add "When this is true we want to avoid showing the popup." Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/falken@chromium.org/8268003/21001
Change committed as 107339 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
