Bug 179215 – Web Inspector: Styles Redesign: Display warnings

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

- Warning icons should NOT knock things out of alignment, like in the old styles sidebar.
- Show warning icons at the end, similar to inline errors/warnings in Resources.
- Allows showing a checkbox and warning/error at the same time. Makes sense for advisory warnings, but not for errors.

<rdar://problem/33948129>

[Nikita Vasilyev]

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

Scrollbar can cover the warning icons, making it hard to get the tooltips when hovering the icons :/

[Nikita Vasilyev]

Created attachment 325943 [details]
Patch

[Nikita Vasilyev]

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

https://www.theverge.com with two invalid inline properties added.

[Nikita Vasilyev]

(In reply to Nikita Vasilyev from comment #3)
> Created attachment 325945 [details]
> [Animated GIF] With patch applied
> 
> https://www.theverge.com with two invalid inline properties added.

The scrollbar partially covers the warning icon. This is the trade-off of having
the warning icons on the right side.

I think this is an improvement over no warning icons at all. However, I'd like to re-iterate on this when working on styling for overridden/unused/invalid properties (rdar://problem/35077806).

[Blaze Burg]

(In reply to Nikita Vasilyev from comment #4)
> (In reply to Nikita Vasilyev from comment #3)
> > Created attachment 325945 [details]
> > [Animated GIF] With patch applied
> > 
> > https://www.theverge.com with two invalid inline properties added.
> 
> The scrollbar partially covers the warning icon. This is the trade-off of
> having
> the warning icons on the right side.
> 
> I think this is an improvement over no warning icons at all. However, I'd
> like to re-iterate on this when working on styling for
> overridden/unused/invalid properties (rdar://problem/35077806).

I think the yellow background is enough for most people to notice it has a problem. It would be nice to show the message tooltip when hovering anywhere in the row, though.

[Build Bot]

Comment on attachment 325943 [details]
Patch

Attachment 325943 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5094850

New failing tests:
http/tests/workers/service/service-worker-clear.html

[Build Bot]

Created attachment 325961 [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.12.6

[Nikita Vasilyev]

Created attachment 325962 [details]
Patch

Unrelated test failure. Re-uploading the same patch.

[Nikita Vasilyev]

(In reply to Brian Burg from comment #5)
> (In reply to Nikita Vasilyev from comment #4)
> > (In reply to Nikita Vasilyev from comment #3)
> > > Created attachment 325945 [details]
> > > [Animated GIF] With patch applied
> > > 
> > > https://www.theverge.com with two invalid inline properties added.
> > 
> > The scrollbar partially covers the warning icon. This is the trade-off of
> > having
> > the warning icons on the right side.
> > 
> > I think this is an improvement over no warning icons at all. However, I'd
> > like to re-iterate on this when working on styling for
> > overridden/unused/invalid properties (rdar://problem/35077806).
> 
> I think the yellow background is enough for most people to notice it has a
> problem. It would be nice to show the message tooltip when hovering anywhere
> in the row, though.

I tried setting the title attribute on the property — it looked fine. I'll do that.

[Nikita Vasilyev]

Created attachment 325965 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 325965 [details]
Patch

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

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:509
> +localizedStrings["Invalid property name"] = "Invalid property name";
> +localizedStrings["Invalid property value"] = "Invalid property value";

Hmm, maybe "Invalid" should be "Unsupported"? A common case where we see this warning is for a property like "-webkit-text-size-adjust: none". That property name is not invalid, its just unsupported.

[Nikita Vasilyev]

Created attachment 326051 [details]
Patch

(In reply to Joseph Pecoraro from comment #11)
> Comment on attachment 325965 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=325965&action=review
> 
> > Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:509
> > +localizedStrings["Invalid property name"] = "Invalid property name";
> > +localizedStrings["Invalid property value"] = "Invalid property value";
> 
> Hmm, maybe "Invalid" should be "Unsupported"? A common case where we see
> this warning is for a property like "-webkit-text-size-adjust: none". That
> property name is not invalid, its just unsupported.

Makes sense. On production websites, we're more likely to see property names and values that are unsupported by this version of WebKit on this particular platform.

[Devin Rousso]

Comment on attachment 326051 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:33
> +    padding-right: var(--css-declaration-side-padding);
> +    padding-left: calc(var(--css-declaration-side-padding) + 17px);

-webkit-padding-start: calc(var(--css-declaration-side-padding) + 17px);
-webkit-padding-end: var(--css-declaration-side-padding);

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:63
> +    left: calc(var(--css-declaration-side-padding) + 1px);

Can you add RTL support for this?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:103
> +    right: 0;

Ditto (63).

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.css:114
> +    padding-left: var(--css-declaration-side-padding);

-webkit-padding-start: var(--css-declaration-side-padding);

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:114
>              if (!propertyNameIsValid || duplicatePropertyExistsBelow(this._property))

So does this mean that we only want to add title text if the property is valid?  Wouldn't it be pretty simple to add another title right here for duplicate properties?

    let isDuplicate = duplicatePropertyExistsBelow(this._property);
    if (!propertyIsValid || isDuplicate) {
        elementTitle = isDuplicate ? WI.UIString("Duplicate property") : WI.UIString("Unsupported property name");
        classNames.push("invalid-name");
    } else {
        elementTitle = WI.UIString("Unsupported property value");
        classNames.push("invalid-value");
    }

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:185
> +            this._warningElement = document.createElement("span");

this._warningElement = this.element.appendChild(document.createElement("span"));

[Blaze Burg]

Comment on attachment 326051 [details]
Patch

r=me provided you address Devin's requested changes.

[Blaze Burg]

Comment on attachment 326051 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.css:114
>> +    padding-left: var(--css-declaration-side-padding);
> 
> -webkit-padding-start: var(--css-declaration-side-padding);

This matches the opening brace as-is in RTL (i.e., both are equally screwed up due to WebKit's version of unicode, whose bidi algorithm flips braces incorrectly)

[Blaze Burg]

Created attachment 326158 [details]
Patch

[Nikita Vasilyev]

Created attachment 326161 [details]
[Image] With Brian's patch applied

I think it's strange to place checkboxes on the right side while everything else is aligned to left.

RTL in the new styles sidebar is basically broken. Before we do anything,
we should decide to either:
1. Keep styles sidebar always LTR. CSS code is strictly left to right.
2. Align text to right, flip property name and value fields.

[Nikita Vasilyev]

Comment on attachment 326158 [details]
Patch

Let's continue discussing RTL in Bug 175357 - Web Inspector: Styles redesign: add RTL support.

[Nikita Vasilyev]

Comment on attachment 326051 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:114
>>              if (!propertyNameIsValid || duplicatePropertyExistsBelow(this._property))
> 
> So does this mean that we only want to add title text if the property is valid?  Wouldn't it be pretty simple to add another title right here for duplicate properties?
> 
>     let isDuplicate = duplicatePropertyExistsBelow(this._property);
>     if (!propertyIsValid || isDuplicate) {
>         elementTitle = isDuplicate ? WI.UIString("Duplicate property") : WI.UIString("Unsupported property name");
>         classNames.push("invalid-name");
>     } else {
>         elementTitle = WI.UIString("Unsupported property value");
>         classNames.push("invalid-value");
>     }

My code only checks duplicate INVALID properties. This is wrong. I carried it over from _createTextMarkerForPropertyIfNeeded.prototype._createTextMarkerForPropertyIfNeeded 🤦‍♂️

[Nikita Vasilyev]

Comment on attachment 326051 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:114
>>>              if (!propertyNameIsValid || duplicatePropertyExistsBelow(this._property))
>> 
>> So does this mean that we only want to add title text if the property is valid?  Wouldn't it be pretty simple to add another title right here for duplicate properties?
>> 
>>     let isDuplicate = duplicatePropertyExistsBelow(this._property);
>>     if (!propertyIsValid || isDuplicate) {
>>         elementTitle = isDuplicate ? WI.UIString("Duplicate property") : WI.UIString("Unsupported property name");
>>         classNames.push("invalid-name");
>>     } else {
>>         elementTitle = WI.UIString("Unsupported property value");
>>         classNames.push("invalid-value");
>>     }
> 
> My code only checks duplicate INVALID properties. This is wrong. I carried it over from _createTextMarkerForPropertyIfNeeded.prototype._createTextMarkerForPropertyIfNeeded 🤦‍♂️

*from WI.CSSStyleDeclarationTextEditor.prototype._createTextMarkerForPropertyIfNeeded 🤦‍♂️🤦‍♂️

[Nikita Vasilyev]

Created attachment 326164 [details]
Patch

Setting r? since I didn't follow most of the Devin's comments.

[Nikita Vasilyev]

Created attachment 326165 [details]
Patch

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 326165 [details]:

svg/animations/svgtransform-animation-1.html bug 179354 (authors: [e-mail], [e-mail], [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 326165 [details]
Patch

Clearing flags on attachment: 326165

Committed r224523: <https://trac.webkit.org/changeset/224523>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Devin Rousso]

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