Bug 185644 - Web Inspector: create a navigation item for toggling the overlay rulers/guides
Summary: Web Inspector: create a navigation item for toggling the overlay rulers/guides
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on: 32263
Blocks:
  Show dependency treegraph
 
Reported: 2018-05-15 01:11 PDT by Devin Rousso
Modified: 2018-05-16 18:38 PDT (History)
10 users (show)

See Also:


Attachments
Patch (16.08 KB, patch)
2018-05-15 01:17 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (26.65 KB, patch)
2018-05-15 18:07 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (27.61 KB, patch)
2018-05-15 18:10 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (27.56 KB, patch)
2018-05-16 01:43 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (26.65 KB, patch)
2018-05-16 17:46 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2018-05-15 01:11:13 PDT
As per Matt Baker's suggestion on <https://webkit.org/b/32263#c6>.
Comment 1 Devin Rousso 2018-05-15 01:17:20 PDT
Created attachment 340397 [details]
Patch
Comment 2 Matt Baker 2018-05-15 13:34:52 PDT
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)
   ...
Comment 3 Devin Rousso 2018-05-15 18:07:37 PDT
Created attachment 340454 [details]
Patch
Comment 4 Devin Rousso 2018-05-15 18:10:14 PDT
Created attachment 340456 [details]
Patch

Forgot to add Rulers.svg 😅
Comment 5 Matt Baker 2018-05-15 18:39:30 PDT
Comment on attachment 340456 [details]
Patch

r-, because rulers still show up when inspecting an element after turning rulers off. Otherwise it looks good.
Comment 6 Devin Rousso 2018-05-15 19:30:51 PDT
(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 🤔
Comment 7 Matt Baker 2018-05-15 21:46:07 PDT
(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.
Comment 8 Devin Rousso 2018-05-16 01:43:39 PDT
Created attachment 340480 [details]
Patch
Comment 9 Matt Baker 2018-05-16 12:44:19 PDT
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.
Comment 10 Devin Rousso 2018-05-16 17:46:41 PDT
Created attachment 340544 [details]
Patch
Comment 11 WebKit Commit Bot 2018-05-16 18:37:03 PDT
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 12 WebKit Commit Bot 2018-05-16 18:37:38 PDT
Comment on attachment 340544 [details]
Patch

Clearing flags on attachment: 340544

Committed r231881: <https://trac.webkit.org/changeset/231881>
Comment 13 WebKit Commit Bot 2018-05-16 18:37:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Radar WebKit Bug Importer 2018-05-16 18:38:16 PDT
<rdar://problem/40317731>