32263 – Web Inspector: Add rulers and guides

RESOLVED FIXED 32263

Web Inspector: Add rulers and guides

https://bugs.webkit.org/show_bug.cgi?id=32263

Summary Web Inspector: Add rulers and guides

Daniel Bates

Reported 2009-12-07 22:18:44 PST

We should show rulers and guides, similar to Firebug, so that you can visually see the dimensions of a highlighted node as will as see its position relative to its parent element/<body>.

Attachments
Patch (35.10 KB, patch) 2018-03-26 20:44 PDT, Devin Rousso	no flags	Details Formatted Diff Diff
[Image] After Patch is applied (167.71 KB, image/png) 2018-03-26 20:44 PDT, Devin Rousso	no flags	Details
[Image] Rulers.svg (708 bytes, image/svg+xml) 2018-05-14 19:33 PDT, Matt Baker	no flags	Details
[Image] Rulers button mockup (124.03 KB, image/png) 2018-05-14 19:34 PDT, Matt Baker	no flags	Details
Patch (32.89 KB, patch) 2018-05-15 00:46 PDT, Devin Rousso	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews204 for win-future (12.76 MB, application/zip) 2018-05-15 06:49 PDT, EWS Watchlist	no flags	Details
Patch (31.89 KB, patch) 2018-05-15 15:41 PDT, Devin Rousso	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Rob Colburn

Comment 1 2012-05-10 11:31:00 PDT

Is this complete now?

Radar WebKit Bug Importer

Comment 2 2014-12-17 11:24:06 PST

<rdar://problem/19281564>

Devin Rousso

Comment 3 2018-03-26 20:44:16 PDT

Created attachment 336565 [details] Patch

Devin Rousso

Comment 4 2018-03-26 20:44:31 PDT

Created attachment 336566 [details] [Image] After Patch is applied

Devin Rousso

Comment 5 2018-03-26 20:45:14 PDT

(In reply to Devin Rousso from comment #3) > Created attachment 336565 [details] > Patch I am not set in stone on the styling, as I really just wanted to see if I could get this working, so any feedback would be greatly appreciated.

Matt Baker

Comment 6 2018-05-14 19:33:09 PDT

Comment on attachment 336565 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336565&action=review Very cool. Also, nice work cleaning up the overlay code. I think after one more revision this should be ready to r+. Here are my general comments: *Style The rulers look great. Just a couple of small nits: - Remove the "0" from the origin. When the vertical ruler is scrolled (which will be more common than the other case), vertical tick labels can overlap with the origin label. - Since both rulers have an opacity, their intersection is less opaque. It would be nice to fix this, but can be done as follow-up/polish. * Add a Rulers Toggle A button in the Elements tab (see mockup) would be nice. Having them appear as an element is selected is distracting/surprising. FireFox has a global toggle, but having a button in the Elements tab makes more sense to me. As a follow-up we should show guides extending from the selected element to the rulers. I suspect you were already planning this. :) * Reduce Patch Churn A bunch of functions in InspectorOverlay.js got moved around, but weren't otherwise modified: log, showPageIndication, hidePageIndication, etc. Unless I'm missing something and there was a reason for this, this should be avoided to make reading the diff easier. > Source/WebCore/inspector/InspectorOverlay.cpp:740 > + .setDeviceScaleFactor(m_page.deviceScaleFactor()) Reduce churn: move this back above .setViewportSize > Source/WebCore/inspector/InspectorOverlayPage.js:440 > + var hasMargin = marginQuad && highlight.marginColor !== transparentColor; Use let throughout.

Matt Baker

Comment 7 2018-05-14 19:33:51 PDT

Created attachment 340386 [details] [Image] Rulers.svg

Matt Baker

Comment 8 2018-05-14 19:34:25 PDT

Created attachment 340387 [details] [Image] Rulers button mockup

Devin Rousso

Comment 9 2018-05-14 19:42:15 PDT

