Bug 225433 - Web Inspector: Layout panel "Grid Overlays" main checkbox has dead space (no interaction) between checkbox and label text
Summary: Web Inspector: Layout panel "Grid Overlays" main checkbox has dead space (no ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-05 21:28 PDT by Patrick Angle
Modified: 2021-05-10 16:19 PDT (History)
5 users (show)

See Also:


Attachments
Screenshot illustrating issue (149.70 KB, image/png)
2021-05-05 21:28 PDT, Patrick Angle
no flags Details
Patch (10.89 KB, patch)
2021-05-06 15:10 PDT, Nikita Vasilyev
hi: review+
hi: commit-queue-
Details | Formatted Diff | Diff
[Video] With patch applied (2.07 MB, video/quicktime)
2021-05-06 15:10 PDT, Nikita Vasilyev
no flags Details
Patch (11.19 KB, patch)
2021-05-10 10:58 PDT, Nikita Vasilyev
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick Angle 2021-05-05 21:28:02 PDT
Created attachment 427848 [details]
Screenshot illustrating issue

The attached screenshot shows a small area where clicking on the "Grid Overlays" checkbox/label does not toggle the item. This is different from the below item for a single overlay, where that same gap space does toggle the item. System checkboxes treat that space as interactive, so our header's behavior is incorrect here.
Comment 1 Radar WebKit Bug Importer 2021-05-05 21:28:55 PDT
<rdar://problem/77590883>
Comment 2 Patrick Angle 2021-05-05 21:36:31 PDT
Comment on attachment 427848 [details]
Screenshot illustrating issue

This diagram is technically wrong, because the "Grid Overlays" hit box doesn't extend this far to the right, but the area of issue is still accurate.
Comment 3 Nikita Vasilyev 2021-05-06 15:10:23 PDT
Created attachment 427941 [details]
Patch
Comment 4 Nikita Vasilyev 2021-05-06 15:10:52 PDT
Created attachment 427942 [details]
[Video] With patch applied
Comment 5 Devin Rousso 2021-05-07 12:37:10 PDT
Comment on attachment 427941 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427941&action=review

r=me with some minor adjustments

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.css:69
> +.css-grid-section input[type="checkbox"] {

Can we be more specific about this selector?
```
    .css-grid-section :is(.setting-editor, .node-overlay-list-item-container) input[type="checkbox"]
```

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:90
> +        labelInner.append(WI.UIString("Grid Overlays", "Page Overlays @ Layout Sidebar Section Header", "Heading for list of grid nodes"));

`.textContent = `

> Source/WebInspectorUI/UserInterface/Views/SettingEditor.css:31
> +    padding-inline-start: 5px;

This used to also have `5px` of padding on the right too inside `WI.CSSGridSection`.  Do we not want that anymore?

Also, this is super NIT, but I personally think `4px` looks visually better 😅

> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.css:108
> +.content-view.tab.settings > .settings-view > .container > .editor-group > .setting-editor input {
>      font-size: inherit;

I feel like we should move more of the generic styles like this to `Source/WebInspectorUI/UserInterface/Views/SettingEditor.css`.  The only stuff that should be in this file are the styles that are necessary to make the Settings Tab look good.
Comment 6 Nikita Vasilyev 2021-05-10 10:57:39 PDT
Comment on attachment 427941 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427941&action=review

>> Source/WebInspectorUI/UserInterface/Views/SettingEditor.css:31
>> +    padding-inline-start: 5px;
> 
> This used to also have `5px` of padding on the right too inside `WI.CSSGridSection`.  Do we not want that anymore?

I don't think we do. I just checked macOS - it doesn't arbitrary increase the clickable area to the right of the label text.

I added 5px padding initially to separate label text and the color picker swatch, and it somehow creeped in to checkbox labels that don't have color picker swatches.
Comment 7 Nikita Vasilyev 2021-05-10 10:58:06 PDT
Created attachment 428184 [details]
Patch
Comment 8 EWS 2021-05-10 12:33:24 PDT
Committed r277284 (237548@main): <https://commits.webkit.org/237548@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 428184 [details].