Bug 185644 – Web Inspector: create a navigation item for toggling the overlay rulers/guides

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

As per Matt Baker's suggestion on <https://webkit.org/b/32263#c6>.

[Devin Rousso]

Created attachment 340397 [details]
Patch

[Matt Baker]

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)
   ...

[Devin Rousso]

Created attachment 340454 [details]
Patch

[Devin Rousso]

Created attachment 340456 [details]
Patch

Forgot to add Rulers.svg 😅

[Matt Baker]

Comment on attachment 340456 [details]
Patch

r-, because rulers still show up when inspecting an element after turning rulers off. Otherwise it looks good.

[Devin Rousso]

(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 🤔

[Matt Baker]

(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.

[Devin Rousso]

Created attachment 340480 [details]
Patch

[Matt Baker]

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.

[Devin Rousso]

Created attachment 340544 [details]
Patch

[WebKit Commit Bot]

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: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 340544 [details]
Patch

Clearing flags on attachment: 340544

Committed r231881: <https://trac.webkit.org/changeset/231881>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/40317731>