Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2952763002:
SearchBoxView now enables/disables cursor based on user interaction. (Closed)
DescriptionSearchBoxView now enables/disables cursor based on user interaction.
Updates to Integration Tests to test new searchbox behavior.
Added searchbox tests to the mus/mash filters.
Added HandleGestureEvent to TextfieldController.
Searchbox behavior added: The searchbox now has an active/inactive
state. If the searchbox is inactive and is clicked/tapped, it
activates.
UX CHANGES:
If the searchbox is Active and the user taps/clicks and the event propogates back to the AppListView, the SearchBox clears and the searchbox becomes inactive, transitioning to PEEKING or FULLSCREEN_ALL_APPS.
If the searchbox is inactive and the user taps/clicks and the event propogates back to the AppListView, the widget closes.
Also in this CL:
Mouse drag has been disabled.
Tests updated to reflect change in behavior of the app list.
BUG=735117
Review-Url: https://codereview.chromium.org/2952763002
Cr-Commit-Position: refs/heads/master@{#485749}
Committed: https://chromium.googlesource.com/chromium/src/+/ef1c07cf957e1b3ca050dec3a003c4d960cde442
Patch Set 1 : SearchBoxView now enables/disables cursor based on user interaction. #
Total comments: 2
Patch Set 2 : SearchBoxView now enables/disables cursor based on user interaction. #
Total comments: 20
Patch Set 3 : Responded to comments #
Total comments: 20
Patch Set 4 : SearchBoxView now enables/disables cursor based on user interaction. #
Total comments: 2
Patch Set 5 : Addressed Comments, refactored. #
Total comments: 16
Patch Set 6 : SearchBoxView now enables/disables cursor based on user interaction. #
Total comments: 9
Patch Set 7 : Responded to comments #Patch Set 8 : SearchBoxView now enables/disables cursor based on user interaction. #
Total comments: 22
Patch Set 9 : SearchBoxView now enables/disables cursor based on user interaction. #Patch Set 10 : Removed filters that are going to dissapear anyways after rebase. #
Total comments: 10
Patch Set 11 : addressed comments. #Patch Set 12 : rebased #Patch Set 13 : rebased. #Messages
Total messages: 100 (72 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
newcomer@chromium.org changed reviewers: + vadimt@chromium.org
PTAL! -Alex
https://codereview.chromium.org/2952763002/diff/40001/ui/app_list/views/searc... File ui/app_list/views/search_box_view.h (right): https://codereview.chromium.org/2952763002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_box_view.h:85: bool is_cursor_enabled() { return is_cursor_enabled_; } const
Quick fix
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
PTAL! -Alex https://codereview.chromium.org/2952763002/diff/40001/ui/app_list/views/searc... File ui/app_list/views/search_box_view.h (right): https://codereview.chromium.org/2952763002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_box_view.h:85: bool is_cursor_enabled() { return is_cursor_enabled_; } On 2017/06/21 17:44:56, vadimt wrote: > const Done.
https://codereview.chromium.org/2952763002/diff/120001/ui/app_list/views/sear... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/2952763002/diff/120001/ui/app_list/views/sear... ui/app_list/views/search_box_view.cc:357: bool SearchBoxView::PassMouseEventForTesting(ui::MouseEvent& mouse_event) { I assume you tried to find a way to inject a mouse event using standard injection approaches that other use for testing? (I assume they exist, but I don't know :)) https://codereview.chromium.org/2952763002/diff/120001/ui/app_list/views/sear... File ui/app_list/views/search_box_view.h (right): https://codereview.chromium.org/2952763002/diff/120001/ui/app_list/views/sear... ui/app_list/views/search_box_view.h:95: void UnitTestsRunning() { unit_tests_running_ = true; } First SetUnitTestsRunning, second, please stop by to talk about this crashing notification. Ideally, there should be no specialcasing for testing in the code. https://codereview.chromium.org/2952763002/diff/120001/ui/app_list/views/sear... ui/app_list/views/search_box_view.h:150: bool is_cursor_enabled_; (false) https://codereview.chromium.org/2952763002/diff/120001/ui/app_list/views/sear... ui/app_list/views/search_box_view.h:151: bool unit_tests_running_; // Whether the state change notification should be Comment into line before. https://codereview.chromium.org/2952763002/diff/120001/ui/app_list/views/sear... File ui/app_list/views/search_box_view_unittest.cc (right): https://codereview.chromium.org/2952763002/diff/120001/ui/app_list/views/sear... ui/app_list/views/search_box_view_unittest.cc:69: void ContinueSetUp(bool enable_fullscreen_app_list) { This reminds me LongMnemonicFunctionName1. Please try coming with another name. https://codereview.chromium.org/2952763002/diff/120001/ui/views/controls/text... File ui/views/controls/textfield/textfield.h (right): https://codereview.chromium.org/2952763002/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield.h:165: SkColor placeholder_text_color() { return placeholder_text_color_; } const https://codereview.chromium.org/2952763002/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield.h:171: int placeholder_text_draw_flags() { return placeholder_text_draw_flags_; } const https://codereview.chromium.org/2952763002/diff/120001/ui/views/controls/text... File ui/views/controls/textfield/textfield_controller.h (right): https://codereview.chromium.org/2952763002/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield_controller.h:46: // This method is called to notify that a gesture event has occured in the Skip "This method is" https://codereview.chromium.org/2952763002/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield_controller.h:47: // textfield. Currently, only tap events are sent here. OnGesture
Responded to comments! https://codereview.chromium.org/2952763002/diff/120001/ui/app_list/views/sear... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/2952763002/diff/120001/ui/app_list/views/sear... ui/app_list/views/search_box_view.cc:357: bool SearchBoxView::PassMouseEventForTesting(ui::MouseEvent& mouse_event) { On 2017/06/22 00:53:05, vadimt wrote: > I assume you tried to find a way to inject a mouse event using standard > injection approaches that other use for testing? (I assume they exist, but I > don't know :)) I couldn't find a way to do this without completely redesigning the way the unit test runs. I tried doing this as an integration test and that also failed. I was waiting for reviewers that might have a better idea than my hacky solution. https://codereview.chromium.org/2952763002/diff/120001/ui/app_list/views/sear... File ui/app_list/views/search_box_view.h (right): https://codereview.chromium.org/2952763002/diff/120001/ui/app_list/views/sear... ui/app_list/views/search_box_view.h:95: void UnitTestsRunning() { unit_tests_running_ = true; } On 2017/06/22 00:53:05, vadimt wrote: > First SetUnitTestsRunning, second, please stop by to talk about this crashing > notification. Ideally, there should be no specialcasing for testing in the code. Fixed by checking if app_list_view_ has been set before calling the function that crashes the unit tests. (app_list_view_ is nullptr by default during unit tests). Removed the bool and this function. https://codereview.chromium.org/2952763002/diff/120001/ui/app_list/views/sear... ui/app_list/views/search_box_view.h:150: bool is_cursor_enabled_; On 2017/06/22 00:53:05, vadimt wrote: > (false) This is done in the constructor. https://codereview.chromium.org/2952763002/diff/120001/ui/app_list/views/sear... ui/app_list/views/search_box_view.h:151: bool unit_tests_running_; // Whether the state change notification should be On 2017/06/22 00:53:05, vadimt wrote: > Comment into line before. Removed this bool as a side effect. https://codereview.chromium.org/2952763002/diff/120001/ui/app_list/views/sear... File ui/app_list/views/search_box_view_unittest.cc (right): https://codereview.chromium.org/2952763002/diff/120001/ui/app_list/views/sear... ui/app_list/views/search_box_view_unittest.cc:69: void ContinueSetUp(bool enable_fullscreen_app_list) { On 2017/06/22 00:53:05, vadimt wrote: > This reminds me LongMnemonicFunctionName1. > Please try coming with another name. I think the new one is better. LMK. https://codereview.chromium.org/2952763002/diff/120001/ui/views/controls/text... File ui/views/controls/textfield/textfield.h (right): https://codereview.chromium.org/2952763002/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield.h:165: SkColor placeholder_text_color() { return placeholder_text_color_; } On 2017/06/22 00:53:05, vadimt wrote: > const Done. https://codereview.chromium.org/2952763002/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield.h:171: int placeholder_text_draw_flags() { return placeholder_text_draw_flags_; } On 2017/06/22 00:53:05, vadimt wrote: > const Done. https://codereview.chromium.org/2952763002/diff/120001/ui/views/controls/text... File ui/views/controls/textfield/textfield_controller.h (right): https://codereview.chromium.org/2952763002/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield_controller.h:46: // This method is called to notify that a gesture event has occured in the On 2017/06/22 00:53:06, vadimt wrote: > Skip "This method is" Done and fixed above comments to match. https://codereview.chromium.org/2952763002/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield_controller.h:47: // textfield. Currently, only tap events are sent here. On 2017/06/22 00:53:05, vadimt wrote: > OnGesture Done.
https://codereview.chromium.org/2952763002/diff/120001/ui/app_list/views/sear... File ui/app_list/views/search_box_view.h (right): https://codereview.chromium.org/2952763002/diff/120001/ui/app_list/views/sear... ui/app_list/views/search_box_view.h:150: bool is_cursor_enabled_; And the preferred way is to initialize in the header via "= value". https://codereview.chromium.org/2952763002/diff/140001/ui/app_list/views/sear... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/2952763002/diff/140001/ui/app_list/views/sear... ui/app_list/views/search_box_view.cc:65: // Color used for placeholder text in zero query state. Color of placeholder ... https://codereview.chromium.org/2952763002/diff/140001/ui/app_list/views/sear... ui/app_list/views/search_box_view.cc:155: is_cursor_enabled_(false) { = false in .h https://codereview.chromium.org/2952763002/diff/140001/ui/app_list/views/sear... ui/app_list/views/search_box_view.cc:240: if (app_list_view_) Is there a way for unit test to make app_list_view_ != null? Still not quite happy about unit test codepaths deviating from the real ones. https://codereview.chromium.org/2952763002/diff/140001/ui/app_list/views/sear... ui/app_list/views/search_box_view.cc:346: void SearchBoxView::SetPlaceholderTextAndEnableCursor(bool enable) { The name is misleading, as you don't set (change) the text; could you come with a different name? https://codereview.chromium.org/2952763002/diff/140001/ui/app_list/views/sear... ui/app_list/views/search_box_view.cc:394: SetPlaceholderTextAndEnableCursor(true); Please make a note: what to do when Meta+Arrow accessibility focus arrives to Search box. https://codereview.chromium.org/2952763002/diff/140001/ui/app_list/views/sear... ui/app_list/views/search_box_view.cc:484: void SearchBoxView::NotifyOnGestureEvent() { Do we need both this and SearchBoxView::OnGestureEvent? https://codereview.chromium.org/2952763002/diff/140001/ui/app_list/views/sear... File ui/app_list/views/search_box_view.h (right): https://codereview.chromium.org/2952763002/diff/140001/ui/app_list/views/sear... ui/app_list/views/search_box_view.h:89: void SetPlaceholderTextAndEnableCursor(bool enable); Please document what exactly is |enable|d https://codereview.chromium.org/2952763002/diff/140001/ui/app_list/views/sear... ui/app_list/views/search_box_view.h:115: void NotifyOnGestureEvent() override; Just "OnGesture()" https://codereview.chromium.org/2952763002/diff/140001/ui/app_list/views/sear... ui/app_list/views/search_box_view.h:147: bool is_cursor_enabled_; Doesn't match the comment. is_cursor_blink_enabled? https://codereview.chromium.org/2952763002/diff/140001/ui/views/controls/text... File ui/views/controls/textfield/textfield_controller.h (right): https://codereview.chromium.org/2952763002/diff/140001/ui/views/controls/text... ui/views/controls/textfield/textfield_controller.h:48: virtual void NotifyOnGestureEvent(); "OnGesture() {}" Note curlies here.
Patchset #4 (id:160001) has been deleted
Patchset #4 (id:180001) has been deleted
Patchset #4 (id:200001) has been deleted
The CQ bit was checked by newcomer@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: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #5 (id:240001) has been deleted
Patchset #4 (id:220001) has been deleted
Patchset #6 (id:300001) has been deleted
Patchset #5 (id:280001) has been deleted
Patchset #4 (id:260001) has been deleted
Addressed comments! -Alex https://codereview.chromium.org/2952763002/diff/120001/ui/app_list/views/sear... File ui/app_list/views/search_box_view.h (right): https://codereview.chromium.org/2952763002/diff/120001/ui/app_list/views/sear... ui/app_list/views/search_box_view.h:150: bool is_cursor_enabled_; On 2017/06/22 22:47:40, vadimt wrote: > And the preferred way is to initialize in the header via "= value". Done! https://codereview.chromium.org/2952763002/diff/140001/ui/app_list/views/sear... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/2952763002/diff/140001/ui/app_list/views/sear... ui/app_list/views/search_box_view.cc:65: // Color used for placeholder text in zero query state. On 2017/06/22 22:47:40, vadimt wrote: > Color of placeholder ... Done. https://codereview.chromium.org/2952763002/diff/140001/ui/app_list/views/sear... ui/app_list/views/search_box_view.cc:155: is_cursor_enabled_(false) { On 2017/06/22 22:47:40, vadimt wrote: > = false in .h Done. https://codereview.chromium.org/2952763002/diff/140001/ui/app_list/views/sear... ui/app_list/views/search_box_view.cc:240: if (app_list_view_) On 2017/06/22 22:47:40, vadimt wrote: > Is there a way for unit test to make app_list_view_ != null? > Still not quite happy about unit test codepaths deviating from the real ones. One solution is to create a class that is a SearchBoxViewObserver, we could have AppListView inherit from that class. Then we can call observers here instead of directly calling AppListView. This would prevent the if statement. https://codereview.chromium.org/2952763002/diff/140001/ui/app_list/views/sear... ui/app_list/views/search_box_view.cc:346: void SearchBoxView::SetPlaceholderTextAndEnableCursor(bool enable) { On 2017/06/22 22:47:40, vadimt wrote: > The name is misleading, as you don't set (change) the text; could you come with > a different name? Renamed to SetSearchBoxActive(bool active) https://codereview.chromium.org/2952763002/diff/140001/ui/app_list/views/sear... ui/app_list/views/search_box_view.cc:394: SetPlaceholderTextAndEnableCursor(true); On 2017/06/22 22:47:40, vadimt wrote: > Please make a note: what to do when Meta+Arrow accessibility focus arrives to > Search box. Created a bug for this. https://codereview.chromium.org/2952763002/diff/140001/ui/app_list/views/sear... ui/app_list/views/search_box_view.cc:484: void SearchBoxView::NotifyOnGestureEvent() { On 2017/06/22 22:47:40, vadimt wrote: > Do we need both this and SearchBoxView::OnGestureEvent? refactored to HandleGestureEvent like the other functions in the base class TextfieldController. https://codereview.chromium.org/2952763002/diff/140001/ui/app_list/views/sear... File ui/app_list/views/search_box_view.h (right): https://codereview.chromium.org/2952763002/diff/140001/ui/app_list/views/sear... ui/app_list/views/search_box_view.h:89: void SetPlaceholderTextAndEnableCursor(bool enable); On 2017/06/22 22:47:40, vadimt wrote: > Please document what exactly is |enable|d Adjusted the comment and renamed the bool to be more informative. https://codereview.chromium.org/2952763002/diff/140001/ui/app_list/views/sear... ui/app_list/views/search_box_view.h:115: void NotifyOnGestureEvent() override; On 2017/06/22 22:47:40, vadimt wrote: > Just "OnGesture()" Just refactored to HandleGestureEvent to keep the consistency of TextfieldController. https://codereview.chromium.org/2952763002/diff/140001/ui/app_list/views/sear... ui/app_list/views/search_box_view.h:147: bool is_cursor_enabled_; On 2017/06/22 22:47:40, vadimt wrote: > Doesn't match the comment. is_cursor_blink_enabled? Done. https://codereview.chromium.org/2952763002/diff/140001/ui/views/controls/text... File ui/views/controls/textfield/textfield_controller.h (right): https://codereview.chromium.org/2952763002/diff/140001/ui/views/controls/text... ui/views/controls/textfield/textfield_controller.h:48: virtual void NotifyOnGestureEvent(); On 2017/06/22 22:47:41, vadimt wrote: > "OnGesture() {}" > Note curlies here. Refactored as a side effect.
lgtm + Spend reasonable effort to pass app list view to test search box instance. https://codereview.chromium.org/2952763002/diff/320001/ui/app_list/views/sear... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/2952763002/diff/320001/ui/app_list/views/sear... ui/app_list/views/search_box_view.cc:345: void SearchBoxView::SetSearchBoxActive(bool active) { Early return if active == is_search_box_active_, otherwise, the concern is that SchedulePaint will cause if not blinking, then perf problems.
Description was changed from ========== SearchBoxView now enables/disables cursor based on user interaction. Tests added. Added a gesture notification method to textfield_controller, and wired it up with textfield and SearchBoxView. BUG=735117 ========== to ========== SearchBoxView now enables/disables cursor based on user interaction. Tests added. Added a gesture notification method to textfield_controller, and wired it up with textfield and SearchBoxView. BUG=735117 ==========
newcomer@chromium.org changed reviewers: + xiyuan@chromium.org
newcomer@chromium.org changed reviewers: + oshima@chromium.org
Patchset #5 (id:340001) has been deleted
Description was changed from ========== SearchBoxView now enables/disables cursor based on user interaction. Tests added. Added a gesture notification method to textfield_controller, and wired it up with textfield and SearchBoxView. BUG=735117 ========== to ========== SearchBoxView now enables/disables cursor based on user interaction. Updates to Integration Tests to test new searchbox behavior. Added searchbox tests to the mus/mash filters. Added HandleGestureEvent to TextfieldController. Searchbox behavior added: The searchbox now has an active/inactive state. If the searchbox is inactive and is clicked/tapped, it activates. If the searchbox is Active and the user taps/clicks anywhere on the widget body, the AppListview closes. BUG=735117 ==========
Patchset #5 (id:360001) has been deleted
Patchset #5 (id:380001) has been deleted
Patchset #5 (id:350013) has been deleted
Patchset #5 (id:410001) has been deleted
Oshima, Xiyuan, PTAL. -Alex https://codereview.chromium.org/2952763002/diff/320001/ui/app_list/views/sear... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/2952763002/diff/320001/ui/app_list/views/sear... ui/app_list/views/search_box_view.cc:345: void SearchBoxView::SetSearchBoxActive(bool active) { On 2017/06/24 00:31:17, vadimt wrote: > Early return if active == is_search_box_active_, otherwise, the concern is that > SchedulePaint will cause if not blinking, then perf problems. Done.
https://codereview.chromium.org/2952763002/diff/430001/ui/app_list/views/sear... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/2952763002/diff/430001/ui/app_list/views/sear... ui/app_list/views/search_box_view.cc:395: app_list_view_->SetState(AppListView::CLOSED); It seems we want to dismiss app list when there is a unhandled (click/tap) event hits the root AppListView. Can we do this in AppListView::OnEvent instead of installing SearchBoxView as a pre target handler? https://codereview.chromium.org/2952763002/diff/430001/ui/app_list/views/sear... ui/app_list/views/search_box_view.cc:400: SetSearchBoxActive(true); This branch is probably not necessary since the events should reach us as |search_box_| (Textfield) events in OnTextfieldEvent(). https://codereview.chromium.org/2952763002/diff/430001/ui/app_list/views/sear... File ui/app_list/views/search_box_view.h (right): https://codereview.chromium.org/2952763002/diff/430001/ui/app_list/views/sear... ui/app_list/views/search_box_view.h:87: // Setting the search box active left aligns the placeholder text, changes nit: get rid of one space between "search" and "box".
https://codereview.chromium.org/2952763002/diff/430001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2952763002/diff/430001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:99: SearchBoxView* search_box_view() const { return search_box_view_; } non const if returning non const internal object (this will allow a caller to modify internal state of the const object) https://codereview.chromium.org/2952763002/diff/430001/ui/app_list/views/sear... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/2952763002/diff/430001/ui/app_list/views/sear... ui/app_list/views/search_box_view.cc:183: app_list_view_->PrependPreTargetHandler(this); just curious. Why you need to use target handler? (because views will receive these events) https://codereview.chromium.org/2952763002/diff/430001/ui/app_list/views/sear... ui/app_list/views/search_box_view.cc:432: HandleSearchBoxEvent(event->AsLocatedEvent(), event->type()); AsLocatedEvent is a utility method to downcast from Event class. You don't need it here because GestureEvent is already LocatedEvent. (and no need to pass type separately as the event already have it) You can just do HandleSearchBoxEvent(event); https://codereview.chromium.org/2952763002/diff/430001/ui/app_list/views/sear... File ui/app_list/views/search_result_page_view.h (right): https://codereview.chromium.org/2952763002/diff/430001/ui/app_list/views/sear... ui/app_list/views/search_result_page_view.h:56: views::View* contents_view() const { return contents_view_; } ditto https://codereview.chromium.org/2952763002/diff/430001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2952763002/diff/430001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:700: if (controller_->HandleGestureEvent(this, *event)) { controller_ may be null, so just check like other places.
Description was changed from ========== SearchBoxView now enables/disables cursor based on user interaction. Updates to Integration Tests to test new searchbox behavior. Added searchbox tests to the mus/mash filters. Added HandleGestureEvent to TextfieldController. Searchbox behavior added: The searchbox now has an active/inactive state. If the searchbox is inactive and is clicked/tapped, it activates. If the searchbox is Active and the user taps/clicks anywhere on the widget body, the AppListview closes. BUG=735117 ========== to ========== SearchBoxView now enables/disables cursor based on user interaction. Updates to Integration Tests to test new searchbox behavior. Added searchbox tests to the mus/mash filters. Added HandleGestureEvent to TextfieldController. Searchbox behavior added: The searchbox now has an active/inactive state. If the searchbox is inactive and is clicked/tapped, it activates. If the searchbox is Active and the user taps/clicks anywhere on the widget body, the SearchBox clears and the searchbox becomes inactive. BUG=735117 ==========
Description was changed from ========== SearchBoxView now enables/disables cursor based on user interaction. Updates to Integration Tests to test new searchbox behavior. Added searchbox tests to the mus/mash filters. Added HandleGestureEvent to TextfieldController. Searchbox behavior added: The searchbox now has an active/inactive state. If the searchbox is inactive and is clicked/tapped, it activates. If the searchbox is Active and the user taps/clicks anywhere on the widget body, the SearchBox clears and the searchbox becomes inactive. BUG=735117 ========== to ========== SearchBoxView now enables/disables cursor based on user interaction. Updates to Integration Tests to test new searchbox behavior. Added searchbox tests to the mus/mash filters. Added HandleGestureEvent to TextfieldController. Searchbox behavior added: The searchbox now has an active/inactive state. If the searchbox is inactive and is clicked/tapped, it activates. If the searchbox is Active and the user taps/clicks anywhere on the widget body, the SearchBox clears and the searchbox becomes inactive. Also in this CL: Mouse drag has been disabled. BUG=735117 ==========
Description was changed from ========== SearchBoxView now enables/disables cursor based on user interaction. Updates to Integration Tests to test new searchbox behavior. Added searchbox tests to the mus/mash filters. Added HandleGestureEvent to TextfieldController. Searchbox behavior added: The searchbox now has an active/inactive state. If the searchbox is inactive and is clicked/tapped, it activates. If the searchbox is Active and the user taps/clicks anywhere on the widget body, the SearchBox clears and the searchbox becomes inactive. Also in this CL: Mouse drag has been disabled. BUG=735117 ========== to ========== SearchBoxView now enables/disables cursor based on user interaction. Updates to Integration Tests to test new searchbox behavior. Added searchbox tests to the mus/mash filters. Added HandleGestureEvent to TextfieldController. Searchbox behavior added: The searchbox now has an active/inactive state. If the searchbox is inactive and is clicked/tapped, it activates. UX CHANGES: If the searchbox is Active and the user taps/clicks and the event propogates back to the AppListView, the SearchBox clears and the searchbox becomes inactive, transitioning to PEEKING or FULLSCREEN_ALL_APPS. If the searchbox is inactive and the user taps/clicks and the event propogates back to the AppListView, the widget closes. Also in this CL: Mouse drag has been disabled. BUG=735117 ==========
Description was changed from ========== SearchBoxView now enables/disables cursor based on user interaction. Updates to Integration Tests to test new searchbox behavior. Added searchbox tests to the mus/mash filters. Added HandleGestureEvent to TextfieldController. Searchbox behavior added: The searchbox now has an active/inactive state. If the searchbox is inactive and is clicked/tapped, it activates. UX CHANGES: If the searchbox is Active and the user taps/clicks and the event propogates back to the AppListView, the SearchBox clears and the searchbox becomes inactive, transitioning to PEEKING or FULLSCREEN_ALL_APPS. If the searchbox is inactive and the user taps/clicks and the event propogates back to the AppListView, the widget closes. Also in this CL: Mouse drag has been disabled. BUG=735117 ========== to ========== SearchBoxView now enables/disables cursor based on user interaction. Updates to Integration Tests to test new searchbox behavior. Added searchbox tests to the mus/mash filters. Added HandleGestureEvent to TextfieldController. Searchbox behavior added: The searchbox now has an active/inactive state. If the searchbox is inactive and is clicked/tapped, it activates. UX CHANGES: If the searchbox is Active and the user taps/clicks and the event propogates back to the AppListView, the SearchBox clears and the searchbox becomes inactive, transitioning to PEEKING or FULLSCREEN_ALL_APPS. If the searchbox is inactive and the user taps/clicks and the event propogates back to the AppListView, the widget closes. Also in this CL: Mouse drag has been disabled. Tests updated to reflect change in behavior of the app list. BUG=735117 ==========
Patchset #6 (id:450001) has been deleted
Patchset #7 (id:490001) has been deleted
Patchset #6 (id:470001) has been deleted
Patchset #6 (id:510001) has been deleted
Patchset #6 (id:530001) has been deleted
Patchset #7 (id:570001) has been deleted
Patchset #6 (id:550001) has been deleted
Patchset #6 (id:590001) has been deleted
Patchset #6 (id:610001) has been deleted
Patchset #6 (id:630001) has been deleted
Thanks for the comments! PTAL. -Alex https://codereview.chromium.org/2952763002/diff/430001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2952763002/diff/430001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:99: SearchBoxView* search_box_view() const { return search_box_view_; } Done! I only did this because the widget's getter was made in a similar way. https://codereview.chromium.org/2952763002/diff/430001/ui/app_list/views/sear... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/2952763002/diff/430001/ui/app_list/views/sear... ui/app_list/views/search_box_view.cc:183: app_list_view_->PrependPreTargetHandler(this); On 2017/06/29 16:58:50, oshima (OOO back on July 5th) wrote: > just curious. Why you need to use target handler? (because views will receive > these events) This has been removed. There were some issues that I fixed in the leaf views that were preventing the events from properly propogating to the AppListView. I'll annotate them in the next patch. https://codereview.chromium.org/2952763002/diff/430001/ui/app_list/views/sear... ui/app_list/views/search_box_view.cc:395: app_list_view_->SetState(AppListView::CLOSED); On 2017/06/27 23:11:10, xiyuan wrote: > It seems we want to dismiss app list when there is a unhandled (click/tap) event > hits the root AppListView. Can we do this in AppListView::OnEvent instead of > installing SearchBoxView as a pre target handler? Handled offline. We found the problems in the leaf views that were preventing the events from propogating. https://codereview.chromium.org/2952763002/diff/430001/ui/app_list/views/sear... ui/app_list/views/search_box_view.cc:400: SetSearchBoxActive(true); On 2017/06/27 23:11:10, xiyuan wrote: > This branch is probably not necessary since the events should reach us as > |search_box_| (Textfield) events in OnTextfieldEvent(). It is necessary because there are two regions of the rendered search box, the textfield and the background(where on mouse hover the mouse does not transition to the cursor, but the mouse is over the white background of the searchbox). This branch detects the latter case. https://codereview.chromium.org/2952763002/diff/430001/ui/app_list/views/sear... ui/app_list/views/search_box_view.cc:432: HandleSearchBoxEvent(event->AsLocatedEvent(), event->type()); On 2017/06/29 16:58:50, oshima (OOO back on July 5th) wrote: > AsLocatedEvent is a utility method to downcast from Event class. > > You don't need it here because GestureEvent is already LocatedEvent. (and no > need to pass type separately as the event already have it) > > You can just do HandleSearchBoxEvent(event); Done. https://codereview.chromium.org/2952763002/diff/430001/ui/app_list/views/sear... File ui/app_list/views/search_box_view.h (right): https://codereview.chromium.org/2952763002/diff/430001/ui/app_list/views/sear... ui/app_list/views/search_box_view.h:87: // Setting the search box active left aligns the placeholder text, changes On 2017/06/27 23:11:10, xiyuan wrote: > nit: get rid of one space between "search" and "box". Done. https://codereview.chromium.org/2952763002/diff/430001/ui/app_list/views/sear... File ui/app_list/views/search_result_page_view.h (right): https://codereview.chromium.org/2952763002/diff/430001/ui/app_list/views/sear... ui/app_list/views/search_result_page_view.h:56: views::View* contents_view() const { return contents_view_; } On 2017/06/29 16:58:50, oshima (OOO back on July 5th) wrote: > ditto Done. https://codereview.chromium.org/2952763002/diff/430001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2952763002/diff/430001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:700: if (controller_->HandleGestureEvent(this, *event)) { On 2017/06/29 16:58:50, oshima (OOO back on July 5th) wrote: > controller_ may be null, so just check like other places. Done.
https://codereview.chromium.org/2952763002/diff/650001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate_unittest.cc (right): https://codereview.chromium.org/2952763002/diff/650001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:288: EnableFullscreenAppList(); If you enable it anyway, why this has to be parameterized? https://codereview.chromium.org/2952763002/diff/650001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2952763002/diff/650001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:513: default: add case CLOSED: break and remove default. This way, compiler can catch when you add new enum member but forgot to handle it. https://codereview.chromium.org/2952763002/diff/650001/ui/app_list/views/sear... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/2952763002/diff/650001/ui/app_list/views/sear... ui/app_list/views/search_box_view.cc:494: return false; OnTextfieldEvent checks this, so you don't need this here. https://codereview.chromium.org/2952763002/diff/650001/ui/app_list/views/sear... ui/app_list/views/search_box_view.cc:501: if (!is_fullscreen_app_list_enabled_) ditto
Thanks Oshima@! Responded to comments. https://codereview.chromium.org/2952763002/diff/650001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate_unittest.cc (right): https://codereview.chromium.org/2952763002/diff/650001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:288: EnableFullscreenAppList(); On 2017/07/05 19:11:59, oshima wrote: > If you enable it anyway, why this has to be parameterized? I couldn't find a way to create multiple parameters, so I used the parameter in this test in a hacky way to parameterize gesture/mouse events while always having the FullscreenAppList feature enabled. I think the solution here is to take EnableFullscreenAppList out of AppListPresenterDelegateTest::SetUp(). https://codereview.chromium.org/2952763002/diff/650001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2952763002/diff/650001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:513: default: On 2017/07/05 19:11:59, oshima wrote: > add > > case CLOSED: > break > > and remove default. This way, compiler can catch when you add new enum member > but forgot to handle it. Done, thanks! https://codereview.chromium.org/2952763002/diff/650001/ui/app_list/views/sear... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/2952763002/diff/650001/ui/app_list/views/sear... ui/app_list/views/search_box_view.cc:494: return false; On 2017/07/05 19:11:59, oshima wrote: > OnTextfieldEvent checks this, so you don't need this here. Done. https://codereview.chromium.org/2952763002/diff/650001/ui/app_list/views/sear... ui/app_list/views/search_box_view.cc:501: if (!is_fullscreen_app_list_enabled_) On 2017/07/05 19:11:59, oshima wrote: > ditto Done.
https://codereview.chromium.org/2952763002/diff/650001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate_unittest.cc (right): https://codereview.chromium.org/2952763002/diff/650001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:288: EnableFullscreenAppList(); On 2017/07/05 22:03:57, newcomer wrote: > On 2017/07/05 19:11:59, oshima wrote: > > If you enable it anyway, why this has to be parameterized? > > I couldn't find a way to create multiple parameters, so I used the parameter in > this test in a hacky way to parameterize gesture/mouse events while always > having the FullscreenAppList feature enabled. > > I think the solution here is to take EnableFullscreenAppList out of > AppListPresenterDelegateTest::SetUp(). You should create a separate base class, which always enable the fullscreen app list. Alternatively, you can use a struct, but I think the former is better.
Added new class, FullscreenAppListPresenterDelegateTest, which always enables the fullscreen app list feature.
Added Patchset 9 which changes the names of the tests in the filters to match the new test class.
https://codereview.chromium.org/2952763002/diff/710001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate_unittest.cc (left): https://codereview.chromium.org/2952763002/diff/710001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:54: EnableFullscreenAppList(); Please keep this logic. Looks like you don't need test_with_fullscreen_, so you can just do if (GetParam()) EnableFullscreenAppList() https://codereview.chromium.org/2952763002/diff/710001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate_unittest.cc (right): https://codereview.chromium.org/2952763002/diff/710001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:96: TEST_F(AppListPresenterDelegateTest, HideOnFocusOut) { Shouldn't this be P? https://codereview.chromium.org/2952763002/diff/710001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:104: } new line https://codereview.chromium.org/2952763002/diff/710001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:118: TEST_F(AppListPresenterDelegateTest, P? https://codereview.chromium.org/2952763002/diff/710001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:192: TEST_F(AppListPresenterDelegateTest, NonPrimaryDisplay) { P? https://codereview.chromium.org/2952763002/diff/710001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:347: StateTransitionsByTapAndClickingAppListBodyFromHalf) { you need to instantiate parameterized tests. Please add comment what boolean parameter means (mouse vs gesture)
Patchset #9 (id:710001) has been deleted
Patchset #8 (id:690001) has been deleted
Patchset #8 (id:730001) has been deleted
Patchset #8 (id:750001) has been deleted
Please address these nits and then lgtm https://codereview.chromium.org/2952763002/diff/770001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate_unittest.cc (right): https://codereview.chromium.org/2952763002/diff/770001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:54: EnableFullscreenAppList(); nit: you need {} in this case https://codereview.chromium.org/2952763002/diff/770001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:289: bool tap_or_click = GetParam(); test_mouse_event (a_or_b sounds like it's true for both cases and false for other cases) https://codereview.chromium.org/2952763002/diff/770001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:294: EXPECT_TRUE(app_list_view->app_list_state() == nit: EXPECT_EQ https://codereview.chromium.org/2952763002/diff/770001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:299: EXPECT_TRUE(app_list_view->app_list_state() == ditto https://codereview.chromium.org/2952763002/diff/770001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:307: outside_search_box.set_x(20); how about BoundsInScreen().top_right() Offset(0, -1); ? This seems to be used in several places, so please define an utility method to get the point outside of the search box. https://codereview.chromium.org/2952763002/diff/770001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:315: app_list::AppListView::AppListState::PEEKING); ditto https://codereview.chromium.org/2952763002/diff/770001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:328: EXPECT_TRUE(app_list_view->app_list_state() == ditto https://codereview.chromium.org/2952763002/diff/770001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:357: app_list::AppListView::AppListState::FULLSCREEN_SEARCH); ditto https://codereview.chromium.org/2952763002/diff/770001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:364: outside_search_box.set_x(20); ditto https://codereview.chromium.org/2952763002/diff/770001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:382: EXPECT_TRUE(app_list_view->app_list_state() == ditto https://codereview.chromium.org/2952763002/diff/770001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:409: search_box_point.set_x(20); ditto
Patchset #9 (id:790001) has been deleted
newcomer@chromium.org changed reviewers: + khmel@chromium.org
Khmel@, PTAL! -Alex https://codereview.chromium.org/2952763002/diff/770001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate_unittest.cc (right): https://codereview.chromium.org/2952763002/diff/770001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:54: EnableFullscreenAppList(); On 2017/07/06 21:05:51, oshima wrote: > nit: you need {} in this case Done. https://codereview.chromium.org/2952763002/diff/770001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:289: bool tap_or_click = GetParam(); On 2017/07/06 21:05:51, oshima wrote: > test_mouse_event (a_or_b sounds like it's true for both cases and false for > other cases) Done. https://codereview.chromium.org/2952763002/diff/770001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:294: EXPECT_TRUE(app_list_view->app_list_state() == On 2017/07/06 21:05:51, oshima wrote: > nit: EXPECT_EQ Done. https://codereview.chromium.org/2952763002/diff/770001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:299: EXPECT_TRUE(app_list_view->app_list_state() == On 2017/07/06 21:05:51, oshima wrote: > ditto Done. https://codereview.chromium.org/2952763002/diff/770001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:307: outside_search_box.set_x(20); On 2017/07/06 21:05:51, oshima wrote: > how about > > BoundsInScreen().top_right() > Offset(0, -1); > > ? > > This seems to be used in several places, so please define an utility method to > get the point outside of the search box. Done. https://codereview.chromium.org/2952763002/diff/770001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:315: app_list::AppListView::AppListState::PEEKING); On 2017/07/06 21:05:51, oshima wrote: > ditto Done. https://codereview.chromium.org/2952763002/diff/770001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:328: EXPECT_TRUE(app_list_view->app_list_state() == On 2017/07/06 21:05:51, oshima wrote: > ditto Done. https://codereview.chromium.org/2952763002/diff/770001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:357: app_list::AppListView::AppListState::FULLSCREEN_SEARCH); On 2017/07/06 21:05:51, oshima wrote: > ditto Done. https://codereview.chromium.org/2952763002/diff/770001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:364: outside_search_box.set_x(20); On 2017/07/06 21:05:51, oshima wrote: > ditto Done. https://codereview.chromium.org/2952763002/diff/770001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:382: EXPECT_TRUE(app_list_view->app_list_state() == On 2017/07/06 21:05:51, oshima wrote: > ditto Done. https://codereview.chromium.org/2952763002/diff/770001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:409: search_box_point.set_x(20); On 2017/07/06 21:05:51, oshima wrote: > ditto Done.
https://codereview.chromium.org/2952763002/diff/830001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate_unittest.cc (right): https://codereview.chromium.org/2952763002/diff/830001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:73: class FullscreenAppListPresenterDelegateTest nit: not sure, but looks like we might merge to one test with 2 parameters. One for full screen and second one for mouse/tap events. Does functionality you test here apply to non-full screen mode also? If applies then we might have 4 modes test here. Full Screen Mouse Full Screen Tap Non-Full Screen Mouse Non-Full Screen Tap WDYT? https://codereview.chromium.org/2952763002/diff/830001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:74: : public test::AshTestBase, nit: It would be nice to add comment what param means for this test. https://codereview.chromium.org/2952763002/diff/830001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:295: bool test_mouse_event = GetParam(); nit: const bool test_mouse_event = ... I would also prefer to have this inside the class def. bool test_mouse_event() const { return GetParam();) https://codereview.chromium.org/2952763002/diff/830001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:345: int top_of_app_list = nit: const int ... https://codereview.chromium.org/2952763002/diff/830001/ui/app_list/views/sear... File ui/app_list/views/search_box_view.h (right): https://codereview.chromium.org/2952763002/diff/830001/ui/app_list/views/sear... ui/app_list/views/search_box_view.h:85: bool is_search_box_active() const { return is_search_box_active_; } Nit: move this to L75 to keep getters/setters in one place.
khmel@, PTAL! -Alex https://codereview.chromium.org/2952763002/diff/830001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate_unittest.cc (right): https://codereview.chromium.org/2952763002/diff/830001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:73: class FullscreenAppListPresenterDelegateTest On 2017/07/10 15:52:22, khmel wrote: > nit: not sure, but looks like we might merge to one test with 2 parameters. One > for full screen and second one for mouse/tap events. > Does functionality you test here apply to non-full screen mode also? > If applies then we might have 4 modes test here. > Full Screen Mouse > Full Screen Tap > Non-Full Screen Mouse > Non-Full Screen Tap > > WDYT? Discussed offline: We are using two classes because the non-fullscreen class is going to be deleted at a later date. https://codereview.chromium.org/2952763002/diff/830001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:74: : public test::AshTestBase, On 2017/07/10 15:52:22, khmel wrote: > nit: It would be nice to add comment what param means for this test. There is a comment on the Initializer for this parameter (line 120), I added a comment on test_mouse_event() as well. https://codereview.chromium.org/2952763002/diff/830001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:295: bool test_mouse_event = GetParam(); On 2017/07/10 15:52:22, khmel wrote: > nit: const bool test_mouse_event = ... > > I would also prefer to have this inside the class def. > > bool test_mouse_event() const { return GetParam();) Done! https://codereview.chromium.org/2952763002/diff/830001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:345: int top_of_app_list = On 2017/07/10 15:52:22, khmel wrote: > nit: const int ... Done. https://codereview.chromium.org/2952763002/diff/830001/ui/app_list/views/sear... File ui/app_list/views/search_box_view.h (right): https://codereview.chromium.org/2952763002/diff/830001/ui/app_list/views/sear... ui/app_list/views/search_box_view.h:85: bool is_search_box_active() const { return is_search_box_active_; } On 2017/07/10 15:52:22, khmel wrote: > Nit: move this to L75 to keep getters/setters in one place. Done.
Thanks! lgtm
The CQ bit was checked by newcomer@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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #13 (id:890001) has been deleted
Patchset #12 (id:870001) has been deleted
The CQ bit was checked by newcomer@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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by newcomer@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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by newcomer@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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by newcomer@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 newcomer@chromium.org
The CQ bit was checked by newcomer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vadimt@chromium.org, oshima@chromium.org, khmel@chromium.org Link to the patchset: https://codereview.chromium.org/2952763002/#ps930001 (title: "rebased.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 930001, "attempt_start_ts": 1499810099520740,
"parent_rev": "f535f3e8476a4727232607d5d887409686405f45", "commit_rev":
"ef1c07cf957e1b3ca050dec3a003c4d960cde442"}
Message was sent while issue was closed.
Description was changed from ========== SearchBoxView now enables/disables cursor based on user interaction. Updates to Integration Tests to test new searchbox behavior. Added searchbox tests to the mus/mash filters. Added HandleGestureEvent to TextfieldController. Searchbox behavior added: The searchbox now has an active/inactive state. If the searchbox is inactive and is clicked/tapped, it activates. UX CHANGES: If the searchbox is Active and the user taps/clicks and the event propogates back to the AppListView, the SearchBox clears and the searchbox becomes inactive, transitioning to PEEKING or FULLSCREEN_ALL_APPS. If the searchbox is inactive and the user taps/clicks and the event propogates back to the AppListView, the widget closes. Also in this CL: Mouse drag has been disabled. Tests updated to reflect change in behavior of the app list. BUG=735117 ========== to ========== SearchBoxView now enables/disables cursor based on user interaction. Updates to Integration Tests to test new searchbox behavior. Added searchbox tests to the mus/mash filters. Added HandleGestureEvent to TextfieldController. Searchbox behavior added: The searchbox now has an active/inactive state. If the searchbox is inactive and is clicked/tapped, it activates. UX CHANGES: If the searchbox is Active and the user taps/clicks and the event propogates back to the AppListView, the SearchBox clears and the searchbox becomes inactive, transitioning to PEEKING or FULLSCREEN_ALL_APPS. If the searchbox is inactive and the user taps/clicks and the event propogates back to the AppListView, the widget closes. Also in this CL: Mouse drag has been disabled. Tests updated to reflect change in behavior of the app list. BUG=735117 Review-Url: https://codereview.chromium.org/2952763002 Cr-Commit-Position: refs/heads/master@{#485749} Committed: https://chromium.googlesource.com/chromium/src/+/ef1c07cf957e1b3ca050dec3a003... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:930001) as https://chromium.googlesource.com/chromium/src/+/ef1c07cf957e1b3ca050dec3a003...
Message was sent while issue was closed.
|
