Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2741863002:
DevTools: Focus background in Toolbars (Closed)
|
Created:
3 years, 9 months ago by einbinder Modified:
3 years, 8 months ago Reviewers:
pfeldman CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chromium-reviews, devtools-reviews_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, pfeldman Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: Focus indicators in Toolbars
This patch adds a light gray background to buttons and comboboxes. It adds a default focus ring to checkboxes. These only appear when an item is focused via keyboard.
http://i.imgur.com/E6j7851.gif
BUG=706699
Review-Url: https://codereview.chromium.org/2741863002
Cr-Commit-Position: refs/heads/master@{#461007}
Committed: https://chromium.googlesource.com/chromium/src/+/4485d3c0dda30d0355fc7086bba089555e604f7a
Patch Set 1 #Patch Set 2 : raf #
Total comments: 12
Patch Set 3 : remove host-context #
Total comments: 4
Patch Set 4 : clean up code #
Messages
Total messages: 19 (6 generated)
Description was changed from ========== DevTools: Focus background in Toolbars BUG= ========== to ========== DevTools: Focus indicators in Toolbars This patch adds a light gray background to buttons and comboboxes. It adds a default focus ring to checkboxes. These only appear when an item is focused via keyboard. BUG=none ==========
einbinder@chromium.org changed reviewers: + pfeldman@chromium.org
ptal https://codereview.chromium.org/2741863002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (left): https://codereview.chromium.org/2741863002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:1420: set visualizeFocus(focus) { This was used to selectively add focus rings to checkboxes. I don't think there is ever a time we don't want a focus ring on checkboxes.
https://codereview.chromium.org/2741863002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js (right): https://codereview.chromium.org/2741863002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js:838: this._selectElement.addEventListener('focus', () => this.element.classList.add('focused')); why not :focused pseudo? https://codereview.chromium.org/2741863002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (right): https://codereview.chromium.org/2741863002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:788: document.body.classList.toggle('keyboard-focus', UI._keyboardFocus); What is this about? https://codereview.chromium.org/2741863002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:1189: document.defaultView.requestAnimationFrame(() => UI._keyboardFocus = false); Seems like technical debt.
https://codereview.chromium.org/2741863002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js (right): https://codereview.chromium.org/2741863002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js:838: this._selectElement.addEventListener('focus', () => this.element.classList.add('focused')); On 2017/03/27 at 21:17:35, pfeldman wrote: > why not :focused pseudo? this._selectElement has focus, but this.element needs the class. https://codereview.chromium.org/2741863002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (right): https://codereview.chromium.org/2741863002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:788: document.body.classList.toggle('keyboard-focus', UI._keyboardFocus); On 2017/03/27 at 21:17:35, pfeldman wrote: > What is this about? To distinguish between tabbing into an element vs clicking on it. https://codereview.chromium.org/2741863002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:1189: document.defaultView.requestAnimationFrame(() => UI._keyboardFocus = false); On 2017/03/27 at 21:17:35, pfeldman wrote: > Seems like technical debt. It is a hack, but it felt like the cleanest hack.
https://codereview.chromium.org/2741863002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (right): https://codereview.chromium.org/2741863002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:1189: document.defaultView.requestAnimationFrame(() => UI._keyboardFocus = false); Let me try to narrate this code: - every time key is down, for example, 'tab', you are settingUI._keyboardFocus to true for exactly one render frame. - if within this render frame, focus changed event is dispatched, you set 'keyboard-focus' class on body - after that, until another key is down, this 'keyboard-focus' class stays on. Few things I don't understand here: - what if i do multiple mouse clicks after that key down? You say it helps disambiguating clicks from tabs, but I don't see now. - focus event bubbling does not correlate with render frames, you mean to simply reset the flag after keydown is dispatched via microtask (Promise.resolve().then(() => UI._keyboardFocus = false)?
https://codereview.chromium.org/2741863002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (right): https://codereview.chromium.org/2741863002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:1189: document.defaultView.requestAnimationFrame(() => UI._keyboardFocus = false); > - every time key is down, for example, 'tab', you are settingUI._keyboardFocus to true for exactly one render frame. > - if within this render frame, focus changed event is dispatched, you set 'keyboard-focus' class on body > - after that, until another key is down, this 'keyboard-focus' class stays on. The keyboard-focus class stays on until the focus is changed, not until the next key is pressed. > - what if i do multiple mouse clicks after that key down? You say it helps disambiguating clicks from tabs, but I don't see now. If the mouse clicks cause the focus to change, then the keyboard-focus class will be removed. > - focus event bubbling does not correlate with render frames, you mean to simply reset the flag after keydown is dispatched via microtask (Promise.resolve().then(() => UI._keyboardFocus = false)? It doesn't perfectly match either. We have code that might cause focus to change after a Promise.resolve. I feel that a key event happening in the same render frame has focus change is a good indication that the key event caused the focus change.
https://codereview.chromium.org/2741863002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (right): https://codereview.chromium.org/2741863002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:788: document.body.classList.toggle('keyboard-focus', UI._keyboardFocus); Somehow I don't like storing the 'last focus was gained by means of key event' bit of information by means of the class on the document body. How do you use it? Does it pierce shadow DOM? https://codereview.chromium.org/2741863002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:1189: document.defaultView.requestAnimationFrame(() => UI._keyboardFocus = false); > It doesn't perfectly match either. We have code that might cause focus to change > after a Promise.resolve. I feel that a key event happening in the same render > frame has focus change is a good indication that the key event caused the focus > change. This blurs the contract you are introducing. Code could do a setTimeout or RAF or protocol roundtrip or whatever. I would understand marking a focus even object with the "keyboardFocus" marker, the result clients would be responsible for.
https://codereview.chromium.org/2741863002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/toolbar.css (right): https://codereview.chromium.org/2741863002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/toolbar.css:272: :host-context(.keyboard-focus) .toolbar-button:focus::after, host-context is for theming only, that's rather something that should be solved using inspectorCommon.css
https://codereview.chromium.org/2741863002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js (left): https://codereview.chromium.org/2741863002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js:748: if (!this._triggerTimeout) Why did this change? https://codereview.chromium.org/2741863002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js (right): https://codereview.chromium.org/2741863002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js:837: Why did this change? https://codereview.chromium.org/2741863002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/toolbar.css (right): https://codereview.chromium.org/2741863002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/toolbar.css:291: :host-context(.keyboard-focus) .focused:not(.toolbar-input)::after{ I don't think you are being entirely honest with us.
https://codereview.chromium.org/2741863002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js (right): https://codereview.chromium.org/2741863002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js:837: On 2017/03/29 at 21:54:20, pfeldman wrote: > Why did this change? You caught me before I looked over the diff.
Description was changed from ========== DevTools: Focus indicators in Toolbars This patch adds a light gray background to buttons and comboboxes. It adds a default focus ring to checkboxes. These only appear when an item is focused via keyboard. BUG=none ========== to ========== DevTools: Focus indicators in Toolbars This patch adds a light gray background to buttons and comboboxes. It adds a default focus ring to checkboxes. These only appear when an item is focused via keyboard. http://i.imgur.com/E6j7851.gif BUG=706699 ==========
ptal
lgtm
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...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490921214284960,
"parent_rev": "903171556a94ceb362dc1681684aa9c82ba41a9b", "commit_rev":
"4485d3c0dda30d0355fc7086bba089555e604f7a"}
Message was sent while issue was closed.
Description was changed from ========== DevTools: Focus indicators in Toolbars This patch adds a light gray background to buttons and comboboxes. It adds a default focus ring to checkboxes. These only appear when an item is focused via keyboard. http://i.imgur.com/E6j7851.gif BUG=706699 ========== to ========== DevTools: Focus indicators in Toolbars This patch adds a light gray background to buttons and comboboxes. It adds a default focus ring to checkboxes. These only appear when an item is focused via keyboard. http://i.imgur.com/E6j7851.gif BUG=706699 Review-Url: https://codereview.chromium.org/2741863002 Cr-Commit-Position: refs/heads/master@{#461007} Committed: https://chromium.googlesource.com/chromium/src/+/4485d3c0dda30d0355fc7086bba0... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/4485d3c0dda30d0355fc7086bba0... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
