Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2644233007:
DevTools: Use real focus in TreeOutline (Closed)
|
Created:
3 years, 11 months ago by einbinder Modified:
3 years, 10 months ago CC:
aboxhall, aboxhall+watch_chromium.org, apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chromium-reviews, devtools-reviews_chromium.org, dmazzoni, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, je_julie, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, nektarios, nektar+watch_chromium.org, pfeldman+blink_chromium.org, yuzo+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: Use real focus in TreeOutline
BUG=none
Review-Url: https://codereview.chromium.org/2644233007
Cr-Commit-Position: refs/heads/master@{#447899}
Committed: https://chromium.googlesource.com/chromium/src/+/75dc75e5f12aa17db7d46d999d3029e2c9658a84
Patch Set 1 #
Total comments: 13
Patch Set 2 : changes #
Total comments: 14
Patch Set 3 : restore focus from contentElement #Patch Set 4 : Add force white icons class on focus #Patch Set 5 : test results #Messages
Total messages: 36 (18 generated)
einbinder@chromium.org changed reviewers: + hans.hillen@gmail.com
ptal
pfeldman@chromium.org changed reviewers: + pfeldman@chromium.org
https://codereview.chromium.org/2644233007/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js (right): https://codereview.chromium.org/2644233007/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:828: treeOutline.focus(); nit: why did the tree lose focus? (seems fragile) https://codereview.chromium.org/2644233007/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.css (right): https://codereview.chromium.org/2644233007/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.css:96: background: unset; background is a shorthand, it should reset everything. background: none should suffice. https://codereview.chromium.org/2644233007/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (left): https://codereview.chromium.org/2644233007/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:1008: if (!this.treeOutline) I'm pretty sure this was happening :(, so removing the check is risky. https://codereview.chromium.org/2644233007/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (right): https://codereview.chromium.org/2644233007/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:138: this.contentElement.focus(); There is no tabIndex on contentElement, is it focusable? https://codereview.chromium.org/2644233007/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:150: element.tabIndex = -1; element does not have tabIndex property. use setAttribute for consistency and compilation-friendliness https://codereview.chromium.org/2644233007/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:200: if (!this.selectedTreeElement || event.target !== this.selectedTreeElement.listItemElement || event.shiftKey || listItemElement can have focusable children, if listeners don't consume their event, you should execute it (inplace editing). https://codereview.chromium.org/2644233007/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:987: this.treeOutline.focus(); Why going into treeOutline just to focus the node you have here?
General concern is around nested tree outlines - listItemElements could have nested focusable items in which case tab traversal within the tree becomes possible.
On 2017/01/21 at 04:58:56, pfeldman wrote: > General concern is around nested tree outlines - listItemElements could have nested focusable items in which case tab traversal within the tree becomes possible. I don't see how this changes tab traversal. If you had tab stops inside the TreeOutline, you would be able to get to them regardless of whether your initial focus is on the TreeOutline or one of its listItemElements.
https://codereview.chromium.org/2644233007/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.css (right): https://codereview.chromium.org/2644233007/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.css:96: background: unset; On 2017/01/21 at 04:57:34, pfeldman wrote: > background is a shorthand, it should reset everything. background: none should suffice. I copy-pasted this from the code in InspectorCommon.css. Maybe someone had some clever reason why it was this way? It's hard to check if the icons all look how they are supposed to. https://codereview.chromium.org/2644233007/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (left): https://codereview.chromium.org/2644233007/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:1008: if (!this.treeOutline) On 2017/01/21 at 04:57:34, pfeldman wrote: > I'm pretty sure this was happening :(, so removing the check is risky. I don't see how this can happen, it just calls focus. On itself. Nothing overrides TreeOutline focus anymore. But I agree that focusing the TreeElement directly will be less risky. https://codereview.chromium.org/2644233007/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (right): https://codereview.chromium.org/2644233007/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:138: this.contentElement.focus(); On 2017/01/21 at 04:57:34, pfeldman wrote: > There is no tabIndex on contentElement, is it focusable? Ah, it isn't. I think its ok for empty TreeOutlines to not be focusable. https://codereview.chromium.org/2644233007/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:150: element.tabIndex = -1; On 2017/01/21 at 04:57:34, pfeldman wrote: > element does not have tabIndex property. use setAttribute for consistency and compilation-friendliness Ok! https://codereview.chromium.org/2644233007/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:200: if (!this.selectedTreeElement || event.target !== this.selectedTreeElement.listItemElement || event.shiftKey || On 2017/01/21 at 04:57:34, pfeldman wrote: > listItemElement can have focusable children, if listeners don't consume their event, you should execute it (inplace editing). This was specifically to guard against that scenario. If you have an inplace editor, we don't want left and right arrow to move you in the treeoutline. https://codereview.chromium.org/2644233007/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:987: this.treeOutline.focus(); On 2017/01/21 at 04:57:34, pfeldman wrote: > Why going into treeOutline just to focus the node you have here? No great reason.
I Did keyboard testing, focus is correctly tracked in treeoutline. lgtm
https://codereview.chromium.org/2644233007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js (left): https://codereview.chromium.org/2644233007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:44: this.setFocusable(false); I like it the way it was https://codereview.chromium.org/2644233007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js (right): https://codereview.chromium.org/2644233007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:828: treeOutline.focus(); Why are we losing focus that this becomes necessary? Does navigator inplace editing work with no need for modifications? https://codereview.chromium.org/2644233007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.css (right): https://codereview.chromium.org/2644233007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.css:91: ol.tree-outline li.selected:focus [is=ui-icon].spritesheet-smallicons { lusha is replacing these with the use of UI.Icon all over the place. What is happening here, why is this only added and not removed anywhere? https://codereview.chromium.org/2644233007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (right): https://codereview.chromium.org/2644233007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:36: constructor(nonFocusable) { I'd remove this and only leave the setFocusable for the API sanity. Component would be focusable by default. https://codereview.chromium.org/2644233007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:139: if (this.selectedTreeElement) What if there is no selected element? Can there be no selected element btw? Previously we could rely upon the fact that treeoutline would pick focus, we no longer can. https://codereview.chromium.org/2644233007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:163: element.listItemElement.removeAttribute('tabIndex'); It seems like you are now balancing this in the select/deselect pair? https://codereview.chromium.org/2644233007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:1029: if (!this.treeOutline || this.treeOutline.selectedTreeElement !== this || !this.selected) You might want to remove the tabIndex unconditionally...
https://codereview.chromium.org/2644233007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js (left): https://codereview.chromium.org/2644233007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:44: this.setFocusable(false); On 2017/01/30 at 18:43:07, pfeldman wrote: > I like it the way it was Done. https://codereview.chromium.org/2644233007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js (right): https://codereview.chromium.org/2644233007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:828: treeOutline.focus(); On 2017/01/30 at 18:43:07, pfeldman wrote: > Why are we losing focus that this becomes necessary? Does navigator inplace editing work with no need for modifications? Removed, see https://codereview.chromium.org/2674453002 https://codereview.chromium.org/2644233007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.css (right): https://codereview.chromium.org/2644233007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.css:91: ol.tree-outline li.selected:focus [is=ui-icon].spritesheet-smallicons { On 2017/01/30 at 18:43:07, pfeldman wrote: > lusha is replacing these with the use of UI.Icon all over the place. What is happening here, why is this only added and not removed anywhere? I removed the focus event listener, which was adding a force-white-icons class. Without copying over the CSS, I'd have to juggle blur and focus event listeners on the selected TreeElements. Edit https://codereview.chromium.org/2644233007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (right): https://codereview.chromium.org/2644233007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:36: constructor(nonFocusable) { On 2017/01/30 at 18:43:07, pfeldman wrote: > I'd remove this and only leave the setFocusable for the API sanity. Component would be focusable by default. Done. https://codereview.chromium.org/2644233007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:139: if (this.selectedTreeElement) On 2017/01/30 at 18:43:07, pfeldman wrote: > What if there is no selected element? Can there be no selected element btw? Previously we could rely upon the fact that treeoutline would pick focus, we no longer can. Selecting the contentElement now, and restoring focus to/from it. https://codereview.chromium.org/2644233007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:163: element.listItemElement.removeAttribute('tabIndex'); On 2017/01/30 at 18:43:07, pfeldman wrote: > It seems like you are now balancing this in the select/deselect pair? Yep, that is better. https://codereview.chromium.org/2644233007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:1029: if (!this.treeOutline || this.treeOutline.selectedTreeElement !== this || !this.selected) On 2017/01/30 at 18:43:07, pfeldman wrote: > You might want to remove the tabIndex unconditionally... Done.
lgtm, please make sure this is in sync with Lusha's icon change. -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
lgtm, please make sure this is in sync with Lusha's icon change. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm
On 2017/02/01 at 22:59:54, pfeldman wrote: > lgtm, please make sure this is in sync with Lusha's icon change. > > -- > You received this message because you are subscribed to the Google Groups "Blink Reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org. > Done.
The CQ bit was checked by einbinder@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from Hans.Hillen@gmail.com, pfeldman@chromium.org Link to the patchset: https://codereview.chromium.org/2644233007/#ps60001 (title: "Add force white icons class on focus")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by einbinder@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by einbinder@chromium.org
The CQ bit was checked by einbinder@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by einbinder@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by einbinder@chromium.org
The CQ bit was checked by einbinder@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from Hans.Hillen@gmail.com, pfeldman@chromium.org Link to the patchset: https://codereview.chromium.org/2644233007/#ps80001 (title: "test results")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1486076076876370,
"parent_rev": "60bd8863c89b4058872e1323173fbc3802efbcfc", "commit_rev":
"75dc75e5f12aa17db7d46d999d3029e2c9658a84"}
Message was sent while issue was closed.
Description was changed from ========== DevTools: Use real focus in TreeOutline BUG=none ========== to ========== DevTools: Use real focus in TreeOutline BUG=none Review-Url: https://codereview.chromium.org/2644233007 Cr-Commit-Position: refs/heads/master@{#447899} Committed: https://chromium.googlesource.com/chromium/src/+/75dc75e5f12aa17db7d46d999d30... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/75dc75e5f12aa17db7d46d999d30... |
