RESOLVED FIXED 37768
Web Inspector: differentiate element highlight colors for margin and padding
https://bugs.webkit.org/show_bug.cgi?id=37768
Summary Web Inspector: differentiate element highlight colors for margin and padding
lensco
Reported 2010-04-18 00:56:33 PDT
The way an element on page is highlighted could be improved by using different colors for padding and margin, so you can easily discern them. Also, don't use borders for the highlight, they are very confusing. Perhaps it would be best, for continuity's sake, to adopt the Firebug way of highlighting: yellow overlay for margins, purple overlay for padding.
Attachments
[PATCH] Suggested solution (14.88 KB, patch)
2011-08-17 09:54 PDT, Alexander Pavlov (apavlov)
no flags
[PATCH] Altered hover backgrounds for the Metrics pane sections to follow the new highlight (16.18 KB, patch)
2011-08-17 10:05 PDT, Alexander Pavlov (apavlov)
webkit.review.bot: commit-queue-
[IMAGE] Screenshot of the new tooltip (101.53 KB, image/png)
2011-08-17 10:08 PDT, Alexander Pavlov (apavlov)
no flags
[PATCH] Compilability fixed (15.93 KB, patch)
2011-08-17 10:44 PDT, Alexander Pavlov (apavlov)
webkit.review.bot: commit-queue-
[PATCH] Global initializers fix attempt (16.17 KB, patch)
2011-08-18 01:04 PDT, Alexander Pavlov (apavlov)
pfeldman: review-
[PATCH] Comments addressed (44.66 KB, patch)
2011-08-19 05:14 PDT, Alexander Pavlov (apavlov)
pfeldman: review-
webkit.review.bot: commit-queue-
[PATCH] Comments addressed (43.76 KB, patch)
2011-08-19 10:14 PDT, Alexander Pavlov (apavlov)
webkit.review.bot: commit-queue-
[PATCH] Compilability fixed for Mac (43.76 KB, patch)
2011-08-19 10:28 PDT, Alexander Pavlov (apavlov)
webkit.review.bot: commit-queue-
[PATCH] Simplified box model parts highlighting (45.46 KB, patch)
2011-08-22 05:59 PDT, Alexander Pavlov (apavlov)
webkit.review.bot: commit-queue-
[PATCH] Build fix (46.12 KB, patch)
2011-08-22 07:24 PDT, Alexander Pavlov (apavlov)
no flags
[PATCH] Commented method removed (45.28 KB, patch)
2011-08-22 07:26 PDT, Alexander Pavlov (apavlov)
pfeldman: review-
[PATCH] Protocol-related comments addressed (51.75 KB, patch)
2011-08-23 06:51 PDT, Alexander Pavlov (apavlov)
pfeldman: review+
matt
Comment 1 2011-03-17 06:25:28 PDT
I would also really like to see this go into the web inspector. I think the yellow for margin, blue for padding color scheme that Firebug introduced is standard in a lot of peoples minds including mine. This simple change could go a long way in improving the usability of the web inspector.
Alexander Pavlov (apavlov)
Comment 2 2011-08-17 08:47:58 PDT
A Chromium UI guy suggests the following colors: margin: rgba(246, 178, 107, .66) overlay, rgb(180, 95, 4) border border: rgba(255, 229, 153, .66) overlay, rgb(127, 96, 0) border padding: rgba(147, 196, 125, .55) overlay, rgb(55, 118, 28) border content: rgba(111, 168, 220, .66) overlay, rgb(9, 83, 148) border
Alexander Pavlov (apavlov)
Comment 3 2011-08-17 09:54:05 PDT
Created attachment 104180 [details] [PATCH] Suggested solution
Alexander Pavlov (apavlov)
Comment 4 2011-08-17 10:05:24 PDT
Created attachment 104186 [details] [PATCH] Altered hover backgrounds for the Metrics pane sections to follow the new highlight
Alexander Pavlov (apavlov)
Comment 5 2011-08-17 10:08:23 PDT
Created attachment 104188 [details] [IMAGE] Screenshot of the new tooltip The tooltip has received an arrow which moves to the left or right edge of the tooltip if the related element is beyond the left or right edge of the viewport, respectively.
WebKit Review Bot
Comment 6 2011-08-17 10:12:35 PDT
Comment on attachment 104186 [details] [PATCH] Altered hover backgrounds for the Metrics pane sections to follow the new highlight Attachment 104186 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9419027
lensco
Comment 7 2011-08-17 10:32:42 PDT
(In reply to comment #2) > A Chromium UI guy suggests the following colors: > > margin: rgba(246, 178, 107, .66) overlay, rgb(180, 95, 4) border > border: rgba(255, 229, 153, .66) overlay, rgb(127, 96, 0) border > padding: rgba(147, 196, 125, .55) overlay, rgb(55, 118, 28) border > content: rgba(111, 168, 220, .66) overlay, rgb(9, 83, 148) border Still think the borders make it more confusing...
Alexander Pavlov (apavlov)
Comment 8 2011-08-17 10:44:51 PDT
Created attachment 104191 [details] [PATCH] Compilability fixed
WebKit Review Bot
Comment 9 2011-08-17 11:09:35 PDT
Comment on attachment 104191 [details] [PATCH] Compilability fixed Attachment 104191 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9404986
Alexander Pavlov (apavlov)
Comment 10 2011-08-18 01:04:58 PDT
Created attachment 104312 [details] [PATCH] Global initializers fix attempt
Pavel Feldman
Comment 11 2011-08-18 02:21:41 PDT
Comment on attachment 104312 [details] [PATCH] Global initializers fix attempt View in context: https://bugs.webkit.org/attachment.cgi?id=104312&action=review > Source/WebCore/inspector/DOMNodeHighlighter.cpp:110 > + DEFINE_STATIC_LOCAL(Color, contentBoxColor, (111, 168, 220, 168)); Defining the {r:,g:,b:,a:} structure and passing it into highlightNode as highlightNode({marginColor: {}, borderOutlineColor: {}, etc...) highlightRect({color:, outlineColor}) > Source/WebCore/inspector/DOMNodeHighlighter.cpp:243 > + if (!fixedFontFamily.isEmpty()) { Consider extracting method > Source/WebCore/inspector/front-end/inspector.css:1892 > + background-color: rgba(246, 178, 107, .66); You could set these from the JavaScript code (along with the highligthNode calls).
Alexander Pavlov (apavlov)
Comment 12 2011-08-19 05:14:02 PDT
Created attachment 104493 [details] [PATCH] Comments addressed
WebKit Review Bot
Comment 13 2011-08-19 06:18:55 PDT
Comment on attachment 104493 [details] [PATCH] Comments addressed Attachment 104493 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9432243
Pavel Feldman
Comment 14 2011-08-19 06:35:45 PDT
Comment on attachment 104493 [details] [PATCH] Comments addressed View in context: https://bugs.webkit.org/attachment.cgi?id=104493&action=review > Source/WebCore/inspector/DOMNodeHighlighter.cpp:186 > + FontFamily* currFamily = 0; currentFamily > Source/WebCore/inspector/DOMNodeHighlighter.h:42 > +struct HighlightColors { I'd rather make it a struct with non-const fields. > Source/WebCore/inspector/DOMNodeHighlighter.h:73 > + const bool hasContent; Why do you need these? > Source/WebCore/inspector/Inspector.json:1063 > + { "name": "frameId", "type": "string", "description": "Identifier of the frame to highlight" }, No need to highlight frame's box model, make it similar to rect. > Source/WebCore/inspector/InspectorDOMAgent.cpp:101 > +Color parseColor(const RefPtr<InspectorObject>* color) colorObject > Source/WebCore/inspector/InspectorDOMAgent.cpp:114 > + bool success = (*color)->getNumber(red, &r); Just inline "r" > Source/WebCore/inspector/InspectorDOMAgent.cpp:125 > + return Color(r, g, b, static_cast<int>(a * 255)); what if a == 5 ? > Source/WebCore/inspector/InspectorDOMAgent.cpp:1050 > +void InspectorDOMAgent::highlightNode(ErrorString* error, int nodeId, const RefPtr<InspectorObject>* contentColor, const RefPtr<InspectorObject>* contentOutlineColor, const RefPtr<InspectorObject>* paddingColor, const RefPtr<InspectorObject>* paddingOutlineColor, const RefPtr<InspectorObject>* borderColor, const RefPtr<InspectorObject>* borderOutlineColor, const RefPtr<InspectorObject>* marginColor, const RefPtr<InspectorObject>* marginOutlineColor) Could you format parameters as a column? > Source/WebCore/inspector/front-end/Color.js:171 > + if ("_protocolRGBA" in this) this._protocolRGBA > Source/WebCore/inspector/front-end/Color.js:681 > + Content: new WebInspector.Color("rgba(111, 168, 220, .66)"), You should define these by components. > Source/WebCore/inspector/front-end/inspector.js:420 > + DOMAgent.highlightNode.apply(DOMAgent, highlightArgs); Please use new DOMAgent.highlightNode.invoke({}) notation.
Alexander Pavlov (apavlov)
Comment 15 2011-08-19 10:14:26 PDT
Created attachment 104519 [details] [PATCH] Comments addressed
WebKit Review Bot
Comment 16 2011-08-19 10:22:39 PDT
Comment on attachment 104519 [details] [PATCH] Comments addressed Attachment 104519 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9438368
Alexander Pavlov (apavlov)
Comment 17 2011-08-19 10:28:55 PDT
Created attachment 104522 [details] [PATCH] Compilability fixed for Mac
WebKit Review Bot
Comment 18 2011-08-19 11:38:47 PDT
Comment on attachment 104522 [details] [PATCH] Compilability fixed for Mac Attachment 104522 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9432392
Alexander Pavlov (apavlov)
Comment 19 2011-08-22 05:59:27 PDT
Created attachment 104665 [details] [PATCH] Simplified box model parts highlighting
WebKit Review Bot
Comment 20 2011-08-22 06:02:57 PDT
Comment on attachment 104665 [details] [PATCH] Simplified box model parts highlighting Attachment 104665 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9461805
Gyuyoung Kim
Comment 21 2011-08-22 06:03:46 PDT
Comment on attachment 104665 [details] [PATCH] Simplified box model parts highlighting Attachment 104665 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9466725
WebKit Review Bot
Comment 22 2011-08-22 06:06:41 PDT
Comment on attachment 104665 [details] [PATCH] Simplified box model parts highlighting Attachment 104665 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9465746
Early Warning System Bot
Comment 23 2011-08-22 06:08:04 PDT
Comment on attachment 104665 [details] [PATCH] Simplified box model parts highlighting Attachment 104665 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9467716
Collabora GTK+ EWS bot
Comment 24 2011-08-22 06:08:24 PDT
Comment on attachment 104665 [details] [PATCH] Simplified box model parts highlighting Attachment 104665 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9466727
Alexander Pavlov (apavlov)
Comment 25 2011-08-22 07:24:47 PDT
Created attachment 104676 [details] [PATCH] Build fix
Alexander Pavlov (apavlov)
Comment 26 2011-08-22 07:26:58 PDT
Created attachment 104677 [details] [PATCH] Commented method removed
WebKit Review Bot
Comment 27 2011-08-22 07:29:01 PDT
Attachment 104676 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/inspector/DOMNodeHighlighter.cpp:72: Should have a space between // and comment [whitespace/comments] [4] Source/WebCore/inspector/DOMNodeHighlighter.cpp:73: Should have a space between // and comment [whitespace/comments] [4] Source/WebCore/inspector/DOMNodeHighlighter.cpp:96: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 3 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Pavel Feldman
Comment 28 2011-08-23 01:40:58 PDT
Comment on attachment 104677 [details] [PATCH] Commented method removed View in context: https://bugs.webkit.org/attachment.cgi?id=104677&action=review > Source/WebCore/inspector/DOMNodeHighlighter.cpp:119 > + if (hasBorder && (!hasPadding || borderQuad != paddingQuad)) { I'd rather get rid of this logic. > Source/WebCore/inspector/Inspector.json:1064 > + { "name": "showInfo", "type": "boolean", "description": "Whether the node info tooltip should be shown." }, optional: true, false by default. > Source/WebCore/inspector/InspectorDOMAgent.cpp:1089 > + m_highlightData = adoptPtr(new HighlightData()); Could you make node and rect belong to this data as well?
Alexander Pavlov (apavlov)
Comment 29 2011-08-23 06:50:31 PDT
(In reply to comment #28) > (From update of attachment 104677 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=104677&action=review > > > Source/WebCore/inspector/DOMNodeHighlighter.cpp:119 > > + if (hasBorder && (!hasPadding || borderQuad != paddingQuad)) { > > I'd rather get rid of this logic. This is not so easy without having a separate highlighting mode, leaving as is for now, as discussed offline. > > Source/WebCore/inspector/Inspector.json:1064 > > + { "name": "showInfo", "type": "boolean", "description": "Whether the node info tooltip should be shown." }, > > optional: true, false by default. Done. > > Source/WebCore/inspector/InspectorDOMAgent.cpp:1089 > > + m_highlightData = adoptPtr(new HighlightData()); > > Could you make node and rect belong to this data as well? Done.
Alexander Pavlov (apavlov)
Comment 30 2011-08-23 06:51:18 PDT
Created attachment 104840 [details] [PATCH] Protocol-related comments addressed
Pavel Feldman
Comment 31 2011-08-23 06:58:55 PDT
Comment on attachment 104840 [details] [PATCH] Protocol-related comments addressed View in context: https://bugs.webkit.org/attachment.cgi?id=104840&action=review > Source/WebCore/inspector/Inspector.json:887 > + "id": "ElementHighlightConfig", BOxHighlightConfig
Alexander Pavlov (apavlov)
Comment 32 2011-08-23 07:39:18 PDT
Alexander Pavlov (apavlov)
Comment 33 2011-08-23 08:33:39 PDT
Mac build fix by pfeldman: http://trac.webkit.org/changeset/93604
Jon Rimmer
Comment 34 2011-08-30 01:06:27 PDT
From lensco's original bug report: "Also, don't use borders for the highlight, they are very confusing" There are still borders around the highlights, even though they make zero sense. So I don't see how this bug is really resolved?
Paul Irish
Comment 35 2011-08-31 14:51:22 PDT
An additional request that I've also received.. > visually show margins differently that have collapsed with other margins? Like maybe give the color a stripy pattern? Possible? Desired?
lensco
Comment 36 2011-09-01 05:37:09 PDT
(In reply to comment #35) > An additional request that I've also received.. > > > visually show margins differently that have collapsed with other margins? Like maybe give the color a stripy pattern? > > Possible? Desired? If possible, a striped pattern might make a lot of sense, and will help people grasp the idea of collapsed margins.
Note You need to log in before you can comment on or make changes to this bug.