Bug 192266 – Web Inspector: Styles: can't select properties of read-only rules

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

Multiple properties selection only works for editable rules right now. It should work for read-only rules, too.

[Nikita Vasilyev]

Created attachment 356262 [details]
Patch

[Nikita Vasilyev]

Created attachment 356263 [details]
[Animated GIF] With patch applied

[Nikita Vasilyev]

Created attachment 356269 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/46390417>

[EWS Watchlist]

Comment on attachment 356269 [details]
Patch

Attachment 356269 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10228058

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/register-closed-window.https.html

[EWS Watchlist]

Created attachment 356305 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Nikita Vasilyev]

Comment on attachment 356269 [details]
Patch

Unrelated test failure.

[Devin Rousso]

Comment on attachment 356269 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:115
> +        if (this.hasSelectedProperties())
> +            this.selectProperties(this._anchorIndex, this._focusIndex);

Wouldn't this theoretically invalidate any blank property added by the previous line?  Should we assert that only one or the other is ever true?

    console.assert(!isNaN(this._pendingAddBlankPropertyIndexOffset) + this.hasSelectedProperties() < 2);

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:518
>          } else if (event.key === "Tab" || event.key === "Enter") {

Are we not adding tabbing support, or is that going to be in a different patch?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:53
> -        if (this._isEditable()) {
> +        if (!this._readOnly) {

Is this change needed for this patch?  It seems like both conditions would not pass for a computed property.

[Nikita Vasilyev]

Comment on attachment 356269 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:518
>>          } else if (event.key === "Tab" || event.key === "Enter") {
> 
> Are we not adding tabbing support, or is that going to be in a different patch?

It's going to be a different patch. In this patch, I'm not changing Tab/Enter behavior of editable rules.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:53
>> +        if (!this._readOnly) {
> 
> Is this change needed for this patch?  It seems like both conditions would not pass for a computed property.

Yes, for computed properties it would be false either way. This patch is intended to keep Computed panel work as before.

For read-only rules in Styles panel, this change is needed to make the selection work.

[Nikita Vasilyev]

Created attachment 356391 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 356391 [details]
Patch

Clearing flags on attachment: 356391

Committed r238813: <https://trac.webkit.org/changeset/238813>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.