Bug 225433

Summary: Web Inspector: Layout panel "Grid Overlays" main checkbox has dead space (no interaction) between checkbox and label text
Product: WebKit Reporter: Patrick Angle <pangle>
Component: Web InspectorAssignee: Nikita Vasilyev <nvasilyev>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, hi, inspector-bugzilla-changes, nvasilyev, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Screenshot illustrating issue
none
Patch
hi: review+, hi: commit-queue-
[Video] With patch applied
none
Patch ews-feeder: commit-queue-

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].