Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2237703004:
[MD History] Focus the search bar on load. (Closed)
|
Created:
4 years, 4 months ago by calamity Modified:
4 years, 4 months ago CC:
chromium-reviews, dbeam+watch-elements_chromium.org, Patrick Dubroy, michaelpg+watch-md-ui_chromium.org, dbeam+watch-history_chromium.org, pam+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, michaelpg+watch-elements_chromium.org, stevenjb+watch-md-settings_chromium.org, chrome-apps-syd-reviews_chromium.org, michaelpg Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[MD History] Focus the search bar on load.
This CL causes the search field on the MD History page to focus on load.
It also keeps the placeholder text visible while the search field is
empty.
BUG=633180
NOPRESUBMIT=true # crisper.js
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/70ee9e02331eeaced2a8a74c8abc6a5c6106d283
Cr-Commit-Position: refs/heads/master@{#413101}
Patch Set 1 #
Total comments: 4
Patch Set 2 : address comments #Patch Set 3 : remove focusing behavior in narrow mode #Patch Set 4 : rebase, vulcanize #Patch Set 5 : fix drawer test #
Total comments: 4
Patch Set 6 : address comments #
Total comments: 7
Patch Set 7 : rebase, use replaceApp #Patch Set 8 : rebase #
Dependent Patchsets: Messages
Total messages: 54 (35 generated)
Description was changed from ========== [MD History] Focus the search bar on load. This CL causes the search field on the MD History page to focus on load. It also keeps the placeholder text visible while the search field is empty. BUG=633180 ========== to ========== [MD History] Focus the search bar on load. This CL causes the search field on the MD History page to focus on load. It also keeps the placeholder text visible while the search field is empty. BUG=633180 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
calamity@chromium.org changed reviewers: + tsergeant@chromium.org
drive-by complaint: does this prevent scrolling via down arrow (or space) without unfocusing the search box first, as in old history?
On 2016/08/11 07:44:07, michaelpg wrote: > drive-by complaint: does this prevent scrolling via down arrow (or space) > without unfocusing the search box first, as in old history? Yes =( This doesn't work at present anyway. We may look into making the down arrow scroll the main view from the search box when the search is empty.
It's a little strange that the search icon is now fully white at page load, but I don't think there's anything we can do about that. https://codereview.chromium.org/2237703004/diff/1/chrome/test/data/webui/md_h... File chrome/test/data/webui/md_history/history_toolbar_test.js (left): https://codereview.chromium.org/2237703004/diff/1/chrome/test/data/webui/md_h... chrome/test/data/webui/md_history/history_toolbar_test.js:57: assertFalse(field.showingSearch); We probably shouldn't blindly delete this -- now we don't know whether the assertions below are because of the command working correctly, or because the field was already focused. Can you blur the field at the start of the test? https://codereview.chromium.org/2237703004/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.html (right): https://codereview.chromium.org/2237703004/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.html:131: <label id="prompt" for="searchInput" hidden$="[[hasSearchText_]]"> I don't think you want to use hidden here. paper-input-container will hide the label itself anyway, and it's important to keep the label around for screen readers to find.
The CQ bit was checked by calamity@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...
There's actually a fun little thing that happens when you load the page in narrow mode... I need to figure out how to disable the animations until firstblur. https://codereview.chromium.org/2237703004/diff/1/chrome/test/data/webui/md_h... File chrome/test/data/webui/md_history/history_toolbar_test.js (left): https://codereview.chromium.org/2237703004/diff/1/chrome/test/data/webui/md_h... chrome/test/data/webui/md_history/history_toolbar_test.js:57: assertFalse(field.showingSearch); On 2016/08/15 03:28:33, tsergeant wrote: > We probably shouldn't blindly delete this -- now we don't know whether the > assertions below are because of the command working correctly, or because the > field was already focused. > > Can you blur the field at the start of the test? Good point. Done. https://codereview.chromium.org/2237703004/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.html (right): https://codereview.chromium.org/2237703004/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.html:131: <label id="prompt" for="searchInput" hidden$="[[hasSearchText_]]"> On 2016/08/15 03:28:33, tsergeant wrote: > I don't think you want to use hidden here. paper-input-container will hide the > label itself anyway, and it's important to keep the label around for screen > readers to find. Removed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/15 07:40:28, calamity wrote: > There's actually a fun little thing that happens when you load the page in > narrow mode... > > I need to figure out how to disable the animations until firstblur. I'm not sure exactly what you were planning on changing, but I think the search field should not be focused on-load in narrow mode.
The CQ bit was checked by calamity@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...
Description was changed from ========== [MD History] Focus the search bar on load. This CL causes the search field on the MD History page to focus on load. It also keeps the placeholder text visible while the search field is empty. BUG=633180 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD History] Focus the search bar on load. This CL causes the search field on the MD History page to focus on load. It also keeps the placeholder text visible while the search field is empty. BUG=633180,637196 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
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 calamity@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_asan_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 calamity@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 2016/08/16 01:29:08, tsergeant wrote: > On 2016/08/15 07:40:28, calamity wrote: > > There's actually a fun little thing that happens when you load the page in > > narrow mode... > > > > I need to figure out how to disable the animations until firstblur. > > I'm not sure exactly what you were planning on changing, but I think the search > field should not be focused on-load in narrow mode. Done.
https://codereview.chromium.org/2237703004/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_history/history.js (right): https://codereview.chromium.org/2237703004/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/history.js:49: requestAnimationFrame(function() { Is it worth moving this logic into a method on <history-app>: `onShown` or `onLoaded` or something similar? That way we avoid things creeping back into this file, and can keep hasDrawer_ as private. https://codereview.chromium.org/2237703004/diff/80001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.html (right): https://codereview.chromium.org/2237703004/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.html:131: <label id="prompt" for="searchInput"> Nit: undo this change
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by calamity@chromium.org to run a CQ dry run
https://codereview.chromium.org/2237703004/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_history/history.js (right): https://codereview.chromium.org/2237703004/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/history.js:49: requestAnimationFrame(function() { On 2016/08/18 03:46:12, tsergeant wrote: > Is it worth moving this logic into a method on <history-app>: `onShown` or > `onLoaded` or something similar? > > That way we avoid things creeping back into this file, and can keep hasDrawer_ > as private. Done. https://codereview.chromium.org/2237703004/diff/80001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.html (right): https://codereview.chromium.org/2237703004/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.html:131: <label id="prompt" for="searchInput"> On 2016/08/18 03:46:12, tsergeant wrote: > Nit: undo this change Done.
lgtm % nits https://codereview.chromium.org/2237703004/diff/100001/chrome/browser/resourc... File chrome/browser/resources/md_history/list_container.js (right): https://codereview.chromium.org/2237703004/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_history/list_container.js:96: if (this.getSelectedList_()) you'll need to do a good old-fashioned rebase for this line https://codereview.chromium.org/2237703004/diff/100001/chrome/test/data/webui... File chrome/test/data/webui/md_history/history_toolbar_test.js (right): https://codereview.chromium.org/2237703004/diff/100001/chrome/test/data/webui... chrome/test/data/webui/md_history/history_toolbar_test.js:11: var TEST_HISTORY_RESULTS Nit: you left the semicolon.... https://codereview.chromium.org/2237703004/diff/100001/chrome/test/data/webui... chrome/test/data/webui/md_history/history_toolbar_test.js:114: ; ...down here https://codereview.chromium.org/2237703004/diff/100001/chrome/test/data/webui... chrome/test/data/webui/md_history/history_toolbar_test.js:117: PolymerTest.clearBody(); Nit: If you rebase past https://codereview.chromium.org/2257703002/, you can use the `replaceApp()` helper from that patch
calamity@chromium.org changed reviewers: + dbeam@chromium.org
+dbeam for cr_toolbar_search_field.html OWNERS. https://codereview.chromium.org/2237703004/diff/100001/chrome/browser/resourc... File chrome/browser/resources/md_history/list_container.js (right): https://codereview.chromium.org/2237703004/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_history/list_container.js:96: if (this.getSelectedList_()) On 2016/08/19 00:21:38, tsergeant wrote: > you'll need to do a good old-fashioned rebase for this line Done. https://codereview.chromium.org/2237703004/diff/100001/chrome/test/data/webui... File chrome/test/data/webui/md_history/history_toolbar_test.js (right): https://codereview.chromium.org/2237703004/diff/100001/chrome/test/data/webui... chrome/test/data/webui/md_history/history_toolbar_test.js:114: ; On 2016/08/19 00:21:38, tsergeant wrote: > ...down here Man, it's always in the last place you look. https://codereview.chromium.org/2237703004/diff/100001/chrome/test/data/webui... chrome/test/data/webui/md_history/history_toolbar_test.js:117: PolymerTest.clearBody(); On 2016/08/19 00:21:38, tsergeant wrote: > Nit: If you rebase past https://codereview.chromium.org/2257703002/, you can use > the `replaceApp()` helper from that patch Done.
The CQ bit was checked by calamity@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.
lgtm you guys should probably own the cr_toolbar element (or cr_elements), shouldn't you?
The CQ bit was checked by calamity@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsergeant@chromium.org Link to the patchset: https://codereview.chromium.org/2237703004/#ps120001 (title: "rebase, use replaceApp")
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
Failed to apply patch for chrome/browser/resources/md_history/app.crisper.js:
While running git apply --index -3 -p1;
error: patch failed: chrome/browser/resources/md_history/app.crisper.js:15146
Falling back to three-way merge...
Applied patch to 'chrome/browser/resources/md_history/app.crisper.js' with
conflicts.
U chrome/browser/resources/md_history/app.crisper.js
Patch: chrome/browser/resources/md_history/app.crisper.js
Index: chrome/browser/resources/md_history/app.crisper.js
diff --git a/chrome/browser/resources/md_history/app.crisper.js
b/chrome/browser/resources/md_history/app.crisper.js
index
b3e7083472441384129b9d2cc24b10a24fd21c5c..b2a5c910aca6c3dad1e60a533aea231d1bd2d4ad
100644
--- a/chrome/browser/resources/md_history/app.crisper.js
+++ b/chrome/browser/resources/md_history/app.crisper.js
@@ -15146,6 +15146,22 @@ Polymer({
}
},
+ onFirstRender: function() {
+ // requestAnimationFrame allows measurement immediately before the next
+ // repaint, but after the first page of <iron-list> items has stamped.
+ requestAnimationFrame(function() {
+ chrome.send(
+ 'metricsHandler:recordTime',
+ ['History.ResultsRenderedTime', window.performance.now()]);
+ });
+
+ // Focus the search field on load. Done here to ensure the history page
+ // is rendered before we try to take focus.
+ if (!this.hasDrawer_) {
+ this.focusToolbarSearchField();
+ }
+ },
+
/** @private */
onMenuTap_: function() {
var drawer = this.$$('#drawer');
@@ -15196,6 +15212,13 @@ Polymer({
},
/**
+ * Focuses the search bar in the toolbar.
+ */
+ focusToolbarSearchField: function() {
+ this.$.toolbar.showSearchField();
+ },
+
+ /**
* Fired when the user presses 'More from this site'.
* @param {{detail: {domain: string}}} e
*/
@@ -15243,7 +15266,7 @@ Polymer({
*/
onCommand_: function(e) {
if (e.command.id == 'find-command' || e.command.id == 'slash-command')
- this.$.toolbar.showSearchField();
+ this.focusToolbarSearchField();
if (e.command.id == 'delete-command')
this.deleteSelected();
},
The CQ bit was checked by calamity@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsergeant@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2237703004/#ps140001 (title: "rebase")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== [MD History] Focus the search bar on load. This CL causes the search field on the MD History page to focus on load. It also keeps the placeholder text visible while the search field is empty. BUG=633180,637196 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD History] Focus the search bar on load. This CL causes the search field on the MD History page to focus on load. It also keeps the placeholder text visible while the search field is empty. BUG=633180 NOPRESUBMIT=true # crisper.js CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by calamity@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [MD History] Focus the search bar on load. This CL causes the search field on the MD History page to focus on load. It also keeps the placeholder text visible while the search field is empty. BUG=633180 NOPRESUBMIT=true # crisper.js CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD History] Focus the search bar on load. This CL causes the search field on the MD History page to focus on load. It also keeps the placeholder text visible while the search field is empty. BUG=633180 NOPRESUBMIT=true # crisper.js CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [MD History] Focus the search bar on load. This CL causes the search field on the MD History page to focus on load. It also keeps the placeholder text visible while the search field is empty. BUG=633180 NOPRESUBMIT=true # crisper.js CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD History] Focus the search bar on load. This CL causes the search field on the MD History page to focus on load. It also keeps the placeholder text visible while the search field is empty. BUG=633180 NOPRESUBMIT=true # crisper.js CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/70ee9e02331eeaced2a8a74c8abc6a5c6106d283 Cr-Commit-Position: refs/heads/master@{#413101} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/70ee9e02331eeaced2a8a74c8abc6a5c6106d283 Cr-Commit-Position: refs/heads/master@{#413101} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
