As per Matt Baker's suggestion on <https://webkit.org/b/32263#c6>.
Created attachment 340397 [details] Patch
Comment on attachment 340397 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340397&action=review Some preliminary feedback. > Source/JavaScriptCore/inspector/protocol/OverlayTypes.json:101 > + { "name": "shouldDrawRulers", "type": "boolean" } "showRulers", see the next comment. > Source/JavaScriptCore/inspector/protocol/Page.json:169 > + "name": "setShouldDrawRulers", What do you think about renaming this "setShowRulers", to better match "setShowPaintRects"? I don't think we use "should" in any of the protocol commands. I'd also be fine with "setRulersEnabled", but since it's a visual thing the verb "show" seems like a better fit. > Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:323 > +localizedStrings["Draw Rulers"] = "Draw Rulers"; "Show Rulers" better matches "Hide Rulers", even if you decide not to change the name of the protocol command. > Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:101 > + let items = [this._shouldDrawRulersButtonNavigationItem, this._showPrintStylesButtonNavigationItem, this._showsShadowDOMButtonNavigationItem]; This should be conditional on backend support: let items = [this._showPrintStylesButtonNavigationItem, this._showsShadowDOMButtonNavigationItem]; if (PageAgent.setShouldDrawRulers) items.unshift(this._shouldDrawRulersButtonNavigationItem); > Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:581 > + PageAgent.setShouldDrawRulers(this._shouldDrawRulersButtonNavigationItem.activated); This needs a compatibility check: // COMPATIBILITY (iOS 11.3) if (PageAgent.setShouldDrawRulers) ...
Created attachment 340454 [details] Patch
Created attachment 340456 [details] Patch Forgot to add Rulers.svg 😅
Comment on attachment 340456 [details] Patch r-, because rulers still show up when inspecting an element after turning rulers off. Otherwise it looks good.
(In reply to Matt Baker from comment #5) > Comment on attachment 340456 [details] > Patch > > r-, because rulers still show up when inspecting an element after turning > rulers off. Otherwise it looks good. I actually wanted to do it specifically this way. I see the "Show Rulers" button more like as a "keep them visible". I think we always want to show the rulers whenever an element is being highlighted on the page, so the button is more of a "always show them" or not. Maybe the wording could be improved 🤔
(In reply to Devin Rousso from comment #6) > (In reply to Matt Baker from comment #5) > > Comment on attachment 340456 [details] > > Patch > > > > r-, because rulers still show up when inspecting an element after turning > > rulers off. Otherwise it looks good. > I actually wanted to do it specifically this way. I see the "Show Rulers" > button more like as a "keep them visible". I think we always want to show > the rulers whenever an element is being highlighted on the page, so the > button is more of a "always show them" or not. Maybe the wording could be > improved 🤔 We might want to solicit more feedback. Personally I think this behavior is too subtle, and would prefer an option exist to turn rulers off completely when highlighting DOM elements. Both Chrome and FireFox allow this in their ruler implementations. Their behaviors do differ though. Once enabled, Chrome only shows rulers during element inspection, whereas in FireFox they are always shown. FWIW, I prefer the approach taken by FF.
Created attachment 340480 [details] Patch
Comment on attachment 340480 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340480&action=review r=me, with the one change I mentioned. After playing with this more, I really like that the zero label is shown along with the first major tick mark on scroll-back. It's definitely a nice touch. > Source/WebInspectorUI/UserInterface/Base/Setting.js:124 > + showRulers: new WI.Setting("show-rulers", true), This should be off by default. We can revisit this in the future as the feature matures.
Created attachment 340544 [details] Patch
The commit-queue encountered the following flaky tests while processing attachment 340544 [details]: media/modern-media-controls/tracks-panel/tracks-panel-up-click-over-media-does-not-dimiss-media-controls-when-media-is-playing.html bug 185709 (author: graouts@apple.com) The commit-queue is continuing to process your patch.
Comment on attachment 340544 [details] Patch Clearing flags on attachment: 340544 Committed r231881: <https://trac.webkit.org/changeset/231881>
All reviewed patches have been landed. Closing bug.
<rdar://problem/40317731>