Bug 192266 - Web Inspector: Styles: can't select properties of read-only rules
Summary: Web Inspector: Styles: can't select properties of read-only rules
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-11-30 16:16 PST by Nikita Vasilyev
Modified: 2018-12-03 12:37 PST (History)
5 users (show)

See Also:


Attachments
Patch (6.08 KB, patch)
2018-11-30 16:50 PST, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
[Animated GIF] With patch applied (37.50 KB, image/gif)
2018-11-30 16:54 PST, Nikita Vasilyev
no flags Details
Patch (10.46 KB, patch)
2018-11-30 17:22 PST, Nikita Vasilyev
hi: review+
Details | Formatted Diff | Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (2.41 MB, application/zip)
2018-12-01 03:21 PST, EWS Watchlist
no flags Details
Patch (10.47 KB, patch)
2018-12-03 12:10 PST, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2018-11-30 16:16:57 PST
Multiple properties selection only works for editable rules right now. It should work for read-only rules, too.
Comment 1 Nikita Vasilyev 2018-11-30 16:50:26 PST
Created attachment 356262 [details]
Patch
Comment 2 Nikita Vasilyev 2018-11-30 16:54:16 PST
Created attachment 356263 [details]
[Animated GIF] With patch applied
Comment 3 Nikita Vasilyev 2018-11-30 17:22:38 PST
Created attachment 356269 [details]
Patch
Comment 4 Radar WebKit Bug Importer 2018-11-30 17:42:20 PST
<rdar://problem/46390417>
Comment 5 EWS Watchlist 2018-12-01 03:21:10 PST
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
Comment 6 EWS Watchlist 2018-12-01 03:21:12 PST
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
Comment 7 Nikita Vasilyev 2018-12-01 08:57:20 PST
Comment on attachment 356269 [details]
Patch

Unrelated test failure.
Comment 8 Devin Rousso 2018-12-02 20:26:15 PST
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.
Comment 9 Nikita Vasilyev 2018-12-03 12:05:46 PST
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.
Comment 10 Nikita Vasilyev 2018-12-03 12:10:44 PST
Created attachment 356391 [details]
Patch
Comment 11 WebKit Commit Bot 2018-12-03 12:37:17 PST
Comment on attachment 356391 [details]
Patch

Clearing flags on attachment: 356391

Committed r238813: <https://trac.webkit.org/changeset/238813>
Comment 12 WebKit Commit Bot 2018-12-03 12:37:19 PST
All reviewed patches have been landed.  Closing bug.