(In reply to Matt Baker from comment #6) > - Remove the "0" from the origin. When the vertical ruler is scrolled (which > will be more common than the other case), vertical tick labels can overlap > with the origin label. I mainly had this here for consistency, but I suppose that it's obvious. I'll remove it. > - Since both rulers have an opacity, their intersection is less opaque. It > would be nice to fix this, but can be done as follow-up/polish. I very specifically remember adding code to make it such that the overlap is the same opacity as the rulers themselves. Unless this has broken since I last checked, it should already be working. > A button in the Elements tab (see mockup) would be nice. Having them appear > as an element is selected is distracting/surprising. FireFox has a global > toggle, but having a button in the Elements tab makes more sense to me. I'm not sure I like the idea of making the rulers toggle-able, especially since we already have so many buttons in the DOM tree area. I think this may be better served as a followup, especially since it will involve greater changes to the protocol. > As a follow-up we should show guides extending from the selected element to > the rulers. I suspect you were already planning this. :) ( ͡° ͜ʖ ͡°) > A bunch of functions in InspectorOverlay.js got moved around, but weren't > otherwise modified: log, showPageIndication, hidePageIndication, etc. Unless > I'm missing something and there was a reason for this, this should be > avoided to make reading the diff easier. Personally, I found that this made the code overall so much cleaner. There were only a few functions that weren't touched by this patch, so I figured that it was fine. I can revert the changes to the ones that truly aren't though.

Matt Baker

Comment 10 2018-05-14 19:55:09 PDT

(In reply to Devin Rousso from comment #9) > (In reply to Matt Baker from comment #6) > > - Remove the "0" from the origin. When the vertical ruler is scrolled (which > > will be more common than the other case), vertical tick labels can overlap > > with the origin label. > I mainly had this here for consistency, but I suppose that it's obvious. > I'll remove it. > > > - Since both rulers have an opacity, their intersection is less opaque. It > > would be nice to fix this, but can be done as follow-up/polish. > I very specifically remember adding code to make it such that the overlap is > the same opacity as the rulers themselves. Unless this has broken since I > last checked, it should already be working. You're right, it looks good. I think I was noticing edge cases where the highlighted element or its margin/padding rects are drawn directly under the ruler's corner. > > A button in the Elements tab (see mockup) would be nice. Having them appear > > as an element is selected is distracting/surprising. FireFox has a global > > toggle, but having a button in the Elements tab makes more sense to me. > I'm not sure I like the idea of making the rulers toggle-able, especially > since we already have so many buttons in the DOM tree area. I think this > may be better served as a followup, especially since it will involve greater > changes to the protocol. Maybe I'd get used to it, but the flickering on/off of the rulers while hovering DOM nodes felt distracting. I also don't think we want this to be always on. I feel like a button is the way to go, but we could see what others think.

Matt Baker

Comment 11 2018-05-14 19:56:26 PDT

One more style suggestion: let's drop the first major tick mark from each ruler. They create a dark line flush with the edge of the page, and clutter up the corner when scrolled to 0, 0.

Devin Rousso

Comment 12 2018-05-15 00:46:32 PDT

Created attachment 340395 [details] Patch I've modified the first tick/label such that it doesn't display if the user isn't scrolled. This is so that if the user decides to over-scroll, we will still show something.

EWS Watchlist

Comment 13 2018-05-15 06:49:24 PDT

Comment on attachment 340395 [details] Patch Attachment 340395 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7688416 New failing tests: http/tests/preload/onload_event.html

EWS Watchlist

Comment 14 2018-05-15 06:49:35 PDT

Created attachment 340408 [details] Archive of layout-test-results from ews204 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews204 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

Matt Baker

Comment 15 2018-05-15 14:05:28 PDT

Comment on attachment 340395 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340395&action=review r=me, with some cleanup and behavior suggestions. Other than that, it looks great. Nice work. > Source/WebCore/inspector/InspectorOverlayPage.html:41 > + <div id="log"></div> Nice cleanup. > Source/WebCore/inspector/InspectorOverlayPage.js:69 > + mirrorY: bounds.minX < DATA.contentInset.width + gridSize && bounds.maxX < DATA.viewportSize.width - gridSize, I don't think we should ever mirror the ruler axis. I understand this is to prevent overlapping, but I think overlapping is acceptable. It was surprising to see the ruler move during element selection. At first I thought it had disappeared. > Source/WebCore/inspector/InspectorOverlayPage.js:222 > + context.lineWidth = 2; Make this magic number a named const with the others (gridSize, etc). > Source/WebCore/inspector/InspectorOverlayPage.js:387 > + context.strokeStyle = "rgb(128, 128, 128)"; We have some other color consts at the top of the file. Let's move these up there too. > Source/WebCore/inspector/InspectorOverlayPage.js:501 > + context.fillText(x, 2, options.mirrorX ? zoom(height) - DATA.contentInset.height - 7 : 13); Please name these constants. > Source/WebCore/inspector/InspectorOverlayPage.js:539 > + context.lineTo(zoom(x), scrollY + ((x % (gridStep * 2)) ? 5 : 8)); Please make these numbers named constants. They get used for both axes, so they should probably go above the "// Draw horizontal grid" comment.

Devin Rousso

Comment 16 2018-05-15 15:39:53 PDT

Comment on attachment 340395 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340395&action=review >> Source/WebCore/inspector/InspectorOverlayPage.js:222 >> + context.lineWidth = 2; > > Make this magic number a named const with the others (gridSize, etc). I personally don't think this needs to be a named const, considering that it's only really used once. Also, this is code that I didn't write, so I'd rather not touch it more than needed in this patch. I'm thinking I'll do a revamp of the overlay's code soon, so I'll address it then.

Devin Rousso

Comment 17 2018-05-15 15:41:24 PDT

Created attachment 340437 [details] Patch

WebKit Commit Bot

Comment 18 2018-05-15 16:20:51 PDT

Comment on attachment 340437 [details] Patch Clearing flags on attachment: 340437 Committed r231819: <https://trac.webkit.org/changeset/231819>

WebKit Commit Bot

Comment 19 2018-05-15 16:20:53 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware All

OS All

Product WebKit

Component Web Inspector

Assignee

Devin Rousso

Reported

2009-12-07 22:18 PST

Modified

2018-05-15 16:20 PDT History

CC List

19 users Show

URL

Keywords InRadar

Depends on

Blocks

185644

Dependencies

tree graph