Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1963503002:
MD WebUI: Add shared cr-toolbar element (Closed)
|
Created:
4 years, 7 months ago by tsergeant Modified:
4 years, 7 months ago CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-history_chromium.org, dbeam+watch-elements_chromium.org, Patrick Dubroy, michaelpg+watch-md-ui_chromium.org, michaelpg+watch-elements_chromium.org, oshima+watch_chromium.org, pam+watch_chromium.org, stevenjb+watch-md-settings_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD WebUI: Add shared cr-toolbar element
The toolbar currently contains the page name and a search field, which
reuses the implementation of the existing <cr-search-field> element. The
toolbar has some responsive behavior and basic animations. This is
sufficient to allow us to replace some of the internals of
<history-toolbar> with a <cr-toolbar>.
The two main features which still need to be implemented are a menu
button in the thin layout, and a spinner when a search is in progress.
BUG=610609
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/66d8b689e6e5e4a8f2b74430e0680e459021c5b8
Cr-Commit-Position: refs/heads/master@{#396115}
Patch Set 1 : Really really this time #Patch Set 2 : #
Total comments: 16
Patch Set 3 : Review comments #Patch Set 4 : Minor tweaks #
Total comments: 16
Patch Set 5 : Review comments #
Total comments: 19
Patch Set 6 : Review comments #Patch Set 7 : Experimental: cr-toolbar-search-field #Patch Set 8 : Remove dom-if #
Total comments: 8
Patch Set 9 : Review comments and input label #Patch Set 10 : Add todo and fix nits #
Total comments: 20
Patch Set 11 : Style review comments #Messages
Total messages: 48 (21 generated)
Description was changed from ========== MD WebUI: Add shared cr-toolbar element Very experimental, do not commit BUG= ========== to ========== MD WebUI: Add shared cr-toolbar element Very experimental, do not commit BUG= CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD WebUI: Add shared cr-toolbar element Very experimental, do not commit BUG= CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD WebUI: Add shared cr-toolbar element The toolbar currently contains the page name and a search field, which reuses the implementation of the existing <cr-search-field> element. The toolbar has some responsive behavior and basic animations. This is sufficient to allow us to replace some of the internals of <history-toolbar> with a <cr-toolbar>. The two main features which still need to be implemented are a menu button in the thin layout, and a spinner when a search is in progress. BUG=610609 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
tsergeant@chromium.org changed reviewers: + calamity@chromium.org
aaaalright, +calamity@ for initial review
Here's a first round. https://codereview.chromium.org/1963503002/diff/140001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js (right): https://codereview.chromium.org/1963503002/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js:16: * @polymerBehavior Can we get a comment about what this Behavior is supposed to represent? https://codereview.chromium.org/1963503002/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js:102: this.hasSearchText_ = this.getValue() != ''; You may want to put this above the delegate call in case the delegate relies this property for some reason. https://codereview.chromium.org/1963503002/diff/140001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html (right): https://codereview.chromium.org/1963503002/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html:28: --paper-icon-button: { QQ: Why is this applied as a mixin? So that will override correctly? https://codereview.chromium.org/1963503002/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html:69: display: none; Some animation jank can happen here from text wrapping. overflow: hidden; white-space: nowrap; https://codereview.chromium.org/1963503002/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html:128: flex-grow: 1; As discussed, this is causing animation jank because it changes width immediately on property change. https://codereview.chromium.org/1963503002/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html:153: <span id="search-prompt">Search [[lowerPageName_]]</span> It would seem that in i18n, the rule is "Do it dumb". Time to make a generic placeholder property that each client fills in! =D https://codereview.chromium.org/1963503002/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html:167: <div id="right-padding"></div> I think we can get rid of this right-padding div and just have centered-content flex: 1 1 0, and left-content have constant width. https://codereview.chromium.org/1963503002/diff/140001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js (right): https://codereview.chromium.org/1963503002/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js:28: onDocumentClick_: function(e) { Hmmmmm. This is cool but makes me a bit nervous. document.activeElement gives the 'element keyboard input will go to'. I don't think will work nicely with input in other pages (e.g clicking a textfield in settings _should_ defocus the search bar). Even in the history page, clicking the side menu doesn't defocus the search box. Can we do onBlur unless hasSearchText_?
Patchset #3 (id:160001) has been deleted
https://codereview.chromium.org/1963503002/diff/140001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js (right): https://codereview.chromium.org/1963503002/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js:16: * @polymerBehavior On 2016/05/16 04:27:34, calamity wrote: > Can we get a comment about what this Behavior is supposed to represent? Done. https://codereview.chromium.org/1963503002/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js:102: this.hasSearchText_ = this.getValue() != ''; On 2016/05/16 04:27:34, calamity wrote: > You may want to put this above the delegate call in case the delegate relies > this property for some reason. Done. https://codereview.chromium.org/1963503002/diff/140001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html (right): https://codereview.chromium.org/1963503002/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html:28: --paper-icon-button: { On 2016/05/16 04:27:34, calamity wrote: > QQ: Why is this applied as a mixin? So that will override correctly? That's a cargo-cult from PDF which is...not necessary after all. Done. https://codereview.chromium.org/1963503002/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html:69: display: none; On 2016/05/16 04:27:34, calamity wrote: > Some animation jank can happen here from text wrapping. > > overflow: hidden; > white-space: nowrap; Done. https://codereview.chromium.org/1963503002/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html:128: flex-grow: 1; On 2016/05/16 04:27:34, calamity wrote: > As discussed, this is causing animation jank because it changes width > immediately on property change. Done. https://codereview.chromium.org/1963503002/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html:153: <span id="search-prompt">Search [[lowerPageName_]]</span> On 2016/05/16 04:27:34, calamity wrote: > It would seem that in i18n, the rule is "Do it dumb". > > Time to make a generic placeholder property that each client fills in! =D Done, and I've added back in clearLabel as well. https://codereview.chromium.org/1963503002/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html:167: <div id="right-padding"></div> On 2016/05/16 04:27:34, calamity wrote: > I think we can get rid of this right-padding div and just have centered-content > flex: 1 1 0, and left-content have constant width. Done. https://codereview.chromium.org/1963503002/diff/140001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js (right): https://codereview.chromium.org/1963503002/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js:28: onDocumentClick_: function(e) { On 2016/05/16 04:27:34, calamity wrote: > Hmmmmm. This is cool but makes me a bit nervous. document.activeElement gives > the 'element keyboard input will go to'. > > I don't think will work nicely with input in other pages (e.g clicking a > textfield in settings _should_ defocus the search bar). Even in the history > page, clicking the side menu doesn't defocus the search box. > > Can we do onBlur unless hasSearchText_? Hmmm, sure. onBlur is way less annoying when you also check hasSearchText. Let's go with that and see how it is received.
lgtm
tsergeant@chromium.org changed reviewers: + dbeam@chromium.org
+dbeam@ for ui/webui owners, as the original author of the search field. CCing dschuyler@ as the person who seems responsible for Search in Settings. We're hoping to get this to a place where it can be shared between several webui pages, so any input is appreciated. (There are mostly-up-to-date screenshots on the bug)
Description was changed from ========== MD WebUI: Add shared cr-toolbar element The toolbar currently contains the page name and a search field, which reuses the implementation of the existing <cr-search-field> element. The toolbar has some responsive behavior and basic animations. This is sufficient to allow us to replace some of the internals of <history-toolbar> with a <cr-toolbar>. The two main features which still need to be implemented are a menu button in the thin layout, and a spinner when a search is in progress. BUG=610609 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD WebUI: Add shared cr-toolbar element The toolbar currently contains the page name and a search field, which reuses the implementation of the existing <cr-search-field> element. The toolbar has some responsive behavior and basic animations. This is sufficient to allow us to replace some of the internals of <history-toolbar> with a <cr-toolbar>. The two main features which still need to be implemented are a menu button in the thin layout, and a spinner when a search is in progress. BUG=610609 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
On 2016/05/18 03:42:14, tsergeant wrote: > +dbeam@ for ui/webui owners, as the original author of the search field. > > CCing dschuyler@ as the person who seems responsible for Search in Settings. > We're hoping to get this to a place where it can be shared between several webui > pages, so any input is appreciated. > > (There are mostly-up-to-date screenshots on the bug) +dpapad@ because it's search related.
dpapad@chromium.org changed reviewers: + dpapad@chromium.org
https://codereview.chromium.org/1963503002/diff/200001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_toolbar.js (right): https://codereview.chromium.org/1963503002/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_history/history_toolbar.js:90: var toolbar = /** @type {CrToolbarElement} */(this.$['main-toolbar']); Is toolbar expected to be null after this line? If not this should be @type {!CrToolbarElement} Also nit: How about changing the ID name to mainToolbar, so that you can reference it easier from JS, like this.$.mainToolbar https://codereview.chromium.org/1963503002/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_history/history_toolbar.js:108: /** @type {CrToolbarElement} */(this.$['main-toolbar']) Nit: !CrToolbarElement https://codereview.chromium.org/1963503002/diff/200001/chrome/test/data/webui... File chrome/test/data/webui/md_history/history_toolbar_test.js (right): https://codereview.chromium.org/1963503002/diff/200001/chrome/test/data/webui... chrome/test/data/webui/md_history/history_toolbar_test.js:55: registerMessageCallback('queryHistory', this, function (info) { FYI, we have established the "browser proxy" pattern in the MD settings code, that makes testing much easier (using a "test proxy") than using the "hacky" registerMessageCallback method. I would gladly send you some examples and help you convert md_history to use that pattern if you are interested. https://codereview.chromium.org/1963503002/diff/200001/chrome/test/data/webui... chrome/test/data/webui/md_history/history_toolbar_test.js:57: flush(function() { Unrelated to this CL, how about making flush() return a Promise, instead of expecting a callback and using window.setTimeout(). Promises also yield to the message loop and are more flexible API-wise flush().then(function() { assertEquals('example.com', toolbar.$['main-toolbar'].getValue()); done(); }); https://codereview.chromium.org/1963503002/diff/200001/chrome/test/data/webui... chrome/test/data/webui/md_history/history_toolbar_test.js:64: element.onMoreFromSiteTap_(); Simulating a tap action seems much better than calling a private method. Is there any good reason for this change? https://codereview.chromium.org/1963503002/diff/200001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html (right): https://codereview.chromium.org/1963503002/diff/200001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html:133: <div id="search-field" on-click="showSearch_"> Should we be using on-tap instead of on-click, which works both for mouse and touch devices? (Here and elsewhere in this file). https://codereview.chromium.org/1963503002/diff/200001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js (right): https://codereview.chromium.org/1963503002/diff/200001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js:25: if (e.target != this.$['clear-search']) Nit (optional): Change id to clearSearch, such that it can be used without quotes, this.$.clearSearch https://codereview.chromium.org/1963503002/diff/200001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js:29: computeLowerPageName_: function(name) { Type annotations for parameter and return value missing.
https://codereview.chromium.org/1963503002/diff/200001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_toolbar.js (right): https://codereview.chromium.org/1963503002/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_history/history_toolbar.js:90: var toolbar = /** @type {CrToolbarElement} */(this.$['main-toolbar']); On 2016/05/18 23:39:13, dpapad wrote: > Is toolbar expected to be null after this line? If not this should be > @type {!CrToolbarElement} > > Also nit: How about changing the ID name to mainToolbar, so that you can > reference it easier from JS, like > this.$.mainToolbar Closure comment: Done. Ack on the comment about renaming the ID. We're still using kebab-case everywhere else in history and haven't decided whether to switch to camelCase yet, so I'd rather just stay consistent for now. https://codereview.chromium.org/1963503002/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_history/history_toolbar.js:108: /** @type {CrToolbarElement} */(this.$['main-toolbar']) On 2016/05/18 23:39:13, dpapad wrote: > Nit: !CrToolbarElement Done. https://codereview.chromium.org/1963503002/diff/200001/chrome/test/data/webui... File chrome/test/data/webui/md_history/history_toolbar_test.js (right): https://codereview.chromium.org/1963503002/diff/200001/chrome/test/data/webui... chrome/test/data/webui/md_history/history_toolbar_test.js:55: registerMessageCallback('queryHistory', this, function (info) { On 2016/05/18 23:39:13, dpapad wrote: > FYI, we have established the "browser proxy" pattern in the MD settings code, > that makes testing much easier (using a "test proxy") than using the "hacky" > registerMessageCallback method. I would gladly send you some examples and help > you convert md_history to use that pattern if you are interested. Cool, I'd be interested to know more. https://codereview.chromium.org/1963503002/diff/200001/chrome/test/data/webui... chrome/test/data/webui/md_history/history_toolbar_test.js:57: flush(function() { On 2016/05/18 23:39:13, dpapad wrote: > Unrelated to this CL, how about making flush() return a Promise, instead of > expecting a callback and using window.setTimeout(). Promises also yield to the > message loop and are more flexible API-wise > > flush().then(function() { > assertEquals('example.com', toolbar.$['main-toolbar'].getValue()); > done(); > }); calamity@ and I talked about this yesterday, actually. I'll do this in a follow-up change. https://codereview.chromium.org/1963503002/diff/200001/chrome/test/data/webui... chrome/test/data/webui/md_history/history_toolbar_test.js:64: element.onMoreFromSiteTap_(); On 2016/05/18 23:39:13, dpapad wrote: > Simulating a tap action seems much better than calling a private method. Is > there any good reason for this change? In a previous version of this patch, simulating a click event would cause the toolbar to close and muck up the test. The toolbar closing behaviour has changed since then, so it's no longer a problem, and I've changed this code back. https://codereview.chromium.org/1963503002/diff/200001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html (right): https://codereview.chromium.org/1963503002/diff/200001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html:133: <div id="search-field" on-click="showSearch_"> On 2016/05/18 23:39:13, dpapad wrote: > Should we be using on-tap instead of on-click, which works both for mouse and > touch devices? (Here and elsewhere in this file). Done. https://codereview.chromium.org/1963503002/diff/200001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js (right): https://codereview.chromium.org/1963503002/diff/200001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js:25: if (e.target != this.$['clear-search']) On 2016/05/18 23:39:13, dpapad wrote: > Nit (optional): Change id to clearSearch, such that it can be used without > quotes, this.$.clearSearch I was sticking with the style used in cr-search-field, but now seems like a good time to convert it over. I've converted all the IDs in this element, as well as in cr-search-field so that the behavior can be consistent between the two. https://codereview.chromium.org/1963503002/diff/200001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js:29: computeLowerPageName_: function(name) { On 2016/05/18 23:39:14, dpapad wrote: > Type annotations for parameter and return value missing. oops, that function is dead code. So, uh Done.
Design question: Why is the cr-toolbar "implement" "cr-search-field" behavior (inheritance model), instead of simply using a canonical <cr-search-field> element (composition model)? If composition is used, then there is no need for the behavior at all, and it seems more intuitive that a "cr-toolbar" is composed of a "cr-search-field" instead of being one. https://codereview.chromium.org/1963503002/diff/220001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_search_field/cr_search_field.html (right): https://codereview.chromium.org/1963503002/diff/220001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_search_field/cr_search_field.html:14: <template is="dom-if" if="[[showingSearch_]]" id="searchContainer"> Is this dom-if really useful? Does it make any noticeable difference in performance? Why not just <paper-input-container hidden="[[!showingSearch_]]"...></paper-input-container> The fact the the text field is within a dom-if forces the UI to repeatedly have to check whether "searchInput" actually exists in the DOM (happens in getValue(), setValue(), focus_()). This can be avoided if there is no good reason. https://codereview.chromium.org/1963503002/diff/220001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_search_field/cr_search_field.html:16: on-keydown="onSearchTermKeydown_" hidden$="[[!showingSearch_]]" Everything within the dom-if is hidden when !showingSearch_. Why is this "hidden$=" necessary (seems redundant)? Same question for the paper-icon-button below. https://codereview.chromium.org/1963503002/diff/220001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js (right): https://codereview.chromium.org/1963503002/diff/220001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js:32: showingSearch_: { @private https://codereview.chromium.org/1963503002/diff/220001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js:39: hasSearchText_: { @private https://codereview.chromium.org/1963503002/diff/220001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js:46: * @return {string} Nit: /** * @return {string} The value of the search field. */ https://codereview.chromium.org/1963503002/diff/220001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js:69: /** @return {Promise<boolean>} */ !Promise<boolean> https://codereview.chromium.org/1963503002/diff/220001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js:76: * @return {Promise<boolean>} !Promise<boolean> https://codereview.chromium.org/1963503002/diff/220001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js:121: if (!searchInput) Can you explain why/when would getSearchInput() return null? Is this a valid case? https://codereview.chromium.org/1963503002/diff/220001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js (right): https://codereview.chromium.org/1963503002/diff/220001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js:19: onInputBlur_: function() { @private
On 2016/05/19 23:19:47, dpapad wrote: > Design question: > Why is the cr-toolbar "implement" "cr-search-field" behavior (inheritance > model), instead of simply using a canonical <cr-search-field> element > (composition model)? > > If composition is used, then there is no need for the behavior at all, and it > seems more intuitive that a "cr-toolbar" is composed of a "cr-search-field" > instead of being one. This was done for a few reasons. The main reason is that because the appearance and behaviour of the new search-field is quite different to the old one, I'm not convinced it would be possible to cleanly reuse the existing cr-search-field implementation (it's mainly CSS that's the issue, although there are a few places where the JS behaviour changes). Secondly, I was being optimistic that eventually all the uses of cr-search-field would be replaced with cr-toolbar. In this case, the behavior can be folded back into cr-toolbar if/when cr-search-field is deleted. Obviously, this is very optimistic considering there are probably uses for a search field outside of a toolbar. Finally, in my initial implementation of this CL the search-field was much more tied into the behavior of the rest of the toolbar, so it didn't really make sense to separate them. The CL has been simplified a lot since then, so this point doesn't really apply anymore. WDYT of this? I'll spend some more time playing with it today and see what I can come up with.
https://codereview.chromium.org/1963503002/diff/220001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_search_field/cr_search_field.html (right): https://codereview.chromium.org/1963503002/diff/220001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_search_field/cr_search_field.html:14: <template is="dom-if" if="[[showingSearch_]]" id="searchContainer"> On 2016/05/19 23:19:46, dpapad wrote: > Is this dom-if really useful? Does it make any noticeable difference in > performance? > Why not just > <paper-input-container hidden="[[!showingSearch_]]"...></paper-input-container> > > The fact the the text field is within a dom-if forces the UI to repeatedly have > to check whether "searchInput" actually exists in the DOM (happens in > getValue(), setValue(), focus_()). This can be avoided if there is no good > reason. I agree, but if possible I'd like to defer this to a follow-up patch (this patch is getting pretty big already). https://codereview.chromium.org/1963503002/diff/220001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_search_field/cr_search_field.html:16: on-keydown="onSearchTermKeydown_" hidden$="[[!showingSearch_]]" On 2016/05/19 23:19:46, dpapad wrote: > Everything within the dom-if is hidden when !showingSearch_. Why is this > "hidden$=" necessary (seems redundant)? Same question for the paper-icon-button > below. As above, I'd like to defer this for now. https://codereview.chromium.org/1963503002/diff/220001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js (right): https://codereview.chromium.org/1963503002/diff/220001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js:32: showingSearch_: { On 2016/05/19 23:19:47, dpapad wrote: > @private Done. https://codereview.chromium.org/1963503002/diff/220001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js:39: hasSearchText_: { On 2016/05/19 23:19:47, dpapad wrote: > @private Done. https://codereview.chromium.org/1963503002/diff/220001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js:46: * @return {string} On 2016/05/19 23:19:47, dpapad wrote: > Nit: > > /** > * @return {string} The value of the search field. > */ Done. https://codereview.chromium.org/1963503002/diff/220001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js:69: /** @return {Promise<boolean>} */ On 2016/05/19 23:19:46, dpapad wrote: > !Promise<boolean> Done. https://codereview.chromium.org/1963503002/diff/220001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js:76: * @return {Promise<boolean>} On 2016/05/19 23:19:46, dpapad wrote: > !Promise<boolean> Done. https://codereview.chromium.org/1963503002/diff/220001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js:121: if (!searchInput) On 2016/05/19 23:19:46, dpapad wrote: > Can you explain why/when would getSearchInput() return null? Is this a valid > case? As above, this is due to the dom-if. https://codereview.chromium.org/1963503002/diff/220001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js (right): https://codereview.chromium.org/1963503002/diff/220001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js:19: onInputBlur_: function() { On 2016/05/19 23:19:47, dpapad wrote: > @private Done.
On 2016/05/20 at 00:26:57, tsergeant wrote: > On 2016/05/19 23:19:47, dpapad wrote: > > Design question: > > Why is the cr-toolbar "implement" "cr-search-field" behavior (inheritance > > model), instead of simply using a canonical <cr-search-field> element > > (composition model)? > > > > If composition is used, then there is no need for the behavior at all, and it > > seems more intuitive that a "cr-toolbar" is composed of a "cr-search-field" > > instead of being one. > > This was done for a few reasons. The main reason is that because the appearance and behaviour of the new search-field is quite different to the old one, I'm not convinced it would be possible to cleanly reuse the existing cr-search-field implementation (it's mainly CSS that's the issue, although there are a few places where the JS behaviour changes). > > Secondly, I was being optimistic that eventually all the uses of cr-search-field would be replaced with cr-toolbar. In this case, the behavior can be folded back into cr-toolbar if/when cr-search-field is deleted. Obviously, this is very optimistic considering there are probably uses for a search field outside of a toolbar. > > Finally, in my initial implementation of this CL the search-field was much more tied into the behavior of the rest of the toolbar, so it didn't really make sense to separate them. The CL has been simplified a lot since then, so this point doesn't really apply anymore. > > WDYT of this? I'll spend some more time playing with it today and see what I can come up with. Thanks for providing context for the design decision. Regarding being able to customize the styling, I think using CSS mixins might be the way to go. This is how various Polymer (paper-*) elements are customized from the outside. Regarding your optimistic view of always using search functionality via a toolbar, even if this happens, I think it is still worth to keep those separate and use composition. A toolbar might need extra logic for other things (for example a back button, other control buttons, title labels), which would require mixing up unrelated functionality (search behavior + extra stuff). The search box UI and its accompanying logic seems like a good coherent unit with well defined responsibilities, perfect for packaging into a re-usable component. It is good to know that this CL has been simplified since you started, so I am eager to see what you come up with.
https://codereview.chromium.org/1963503002/diff/220001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_search_field/cr_search_field.html (right): https://codereview.chromium.org/1963503002/diff/220001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_search_field/cr_search_field.html:14: <template is="dom-if" if="[[showingSearch_]]" id="searchContainer"> On 2016/05/20 at 00:27:12, tsergeant wrote: > On 2016/05/19 23:19:46, dpapad wrote: > > Is this dom-if really useful? Does it make any noticeable difference in > > performance? > > Why not just > > <paper-input-container hidden="[[!showingSearch_]]"...></paper-input-container> > > > > The fact the the text field is within a dom-if forces the UI to repeatedly have > > to check whether "searchInput" actually exists in the DOM (happens in > > getValue(), setValue(), focus_()). This can be avoided if there is no good > > reason. > > I agree, but if possible I'd like to defer this to a follow-up patch (this patch is getting pretty big already). Follow up CL SG. Perhaps add a TODO for this?
I've looked into this over the last couple of days, and my conclusion is that implementing cr-toolbar by composing a cr-search-field isn't the best way to go. There's a (very WIP) CL which starts this here: https://codereview.chromium.org/2004263002 Implementing search in this way ends up spreading the code for the search field out in a weird way. All of the HTML and most of the JS for the search field is in cr-search-field, but then the CSS is spread out between cr-search-field and cr-toolbar in order to get the correct appearance in both cases. The result is that the two elements are very tightly coupled at the CSS level, which is spooky and hard to test. On top of that, the new search field has some intricate behavior which is hard to get correct (for example, color changes when the toolbar is in narrow mode, and changes to padding in certain cases) in this model just using CSS mixins. Getting all of these things correct is just going to keep tying the elements together. As a middle-ground between the two solutions, how about splitting a new element, cr-toolbar-search-field out of cr-toolbar. cr-toolbar-search-field splits out all of the search-related code from cr-toolbar (so the toolbar is just a toolbar), and makes it possible to use the special new search-field by itself.
On 2016/05/24 at 00:39:49, tsergeant wrote: > I've looked into this over the last couple of days, and my conclusion is that implementing cr-toolbar by composing a cr-search-field isn't the best way to go. There's a (very WIP) CL which starts this here: https://codereview.chromium.org/2004263002 > > Implementing search in this way ends up spreading the code for the search field out in a weird way. All of the HTML and most of the JS for the search field is in cr-search-field, but then the CSS is spread out between cr-search-field and cr-toolbar in order to get the correct appearance in both cases. The result is that the two elements are very tightly coupled at the CSS level, which is spooky and hard to test. > > On top of that, the new search field has some intricate behavior which is hard to get correct (for example, color changes when the toolbar is in narrow mode, and changes to padding in certain cases) in this model just using CSS mixins. Getting all of these things correct is just going to keep tying the elements together. > > As a middle-ground between the two solutions, how about splitting a new element, cr-toolbar-search-field out of cr-toolbar. cr-toolbar-search-field splits out all of the search-related code from cr-toolbar (so the toolbar is just a toolbar), and makes it possible to use the special new search-field by itself. Thanks for investigating. A few questions/comments on the findings and proposed hybrid approach below. Having all the search field related HTML and JS encapsulated in cr-search-field sounds like a win, so the issue seems to be the CSS that is spread between cr-search-field and cr-toolbar, in the form of CSS mixins. How big is that CSS code, how many mixins? As a reference point paper-checkbox accepts 10 different mixins/properties (did not find any paper-element that accepts more). 1) Can we reduce the size of the CSS that is passed from the cr-toolbar to the search field by making as few things customizable as necessary (and hardcode everything else within cr-search-field CSS)? 2) Instead of using only CSS mixins, would it help if the cr-search-field responded to a few public attributes (just like "hidden"). For example when the toolbar enters "narrow" mode, the "narrow" attribute could be applied to the search field, and it would be responsible of updating its style. 3) It is not clear to me in which way the proposed hybrid solution differs from the composition approach? Why is not CSS a problem for the hybrid solution? Basically what is the difference between cr-search-field and cr-toolbar-search-field?
Patchset #7 (id:260001) has been deleted
On 2016/05/24 01:22:18, dpapad wrote: > Having all the search field related HTML and JS encapsulated in cr-search-field > sounds like a win, so the issue seems to be the CSS that is spread between > cr-search-field and cr-toolbar, in the form of CSS mixins. > How big is that CSS code, how many mixins? As a reference point > paper-checkbox accepts 10 different mixins/properties (did not find any > paper-element > that accepts more). In terms of sheer volume, probably not all that large. There are about 5 elements in cr-search-field that need to be customizable, with a few extra permutations from different modes (narrow vs wide, search open vs closed), plus a few variables. The code also needs to touch 4 or 5 --paper mixins. The complexity of getting this to work together is much higher than just implementing some --paper-checkbox mixins, though. > 1) Can we reduce the size of the CSS that is passed from the cr-toolbar to the > search field by making as few things customizable as necessary (and hardcode > everything else within cr-search-field CSS)? The problem with changing how anything is hardcoded in cr-search-field.css is that it already has hardcoded styling for the original search field which we don't want to break. > 2) Instead of using only CSS mixins, would it help if the cr-search-field > responded to a few public attributes (just like "hidden"). For example when > the > toolbar enters "narrow" mode, the "narrow" attribute could be applied to the > search field, and it would be responsible of updating its style. I'm not sure if this would help. Our styles for a narrow search field (which are mainly just small tweaks) don't make any sense without the rest of the toolbar search field styling. An extension of that approach might be to have a `toolbar` property which brings in the rest of those styles. At this stage, cr-search-field would be bimodal, with two completely different appearances controlled by attributes. > 3) It is not clear to me in which way the proposed hybrid solution differs from > the composition approach? Why is not CSS a problem for the hybrid solution? > Basically > what is the difference between cr-search-field and cr-toolbar-search-field? cr-toolbar-search-field would implement its own HTML and CSS from scratch while reusing the JS Behavior. It's basically the relevant code from cr-toolbar in this CL pulled out into its own element. I've uploaded an implementation of this onto this CL. I don't see having two separate HTML/CSS implementations as a downside -- it reflects that there are two different types of search fields with different purposes and different styling. The two implementations are free to diverge as necessary without worrying about affecting each other.
On 2016/05/24 at 04:52:57, tsergeant wrote: > On 2016/05/24 01:22:18, dpapad wrote: > > Having all the search field related HTML and JS encapsulated in cr-search-field > > sounds like a win, so the issue seems to be the CSS that is spread between > > cr-search-field and cr-toolbar, in the form of CSS mixins. > > How big is that CSS code, how many mixins? As a reference point > > paper-checkbox accepts 10 different mixins/properties (did not find any > > paper-element > > that accepts more). > > In terms of sheer volume, probably not all that large. There are about 5 elements in cr-search-field that need to be customizable, with a few extra permutations from different modes (narrow vs wide, search open vs closed), plus a few variables. The code also needs to touch 4 or 5 --paper mixins. The complexity of getting this to work together is much higher than just implementing some --paper-checkbox mixins, though. > > > 1) Can we reduce the size of the CSS that is passed from the cr-toolbar to the > > search field by making as few things customizable as necessary (and hardcode > > everything else within cr-search-field CSS)? > > The problem with changing how anything is hardcoded in cr-search-field.css is that it already has hardcoded styling for the original search field which we don't want to break. > > > 2) Instead of using only CSS mixins, would it help if the cr-search-field > > responded to a few public attributes (just like "hidden"). For example when > > the > > toolbar enters "narrow" mode, the "narrow" attribute could be applied to the > > search field, and it would be responsible of updating its style. > > I'm not sure if this would help. Our styles for a narrow search field (which are mainly just small tweaks) don't make any sense without the rest of the toolbar search field styling. An extension of that approach might be to have a `toolbar` property which brings in the rest of those styles. At this stage, cr-search-field would be bimodal, with two completely different appearances controlled by attributes. > > > 3) It is not clear to me in which way the proposed hybrid solution differs from > > the composition approach? Why is not CSS a problem for the hybrid solution? > > Basically > > what is the difference between cr-search-field and cr-toolbar-search-field? > > cr-toolbar-search-field would implement its own HTML and CSS from scratch while reusing the JS Behavior. It's basically the relevant code from cr-toolbar in this CL pulled out into its own element. I've uploaded an implementation of this onto this CL. > > I don't see having two separate HTML/CSS implementations as a downside -- it reflects that there are two different types of search fields with different purposes and different styling. The two implementations are free to diverge as necessary without worrying about affecting each other. Thanks for the answers. After reading, I understand how having cr-search-field and cr-toolbar-search-field as separate elements makes things easier and avoids having a bi-modal cr-search-field element. Having said that, I still think the fact that cr-search-field is using dom-if is unnecessary and complicates the implementation of the cr_search_field_behavior.js, which is now shared by cr-toolbar-search-field, which in turn does not use dom-if. As discussed this can be fixed on a separate CL. Preferably I would remove the dom-if and simplify the behavior before introducing cr-toolbar-search-field, but up to you to decide on the order that works best for you.
Cool! I tend to agree with you that it makes sense to remove the dom-if now since it's affecting how both elements are implemented. I've uploaded a new patchset with that implemented, PTAL.
Do you plan to add some tests in a follow up CL? https://codereview.chromium.org/1963503002/diff/300001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js (right): https://codereview.chromium.org/1963503002/diff/300001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js:92: getSearchInput_: function() { Does this wrapper method add any value anymore? If so, then at least let's fix the @return type which should no longer be null. @return {!Element} https://codereview.chromium.org/1963503002/diff/300001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js:123: } Nit: No need to remove the comma at the end. https://codereview.chromium.org/1963503002/diff/300001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.html (right): https://codereview.chromium.org/1963503002/diff/300001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.html:86: <span id="prompt">[[label]]</span> What is this used for? Can't we use the native "placeholder" property instead, just like cr-search-field does (https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources...) https://codereview.chromium.org/1963503002/diff/300001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.js (right): https://codereview.chromium.org/1963503002/diff/300001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.js:29: if (!this.hasSearchText_) The underscore at the end of hasSearchText_ implies that it is a private member variable, so either it should not be accessed here, or should be made public in the behavior.
Patchset #9 (id:320001) has been deleted
I'll add some tests for this -- if you're happy for them to be in a followup then I'll do that (there's already basic coverage in the MD History tests, though). https://codereview.chromium.org/1963503002/diff/300001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js (right): https://codereview.chromium.org/1963503002/diff/300001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js:92: getSearchInput_: function() { On 2016/05/25 01:20:10, dpapad wrote: > Does this wrapper method add any value anymore? If so, then at least let's fix > the @return type which should no longer be null. > @return {!Element} I was on the fence about keeping the wrapper, but I don't think it helps. Done. https://codereview.chromium.org/1963503002/diff/300001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js:123: } On 2016/05/25 01:20:10, dpapad wrote: > Nit: No need to remove the comma at the end. Done. https://codereview.chromium.org/1963503002/diff/300001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.html (right): https://codereview.chromium.org/1963503002/diff/300001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.html:86: <span id="prompt">[[label]]</span> On 2016/05/25 01:20:10, dpapad wrote: > What is this used for? Can't we use the native "placeholder" property instead, > just like cr-search-field does > (https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources...) The original reason I avoided placeholder was because the text behavior is roughly the opposite of what happens in the old search field. After looking into it more, I've switched it to use a <label> inside the <paper-input-container>. This is closer to the normal Polymer way of doing things, and should be better for a11y. https://codereview.chromium.org/1963503002/diff/300001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.js (right): https://codereview.chromium.org/1963503002/diff/300001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.js:29: if (!this.hasSearchText_) On 2016/05/25 01:20:10, dpapad wrote: > The underscore at the end of hasSearchText_ implies that it is a private member > variable, so either it should not be accessed here, or should be made public in > the behavior. Done, made public.
lgtm LGTM with mostly style comments. Regarding tests, since this is a re-usable component, it would be nice to test it on its own, instead of relying of client code tests (e.g md_history), separate CL is fine. https://codereview.chromium.org/1963503002/diff/360001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js (right): https://codereview.chromium.org/1963503002/diff/360001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js:40: hasSearchText: { Nit: I think this is equivalent to the shorthand hasSearchText: Boolean, https://codereview.chromium.org/1963503002/diff/360001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js:46: * @return {!string} The value of the search field. "!" is implied for primitive types (number, boolean, string) and therefore not needed, here and elsewhere. https://codereview.chromium.org/1963503002/diff/360001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html (right): https://codereview.chromium.org/1963503002/diff/360001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html:49: showing-search="{{showingSearch_}}"> Indentation is a bit unusual here, how about <cr-toolbar-search-field id="search" narrow="[[narrow_]]" label="[[searchPrompt]]" clear-label="[[clearLabel]]" showing-search="{{showingSearch_}}"> </cr-toolbar-search-field> https://codereview.chromium.org/1963503002/diff/360001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js (right): https://codereview.chromium.org/1963503002/diff/360001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js:9: narrow_: { /** @private */ https://codereview.chromium.org/1963503002/diff/360001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js:13: showingSearch_: { Blank line between polymer properties. Also add @private here too. https://codereview.chromium.org/1963503002/diff/360001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js:25: /** @return {CrToolbarSearchFieldElement} */ !CrToolbarSearchFieldElement https://codereview.chromium.org/1963503002/diff/360001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.html (right): https://codereview.chromium.org/1963503002/diff/360001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.html:20: [hidden] { Can you verify whether this rule is needed? It has always worked for me without the need for explicitly specifying it. Maybe it is inherited from https://code.google.com/p/chromium/codesearch#search/&q=%22html%20/deep/%20%5... or somewhere else? https://codereview.chromium.org/1963503002/diff/360001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.html:82: margin-left: 18px; I don't think this works for RTL. How about --webkit-margin-start https://codereview.chromium.org/1963503002/diff/360001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.js (right): https://codereview.chromium.org/1963503002/diff/360001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.js:15: }, Nit: Blank lines between polymer properties. https://codereview.chromium.org/1963503002/diff/360001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.js:25: 'searchInput.blur': 'onInputBlur_' This listener is already registered in the HTML template.
Thanks! Tests will follow in a day or two. https://codereview.chromium.org/1963503002/diff/360001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js (right): https://codereview.chromium.org/1963503002/diff/360001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js:40: hasSearchText: { On 2016/05/26 00:42:52, dpapad wrote: > Nit: I think this is equivalent to the shorthand > > hasSearchText: Boolean, Done. https://codereview.chromium.org/1963503002/diff/360001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js:46: * @return {!string} The value of the search field. On 2016/05/26 00:42:52, dpapad wrote: > "!" is implied for primitive types (number, boolean, string) and therefore not > needed, here and elsewhere. Makes sense, done. https://codereview.chromium.org/1963503002/diff/360001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html (right): https://codereview.chromium.org/1963503002/diff/360001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html:49: showing-search="{{showingSearch_}}"> On 2016/05/26 00:42:52, dpapad wrote: > Indentation is a bit unusual here, how about > > <cr-toolbar-search-field id="search" narrow="[[narrow_]]" > label="[[searchPrompt]]" clear-label="[[clearLabel]]" > showing-search="{{showingSearch_}}"> > </cr-toolbar-search-field> "a poor craftsman blames their tools", but vim has been REALLY eager to align html like that recently. Fixed. https://codereview.chromium.org/1963503002/diff/360001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js (right): https://codereview.chromium.org/1963503002/diff/360001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js:9: narrow_: { On 2016/05/26 00:42:52, dpapad wrote: > /** @private */ Done. https://codereview.chromium.org/1963503002/diff/360001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js:13: showingSearch_: { On 2016/05/26 00:42:52, dpapad wrote: > Blank line between polymer properties. > > Also add @private here too. Done. https://codereview.chromium.org/1963503002/diff/360001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js:25: /** @return {CrToolbarSearchFieldElement} */ On 2016/05/26 00:42:52, dpapad wrote: > !CrToolbarSearchFieldElement Done. https://codereview.chromium.org/1963503002/diff/360001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.html (right): https://codereview.chromium.org/1963503002/diff/360001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.html:20: [hidden] { On 2016/05/26 00:42:52, dpapad wrote: > Can you verify whether this rule is needed? It has always worked for me without > the need for explicitly specifying it. > > Maybe it is inherited from > https://code.google.com/p/chromium/codesearch#search/&q=%22html%20/deep/%20%5... > or somewhere else? It's a bit weird, but I think it's coming from somewhere like that. The search fields in History and Downloads don't hide correctly without manually specifying this rule, and the only reason it -used- to work was because the dom-if was making the hidden properties redundant. https://codereview.chromium.org/1963503002/diff/360001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.html:82: margin-left: 18px; On 2016/05/26 00:42:52, dpapad wrote: > I don't think this works for RTL. How about --webkit-margin-start Oops, done. https://codereview.chromium.org/1963503002/diff/360001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.js (right): https://codereview.chromium.org/1963503002/diff/360001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.js:15: }, On 2016/05/26 00:42:52, dpapad wrote: > Nit: Blank lines between polymer properties. Done. https://codereview.chromium.org/1963503002/diff/360001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.js:25: 'searchInput.blur': 'onInputBlur_' On 2016/05/26 00:42:52, dpapad wrote: > This listener is already registered in the HTML template. Done (and I moved the one above it back to HTML for more consistency).
The CQ bit was checked by tsergeant@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from calamity@chromium.org, dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/1963503002/#ps380001 (title: "Style review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1963503002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1963503002/380001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_clang on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by tsergeant@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1963503002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1963503002/380001
Message was sent while issue was closed.
Description was changed from ========== MD WebUI: Add shared cr-toolbar element The toolbar currently contains the page name and a search field, which reuses the implementation of the existing <cr-search-field> element. The toolbar has some responsive behavior and basic animations. This is sufficient to allow us to replace some of the internals of <history-toolbar> with a <cr-toolbar>. The two main features which still need to be implemented are a menu button in the thin layout, and a spinner when a search is in progress. BUG=610609 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD WebUI: Add shared cr-toolbar element The toolbar currently contains the page name and a search field, which reuses the implementation of the existing <cr-search-field> element. The toolbar has some responsive behavior and basic animations. This is sufficient to allow us to replace some of the internals of <history-toolbar> with a <cr-toolbar>. The two main features which still need to be implemented are a menu button in the thin layout, and a spinner when a search is in progress. BUG=610609 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #11 (id:380001)
Message was sent while issue was closed.
Description was changed from ========== MD WebUI: Add shared cr-toolbar element The toolbar currently contains the page name and a search field, which reuses the implementation of the existing <cr-search-field> element. The toolbar has some responsive behavior and basic animations. This is sufficient to allow us to replace some of the internals of <history-toolbar> with a <cr-toolbar>. The two main features which still need to be implemented are a menu button in the thin layout, and a spinner when a search is in progress. BUG=610609 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD WebUI: Add shared cr-toolbar element The toolbar currently contains the page name and a search field, which reuses the implementation of the existing <cr-search-field> element. The toolbar has some responsive behavior and basic animations. This is sufficient to allow us to replace some of the internals of <history-toolbar> with a <cr-toolbar>. The two main features which still need to be implemented are a menu button in the thin layout, and a spinner when a search is in progress. BUG=610609 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/66d8b689e6e5e4a8f2b74430e0680e459021c5b8 Cr-Commit-Position: refs/heads/master@{#396115} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/66d8b689e6e5e4a8f2b74430e0680e459021c5b8 Cr-Commit-Position: refs/heads/master@{#396115} |
