Bug 191435 – Web Inspector: Styles: Command-A should select all properties

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

When focused on a property, Command-A should select all properties of the rule.

[Radar WebKit Bug Importer]

<rdar://problem/45921373>

[Nikita Vasilyev]

Created attachment 354267 [details]
Patch

[Nikita Vasilyev]

Created attachment 354271 [details]
Patch

[Devin Rousso]

Comment on attachment 354271 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:553
> +            if (!this._hasSelectedProperties() || this._propertyViews.length === 0)

This makes me think that we would only respond to this event if something is selected.  That doesn't make a lot of sense.  I think what we want to do is bail if any property is currently being edited.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:557
> +            this.selectProperties(0, this._propertyViews.length - 1);

It looks like every call to `selectProperties` is wrapped by `_suppressBlur`.  Maybe move those lines inside the function, so future callers won't need to think about that.

[Nikita Vasilyev]

Comment on attachment 354271 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:557
>> +            this.selectProperties(0, this._propertyViews.length - 1);
> 
> It looks like every call to `selectProperties` is wrapped by `_suppressBlur`.  Maybe move those lines inside the function, so future callers won't need to think about that.

Good catch! One call in spreadsheetCSSStyleDeclarationEditorPropertyMouseEnter wasn't wrapped by `_suppressBlur` but it was safe to move those lines inside the function anyway.

[Nikita Vasilyev]

Created attachment 354358 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:553
>> +            if (!this._hasSelectedProperties() || this._propertyViews.length === 0)
> 
> This makes me think that we would only respond to this event if something is selected.  That doesn't make a lot of sense.  I think what we want to do is bail if any property is currently being edited.

I think it's fine as it is. Currently, I only want Command-A to work when at least one property is selected. Property can't be selected when I edit a name or value.

I want to refactor `editing` getter because it isn't longer accurate but I don't want to do it in this patch.

[Nikita Vasilyev]

Created attachment 354599 [details]
Patch

I created "Bug 191567 - Web Inspector: Styles: SpreadsheetCSSStyleDeclarationEditor.prototype.editing getter is inaccurate",
and I left a comment in the code. I have a plan for refactoring, but it may break things and I'd hate to introduce a regression in the current style editor while working on the experimental feature.

[Nikita Vasilyev]

Created attachment 354612 [details]
Patch

[Devin Rousso]

Comment on attachment 354612 [details]
Patch

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

r=me, nice refactoring :)

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:559
> +            if (!this._hasSelectedProperties() || this._propertyViews.length === 0)

Style: `!this._propertyViews.length`

[Nikita Vasilyev]

Created attachment 354678 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 354678 [details]
Patch

Clearing flags on attachment: 354678

Committed r238135: <https://trac.webkit.org/changeset/238135>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.