Bug 178022 - Web Inspector: [PARITY] Styles Redesign: clicking on the white space after the property should create a blank property
Summary: Web Inspector: [PARITY] Styles Redesign: clicking on the white space after th...
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
: 178500 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-10-06 12:18 PDT by Nikita Vasilyev
Modified: 2017-10-30 01:43 PDT (History)
5 users (show)

See Also:


Attachments
Patch (15.80 KB, patch)
2017-10-27 00:59 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
[Animated GIF] With patch applied (169.59 KB, image/gif)
2017-10-27 00:59 PDT, Nikita Vasilyev
no flags Details
Patch (17.99 KB, patch)
2017-10-27 17:34 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Patch (18.19 KB, patch)
2017-10-27 18:48 PDT, Nikita Vasilyev
mattbaker: review-
Details | Formatted Diff | Diff
Patch (19.98 KB, patch)
2017-10-29 20:20 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
[Animated GIF] With patch applied (164.96 KB, image/gif)
2017-10-29 20:26 PDT, Nikita Vasilyev
no flags Details
[Animated GIF] Checking/unchecking new property behavior (159.53 KB, image/gif)
2017-10-30 01:00 PDT, Matt Baker
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2017-10-06 12:18:28 PDT
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.
Comment 1 Radar WebKit Bug Importer 2017-10-06 12:18:43 PDT
<rdar://problem/34861687>
Comment 2 Nikita Vasilyev 2017-10-18 19:21:57 PDT
*** Bug 178500 has been marked as a duplicate of this bug. ***
Comment 3 Nikita Vasilyev 2017-10-27 00:59:18 PDT
Created attachment 325134 [details]
Patch
Comment 4 Nikita Vasilyev 2017-10-27 00:59:46 PDT
Created attachment 325135 [details]
[Animated GIF] With patch applied
Comment 5 Nikita Vasilyev 2017-10-27 17:34:18 PDT
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.
Comment 6 Nikita Vasilyev 2017-10-27 18:48:13 PDT
Created attachment 325230 [details]
Patch

Rebaselined.
Comment 7 Matt Baker 2017-10-28 23:05:04 PDT
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.
Comment 8 Nikita Vasilyev 2017-10-29 14:15:29 PDT
(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.
Comment 9 Nikita Vasilyev 2017-10-29 20:20:42 PDT
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.
Comment 10 Nikita Vasilyev 2017-10-29 20:26:21 PDT
Created attachment 325317 [details]
[Animated GIF] With patch applied
Comment 11 Matt Baker 2017-10-30 00:59:45 PDT
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.
Comment 12 Matt Baker 2017-10-30 01:00:43 PDT
Created attachment 325337 [details]
[Animated GIF] Checking/unchecking new property behavior
Comment 13 Nikita Vasilyev 2017-10-30 01:12:04 PDT
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?" :)
Comment 14 Matt Baker 2017-10-30 01:24:07 PDT
Ah okay then!
Comment 15 Matt Baker 2017-10-30 01:24:52 PDT
Comment on attachment 325315 [details]
Patch

r=me 🇯🇵
Comment 16 WebKit Commit Bot 2017-10-30 01:43:56 PDT
Comment on attachment 325315 [details]
Patch

Clearing flags on attachment: 325315

Committed r224174: <https://trac.webkit.org/changeset/224174>
Comment 17 WebKit Commit Bot 2017-10-30 01:43:58 PDT
All reviewed patches have been landed.  Closing bug.