Bug 239092

Summary:

REGRESSION (r266069): Web Inspector: Can't create new CSS properties for inherited rules

Product:

WebKit

Reporter:

Nikita Vasilyev <nvasilyev>

Component:

Web Inspector

Assignee:

Nikita Vasilyev <nvasilyev>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ews-watchlist, hi, inspector-bugzilla-changes, pangle, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	ews-feeder: commit-queue-
Patch	nvasilyev: review?, ews-feeder: commit-queue-

Nikita Vasilyev

Reported 2022-04-11 15:52:17 PDT

Steps: 1. Open https://webkit.org 2. Inspect <body> 3. In the right side bar scroll down to ":root" rule 4. Click on the white space to create a new CSS property Actual: Nothing happens. No uncaught exception either. Notes: Reproduces in STP 143.

Attachments
Patch (1.74 KB, patch) 2022-04-21 13:34 PDT, Nikita Vasilyev	ews-feeder: commit-queue-	Details Formatted Diff Diff
Patch (5.30 KB, patch) 2022-05-10 10:55 PDT, Nikita Vasilyev	nvasilyev: review? ews-feeder: commit-queue-	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2022-04-11 15:52:27 PDT

<rdar://problem/91593601>

Nikita Vasilyev

Comment 2 2022-04-13 11:16:12 PDT

Reproduces in r270001 and I can't easily bisect much further back. It has been broken for at least 17 months, it seems, which is surprising to me.

Nikita Vasilyev

Comment 3 2022-04-14 15:41:22 PDT

Broke in https://trac.webkit.org/changeset/266069/webkit

Nikita Vasilyev

Comment 4 2022-04-21 13:29:17 PDT

This broke because blank properties are not inherited. I think the reasonable way to fix this is to display modified properties (which includes blank properties) alongside inherited properties. This would allows to add a new property and it wouldn't disaster once it's added.

Nikita Vasilyev

Comment 5 2022-04-21 13:34:35 PDT

Created attachment 458087 [details] Patch

Devin Rousso

Comment 6 2022-04-21 13:58:03 PDT

Comment on attachment 458087 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458087&action=review > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:256 > + properties = properties.filter((property) => property.inherited || property.modified); wouldn't this also include any modified properties of any inherited rule? e.g. ``` <div> <span></span> </div> <style> div { color: red; } <style> ``` if I added a `display: block;` after the `color: red;`, that'd be a modified property, right?

Nikita Vasilyev

Comment 7 2022-04-21 14:18:44 PDT

Devin, yes, in your example, if you're inspecting <span>, you'd see both `color: red` and `display: block`. I think that modified CSS is likely relevant to the user and I don't see harm showing it. Also, it's better than some of the alternatives. I'd rather not make inherited rules read-only. I'd rather not roll back r266069. I'd rather not make an added properly disappear if it isn't inherited.

Devin Rousso

Comment 8 2022-04-21 14:31:25 PDT

(In reply to Nikita Vasilyev from comment #7) > I think that modified CSS is likely relevant to the user and I don't see harm showing it. I don't see how a property that is not inheritable is relevant. > Also, it's better than some of the alternatives. I'd rather not make inherited rules read-only. I'd rather not roll back r266069. I'd rather not make an added properly disappear if it isn't inherited. Why are those the only alternatives? We could just as easily add a `WI.CSSProperty.prototype.get isBlankProperty` that's set when creating the `WI.CSSProperty` inside `WI.CSSStyleDeclaration.prototype.newBlankProperty` and then unset in `WI.CSSProperty.prototype._updateStyleText` (or somewhere similar) if both the `name` and `rawValue` are set.

Nikita Vasilyev

Comment 9 2022-05-10 10:47:06 PDT

(In reply to Devin Rousso from comment #8) > ... We could just as easily add a > `WI.CSSProperty.prototype.get isBlankProperty` that's set when creating the > `WI.CSSProperty` inside `WI.CSSStyleDeclaration.prototype.newBlankProperty` > and then unset in `WI.CSSProperty.prototype._updateStyleText` (or somewhere > similar) if both the `name` and `rawValue` are set. We can't unset `isBlankProperty` in _updateStyleText because it would fire as soon as you type the first character of the CSSProperty value. It can be unset on blur event instead, I suppose.

Nikita Vasilyev

Comment 10 2022-05-10 10:55:59 PDT

Created attachment 459124 [details] Patch

Nikita Vasilyev

Comment 11 2022-05-10 10:58:48 PDT

I still like my first patch more. The new patch is more complex and likely more error prone. (In reply to Devin Rousso from comment #8) > (In reply to Nikita Vasilyev from comment #7) > > I think that modified CSS is likely relevant to the user and I don't see harm showing it. > I don't see how a property that is not inheritable is relevant. We are trying to display properties that are relevant to a web developer. Inherited properties are more relevant than non-inherited, it makes sense. I property that the web developer just added and is very likely to be relevant, too.

Nikita Vasilyev

Comment 12 2022-05-31 15:31:59 PDT

Pull request: https://github.com/WebKit/WebKit/pull/1193

EWS

Comment 13 2022-08-18 18:25:43 PDT

Committed 253576@main (544098974025): <https://commits.webkit.org/253576@main> Reviewed commits have been landed. Closing PR #1193 and removing active labels.

Note You need to log in before you can comment on or make changes to this bug.