Bug 148254 – Web Inspector: Add warnings to section that require specific values of other properties

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

As an example, the values in the "Flexbox" section require "display: flex;" to work.  Adding small notices that state this would be helpful in the case that the user is adding properties and not seeing any result.

[Radar WebKit Bug Importer]

<rdar://problem/22369751>

[Devin Rousso]

Created attachment 268125 [details]
Patch

[Blaze Burg]

Comment on attachment 268125 [details]
Patch

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

This is awesome. I would appreciate before/after screenshots for posterity, and it would make it easier to give design feedback.

For example, I wonder whether we should dim/ignore inputs to dependent properties and add a hyperlink that changes the value to something reasonable, so the user doesn't have to backtrack. But, it may be obvious enough with your visual design that this is unnecessary.

> Source/WebInspectorUI/ChangeLog:17
> +        (WebInspector.VisualStyleDetailsPanel.prototype._populateFlexboxSection):

Please explain a little bit why some of the events, etc are no longer necessary.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleDetailsPanel.js:432
> +        let positionDependencyValues = ["relative", "absolute", "fixed", "-webkit-sticky"];

I would prefer naming more along the lines of:

let allowedPositionValues = [...];
properties.zIndex.enableIfPropertyHasValue("position", allowedPositionValues)

I think it's fine that either parameter can be singular or plural; it's way more descriptive than 'addDependency'.

> Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.js:413
> +            this._dependencies.set(property, propertyValues || []);

Please default this above, like for propertyNames.

[Blaze Burg]

Comment on attachment 268125 [details]
Patch

Something that is probably a followup bug, is detecting similar situations where properties don't do anything (i.e., vertical-align in an inline context, top/left without out-of-flow parent, etc). This is probably harder to detect since inline and block contexts are not always easy to infer from the computed style.

[Devin Rousso]

Created attachment 268164 [details]
After Patch is Applied

[Devin Rousso]

Comment on attachment 268164 [details]
After Patch is Applied

(In reply to comment #3)
> This is awesome. I would appreciate before/after screenshots for posterity,
> and it would make it easier to give design feedback.

Sorry I forgot to upload them :P

> For example, I wonder whether we should dim/ignore inputs to dependent
> properties and add a hyperlink that changes the value to something
> reasonable, so the user doesn't have to backtrack. But, it may be obvious
> enough with your visual design that this is unnecessary.

So I would rather not change the appearance of any of the editors if they are missing a dependency, because it is also possible that a :hover will apply the necessary dependency (like going from "display: none;" to "display: flex;" on :hover).  I just want to inform the user that this won't actually do anything unless the required property has a value that works.

[Timothy Hatcher]

Comment on attachment 268125 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.js:546
> +                title += "\n " + property.name + ": " + dependencyValues.join("/");

It is a little weird to show raw CSS here when the visual sidebar shows prettified keywords and values. It might cause the user to get lost trying to mentally map back to the names used i the visual sidebar (especially once localization is used for labels but not values).

> Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.js:550
> +        this._warningElement.title = !!title.length ? WebInspector.UIString("Missing Dependencies (Computed):%s").format(title) : null;

What does "(Computed)" try to communicate here? I am not sure it is needed.

[Devin Rousso]

Comment on attachment 268125 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.js:546
>> +                title += "\n " + property.name + ": " + dependencyValues.join("/");
> 
> It is a little weird to show raw CSS here when the visual sidebar shows prettified keywords and values. It might cause the user to get lost trying to mentally map back to the names used i the visual sidebar (especially once localization is used for labels but not values).

That is a very good point.  I'm not sure how to go about future-proofing this though...maybe use UIString for the name and value(s) when they are passed in via addDependency() ?

>> Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.js:550
>> +        this._warningElement.title = !!title.length ? WebInspector.UIString("Missing Dependencies (Computed):%s").format(title) : null;
> 
> What does "(Computed)" try to communicate here? I am not sure it is needed.

The idea behind the (Computed) was to try to explain that the dependency is based off of the final, aka computed, value of the CSS property.  Essentially, you don't have to have "display: flex;" in the same rule so long as it is set in one of the other rules and is not overridden (hence it is the Computed value).

[Timothy Hatcher]

Comment on attachment 268125 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.js:546
>>> +                title += "\n " + property.name + ": " + dependencyValues.join("/");
>> 
>> It is a little weird to show raw CSS here when the visual sidebar shows prettified keywords and values. It might cause the user to get lost trying to mentally map back to the names used i the visual sidebar (especially once localization is used for labels but not values).
> 
> That is a very good point.  I'm not sure how to go about future-proofing this though...maybe use UIString for the name and value(s) when they are passed in via addDependency() ?

Only property names are localized now. The keyword values are not, but the values are prettified with spaces and title case. You would need to make a global lookup map for this. This can land as-is. I think there are other tooltips that have the same issue.

>>> Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.js:550
>>> +        this._warningElement.title = !!title.length ? WebInspector.UIString("Missing Dependencies (Computed):%s").format(title) : null;
>> 
>> What does "(Computed)" try to communicate here? I am not sure it is needed.
> 
> The idea behind the (Computed) was to try to explain that the dependency is based off of the final, aka computed, value of the CSS property.  Essentially, you don't have to have "display: flex;" in the same rule so long as it is set in one of the other rules and is not overridden (hence it is the Computed value).

I think we can drop (Computed) then.

[Devin Rousso]

Created attachment 268481 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 268481 [details]
Patch

Rejecting attachment 268481 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 268481, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
5a845be809694d2a2d3564da3e4db9a299c
r194723 = a2b0d9948cf25af5fd569fd4f61b67395a1330db
r194725 = 19ab57dde83f40920c3eeaf4fb0e23afb78b1084
r194726 = 0c0c2eeb5734d1c94f24e01beb06b30f37d1ae1a
r194727 = fdf1507a9bd16159a20825dd6acaba6b332d437d
r194728 = 98af212366ebe4d6d545bb91b531751c851c6813
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: http://webkit-queues.webkit.org/results/664028

[Devin Rousso]

Created attachment 268500 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 268500 [details]
Patch

Clearing flags on attachment: 268500

Committed r194737: <http://trac.webkit.org/changeset/194737>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.