- 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>
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 :/
Created attachment 325943 [details] Patch
Created attachment 325945 [details] [Animated GIF] With patch applied https://www.theverge.com with two invalid inline properties added.
(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).
(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.
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
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
Created attachment 325962 [details] Patch Unrelated test failure. Re-uploading the same patch.
(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.
Created attachment 325965 [details] Patch
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.
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.
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"));
Comment on attachment 326051 [details] Patch r=me provided you address Devin's requested changes.
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)
Created attachment 326158 [details] Patch
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.
Comment on attachment 326158 [details] Patch Let's continue discussing RTL in Bug 175357 - Web Inspector: Styles redesign: add RTL support.
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 🤦♂️
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 🤦♂️🤦♂️
Created attachment 326164 [details] Patch Setting r? since I didn't follow most of the Devin's comments.
Created attachment 326165 [details] Patch
The commit-queue encountered the following flaky tests while processing attachment 326165 [details]: svg/animations/svgtransform-animation-1.html bug 179354 (authors: ap@webkit.org, arv@chromium.org, krit@webkit.org, mark.lam@apple.com, and zimmermann@kde.org) The commit-queue is continuing to process your patch.
Comment on attachment 326165 [details] Patch Clearing flags on attachment: 326165 Committed r224523: <https://trac.webkit.org/changeset/224523>
All reviewed patches have been landed. Closing bug.
*** Bug 174338 has been marked as a duplicate of this bug. ***