Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
683813004:
Fewer focusable items in chrome://settings/searchEngines (Closed)
DescriptionFewer focusable items in chrome://settings/searchEngines
Fixes https://code.google.com/p/chromium/issues/detail?id=428063
This CL is part of splitting https://codereview.chromium.org/664583006/
into several smaller CLs. All applicable reviewer comments from that CL
have been addressed.
The general approach of this CL is to set tabindex=0 on a list item's
edit fields when it is the lead item, and set tabindex=-1 when it is
not the lead item. Things are complicated by most fields having
separate editable and static elements. When the list is focused the
editable elements of the lead item are visible and should have
tabindex=0. When the list is not focused the static elements of the
lead item are visible and should have tabindex=0.
The approach in the previous CL was to get rid of the static elements
and instead change the CSS styling on the editable elements when they
weren't focused. Unfortunately this did not work with editable
elements that weren't simple text, for example the dropdown option
boxes in content_settings_exception_area.js. That's why I switched to
this new approach.
The next CL will build on this one and preserve the columnIndex when
changing list items with the up/down keys.
BUG=428063
Committed: https://crrev.com/3df55f37eb3bcc896cbeab78f4769849d446b5c1
Cr-Commit-Position: refs/heads/master@{#303849}
Patch Set 1 #
Total comments: 40
Patch Set 2 : Incorporate reviewer comments from dbeam@ #
Total comments: 7
Patch Set 3 : Incorporate 2nd round of dbeam@ comments + re-add removeAttribute in set*ValuesFocusable #
Total comments: 3
Patch Set 4 : set*ValuesFocusable changes #
Total comments: 12
Patch Set 5 : Fix nits from dbeam@ comments on patch set #4 #
Total comments: 3
Patch Set 6 : Gah, change return -> continue #Patch Set 7 : Add browser test. WIP, uploaded for discussion #
Total comments: 17
Patch Set 8 : Load test-only resources from browser_tests.pak #
Total comments: 6
Patch Set 9 : Fix nits #Patch Set 10 : Rebase, fix failing browser_tests, add .pak file to GN #
Total comments: 6
Patch Set 11 : Add comment about weird behavior in autofill_options_browsertest.js #Messages
Total messages: 37 (6 generated)
bondd@chromium.org changed reviewers: + dbeam@chromium.org
bondd@chromium.org changed required reviewers: + dbeam@chromium.org
Ready for review. https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... File chrome/browser/resources/options/deletable_item_list.js (right): https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/deletable_item_list.js:106: handleFocus: function() { Made this protected because existing code in inline_editable_list.js was already using it that way. https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... File chrome/browser/resources/options/inline_editable_list.js (left): https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/inline_editable_list.js:142: } else { No longer required for new way of focusing elements. https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... File chrome/browser/resources/options/inline_editable_list.js (right): https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/inline_editable_list.js:463: editFields[i].staticVersion == clickTarget)) { Oops, double parens (( )) https://codereview.chromium.org/683813004/diff/1/ui/webui/resources/js/cr/ui/... File ui/webui/resources/js/cr/ui/list.js (right): https://codereview.chromium.org/683813004/diff/1/ui/webui/resources/js/cr/ui/... ui/webui/resources/js/cr/ui/list.js:116: return; Bad diff output. All I did was change if (this.dataModel_ != dataModel) { to if (this.dataModel_ == dataModel) return; and add this.onSetDataModelComplete(); at the bottom.
https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... File chrome/browser/resources/options/deletable_item_list.js (right): https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/deletable_item_list.js:44: * @type {boolean} @protected https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... File chrome/browser/resources/options/inline_editable_list.js (right): https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/inline_editable_list.js:227: * keyboard+mouse, mouse only, or neither. using the keyboard. https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/inline_editable_list.js:234: * else: focusable by mouse only ^ I don't think this is required, unfocusable means you can still see them (and hence, click) ;) https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/inline_editable_list.js:238: setEditableValuesFocusable: function(wantFocusable) { wantFocusable => focusable https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/inline_editable_list.js:238: setEditableValuesFocusable: function(wantFocusable) { this.editable is loop invariant, I believe, so it'd probably be the easiest to just: focusable = focusable && this.editable; https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/inline_editable_list.js:242: if (wantFocusable && !editFields[i].disabled) can't this whole loop body be editFields[i].tabIndex = focusable && !editFields[i].disabled ? 0 : -1; https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/inline_editable_list.js:242: if (wantFocusable && !editFields[i].disabled) it does seem like [disabled][tabindex=0] is actually focusable http://jsfiddle.net/na5dehp4/show/ https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/inline_editable_list.js:243: editFields[i].tabIndex = 0; indent off https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/inline_editable_list.js:246: } } else { https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/inline_editable_list.js:273: if (this.editable) { same general nits here as in setEditableValuesFocusable() https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/inline_editable_list.js:281: } indent off (-2 \s) https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/inline_editable_list.js:290: setCloseButtonFocusable: function(wantFocusable) { why can't we just use an ES5 getter/setter for this? https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/inline_editable_list.js:366: control.staticVersion.addEventListener('focus', function() { control.staticVersion.addEventListener('focus', this.handleFocus.bind(this)); https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/inline_editable_list.js:460: var clickField; clickField => editClickTarget https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/inline_editable_list.js:472: // focus from currently selected edit field. wrap at 80 cols https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/inline_editable_list.js:541: } } else { https://codereview.chromium.org/683813004/diff/1/ui/webui/resources/js/cr/ui/... File ui/webui/resources/js/cr/ui/list.js (right): https://codereview.chromium.org/683813004/diff/1/ui/webui/resources/js/cr/ui/... ui/webui/resources/js/cr/ui/list.js:158: * Override to be notified when |this.dataModel| is set. @protected
Addressed reviewer comments from dbeam@. https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... File chrome/browser/resources/options/deletable_item_list.js (right): https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/deletable_item_list.js:44: * @type {boolean} On 2014/10/29 01:37:12, Dan Beam wrote: > @protected Done. https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... File chrome/browser/resources/options/inline_editable_list.js (right): https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/inline_editable_list.js:227: * keyboard+mouse, mouse only, or neither. On 2014/10/29 01:37:13, Dan Beam wrote: > using the keyboard. Done. https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/inline_editable_list.js:234: * else: focusable by mouse only On 2014/10/29 01:37:13, Dan Beam wrote: > ^ I don't think this is required, unfocusable means you can still see them (and > hence, click) ;) I removed the entire "Result" comment block. Is that what you intended? https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/inline_editable_list.js:238: setEditableValuesFocusable: function(wantFocusable) { On 2014/10/29 01:37:13, Dan Beam wrote: > wantFocusable => focusable Done. https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/inline_editable_list.js:242: if (wantFocusable && !editFields[i].disabled) On 2014/10/29 01:37:13, Dan Beam wrote: > it does seem like [disabled][tabindex=0] is actually focusable > http://jsfiddle.net/na5dehp4/show/ Did you mean "it doesn't seem"? That button you linked is not focusable for me. https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/inline_editable_list.js:242: if (wantFocusable && !editFields[i].disabled) On 2014/10/29 01:37:14, Dan Beam wrote: > can't this whole loop body be > > editFields[i].tabIndex = focusable && !editFields[i].disabled ? 0 : -1; Done. I had guessed that condensing logic like this would be frowned upon, but I finally went and read the whole style guide and found out that I guessed wrong. :) https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/inline_editable_list.js:248: editFields[i].removeAttribute('tabindex'); I got rid of this per your above comment re: condensing the loop body. Tested it with this line removed and everything still works. https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/inline_editable_list.js:273: if (this.editable) { On 2014/10/29 01:37:13, Dan Beam wrote: > same general nits here as in setEditableValuesFocusable() Done. https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/inline_editable_list.js:290: setCloseButtonFocusable: function(wantFocusable) { On 2014/10/29 01:37:13, Dan Beam wrote: > why can't we just use an ES5 getter/setter for this? To keep it consistent with setStaticValuesFocusable and setEditableValuesFocusable, since all 3 are usually used together. Google JavaScript Style Guide says "EcmaScript 5 getters and setters for properties are discouraged". Maybe that just means don't wrap variables, but putting functionality inside them is okay (?). What is your interpretation of that sentence from the style guide? Either way it seems we use ES5 getters and setters all over the place. Let me know if you want me to change this into an ES5 setter. https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/inline_editable_list.js:366: control.staticVersion.addEventListener('focus', function() { On 2014/10/29 01:37:13, Dan Beam wrote: > control.staticVersion.addEventListener('focus', this.handleFocus.bind(this)); Done. https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/inline_editable_list.js:460: var clickField; On 2014/10/29 01:37:14, Dan Beam wrote: > clickField => editClickTarget Done. https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/inline_editable_list.js:463: editFields[i].staticVersion == clickTarget)) { On 2014/10/28 22:29:57, bondd wrote: > Oops, double parens (( )) Done. https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/inline_editable_list.js:472: // focus from currently selected edit field. On 2014/10/29 01:37:13, Dan Beam wrote: > wrap at 80 cols Done. https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/inline_editable_list.js:541: } On 2014/10/29 01:37:13, Dan Beam wrote: > } else { Done. https://codereview.chromium.org/683813004/diff/1/ui/webui/resources/js/cr/ui/... File ui/webui/resources/js/cr/ui/list.js (right): https://codereview.chromium.org/683813004/diff/1/ui/webui/resources/js/cr/ui/... ui/webui/resources/js/cr/ui/list.js:158: * Override to be notified when |this.dataModel| is set. On 2014/10/29 01:37:14, Dan Beam wrote: > @protected Done.
https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... File chrome/browser/resources/options/inline_editable_list.js (right): https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/inline_editable_list.js:242: if (wantFocusable && !editFields[i].disabled) On 2014/10/29 17:19:08, bondd wrote: > On 2014/10/29 01:37:13, Dan Beam wrote: > > it does seem like [disabled][tabindex=0] is actually focusable > > http://jsfiddle.net/na5dehp4/show/ > > Did you mean "it doesn't seem"? That button you linked is not focusable for me. doesn't** https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/inline_editable_list.js:290: setCloseButtonFocusable: function(wantFocusable) { On 2014/10/29 17:19:08, bondd wrote: > On 2014/10/29 01:37:13, Dan Beam wrote: > > why can't we just use an ES5 getter/setter for this? > > To keep it consistent with setStaticValuesFocusable and > setEditableValuesFocusable, since all 3 are usually used together. > > Google JavaScript Style Guide says "EcmaScript 5 getters and setters for > properties are discouraged". Maybe that just means don't wrap variables, but > putting functionality inside them is okay (?). What is your interpretation of > that sentence from the style guide? Either way it seems we use ES5 getters and > setters all over the place. Let me know if you want me to change this into an > ES5 setter. that doesn't apply to Chrome code, as you've determined -- we use them everywhere. i think the internal methods can stay as set*() but it'd be weird if a caller or downstream class said: list.closeButtonFocusAllowed = false; and it didn't have any immediate effect. https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/inline_editable_list.js:460: var clickField; On 2014/10/29 17:19:08, bondd wrote: > On 2014/10/29 01:37:14, Dan Beam wrote: > > clickField => editClickTarget > > Done. editClickTarget (note the "Target") https://codereview.chromium.org/683813004/diff/20001/chrome/browser/resources... File chrome/browser/resources/options/inline_editable_list.js (right): https://codereview.chromium.org/683813004/diff/20001/chrome/browser/resources... chrome/browser/resources/options/inline_editable_list.js:229: setEditableValuesFocusable: function(focusable) { make @private, change to setEditableValuesFocusable_ https://codereview.chromium.org/683813004/diff/20001/chrome/browser/resources... chrome/browser/resources/options/inline_editable_list.js:241: setStaticValuesFocusable: function(focusable) { make private https://codereview.chromium.org/683813004/diff/20001/chrome/browser/resources... chrome/browser/resources/options/inline_editable_list.js:260: this.closeButtonElement.tabIndex = wantFocusable ? 0 : -1; same nit: wantFocusable => focusable and focusable = focusable && this.closeButtonFocusAllowed; this.closeButtonElement.tabIndex = focusable ? 0 : -1; or this.closeButtonElement.tabIndex = focusable && this.closeButtonFocusAllowed ? 0 : -1; https://codereview.chromium.org/683813004/diff/20001/chrome/browser/resources... chrome/browser/resources/options/inline_editable_list.js:332: control.staticVersion.addEventListener('focus', this will only ever be called once, right? else we'd get double event registration happening...
Incorporate 2nd round of dbeam@ comments + re-add removeAttribute in set*ValuesFocusable. https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... File chrome/browser/resources/options/inline_editable_list.js (right): https://codereview.chromium.org/683813004/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/inline_editable_list.js:460: var clickField; On 2014/10/29 22:28:11, Dan Beam wrote: > On 2014/10/29 17:19:08, bondd wrote: > > On 2014/10/29 01:37:14, Dan Beam wrote: > > > clickField => editClickTarget > > > > Done. > > editClickTarget (note the "Target") Done. https://codereview.chromium.org/683813004/diff/20001/chrome/browser/resources... File chrome/browser/resources/options/inline_editable_list.js (right): https://codereview.chromium.org/683813004/diff/20001/chrome/browser/resources... chrome/browser/resources/options/inline_editable_list.js:229: setEditableValuesFocusable: function(focusable) { On 2014/10/29 22:28:11, Dan Beam wrote: > make @private, change to setEditableValuesFocusable_ These are called publicly from InlineEditableItemList. This file is a bit confusing because it contains both InlineEditableItem and InlineEditableItemList. https://codereview.chromium.org/683813004/diff/20001/chrome/browser/resources... chrome/browser/resources/options/inline_editable_list.js:260: this.closeButtonElement.tabIndex = wantFocusable ? 0 : -1; On 2014/10/29 22:28:11, Dan Beam wrote: > same nit: > > wantFocusable => focusable > > and > > focusable = focusable && this.closeButtonFocusAllowed; > this.closeButtonElement.tabIndex = focusable ? 0 : -1; > > or > > this.closeButtonElement.tabIndex = > focusable && this.closeButtonFocusAllowed ? 0 : -1; Done. https://codereview.chromium.org/683813004/diff/20001/chrome/browser/resources... chrome/browser/resources/options/inline_editable_list.js:332: control.staticVersion.addEventListener('focus', On 2014/10/29 22:28:11, Dan Beam wrote: > this will only ever be called once, right? else we'd get double event > registration happening... Yes, addEditField will only be called once for each edit field. https://codereview.chromium.org/683813004/diff/40001/chrome/browser/resources... File chrome/browser/resources/options/inline_editable_list.js (right): https://codereview.chromium.org/683813004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/inline_editable_list.js:236: editFields[i].removeAttribute('tabindex'); Re-added removeAttribute('tabindex'); because I found a case that failed when it was removed. If a text field has tabIndex=-1 and it is clicked on, it will get focus and have a selection outline drawn around it. We don't want that outline when !this.editable. An alternate solution would be to change the CSS styling, but that might cause a cascade (get it?) of unintended consequences so I'd rather do it like this.
https://codereview.chromium.org/683813004/diff/40001/chrome/browser/resources... File chrome/browser/resources/options/inline_editable_list.js (right): https://codereview.chromium.org/683813004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/inline_editable_list.js:236: editFields[i].removeAttribute('tabindex'); On 2014/10/29 23:08:54, bondd wrote: > Re-added > removeAttribute('tabindex'); > because I found a case that failed when it was removed. If a text field has > tabIndex=-1 and it is clicked on, it will get focus and have a selection outline > drawn around it. We don't want that outline when !this.editable. > > An alternate solution would be to change the CSS styling, but that might cause a > cascade (get it?) of unintended consequences so I'd rather do it like this. maybe this method should just ignore if editFields[i] == document.activeElement?
Made changes to set*ValuesFocusable. https://codereview.chromium.org/683813004/diff/40001/chrome/browser/resources... File chrome/browser/resources/options/inline_editable_list.js (right): https://codereview.chromium.org/683813004/diff/40001/chrome/browser/resources... chrome/browser/resources/options/inline_editable_list.js:236: editFields[i].removeAttribute('tabindex'); On 2014/10/29 23:30:13, Dan Beam wrote: > On 2014/10/29 23:08:54, bondd wrote: > > Re-added > > removeAttribute('tabindex'); > > because I found a case that failed when it was removed. If a text field has > > tabIndex=-1 and it is clicked on, it will get focus and have a selection > outline > > drawn around it. We don't want that outline when !this.editable. > > > > An alternate solution would be to change the CSS styling, but that might cause > a > > cascade (get it?) of unintended consequences so I'd rather do it like this. > > maybe this method should just ignore if editFields[i] == document.activeElement? I put this back to the way it was in patch set #2. After further investigation it turns out that only staticVersion needs the removeAttribute call. https://codereview.chromium.org/683813004/diff/60001/chrome/browser/resources... File chrome/browser/resources/options/inline_editable_list.js (right): https://codereview.chromium.org/683813004/diff/60001/chrome/browser/resources... chrome/browser/resources/options/inline_editable_list.js:252: // have selection box drawn around it. Added comment to explain why removeAttribute call is here.
https://codereview.chromium.org/683813004/diff/60001/chrome/browser/resources... File chrome/browser/resources/options/inline_editable_list.js (right): https://codereview.chromium.org/683813004/diff/60001/chrome/browser/resources... chrome/browser/resources/options/inline_editable_list.js:233: editFields[i].tabIndex = focusable && !editFields[i].disabled ? 0 : -1; i think when editFields[i].disabled == true, tabIndex isn't respected anyways, so can't this just be: editFields[i].tabIndex = focusable ? 0 : -1; https://codereview.chromium.org/683813004/diff/60001/chrome/browser/resources... chrome/browser/resources/options/inline_editable_list.js:245: if (staticVersion) { if (!staticVersion) continue; ... less indent ... https://codereview.chromium.org/683813004/diff/60001/chrome/browser/resources... chrome/browser/resources/options/inline_editable_list.js:261: * nit: remove extra line https://codereview.chromium.org/683813004/diff/60001/chrome/browser/resources... chrome/browser/resources/options/inline_editable_list.js:265: this.closeButtonElement.tabIndex = indent off (2\s is default, 4\s for continuations) https://codereview.chromium.org/683813004/diff/60001/chrome/browser/resources... File chrome/browser/resources/options/search_engine_manager_engine_list.js (left): https://codereview.chromium.org/683813004/diff/60001/chrome/browser/resources... chrome/browser/resources/options/search_engine_manager_engine_list.js:114: this.closeButtonElement.tabIndex = 0; this took immediate effect https://codereview.chromium.org/683813004/diff/60001/chrome/browser/resources... File chrome/browser/resources/options/search_engine_manager_engine_list.js (right): https://codereview.chromium.org/683813004/diff/60001/chrome/browser/resources... chrome/browser/resources/options/search_engine_manager_engine_list.js:114: this.closeButtonFocusAllowed = true; when does this change behavior? why can't we make this an ES5 setter and update when it changes?
Fix nits and reply to dbeam@ reviewer comments. https://codereview.chromium.org/683813004/diff/60001/chrome/browser/resources... File chrome/browser/resources/options/inline_editable_list.js (right): https://codereview.chromium.org/683813004/diff/60001/chrome/browser/resources... chrome/browser/resources/options/inline_editable_list.js:233: editFields[i].tabIndex = focusable && !editFields[i].disabled ? 0 : -1; On 2014/10/30 01:45:49, Dan Beam wrote: > i think when editFields[i].disabled == true, tabIndex isn't respected anyways, > so can't this just be: > > editFields[i].tabIndex = focusable ? 0 : -1; > Done. https://codereview.chromium.org/683813004/diff/60001/chrome/browser/resources... chrome/browser/resources/options/inline_editable_list.js:245: if (staticVersion) { On 2014/10/30 01:45:49, Dan Beam wrote: > if (!staticVersion) > continue; > > ... less indent ... Done. https://codereview.chromium.org/683813004/diff/60001/chrome/browser/resources... chrome/browser/resources/options/inline_editable_list.js:261: * On 2014/10/30 01:45:49, Dan Beam wrote: > nit: remove extra line Done. https://codereview.chromium.org/683813004/diff/60001/chrome/browser/resources... chrome/browser/resources/options/inline_editable_list.js:265: this.closeButtonElement.tabIndex = On 2014/10/30 01:45:49, Dan Beam wrote: > indent off (2\s is default, 4\s for continuations) Done. https://codereview.chromium.org/683813004/diff/60001/chrome/browser/resources... File chrome/browser/resources/options/search_engine_manager_engine_list.js (right): https://codereview.chromium.org/683813004/diff/60001/chrome/browser/resources... chrome/browser/resources/options/search_engine_manager_engine_list.js:114: this.closeButtonFocusAllowed = true; On 2014/10/30 01:45:49, Dan Beam wrote: > when does this change behavior? why can't we make this an ES5 setter and update > when it changes? closeButtonFocusAllowed indicates whether the button can *ever* have keyboard focus; tabIndex itself is modified when the list item gains/loses lead status and when the list itself gains/loses focus (see calls to setCloseButtonFocusable in inline_editable_list.js). closeButtonFocusAllowed is set on init and is not intended to change after that. Value depends on list type, e.g. "Manage search engines" has it and other lists don't (but may in the future). I can't see closeButtonFocusAllowed ever changing dynamically while the list is in use. Making it take effect immediately could be tricky because it's not directly coupled to tabIndex, and the code to do that will be dead code anyway since there is no reason to change closeButtonFocusAllowed after the list is initialized. The simplest solution might be to write a comment on the closeButtonFocusAllowed variable declaration making it more clear what it does, and that it is only to be set on init. I could also rename it to be more descriptive, like closeButtonFocusEverAllowed (or whatever you suggest).
https://codereview.chromium.org/683813004/diff/80001/chrome/browser/resources... File chrome/browser/resources/options/inline_editable_list.js (right): https://codereview.chromium.org/683813004/diff/80001/chrome/browser/resources... chrome/browser/resources/options/inline_editable_list.js:246: return; shouldn't this be continue...? do you have any tests for this code?
https://codereview.chromium.org/683813004/diff/80001/chrome/browser/resources... File chrome/browser/resources/options/inline_editable_list.js (right): https://codereview.chromium.org/683813004/diff/80001/chrome/browser/resources... chrome/browser/resources/options/inline_editable_list.js:246: return; On 2014/10/30 22:20:43, Dan Beam wrote: > shouldn't this be continue...? do you have any tests for this code? Done. Gah, my bad. Typo. There are currently no dialogs that behave differently with a return vs. continue here, so none of my manual tests failed. All list items with staticVersion == null have staticVersion == null for all edit fields on that line. There would need to be an edit field with staticVersion == null before one with staticVersion != null for return vs. continue to make a difference here. That case may exist in the future but right now it doesn't.
https://codereview.chromium.org/683813004/diff/80001/chrome/browser/resources... File chrome/browser/resources/options/inline_editable_list.js (right): https://codereview.chromium.org/683813004/diff/80001/chrome/browser/resources... chrome/browser/resources/options/inline_editable_list.js:246: return; On 2014/10/30 22:45:29, bondd wrote: > On 2014/10/30 22:20:43, Dan Beam wrote: > > shouldn't this be continue...? do you have any tests for this code? > > Done. > Gah, my bad. Typo. There are currently no dialogs that behave differently with a > return vs. continue here, so none of my manual tests failed. All list items with > staticVersion == null have staticVersion == null for all edit fields on that > line. There would need to be an edit field with staticVersion == null before one > with staticVersion != null for return vs. continue to make a difference here. > That case may exist in the future but right now it doesn't. we should probably still have some tests for all this
Uploaded patch #7 for discussion of a few questions I have about it. This was a bit trickier than I thought it would be because InlineEditableItemList is part of options (not webui like the base class List stuff) and is not tied to a specific chrome:// page. As far as I can tell there are no other classes like that with unit tests, so I couldn’t just copy the structure from one of those. Most of the *_browsertest.js tests do browsePreload: 'chrome://history-frame', which sets things up nicely for the test. InlineEditableItemList doesn’t have an associated chrome:// page so I went a different way. Is there a way to load a custom test page using browsePreload? I looked but didn’t see anything. If I could load an arbitrary test .html file with browsePreload, or register a test page and access it via chrome://my_test_page_name, then I could hook the tests in more like history_browsertest.js. It might be cleaner and more consistent that way, and get around the couple of hacky things I’ve done to get things working. https://codereview.chromium.org/683813004/diff/120001/chrome/test/data/webui/... File chrome/test/data/webui/inline_editable_list_test.html (right): https://codereview.chromium.org/683813004/diff/120001/chrome/test/data/webui/... chrome/test/data/webui/inline_editable_list_test.html:7: function setUp() { Need to do things in setUp for reason given in webui_resource_browsertest.cc: // Queues the library corresponding to |resource_id| for injection into the // test. The code injection is performed post-load, so any common test // initialization that depends on the library should be placed in a setUp // function. https://codereview.chromium.org/683813004/diff/120001/chrome/test/data/webui/... chrome/test/data/webui/inline_editable_list_test.html:8: loadTimeData.data = { This seems like a total hack. Without this I get the callstack below. Is there way to include the proper strings.js? I looked for a while and couldn't figure it out. [32353:32353:1105/183001:INFO:CONSOLE(3640)] "Unexpected condition on file:///usr/local/google/home/bondd/src/chrome/test/data/webui/inline_editable_list_test.html: No data. Did you remember to include strings.js?", source: (3640) [32353:32353:1105/183001:INFO:CONSOLE(179)] "Failure in test testUpDownFocus TypeError: Cannot read property 'deletableItemDeleteButtonTitle' of undefined", source: (179) [32353:32353:1105/183001:INFO:CONSOLE(180)] "TypeError: Cannot read property 'deletableItemDeleteButtonTitle' of undefined at Object.LoadTimeData.getValue (<anonymous>:3562:29) at Object.LoadTimeData.getString (<anonymous>:3573:24) at HTMLLIElement.DeletableItem.decorate (<anonymous>:3730:24) at HTMLLIElement.InlineEditableItem.decorate (<anonymous>:3946:40) https://codereview.chromium.org/683813004/diff/120001/chrome/test/data/webui/... File chrome/test/data/webui/webui_resource_browsertest.cc (right): https://codereview.chromium.org/683813004/diff/120001/chrome/test/data/webui/... chrome/test/data/webui/webui_resource_browsertest.cc:120: AddLibrary(IDR_OPTIONS_INLINE_EDITABLE_LIST); deletable_item_list.js and inline_editable_list.js are in chrome/browser/resources/options/. Base class list stuff is in ui/webui/resources/js/cr/ui/. Very different locations. Is it okay to add the inline_editable_list tests in here, or should I make an options_resource_browsertest.cc (or similar) somewhere? https://codereview.chromium.org/683813004/diff/120001/ui/webui/resources/webu... File ui/webui/resources/webui_resources.grd (right): https://codereview.chromium.org/683813004/diff/120001/ui/webui/resources/webu... ui/webui/resources/webui_resources.grd:312: type="chrome_html" /> These are the only chrome/browser/ resources in this entire file, which leads me to believe that this might just be the wrong spot for them. :) Should I move them? Where to?
https://codereview.chromium.org/683813004/diff/120001/chrome/test/data/webui/... File chrome/test/data/webui/inline_editable_list_test.html (right): https://codereview.chromium.org/683813004/diff/120001/chrome/test/data/webui/... chrome/test/data/webui/inline_editable_list_test.html:22: function TestItem(name) { why do we need this class? https://codereview.chromium.org/683813004/diff/120001/chrome/test/data/webui/... chrome/test/data/webui/inline_editable_list_test.html:64: var TestItemList = cr.ui.define('list'); and this one?
https://codereview.chromium.org/683813004/diff/120001/chrome/test/data/webui/... File chrome/test/data/webui/webui_resource_browsertest.cc (right): https://codereview.chromium.org/683813004/diff/120001/chrome/test/data/webui/... chrome/test/data/webui/webui_resource_browsertest.cc:120: AddLibrary(IDR_OPTIONS_INLINE_EDITABLE_LIST); On 2014/11/06 02:45:41, bondd wrote: > deletable_item_list.js and inline_editable_list.js are in > chrome/browser/resources/options/. Base class list stuff is in > ui/webui/resources/js/cr/ui/. Very different locations. Is it okay to add the > inline_editable_list tests in here, or should I make an > options_resource_browsertest.cc (or similar) somewhere? adding the tests in here and pre-loading all of ui/webui stuff is fine. why can't you just include chrome/browser/resources stuff via a local <script src="../../../browser/resources/options/..."> like I mentioned before? https://codereview.chromium.org/683813004/diff/120001/ui/webui/resources/webu... File ui/webui/resources/webui_resources.grd (right): https://codereview.chromium.org/683813004/diff/120001/ui/webui/resources/webu... ui/webui/resources/webui_resources.grd:312: type="chrome_html" /> On 2014/11/06 02:45:41, bondd wrote: > These are the only chrome/browser/ resources in this entire file, which leads me > to believe that this might just be the wrong spot for them. :) Should I move > them? Where to? not in this file
https://codereview.chromium.org/683813004/diff/120001/chrome/test/data/webui/... File chrome/test/data/webui/webui_resource_browsertest.cc (right): https://codereview.chromium.org/683813004/diff/120001/chrome/test/data/webui/... chrome/test/data/webui/webui_resource_browsertest.cc:120: AddLibrary(IDR_OPTIONS_INLINE_EDITABLE_LIST); On 2014/11/06 02:56:13, Dan Beam wrote: > On 2014/11/06 02:45:41, bondd wrote: > > deletable_item_list.js and inline_editable_list.js are in > > chrome/browser/resources/options/. Base class list stuff is in > > ui/webui/resources/js/cr/ui/. Very different locations. Is it okay to add the > > inline_editable_list tests in here, or should I make an > > options_resource_browsertest.cc (or similar) somewhere? > > adding the tests in here and pre-loading all of ui/webui stuff is fine. > > why can't you just include chrome/browser/resources stuff via a local <script > src="../../../browser/resources/options/..."> like I mentioned before? I tried that. The AddLibrary libs are added after the .html is loaded, so <script src ="../../../browser/resources/options/..."> fails because all this AddLibrary stuff (cr, ui, lists, etc.) isn't loaded yet. I tried to do a delayed load of the browser/resources/options/ scripts in inline_editable_list_test.html, but the scripts load asynchronously and so aren't ready for immediate use. Any recommendations?
https://codereview.chromium.org/683813004/diff/120001/chrome/test/data/webui/... File chrome/test/data/webui/webui_resource_browsertest.cc (right): https://codereview.chromium.org/683813004/diff/120001/chrome/test/data/webui/... chrome/test/data/webui/webui_resource_browsertest.cc:120: AddLibrary(IDR_OPTIONS_INLINE_EDITABLE_LIST); On 2014/11/06 03:02:36, bondd wrote: > On 2014/11/06 02:56:13, Dan Beam wrote: > > On 2014/11/06 02:45:41, bondd wrote: > > > deletable_item_list.js and inline_editable_list.js are in > > > chrome/browser/resources/options/. Base class list stuff is in > > > ui/webui/resources/js/cr/ui/. Very different locations. Is it okay to add > the > > > inline_editable_list tests in here, or should I make an > > > options_resource_browsertest.cc (or similar) somewhere? > > > > adding the tests in here and pre-loading all of ui/webui stuff is fine. > > > > why can't you just include chrome/browser/resources stuff via a local <script > > src="../../../browser/resources/options/..."> like I mentioned before? > > I tried that. The AddLibrary libs are added after the .html is loaded, so > <script src ="../../../browser/resources/options/..."> fails because all this > AddLibrary stuff (cr, ui, lists, etc.) isn't loaded yet. I tried to do a > delayed load of the browser/resources/options/ scripts in > inline_editable_list_test.html, but the scripts load asynchronously and so > aren't ready for immediate use. Any recommendations? do what we talked about wrt making a test-only .grd for this kind of thing that's in chrome/browser/resources/ somewhere
Okay so it's getting a bit comical how much is involved in adding this one simple browser test. That said I think it's pretty much done now. This is what we talked about yesterday afternoon -- I added options_test_resources.grd. The one unexpected (by me at least) wrinkle is that all the .pak files are repacked into one monolithic resources.pak and loaded all together. We can't add options_test_resources.grd to that or it will get shipped with the browser. So I created the new browser_tests.pak and repacked options_test_resources.pak into that. ui_test.pak does a similar thing for UI test resources. I create browser_tests.pak because I'm following the example set by ui_test.pak. Maybe I could load options_test_resources.pak directly from gen/chrome/options_test_resources.pak, but it doesn't look like anyone else is doing it that way so I'm avoiding it. Loading something from a "gen" directory might be bad. I don't want to copy options_test_resources.pak straight into the top level directory with resources.pak, because there are only 7 other .pak files there and I don't want to pollute the directory with a bunch of extra files. Better to create a single .pak file per executable (e.g. browser_tests) rather than a .pak file for each resource, in case more browser_tests-specific resources are added in the future. If this test needs to run on Mac then I think I need to add a few Mac-specific lines for the browser_tests_pak target in chrome_resources.gyp because of the different way Mac resources are loaded. I'll start looking into whether that is needed now. I wanted to get the rest of this uploaded for review first. https://codereview.chromium.org/683813004/diff/120001/chrome/test/data/webui/... File chrome/test/data/webui/inline_editable_list_test.html (right): https://codereview.chromium.org/683813004/diff/120001/chrome/test/data/webui/... chrome/test/data/webui/inline_editable_list_test.html:22: function TestItem(name) { On 2014/11/06 02:54:08, Dan Beam wrote: > why do we need this class? This is the pattern followed by all classes that use inline_editable_item.js. derivedItemList.createItem creates derivedItem and returns it. I thought maybe I could set List.itemConstructor and avoid creating these two extra classes, but all existing code overrides createItem without calling up to base class (which has its own logic that is not getting called). So getting itemConstructor to work would be quite different than what is happening everywhere else. So these two classes are here because this is how InlineEditableItemLists are created everywhere else in the code base, and doing it without creating derived classes is not trivial. https://codereview.chromium.org/683813004/diff/120001/chrome/test/data/webui/... chrome/test/data/webui/inline_editable_list_test.html:64: var TestItemList = cr.ui.define('list'); On 2014/11/06 02:54:08, Dan Beam wrote: > and this one? See above comment. https://codereview.chromium.org/683813004/diff/120001/ui/webui/resources/webu... File ui/webui/resources/webui_resources.grd (right): https://codereview.chromium.org/683813004/diff/120001/ui/webui/resources/webu... ui/webui/resources/webui_resources.grd:312: type="chrome_html" /> On 2014/11/06 02:56:13, Dan Beam wrote: > On 2014/11/06 02:45:41, bondd wrote: > > These are the only chrome/browser/ resources in this entire file, which leads > me > > to believe that this might just be the wrong spot for them. :) Should I move > > them? Where to? > > not in this file Done. https://codereview.chromium.org/683813004/diff/140001/chrome/test/data/webui/... File chrome/test/data/webui/webui_resource_browsertest.cc (right): https://codereview.chromium.org/683813004/diff/140001/chrome/test/data/webui/... chrome/test/data/webui/webui_resource_browsertest.cc:26: ResourceBundle::GetSharedInstance().AddDataPackFromPath( I don't see any way to check whether AddDataPackFromPath succeeded. I looked for a different way to load the pack that would indicate success/failure but didn't find one.
> If this test needs to run on Mac we certainly want to if possible, but it could wait until later (follow up CL) this looks really close to good to me, but I just want to know why this test is still guaranteed to be valid/useful if we're creating fake-ish items. https://codereview.chromium.org/683813004/diff/120001/chrome/test/data/webui/... File chrome/test/data/webui/inline_editable_list_test.html (right): https://codereview.chromium.org/683813004/diff/120001/chrome/test/data/webui/... chrome/test/data/webui/inline_editable_list_test.html:7: function setUp() { On 2014/11/06 02:45:41, bondd wrote: > Need to do things in setUp for reason given in webui_resource_browsertest.cc: > // Queues the library corresponding to |resource_id| for injection into the > // test. The code injection is performed post-load, so any common test > // initialization that depends on the library should be placed in a setUp > // function. Acknowledged. https://codereview.chromium.org/683813004/diff/120001/chrome/test/data/webui/... chrome/test/data/webui/inline_editable_list_test.html:8: loadTimeData.data = { On 2014/11/06 02:45:41, bondd wrote: > This seems like a total hack. Without this I get the callstack below. Is there > way to include the proper strings.js? I looked for a while and couldn't figure > it out. > > [32353:32353:1105/183001:INFO:CONSOLE(3640)] "Unexpected condition on > file:///usr/local/google/home/bondd/src/chrome/test/data/webui/inline_editable_list_test.html: > No data. Did you remember to include strings.js?", source: (3640) > [32353:32353:1105/183001:INFO:CONSOLE(179)] "Failure in test testUpDownFocus > TypeError: Cannot read property 'deletableItemDeleteButtonTitle' of undefined", > source: (179) > [32353:32353:1105/183001:INFO:CONSOLE(180)] "TypeError: Cannot read property > 'deletableItemDeleteButtonTitle' of undefined > at Object.LoadTimeData.getValue (<anonymous>:3562:29) > at Object.LoadTimeData.getString (<anonymous>:3573:24) > at HTMLLIElement.DeletableItem.decorate (<anonymous>:3730:24) > at HTMLLIElement.InlineEditableItem.decorate (<anonymous>:3946:40) Acknowledged. https://codereview.chromium.org/683813004/diff/120001/chrome/test/data/webui/... chrome/test/data/webui/inline_editable_list_test.html:22: function TestItem(name) { On 2014/11/07 17:36:14, bondd wrote: > On 2014/11/06 02:54:08, Dan Beam wrote: > > why do we need this class? > > This is the pattern followed by all classes that use inline_editable_item.js. > derivedItemList.createItem creates derivedItem and returns it. I thought maybe I > could set List.itemConstructor and avoid creating these two extra classes, but > all existing code overrides createItem without calling up to base class (which > has its own logic that is not getting called). So getting itemConstructor to > work would be quite different than what is happening everywhere else. > > So these two classes are here because this is how InlineEditableItemLists are > created everywhere else in the code base, and doing it without creating derived > classes is not trivial. ok but doesn't creating different types of items mess with the validity of your test? https://codereview.chromium.org/683813004/diff/140001/chrome/chrome_resources... File chrome/chrome_resources.gyp (right): https://codereview.chromium.org/683813004/diff/140001/chrome/chrome_resources... chrome/chrome_resources.gyp:576: # Create pak file for browser_tests. unless this is follow a convention, don't see a reason for this comment (pretty self-evident) https://codereview.chromium.org/683813004/diff/140001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/683813004/diff/140001/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:1810: 'browser', e.g. why not here? https://codereview.chromium.org/683813004/diff/140001/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:1815: 'chrome_resources.gyp:browser_tests_pak', nit: unless this must be lower (for dependency reasons), I'd just insert alphabetically
https://codereview.chromium.org/683813004/diff/120001/chrome/test/data/webui/... File chrome/test/data/webui/inline_editable_list_test.html (right): https://codereview.chromium.org/683813004/diff/120001/chrome/test/data/webui/... chrome/test/data/webui/inline_editable_list_test.html:22: function TestItem(name) { On 2014/11/07 19:43:58, Dan Beam wrote: > On 2014/11/07 17:36:14, bondd wrote: > > On 2014/11/06 02:54:08, Dan Beam wrote: > > > why do we need this class? > > > > This is the pattern followed by all classes that use inline_editable_item.js. > > derivedItemList.createItem creates derivedItem and returns it. I thought maybe > I > > could set List.itemConstructor and avoid creating these two extra classes, but > > all existing code overrides createItem without calling up to base class (which > > has its own logic that is not getting called). So getting itemConstructor to > > work would be quite different than what is happening everywhere else. > > > > So these two classes are here because this is how InlineEditableItemLists are > > created everywhere else in the code base, and doing it without creating > derived > > classes is not trivial. > > ok but doesn't creating different types of items mess with the validity of your > test? Nope, I think it's fine. The list is populated with items that are created the same way everywhere InlineEditableItemLists are used. createEditableTextCell is called, which creates cells with an editable element and a static element. Those "fully created" cells are needed so the test can verify that all elements are having their tabIndexes set properly when up/down keys are pressed. I'd argue that creating the test list like this makes the test more useful and valid rather than less. https://codereview.chromium.org/683813004/diff/140001/chrome/chrome_resources... File chrome/chrome_resources.gyp (right): https://codereview.chromium.org/683813004/diff/140001/chrome/chrome_resources... chrome/chrome_resources.gyp:576: # Create pak file for browser_tests. On 2014/11/07 19:43:59, Dan Beam wrote: > unless this is follow a convention, don't see a reason for this comment (pretty > self-evident) Done. https://codereview.chromium.org/683813004/diff/140001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/683813004/diff/140001/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:1815: 'chrome_resources.gyp:browser_tests_pak', On 2014/11/07 19:43:59, Dan Beam wrote: > nit: unless this must be lower (for dependency reasons), I'd just insert > alphabetically Done.
lgtm, but you'll probably need additional reviews now
thestig@chromium.org changed reviewers: + thestig@chromium.org
You need to do the GN version as well.
Patchset #11 (id:200001) has been deleted
Patchset #10 (id:180001) has been deleted
Did the GN version as requested by thestig@. Fixed browser_tests failing on trybots. Rebased. https://codereview.chromium.org/683813004/diff/220001/chrome/browser/resource... File chrome/browser/resources/options/autofill_options_list.js (right): https://codereview.chromium.org/683813004/diff/220001/chrome/browser/resource... chrome/browser/resources/options/autofill_options_list.js:458: firstItem.handleFocus(); Renamed this in DeletableItem and missed updating it here. https://codereview.chromium.org/683813004/diff/220001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/autofill_options_browsertest.js (right): https://codereview.chromium.org/683813004/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/autofill_options_browsertest.js:198: phoneList.blur(); The way that InlineEditableItem triggers validation changed and it broke this test. "input->phoneList" fixes the test. The purpose of this test is to verify that validation is triggered when text is changed. Validation was previously triggered when the InlineEditableItem was blurred. Now it is triggered when the lead item changes or the entire list gains/loses focus. The InlineEditableItem cannot be blurred without one of those two other events happening (as far as I know). I changed the test to blur the entire list rather than just the item. I considered adding an additional test for changing the lead item, but the main purpose of this existing test is to verify the validation-specific code in autofill_options_list.js, not to verify details of inline_editable_item.js. So I think this solution is reasonable as-is.
https://codereview.chromium.org/683813004/diff/220001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/autofill_options_browsertest.js (right): https://codereview.chromium.org/683813004/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/autofill_options_browsertest.js:198: phoneList.blur(); On 2014/11/11 23:43:24, bondd wrote: > The way that InlineEditableItem triggers validation changed and it broke this > test. > "input->phoneList" fixes the test. > > The purpose of this test is to verify that validation is triggered when text is > changed. Validation was previously triggered when the InlineEditableItem was > blurred. Now it is triggered when the lead item changes or the entire list > gains/loses focus. The InlineEditableItem cannot be blurred without one of those > two other events happening (as far as I know). I changed the test to blur the > entire list rather than just the item. this does nothing afaict: http://jsfiddle.net/6pwo54co/ > > I considered adding an additional test for changing the lead item, but the main > purpose of this existing test is to verify the validation-specific code in > autofill_options_list.js, not to verify details of inline_editable_item.js. So I > think this solution is reasonable as-is. what breaks and how was this supposed to fix?
https://codereview.chromium.org/683813004/diff/220001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/autofill_options_browsertest.js (right): https://codereview.chromium.org/683813004/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/autofill_options_browsertest.js:198: phoneList.blur(); On 2014/11/11 23:59:29, Dan Beam wrote: > On 2014/11/11 23:43:24, bondd wrote: > > The way that InlineEditableItem triggers validation changed and it broke this > > test. > > "input->phoneList" fixes the test. > > > > The purpose of this test is to verify that validation is triggered when text > is > > changed. Validation was previously triggered when the InlineEditableItem was > > blurred. Now it is triggered when the lead item changes or the entire list > > gains/loses focus. The InlineEditableItem cannot be blurred without one of > those > > two other events happening (as far as I know). I changed the test to blur the > > entire list rather than just the item. > > this does nothing afaict: http://jsfiddle.net/6pwo54co/ > > > > > I considered adding an additional test for changing the lead item, but the > main > > purpose of this existing test is to verify the validation-specific code in > > autofill_options_list.js, not to verify details of inline_editable_item.js. So > I > > think this solution is reasonable as-is. > > what breaks and how was this supposed to fix? What breaks: doneValidating() never fires, so test times out. How this fixes it: removes focus from list. InlineEditableItemList.handleListFocusChange_ gets called and causes "commitedit" event, resulting in validation happening.
https://codereview.chromium.org/683813004/diff/220001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/autofill_options_browsertest.js (right): https://codereview.chromium.org/683813004/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/autofill_options_browsertest.js:198: phoneList.blur(); On 2014/11/12 00:43:56, bondd wrote: > On 2014/11/11 23:59:29, Dan Beam wrote: > > On 2014/11/11 23:43:24, bondd wrote: > > > The way that InlineEditableItem triggers validation changed and it broke > this > > > test. > > > "input->phoneList" fixes the test. > > > > > > The purpose of this test is to verify that validation is triggered when text > > is > > > changed. Validation was previously triggered when the InlineEditableItem was > > > blurred. Now it is triggered when the lead item changes or the entire list > > > gains/loses focus. The InlineEditableItem cannot be blurred without one of > > those > > > two other events happening (as far as I know). I changed the test to blur > the > > > entire list rather than just the item. > > > > this does nothing afaict: http://jsfiddle.net/6pwo54co/ > > > > > > > > I considered adding an additional test for changing the lead item, but the > > main > > > purpose of this existing test is to verify the validation-specific code in > > > autofill_options_list.js, not to verify details of inline_editable_item.js. > So > > I > > > think this solution is reasonable as-is. > > > > what breaks and how was this supposed to fix? > > What breaks: doneValidating() never fires, so test times out. > > How this fixes it: removes focus from list. that doesn't happen because you can't blur() an element that doesn't have focus. what actually happens is that a synthetic 'blur' event triggers this code: https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... > InlineEditableItemList.handleListFocusChange_ gets called and causes > "commitedit" event, resulting in validation happening.
I'm ok with having a browser_tests only pak file, so gyp/gn changes lgtm.
https://codereview.chromium.org/683813004/diff/220001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/autofill_options_browsertest.js (right): https://codereview.chromium.org/683813004/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/autofill_options_browsertest.js:198: phoneList.blur(); On 2014/11/12 01:02:28, Dan Beam wrote: > On 2014/11/12 00:43:56, bondd wrote: > > On 2014/11/11 23:59:29, Dan Beam wrote: > > > On 2014/11/11 23:43:24, bondd wrote: > > > > The way that InlineEditableItem triggers validation changed and it broke > > this > > > > test. > > > > "input->phoneList" fixes the test. > > > > > > > > The purpose of this test is to verify that validation is triggered when > text > > > is > > > > changed. Validation was previously triggered when the InlineEditableItem > was > > > > blurred. Now it is triggered when the lead item changes or the entire list > > > > gains/loses focus. The InlineEditableItem cannot be blurred without one of > > > those > > > > two other events happening (as far as I know). I changed the test to blur > > the > > > > entire list rather than just the item. > > > > > > this does nothing afaict: http://jsfiddle.net/6pwo54co/ > > > > > > > > > > > I considered adding an additional test for changing the lead item, but the > > > main > > > > purpose of this existing test is to verify the validation-specific code in > > > > autofill_options_list.js, not to verify details of > inline_editable_item.js. > > So > > > I > > > > think this solution is reasonable as-is. > > > > > > what breaks and how was this supposed to fix? > > > > What breaks: doneValidating() never fires, so test times out. > > > > How this fixes it: removes focus from list. > > that doesn't happen because you can't blur() an element that doesn't have focus. > what actually happens is that a synthetic 'blur' event triggers this code: > https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... > > > InlineEditableItemList.handleListFocusChange_ gets called and causes > > "commitedit" event, resulting in validation happening. nothing is real, this code makes no sense. lgtm
The CQ bit was checked by bondd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/683813004/240001
Message was sent while issue was closed.
Committed patchset #11 (id:240001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/3df55f37eb3bcc896cbeab78f4769849d446b5c1 Cr-Commit-Position: refs/heads/master@{#303849} |
