[webkit-reviews] review granted: [Bug 177711] Web Inspector: Styles Redesign: Add support for keyboard navigation (Tab, Shift-Tab, Enter, Esc) : [Attachment 322932] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Oct 5 18:36:29 PDT 2017


Joseph Pecoraro <joepeck at webkit.org> has granted Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 177711: Web Inspector: Styles Redesign: Add support for keyboard navigation
(Tab, Shift-Tab, Enter, Esc)
https://bugs.webkit.org/show_bug.cgi?id=177711

Attachment 322932: Patch

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




--- Comment #11 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 322932
  --> https://bugs.webkit.org/attachment.cgi?id=322932
Patch

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

r=me.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetSelectorField.js:85
> +    _handleClick(event)

What happens if you Right Click? (event.button !== 1)

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetSelectorField.js:100
> +	       this._delegate.spreadsheetSelectorFieldDidChange();

Might be worth passing `null` here as a sign that you know you are sending null
as the direction.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:1
> +/*

Much nicer having these classes in their own file!

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:26
> +WI.SpreadsheetTextField = class SpreadsheetTextField

I think the SpreadsheetSelectorField and SpreadsheetTextField should eventually
be merged. They are mostly identical so it is just confusing to have two
classes that do nearly the same thing.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:51
> +    get element() { return this._element; }

Style: I'd recommend putting element at the top of the list of getters. Its
normally the one people look for things that have an element.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:55
> +	   if (this._editing) return;

Style: put the return on a newline.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:111
> +    _handleClick(event)
> +    {
> +	   if (!this._editing)
> +	       return;
> +
> +	   this.startEditing();
> +    }

This doesn't appear to do anything.

    • if you are not editing it returns
    • if you are editing it returns immediately in startEditing

One possible solution is to remove this and the event registration.
Another is to actually make this startEditing if we aren't editing (like the
SelectorField does).

Which should this do?

What happens if you Right Click? (event.button !== 1)


More information about the webkit-reviews mailing list