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.
<rdar://problem/22369751>
Created attachment 268125 [details] Patch
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.
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.
Created attachment 268164 [details] After Patch is Applied
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.
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.
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).
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.
Created attachment 268481 [details] Patch
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
Created attachment 268500 [details] Patch
Comment on attachment 268500 [details] Patch Clearing flags on attachment: 268500 Committed r194737: <http://trac.webkit.org/changeset/194737>
All reviewed patches have been landed. Closing bug.