Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2729133005:
Fix: Cursor missing in omnibox after entering a alphabet in NTP 'Search box' (Closed)
DescriptionFix: Cursor missing in omnibox after entering a alphabet in NTP 'Search box'
After any alphabet is entered in 'Search box', Textfield::DoInsertChar is
called and it first updates the cursor visibility by calling UpdateAfterChange
then it calls OnAfterUserAction to update the textfield focus and which may
change the cursor visibility. If the cursor visibility is changed in
OnAfterUserAction, the text cursor will not being updated again. The previous
CL (https://codereview.chromium.org/2728433002/) exposed this wrong order of
updating.
TEST=TextfieldTest.CursorVisibilityChangeAfterInserting
BUG=698172
Review-Url: https://codereview.chromium.org/2729133005
Cr-Commit-Position: refs/heads/master@{#456444}
Committed: https://chromium.googlesource.com/chromium/src/+/c57eaf40e82246d8acb9f58a350ef1a619cb19b1
Patch Set 1 #
Total comments: 1
Patch Set 2 : add tests #
Total comments: 3
Patch Set 3 : address comments #
Total comments: 8
Patch Set 4 : address comments #
Total comments: 3
Patch Set 5 : new approach #
Total comments: 28
Patch Set 6 : address comments #
Total comments: 8
Patch Set 7 : address nits #
Total comments: 1
Patch Set 8 : fix spelling #
Messages
Total messages: 61 (34 generated)
Description was changed from ========== fix BUG= ========== to ========== Fix: Cursor missing in omnibox after entering a alphabet in NTP 'Search box' After any alphabet is entered in 'Search box', Textfield::DoInsertChar is called and it first updates the cursor visibility by calling UpdateAfterChange then it calls OnAfterUserAction to update the textfield focus and which may change the cursor visibility. If the cursor visibility is changed in OnAfterUserAction, the text cursor will not being updated again. The previous CL (https://codereview.chromium.org/2728433002/) exposed this wrong order of updating. TEST=TextfieldTest.CursorVisibilityChangeAfterInserting BUG=698172 ==========
yiyix@chromium.org changed reviewers: + sadrul@chromium.org
@sadrul, could you please take a look? Thank you.
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2729133005/diff/1/ui/views/controls/textfield... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2729133005/diff/1/ui/views/controls/textfield... ui/views/controls/textfield/textfield.cc:1267: OnAfterUserAction(); There are a few other places where OnAfterUserAction() is called after the call to UpdateAfterChange() (e.g. here, ConfirmCompositionText(), ClearCompositionText(), InsertText(), ExecuteTextEditCommand()). Should these be reversed as well?
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/04 04:05:17, sadrul wrote: > https://codereview.chromium.org/2729133005/diff/1/ui/views/controls/textfield... > File ui/views/controls/textfield/textfield.cc (right): > > https://codereview.chromium.org/2729133005/diff/1/ui/views/controls/textfield... > ui/views/controls/textfield/textfield.cc:1267: OnAfterUserAction(); > There are a few other places where OnAfterUserAction() is called after the call > to UpdateAfterChange() (e.g. here, ConfirmCompositionText(), > ClearCompositionText(), InsertText(), ExecuteTextEditCommand()). Should these be > reversed as well? I had resersed all of them. Could you please take another look? Thank you.
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2729133005/diff/60001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2729133005/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:772: UpdateAfterChange(false, true); We shouldn't need this here? ExecuteTextEditCommand() should (and looks like does) make the necessary call to UpdateAfterChange() when appropriate. Are there cases when that does not happen?
https://codereview.chromium.org/2729133005/diff/60001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2729133005/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:772: UpdateAfterChange(false, true); On 2017/03/08 02:02:52, sadrul wrote: > We shouldn't need this here? ExecuteTextEditCommand() should (and looks like > does) make the necessary call to UpdateAfterChange() when appropriate. Are there > cases when that does not happen? ExecuteTextEditCommand first calls OmniboxViewViews::ExecuteTextEditCommand, command "move up", "move down" and "paste" is implemented in the OmniboxViewViews. OnAfterPossibleChange is called after paste. So the cursor's visibility may change after calling paste and it is not updated in UpdateAfterChange. To catch this particular path, i added this line.
Happy to discuss offline to explain things better. https://codereview.chromium.org/2729133005/diff/60001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2729133005/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:772: UpdateAfterChange(false, true); On 2017/03/08 03:32:41, yiyix wrote: > On 2017/03/08 02:02:52, sadrul wrote: > > We shouldn't need this here? ExecuteTextEditCommand() should (and looks like > > does) make the necessary call to UpdateAfterChange() when appropriate. Are > there > > cases when that does not happen? > > ExecuteTextEditCommand first calls OmniboxViewViews::ExecuteTextEditCommand, > command "move up", "move down" and "paste" is implemented in the > OmniboxViewViews. OnAfterPossibleChange is called after paste. So the cursor's > visibility may change after calling paste and it is not updated in > UpdateAfterChange. To catch this particular path, i added this line. OK. So it looks like there *are* some cases where UpdateAfterChange() does not get called. That's a bit unfortunate. For the call to UpdateAfterChange() here, why do we use false for text-change and true for cursor-change? I think we can make this code a little cleaner. We can probably split ExecuteTextEditCommand() into two parts: non-virtual ExecuteTextEditCommand(), which calls a virtual ExecuteTextEditCommandImpl() (or a better name). OmniboxViewViews overrides the latter. For Textfield, ExecuteTextEditCommandImpl() includes only the big switch-case block, and the rest remains in ExecuteTextEditCommand(). That way, ExecuteTextEditCommand() always calls UpdateAfterChange() when necessary, with the right parameters. OmniboxViewViews::ExecuteTextEditCommand() can also be cleaned a little (e.g. we could remove 829:836 in that file.)
@sadrul, Thank you for the feedback. I have separated ExecuteTextEditCommand to ExecuteTextEditCommand AND ExecuteTextEditCommandImpl. Could you please take another look? Thank you
https://codereview.chromium.org/2729133005/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/2729133005/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:828: return false; I feel like this should be true? (also, below for MOVE_DOWN) https://codereview.chromium.org/2729133005/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:829: break; You should remove the break; (here and below) https://codereview.chromium.org/2729133005/diff/80001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2729133005/diff/80001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:1645: } This part where we may change |command| should move into ExecuteTextEditCommand(). https://codereview.chromium.org/2729133005/diff/80001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.h (right): https://codereview.chromium.org/2729133005/diff/80001/ui/views/controls/textf... ui/views/controls/textfield/textfield.h:340: virtual bool ExecuteTextEditCommandImpl(ui::TextEditCommand command); Document the return value.
@sadrul, could you please take another look? Thank you. https://codereview.chromium.org/2729133005/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/2729133005/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:828: return false; On 2017/03/09 01:47:38, sadrul wrote: > I feel like this should be true? (also, below for MOVE_DOWN) Only OnPaste calls OnAfterPossibleChange, that's why I set the return value to true to OnPaste only. The MOVE_UP and MOVE_DOWN are used to choose a word in a popup window for composition text. The cursor visibility is not changed by moving up or moving down in he popup window. https://codereview.chromium.org/2729133005/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:829: break; On 2017/03/09 01:47:38, sadrul wrote: > You should remove the break; (here and below) No break needed after return. Right. I will watch out for that. https://codereview.chromium.org/2729133005/diff/80001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2729133005/diff/80001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:1645: } On 2017/03/09 01:47:38, sadrul wrote: > This part where we may change |command| should move into > ExecuteTextEditCommand(). Done. It doesn't change the current implementation, but better for future. Thanks. https://codereview.chromium.org/2729133005/diff/80001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.h (right): https://codereview.chromium.org/2729133005/diff/80001/ui/views/controls/textf... ui/views/controls/textfield/textfield.h:340: virtual bool ExecuteTextEditCommandImpl(ui::TextEditCommand command); On 2017/03/09 01:47:38, sadrul wrote: > Document the return value. Done.
sadrul@chromium.org changed reviewers: + sky@chromium.org
+sky@ for chrome changes. lgtm
sky@chromium.org changed reviewers: + msw@chromium.org - sky@chromium.org
msw is a better reviewer than I for this. sky->msw
https://codereview.chromium.org/2729133005/diff/100001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2729133005/diff/100001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1432: UpdateAfterChange(true, true); I really don't understand this change well enough... Why doesn't this CL flip all calls to UpdateAfterChange and OnAfterUserAction? I'm not sure I understand why we need to flip calls in the first place. Since this CL is meant to address a beta blocker, should we be looking to revert prior changes instead?
On 2017/03/09 17:07:45, msw wrote: > https://codereview.chromium.org/2729133005/diff/100001/ui/views/controls/text... > File ui/views/controls/textfield/textfield.cc (right): > > https://codereview.chromium.org/2729133005/diff/100001/ui/views/controls/text... > ui/views/controls/textfield/textfield.cc:1432: UpdateAfterChange(true, true); > I really don't understand this change well enough... Why doesn't this CL flip > all calls to UpdateAfterChange and OnAfterUserAction? I'm not sure I understand > why we need to flip calls in the first place. Since this CL is meant to address > a beta blocker, should we be looking to revert prior changes instead? The function UpdateAfterChange prints the cursor to screen and set it to the blinking mode if needed; and the function OnAfterUserAction updated |cursor_enabled| value. It was not a problem before because the cursor used to be painted on texture layer and schedulepaint is called after OnAfterUserAction. After my earlier CL, https://codereview.chromium.org/2660593002/, the cursor now paints on a solid color layer, the Schedulepaint stops catching the changed of the visibility. That's the cursor starts to act as normal after inserting more than 1 char. I want to properly change it, so i uploaded this cl.
https://codereview.chromium.org/2729133005/diff/100001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2729133005/diff/100001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1432: UpdateAfterChange(true, true); On 2017/03/09 17:07:45, msw wrote: > I really don't understand this change well enough... Why doesn't this CL flip > all calls to UpdateAfterChange and OnAfterUserAction? I'm not sure I understand > why we need to flip calls in the first place. Since this CL is meant to address > a beta blocker, should we be looking to revert prior changes instead? I missed this. Thanks for catching this. I think we should be consistent, and make sure the calls are flipped in all cases. So this needs to be flipped too.
sky@chromium.org changed reviewers: + sky@chromium.org
https://codereview.chromium.org/2729133005/diff/100001/ui/views/controls/text... File ui/views/controls/textfield/textfield.h (right): https://codereview.chromium.org/2729133005/diff/100001/ui/views/controls/text... ui/views/controls/textfield/textfield.h:341: virtual bool ExecuteTextEditCommandImpl(ui::TextEditCommand command); Having to return whether state is modified by subclasses is error prone, especially when the textfield maintains its own state and should be able to detect when it's modified. Additionally having two Execute functions results in an awkward API. Textfield should be able to detect when the model and/or caret visibility changes and take appropriate action. Is the bug that SetCursorEnabled() is a straight callthrough to RenderText when it potentially needs to update the cursor view?
The cursor is now updated in SetCursorEnabled instead. Could you please take look? Thank you.
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
This is *MUCH* better; thank you! I still have some comments. Hopefully we can inline ShowCursor in UpdateCursorView, and test the omnibox instead of adding similar quirky behavior to the TestTextfield class. https://codereview.chromium.org/2729133005/diff/120001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2729133005/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:458: if (GetRenderText()->cursor_enabled() == enabled) { nit: curlies not needed https://codereview.chromium.org/2729133005/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1928: void Textfield::ShowCursor() { Can we inline this into UpdateCursorView()? Otherwise consider renaming this "UpdateCursorVisibility" or similar; the name "ShowCursor" doesn't capture the possibility that this may actually hide the cursor and stop its blinking. https://codereview.chromium.org/2729133005/diff/120001/ui/views/controls/text... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2729133005/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:255: // A Textfield wrapper to intercept OnKey[Pressed|Released]() ressults. optional nit: please fix the spelling of 'results' if you don't mind. https://codereview.chromium.org/2729133005/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:256: // After user event, the textfield sets cursor_enabled in RenderText to true. Move this comment to OnAfterUserAction and explain why this is done, rather than reiterating what the code does; use vertical bars, like |cursor_enabled_|, when referencing a variable identifier. https://codereview.chromium.org/2729133005/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:400: textfield_->SetCursorEnabled(true); What is the motivation for this change? I'm not sure why we want this. https://codereview.chromium.org/2729133005/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:3123: render_text->SetCursorEnabled(false); Should this call textfield_->SetCursorEnabled(false) and then be able to check EXPECT_FALSE(test_api_->IsCursorVisible()) before calling OnBlur/OnFocus? https://codereview.chromium.org/2729133005/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:3128: render_text->SetCursorEnabled(true); Similarly, should this call textfield_->SetCursorEnabled(true) and then be able to check EXPECT_TRUE(test_api_->IsCursorVisible()) before calling OnBlur/OnFocus? https://codereview.chromium.org/2729133005/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:3134: // Verifies if the cursor visibility is changed to true from false after insert nit: 'inserting' https://codereview.chromium.org/2729133005/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:3135: // charactor in textfield. nit: 'a character in the textfield'. https://codereview.chromium.org/2729133005/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:3139: test_api_->GetRenderText()->SetCursorEnabled(false); Ditto: Should this call textfield_->SetCursorEnabled(false) and then be able to check EXPECT_FALSE(test_api_->IsCursorVisible()) before calling OnBlur/OnFocus? https://codereview.chromium.org/2729133005/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:3143: SendKeyEvent('a'); Why should inserting a character make the cursor visible if it was explicitly set to be not visible? I'm guessing this happens because of the new |textfield_->SetCursorEnabled(true)| call in OnAfterUserAction above, which is added to match the omnibox's quirky behavior. Can we instead just test that OmniboxViewViews does the right thing, instead of tweaking a test impl class to mimic its behavior? https://codereview.chromium.org/2729133005/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:3147: // Verifies if the cursor visibility is changed to true from false after paste nit: 'pasting' https://codereview.chromium.org/2729133005/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:3152: test_api_->GetRenderText()->SetCursorEnabled(false); ditto https://codereview.chromium.org/2729133005/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:3156: textfield_->SetText(base::string16()); Is this needed? https://codereview.chromium.org/2729133005/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:3157: SendKeyEvent(ui::VKEY_V, false, true); Ditto, can we test the omnibox instead?
Please also remember to update the CL description
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
@msw, Could you please take a look at this new patch? I rewrite the test in omnibox_view_views_unittests. https://codereview.chromium.org/2729133005/diff/120001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2729133005/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:458: if (GetRenderText()->cursor_enabled() == enabled) { On 2017/03/09 21:05:45, msw wrote: > nit: curlies not needed Done. https://codereview.chromium.org/2729133005/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1928: void Textfield::ShowCursor() { On 2017/03/09 21:05:45, msw wrote: > Can we inline this into UpdateCursorView()? Otherwise consider renaming this > "UpdateCursorVisibility" or similar; the name "ShowCursor" doesn't capture the > possibility that this may actually hide the cursor and stop its blinking. I don't think inline this into UpdateCursorView is a good idea. Since the function OnCursorBlinkTimerFired() calls UpdateCursorView and change the visibility, the updateCursorView should not call blink. I will update the name. https://codereview.chromium.org/2729133005/diff/120001/ui/views/controls/text... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2729133005/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:255: // A Textfield wrapper to intercept OnKey[Pressed|Released]() ressults. On 2017/03/09 21:05:45, msw wrote: > optional nit: please fix the spelling of 'results' if you don't mind. Sorry, I missed this. https://codereview.chromium.org/2729133005/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:256: // After user event, the textfield sets cursor_enabled in RenderText to true. On 2017/03/09 21:05:46, msw wrote: > Move this comment to OnAfterUserAction and explain why this is done, rather than > reiterating what the code does; use vertical bars, like |cursor_enabled_|, when > referencing a variable identifier. As explained below, I can remove this comment now. https://codereview.chromium.org/2729133005/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:400: textfield_->SetCursorEnabled(true); On 2017/03/09 21:05:46, msw wrote: > What is the motivation for this change? I'm not sure why we want this. I don't need this line now. I added this line to test the order between OnafterUserAction and other commends. https://codereview.chromium.org/2729133005/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:3123: render_text->SetCursorEnabled(false); On 2017/03/09 21:05:46, msw wrote: > Should this call textfield_->SetCursorEnabled(false) and then be able to check > EXPECT_FALSE(test_api_->IsCursorVisible()) before calling OnBlur/OnFocus? Done. https://codereview.chromium.org/2729133005/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:3128: render_text->SetCursorEnabled(true); On 2017/03/09 21:05:46, msw wrote: > Similarly, should this call textfield_->SetCursorEnabled(true) and then be able > to check EXPECT_TRUE(test_api_->IsCursorVisible()) before calling > OnBlur/OnFocus? In the approach I had before, the the cursor visibility is changed after OnBlur/OnFocus. In this new approach, the cursor visibility is updated in setcursorEnabled. I will update the test case to match the current approach. https://codereview.chromium.org/2729133005/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:3134: // Verifies if the cursor visibility is changed to true from false after insert On 2017/03/09 21:05:46, msw wrote: > nit: 'inserting' Done. https://codereview.chromium.org/2729133005/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:3135: // charactor in textfield. On 2017/03/09 21:05:46, msw wrote: > nit: 'a character in the textfield'. Done. https://codereview.chromium.org/2729133005/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:3143: SendKeyEvent('a'); On 2017/03/09 21:05:46, msw wrote: > Why should inserting a character make the cursor visible if it was explicitly > set to be not visible? I'm guessing this happens because of the new > |textfield_->SetCursorEnabled(true)| call in OnAfterUserAction above, which is > added to match the omnibox's quirky behavior. Can we instead just test that > OmniboxViewViews does the right thing, instead of tweaking a test impl class to > mimic its behavior? I will remove this test case and add some new test case in omnibox_view_views. https://codereview.chromium.org/2729133005/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:3147: // Verifies if the cursor visibility is changed to true from false after paste On 2017/03/09 21:05:46, msw wrote: > nit: 'pasting' Done. https://codereview.chromium.org/2729133005/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:3157: SendKeyEvent(ui::VKEY_V, false, true); On 2017/03/09 21:05:46, msw wrote: > Ditto, can we test the omnibox instead? I added a new test case in omnibox.
Awesome, lgtm with minor comments; thank you for addressing my concerns. I'll keep an eye on subsequent patch sets in case anything else comes up. https://codereview.chromium.org/2729133005/diff/120001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2729133005/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1928: void Textfield::ShowCursor() { On 2017/03/10 20:19:29, yiyix wrote: > On 2017/03/09 21:05:45, msw wrote: > > Can we inline this into UpdateCursorView()? Otherwise consider renaming this > > "UpdateCursorVisibility" or similar; the name "ShowCursor" doesn't capture the > > possibility that this may actually hide the cursor and stop its blinking. > > I don't think inline this into UpdateCursorView is a good idea. Since the > function OnCursorBlinkTimerFired() calls UpdateCursorView and change the > visibility, the updateCursorView should not call blink. > > I will update the name. Acknowledged. https://codereview.chromium.org/2729133005/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc (right): https://codereview.chromium.org/2729133005/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:273: // Checks that if the text cursor is set to enabled after inserting a single nit: no 'if'; cite the bug (and relation to the fake search bar)? https://codereview.chromium.org/2729133005/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:280: omnibox_textfield()->InsertChar(char_event); Should this also have a test for pasting, like the old TextfieldTest, CursorVisibilityChangeAfterPaste? https://codereview.chromium.org/2729133005/diff/140001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2729133005/diff/140001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:991: cursor_view_.SetVisible(true); optional nit: should this pass ShouldShowCursor(), or just call UpdateCursorVisibility() and remove the blinking check below, instead of passing |true|? Feel free to pursue that in a separate CL if you're hesitant to change that here. https://codereview.chromium.org/2729133005/diff/140001/ui/views/controls/text... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2729133005/diff/140001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:3115: // Verifies if the cursor visibility is controlled by |cursor_enabled_| in nit: "Verify that cursor visibility is controlled by SetCursorEnabled."
Patchset #7 (id:160001) has been deleted
https://codereview.chromium.org/2729133005/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc (right): https://codereview.chromium.org/2729133005/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:273: // Checks that if the text cursor is set to enabled after inserting a single On 2017/03/10 21:04:30, msw wrote: > nit: no 'if'; cite the bug (and relation to the fake search bar)? Done. https://codereview.chromium.org/2729133005/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:280: omnibox_textfield()->InsertChar(char_event); On 2017/03/10 21:04:30, msw wrote: > Should this also have a test for pasting, like the old TextfieldTest, > CursorVisibilityChangeAfterPaste? With the old approach, paste and insert char uses different code path. Since they use the same code path now, I only added this test case. https://codereview.chromium.org/2729133005/diff/140001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2729133005/diff/140001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:991: cursor_view_.SetVisible(true); On 2017/03/10 21:04:30, msw wrote: > optional nit: should this pass ShouldShowCursor(), or just call > UpdateCursorVisibility() and remove the blinking check below, instead of passing > |true|? Feel free to pursue that in a separate CL if you're hesitant to change > that here. Since it is a fix of dev block bug, I hesitate to make any functionality changed. I will include it in a separate CL. Thank you so much for point out. https://codereview.chromium.org/2729133005/diff/140001/ui/views/controls/text... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2729133005/diff/140001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:3115: // Verifies if the cursor visibility is controlled by |cursor_enabled_| in On 2017/03/10 21:04:30, msw wrote: > nit: "Verify that cursor visibility is controlled by SetCursorEnabled." Done.
The CQ bit was checked by yiyix@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/2729133005/#ps180001 (title: "address nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
still lgtm with one comment nit https://codereview.chromium.org/2729133005/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc (right): https://codereview.chromium.org/2729133005/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:274: // alphabet in NTP 'Search box'. Test for crbug.com/698172. nit: s/alphabet/character/
The CQ bit was checked by yiyix@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2729133005/#ps200001 (title: "fix spelling")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/13 17:15:24, msw wrote: > still lgtm with one comment nit > > https://codereview.chromium.org/2729133005/diff/180001/chrome/browser/ui/view... > File chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc (right): > > https://codereview.chromium.org/2729133005/diff/180001/chrome/browser/ui/view... > chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:274: // alphabet > in NTP 'Search box'. Test for crbug.com/698172. > nit: s/alphabet/character/ Fix the spelling. Thank you for paying attention to all details.
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1489427412290180,
"parent_rev": "c20ee868a144b9f01eaeae39bba8e9921dee2635", "commit_rev":
"c57eaf40e82246d8acb9f58a350ef1a619cb19b1"}
Message was sent while issue was closed.
Description was changed from ========== Fix: Cursor missing in omnibox after entering a alphabet in NTP 'Search box' After any alphabet is entered in 'Search box', Textfield::DoInsertChar is called and it first updates the cursor visibility by calling UpdateAfterChange then it calls OnAfterUserAction to update the textfield focus and which may change the cursor visibility. If the cursor visibility is changed in OnAfterUserAction, the text cursor will not being updated again. The previous CL (https://codereview.chromium.org/2728433002/) exposed this wrong order of updating. TEST=TextfieldTest.CursorVisibilityChangeAfterInserting BUG=698172 ========== to ========== Fix: Cursor missing in omnibox after entering a alphabet in NTP 'Search box' After any alphabet is entered in 'Search box', Textfield::DoInsertChar is called and it first updates the cursor visibility by calling UpdateAfterChange then it calls OnAfterUserAction to update the textfield focus and which may change the cursor visibility. If the cursor visibility is changed in OnAfterUserAction, the text cursor will not being updated again. The previous CL (https://codereview.chromium.org/2728433002/) exposed this wrong order of updating. TEST=TextfieldTest.CursorVisibilityChangeAfterInserting BUG=698172 Review-Url: https://codereview.chromium.org/2729133005 Cr-Commit-Position: refs/heads/master@{#456444} Committed: https://chromium.googlesource.com/chromium/src/+/c57eaf40e82246d8acb9f58a350e... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:200001) as https://chromium.googlesource.com/chromium/src/+/c57eaf40e82246d8acb9f58a350e...
Message was sent while issue was closed.
Patchset #9 (id:220001) has been deleted |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
