Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2096183002:
DevTools: Added styling to focused checkboxes (Closed)
|
Created:
4 years, 6 months ago by flandy Modified:
4 years, 5 months ago CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, sergeyv+blink_chromium.org, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: Added styling to checkboxes so that a focus state is displayed when tabbing through the checkboxes (for keyboard accessibility).
BUG=580199
Committed: https://crrev.com/dbb205b9ea85ecde57547e2840825030e6375dd8
Cr-Commit-Position: refs/heads/master@{#405215}
Patch Set 1 #Patch Set 2 : Use better looking box-shadow instead of outline #Patch Set 3 : Use default outline and allow focus styling opt-in #
Total comments: 4
Patch Set 4 : Address comments #
Messages
Total messages: 36 (13 generated)
Description was changed from ========== added styling to checkbox focus BUG= ========== to ========== DevTools: Added styling to checkboxes so that a focus state is displayed when tabbing through the checkboxes (for keyboard accessibility). BUG=580199 ==========
flandy@google.com changed reviewers: + dgozman@chromium.org
Please take a look at this. All checkboxes, like those in ':hov' Toggle Element State as well as those listed in the bug, should now receive an outline when tabbed through.
On 2016/06/25 00:05:54, flandy wrote: > Please take a look at this. All checkboxes, like those in ':hov' Toggle Element > State as well as those listed in the bug, should now receive an outline when > tabbed through. We usually accompany UI changes with a screenshot attached to the issue. Could you please do that?
Sure thing! https://crbug.com/580199
lushnikov@chromium.org changed reviewers: + lushnikov@chromium.org
What happened to the default focus state rendering? AFAIK focused input controls have outline by default.
On 2016/06/27 23:07:13, flandy wrote: > Sure thing! > > https://crbug.com/580199 Thanks! Let's follow the chrome://settings style here. We try to follow it when dealing with standard controls if possible. The default one doesn't look that good.
On 2016/06/27 23:15:35, lushnikov wrote: > What happened to the default focus state rendering? AFAIK focused input controls > have outline by default. I'm not sure. It may have to do with registering a custom element for the Checkbox label. I will try to look into this more. On 2016/06/27 23:17:40, dgozman wrote: > Thanks! > > Let's follow the chrome://settings style here. We try to follow it when dealing > with standard controls if possible. The default one doesn't look that good. I agree the default one doesn't look that good.
What's the status of this?
On 2016/07/06 22:28:21, dgozman wrote: > What's the status of this? Paused to work on other project. Will resume shortly. We have to style a new checkbox from scratch in order to follow chrome://settings. On 2016/06/27 23:37:19, flandy wrote: > On 2016/06/27 23:15:35, lushnikov wrote: > > What happened to the default focus state rendering? AFAIK focused input > controls > > have outline by default. > > I'm not sure. It may have to do with registering a custom element for the > Checkbox label. I will try to look into this more. There's nothing wrong with custom elements, we just turn the default focus outline off.
Please take another look. I've added a box shadow for focus which looks better than the default outline. https://crbug.com/580199 This avoids some issues with the chrome://settings checkbox, such as using a check mark image that does not scale well when zooming in or look great with the dark theme. Note that the new box-shadow is applied both when clicking on and tabbing to a checkbox.
I'll maybe go with a thinner border of 1px instead of 2px, but overall looks good for me. I, however, would refrain from showing focus state for all checkboxes in the devtools. The focus state doesn't make sense for a lot of checkbox controls in our ui, e.g. Sources Panel would not benefit from this. Let's do focus state opt-in.
> I, however, would refrain from showing focus state for all checkboxes in the > devtools. The focus state doesn't make sense for a lot of checkbox controls in > our ui, e.g. Sources Panel would not benefit from this. Do we have many places where the focus by default is on the checkbox though?
> Do we have many places where the focus by default is on the checkbox though? I'm not sure if we have places where the focus by default is on the checkbox. In Patch Set 3, I've gone back to the default outline, as suggested in person, and added the ability to opt-in to focus styling. This way, the focus styling will only (currently) apply to the checkboxes in the elements classes pane widget, as requested in the bug.
lgtm https://codereview.chromium.org/2096183002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (right): https://codereview.chromium.org/2096183002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:1511: set focusVisible(focus) 1. let's make this a function (we tend to avoid setters/getters when possible) 2. let's name this function and classes as setVisualizeFocus/dt-checkbox-visualize-focus https://codereview.chromium.org/2096183002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:1514: this.checkboxElement.classList.add("dt-checkbox-focus-visible"); this "if" could be written in a one line: this.checkboxElement.classList.toggle("dt-checkbox-focus-visible", focus);
https://codereview.chromium.org/2096183002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (right): https://codereview.chromium.org/2096183002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:1511: set focusVisible(focus) On 2016/07/08 23:01:22, lushnikov wrote: > 1. let's make this a function (we tend to avoid setters/getters when possible) > 2. let's name this function and classes as > setVisualizeFocus/dt-checkbox-visualize-focus 1. Using a function causes an issue with the Closure Compiler since we are using custom HTML elements. Let's keep it as a setter like the surrounding code. 2. Done https://codereview.chromium.org/2096183002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:1514: this.checkboxElement.classList.add("dt-checkbox-focus-visible"); On 2016/07/08 23:01:22, lushnikov wrote: > this "if" could be written in a one line: > > this.checkboxElement.classList.toggle("dt-checkbox-focus-visible", focus); Done
The CQ bit was checked by lushnikov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lushnikov@chromium.org Link to the patchset: https://codereview.chromium.org/2096183002/#ps60001 (title: "Address comments")
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_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lushnikov@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 commit-bot@chromium.org
Exceeded global retry quota
The CQ bit was checked by flandy@google.com
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 flandy@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== DevTools: Added styling to checkboxes so that a focus state is displayed when tabbing through the checkboxes (for keyboard accessibility). BUG=580199 ========== to ========== DevTools: Added styling to checkboxes so that a focus state is displayed when tabbing through the checkboxes (for keyboard accessibility). BUG=580199 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== DevTools: Added styling to checkboxes so that a focus state is displayed when tabbing through the checkboxes (for keyboard accessibility). BUG=580199 ========== to ========== DevTools: Added styling to checkboxes so that a focus state is displayed when tabbing through the checkboxes (for keyboard accessibility). BUG=580199 Committed: https://crrev.com/dbb205b9ea85ecde57547e2840825030e6375dd8 Cr-Commit-Position: refs/heads/master@{#405215} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/dbb205b9ea85ecde57547e2840825030e6375dd8 Cr-Commit-Position: refs/heads/master@{#405215} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
