Bug 32263

Summary:

Web Inspector: Add rulers and guides

Product:

WebKit

Reporter:

Daniel Bates <dbates>

Component:

Web Inspector

Assignee:

Devin Rousso <hi>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bburg, bweinstein, commit-queue, ericgwill, ews-watchlist, graouts, hi, inspector-bugzilla-changes, joepeck, jonowells, keith_miller, mark.lam, mattbaker, msaboff, pfeldman, robert.colburn+bugzilla, saam, webkit-bug-importer, yurys

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

185644

Attachments:

Description	Flags
Patch	none
[Image] After Patch is applied	none
[Image] Rulers.svg	none
[Image] Rulers button mockup	none
Patch	none
Archive of layout-test-results from ews204 for win-future	none
Patch	none

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.