Bug 178022 – Web Inspector: [PARITY] Styles Redesign: clicking on the white space after the property should create a blank property

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

div { [------------ 1]
      font-size: 14px; [2]
      color: #333; [--- 3]
    } [---------------- 3]


- Clicking on the area [1] should create a new blank property before "font-size: 14px;"
- Clicking on the area [2] should create a new blank property after "font-size: 14px;"
- Clicking on the area [3] should create a new blank property after "color: #333;"


Bug 177711 - Web Inspector: Styles Redesign: Add support for keyboard navigation (Tab, Shift-Tab, Enter, Esc)
implemented adding new properties when tabbing from the last property in a rule. This is currently the only
way to add properties.

[Radar WebKit Bug Importer]

<rdar://problem/34861687>

[Nikita Vasilyev]

*** Bug 178500 has been marked as a duplicate of this bug. ***

[Nikita Vasilyev]

Created attachment 325134 [details]
Patch

[Nikita Vasilyev]

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

[Nikita Vasilyev]

Created attachment 325224 [details]
Patch

In the previous patch I didn't address clicking on the whitespace before the first property:

(In reply to Nikita Vasilyev from comment #0)
> div { [------------ 1]
>       font-size: 14px; [2]
>       color: #333; [--- 3]
>     } [---------------- 3]
> 
> 
> - Clicking on the area [1] should create a new blank property before
> "font-size: 14px;"

It's addressed now.

[Nikita Vasilyev]

Created attachment 325230 [details]
Patch

Rebaselined.

[Matt Baker]

Comment on attachment 325230 [details]
Patch

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

r-, for the following issue:

Style Attribute [--- region 1 ---]
{  [--- region 2 ---]
    [✓] border: none;
}

Clicking in region 2 adds a new property after "border", instead of before. Clicking in region 1 has the expected behavior. Also, I noticed with this patch that the open brace is now on its own line. Was this a recent change to our design, or was it unintentional? Personally I prefer having the open brace on the same line as the selector. Having it on its own line wastes space, IMO.

Also, clicking in the space before and after the checkbox creates a new property after "border". My expectation was that the new property would be added before the "border" property. Chrome expands the hit region for the checkbox to include the surrounding whitespace, and we may want to do the same. This is an extreme edge case, so I don't think its necessary to address in this patch, unless its a simple change.

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:125
> +        let forceRemove = true;

const

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:345
> +            // Newly added properties are initially blank. It should be possible to remove them.

I don't think this comment is necessary. The call to _updateStyleText which sets forceRemove to true has an explanatory comment that suffices.

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:348
> +                let columnDelta = 0;

Both of these should be const.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:202
> +        let newlyAdded = this._valueTextField.startEditingValue === "";

Since `startEditingValue` is only accessed to test for the newly added property case, SpreadsheetTextField should just expose a boolean property that performs this test internally:

get isNewProperty()
{
    return this._startEditingValue === "";
}

`propertyWasAdded` could work as a name too.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:61
> +    get startEditingValue() { return this._startEditingValue; }

See my comment above regarding this property. The name of the backing member variable is a little confusing to me as well. I had to read the code to figure it out. Maybe `_valueBeforeEditing` would be better.

[Nikita Vasilyev]

(In reply to Matt Baker from comment #7)
> Comment on attachment 325230 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=325230&action=review
> 
> r-, for the following issue:
> 
> Style Attribute [--- region 1 ---]
> {  [--- region 2 ---]
>     [✓] border: none;
> }
> 
> Clicking in region 2 adds a new property after "border", instead of before.
> Clicking in region 1 has the expected behavior. Also, I noticed with this
> patch that the open brace is now on its own line. Was this a recent change
> to our design, or was it unintentional? Personally I prefer having the open
> brace on the same line as the selector. Having it on its own line wastes
> space, IMO.

Open brace should never be on its own line. This is my mistake.

> Also, clicking in the space before and after the checkbox creates a new
> property after "border". My expectation was that the new property would be
> added before the "border" property. Chrome expands the hit region for the
> checkbox to include the surrounding whitespace, and we may want to do the
> same. This is an extreme edge case, so I don't think its necessary to
> address in this patch, unless its a simple change.

Yes, I'd like to improve this as a follow up.

Joe also suggested to focus on "border" when clicking on ":",
and focus on "none" when clicking on ";".

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

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:202
>> +        let newlyAdded = this._valueTextField.startEditingValue === "";
> 
> Since `startEditingValue` is only accessed to test for the newly added property case, SpreadsheetTextField should just expose a boolean property that performs this test internally:
> 
> get isNewProperty()
> {
>     return this._startEditingValue === "";
> }
> 
> `propertyWasAdded` could work as a name too.

I'm planning to merge WI.SpreadsheetTextField and WI.SpreadsheetSelectorField into a single class. To do that, WI.SpreadsheetTextField shouldn't assume it's a CSS property.
There's currently one abstraction violation in _getCompletionPrefix, but I'm trying not to add more.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:61
>> +    get startEditingValue() { return this._startEditingValue; }
> 
> See my comment above regarding this property. The name of the backing member variable is a little confusing to me as well. I had to read the code to figure it out. Maybe `_valueBeforeEditing` would be better.

_valueBeforeEditing does sound better.



> 
> > Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:125
> > +        let forceRemove = true;
> 
> const
> 
> > Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:345
> > +            // Newly added properties are initially blank. It should be possible to remove them.
> 
> I don't think this comment is necessary. The call to _updateStyleText which
> sets forceRemove to true has an explanatory comment that suffices.
> 
> > Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:348
> > +                let columnDelta = 0;
> 
> Both of these should be const.
> 
> > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:202
> > +        let newlyAdded = this._valueTextField.startEditingValue === "";
> 
> Since `startEditingValue` is only accessed to test for the newly added
> property case, SpreadsheetTextField should just expose a boolean property
> that performs this test internally:
> 
> get isNewProperty()
> {
>     return this._startEditingValue === "";
> }
> 
> `propertyWasAdded` could work as a name too.
> 
> > Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:61
> > +    get startEditingValue() { return this._startEditingValue; }
> 
> See my comment above regarding this property. The name of the backing member
> variable is a little confusing to me as well. I had to read the code to
> figure it out. Maybe `_valueBeforeEditing` would be better.

[Nikita Vasilyev]

Created attachment 325315 [details]
Patch

The closing curly brace had to move to its own line when there're no properties.

    Style Attribute {
    }

Before this patch it was:

    Style Attribute {}

I intend to fix this as a follow up. I need to make a fair amount of changes to SpreadsheetCSSStyleDeclarationSection and SpreadsheetCSSStyleDeclarationEditor to make it work.

[Nikita Vasilyev]

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

[Matt Baker]

Comment on attachment 325315 [details]
Patch

r-, for some wacky behavior when toggling a newly added property via the checkbox (see attached GIF). Not sure if this is new with this revision of the patch, or if I just didn't catch it the first time.

[Matt Baker]

Created attachment 325337 [details]
[Animated GIF] Checking/unchecking new property behavior

[Nikita Vasilyev]

Comment on attachment 325315 [details]
Patch

(In reply to Matt Baker from comment #11)
> Comment on attachment 325315 [details]
> Patch
> 
> r-, for some wacky behavior when toggling a newly added property via the
> checkbox (see attached GIF). Not sure if this is new with this revision of
> the patch, or if I just didn't catch it the first time.

This wacky behavior is exactly what I addressed in Bug 178328 - Web Inspector: [PARITY] Styles Redesign: Ability to modify style attributes. 

Setting back to "r?" :)

[Matt Baker]

Ah okay then!

[Matt Baker]

Comment on attachment 325315 [details]
Patch

r=me 🇯🇵

[WebKit Commit Bot]

Comment on attachment 325315 [details]
Patch

Clearing flags on attachment: 325315

Committed r224174: <https://trac.webkit.org/changeset/224174>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.