[webkit-reviews] review denied: [Bug 181973] Web Inspector: Styles Redesign: ensure that tabbing through the last section wraps back to the first : [Attachment 335760] Patch

[bugzilla-daemon at webkit.org]

Matt Baker <mattbaker at apple.com> has denied Devin Rousso
<webkit at devinrousso.com>'s request for review:
Bug 181973: Web Inspector: Styles Redesign: ensure that tabbing through the
last section wraps back to the first
https://bugs.webkit.org/show_bug.cgi?id=181973

Attachment 335760: Patch

https://bugs.webkit.org/attachment.cgi?id=335760&action=review




--- Comment #16 from Matt Baker <mattbaker at apple.com> ---
Comment on attachment 335760
  --> https://bugs.webkit.org/attachment.cgi?id=335760
Patch

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

I think we should keep the checkbox tabbing behavior as-is. It's true it isn't
needed when "All Controls" are made made tab-accessible via the system setting,
and ideally we'd match the system. But not knowing the system setting in the
frontend means we can't determine what to focus after tabbing forward from the
filter field, or backward from the Style Attribute {} field.

Still r-, because checking the last pseudo-class checkbox and pressing tab
skips the Style Attribute {} and focuses the next rule (body). I can reproduce
this regardless of my system setting.

> Source/WebInspectorUI/ChangeLog:9
> +	   simplicity.

This is true, but is just a detail of the patch and not the main focus.
Mentioning the user-impact of the new tabbing behavior makes more sense as a
summary.

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:
247
> +   
spreadsheetCSSStyleDeclarationSectionStartEditingAdjacentRule(currentSection,
delta)

It's weird that here we take a delta, and in
cssStyleDeclarationTextEditorStartEditingAdjacentRule we take a bool indicating
"backward". Personally I don't like the delta, unless we plan to pass a value
other than 1 or -1 some day. Is there a reason this can't just take a bool?