Bug 37768

Summary: Web Inspector: differentiate element highlight colors for margin and padding
Product: WebKit Reporter: lensco <lensco>
Component: Web Inspector (Deprecated)Assignee: Alexander Pavlov (apavlov) <apavlov>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, bweinstein, dglazkov, efidler, gustavo.noronha, gustavo, joepeck, jon.rimmer, keishi, m4olivei, paulirish, pfeldman, pmuellr, rik, webkit.review.bot, xan.lopez, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 66779    
Bug Blocks:    
Attachments:
Description Flags
[PATCH] Suggested solution
none
[PATCH] Altered hover backgrounds for the Metrics pane sections to follow the new highlight
webkit.review.bot: commit-queue-
[IMAGE] Screenshot of the new tooltip
none
[PATCH] Compilability fixed
webkit.review.bot: commit-queue-
[PATCH] Global initializers fix attempt
pfeldman: review-
[PATCH] Comments addressed
pfeldman: review-, webkit.review.bot: commit-queue-
[PATCH] Comments addressed
webkit.review.bot: commit-queue-
[PATCH] Compilability fixed for Mac
webkit.review.bot: commit-queue-
[PATCH] Simplified box model parts highlighting
webkit.review.bot: commit-queue-
[PATCH] Build fix
none
[PATCH] Commented method removed
pfeldman: review-
[PATCH] Protocol-related comments addressed pfeldman: review+

Description lensco 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.
Comment 1 matt 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.
Comment 2 Alexander Pavlov (apavlov) 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
Comment 3 Alexander Pavlov (apavlov) 2011-08-17 09:54:05 PDT
Created attachment 104180 [details]
[PATCH] Suggested solution
Comment 4 Alexander Pavlov (apavlov) 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
Comment 5 Alexander Pavlov (apavlov) 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.
Comment 6 WebKit Review Bot 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
Comment 7 lensco 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...
Comment 8 Alexander Pavlov (apavlov) 2011-08-17 10:44:51 PDT
Created attachment 104191 [details]
[PATCH] Compilability fixed
Comment 9 WebKit Review Bot 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
Comment 10 Alexander Pavlov (apavlov) 2011-08-18 01:04:58 PDT
Created attachment 104312 [details]
[PATCH] Global initializers fix attempt
Comment 11 Pavel Feldman 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).
Comment 12 Alexander Pavlov (apavlov) 2011-08-19 05:14:02 PDT
Created attachment 104493 [details]
[PATCH] Comments addressed
Comment 13 WebKit Review Bot 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
Comment 14 Pavel Feldman 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.
Comment 15 Alexander Pavlov (apavlov) 2011-08-19 10:14:26 PDT
Created attachment 104519 [details]
[PATCH] Comments addressed
Comment 16 WebKit Review Bot 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
Comment 17 Alexander Pavlov (apavlov) 2011-08-19 10:28:55 PDT
Created attachment 104522 [details]
[PATCH] Compilability fixed for Mac
Comment 18 WebKit Review Bot 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
Comment 19 Alexander Pavlov (apavlov) 2011-08-22 05:59:27 PDT
Created attachment 104665 [details]
[PATCH] Simplified box model parts highlighting
Comment 20 WebKit Review Bot 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
Comment 21 Gyuyoung Kim 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
Comment 22 WebKit Review Bot 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
Comment 23 Early Warning System Bot 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
Comment 24 Collabora GTK+ EWS bot 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
Comment 25 Alexander Pavlov (apavlov) 2011-08-22 07:24:47 PDT
Created attachment 104676 [details]
[PATCH] Build fix
Comment 26 Alexander Pavlov (apavlov) 2011-08-22 07:26:58 PDT
Created attachment 104677 [details]
[PATCH] Commented method removed
Comment 27 WebKit Review Bot 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.
Comment 28 Pavel Feldman 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?
Comment 29 Alexander Pavlov (apavlov) 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.
Comment 30 Alexander Pavlov (apavlov) 2011-08-23 06:51:18 PDT
Created attachment 104840 [details]
[PATCH] Protocol-related comments addressed
Comment 31 Pavel Feldman 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
Comment 32 Alexander Pavlov (apavlov) 2011-08-23 07:39:18 PDT
Committed r93603: <http://trac.webkit.org/changeset/93603>
Comment 33 Alexander Pavlov (apavlov) 2011-08-23 08:33:39 PDT
Mac build fix by pfeldman: http://trac.webkit.org/changeset/93604
Comment 34 Jon Rimmer 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?
Comment 35 Paul Irish 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?
Comment 36 lensco 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.