Web Inspector: differentiate element highlight colors for margin and padding

[lensco]

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.

[matt]

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)]

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)]

Created attachment 104180 [details]
[PATCH] Suggested solution

[Alexander Pavlov (apavlov)]

Created attachment 104186 [details]
[PATCH] Altered hover backgrounds for the Metrics pane sections to follow the new highlight

[Alexander Pavlov (apavlov)]

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 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]

(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)]

Created attachment 104191 [details]
[PATCH] Compilability fixed

[WebKit Review Bot]

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)]

Created attachment 104312 [details]
[PATCH] Global initializers fix attempt

[Pavel Feldman]

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)]

Created attachment 104493 [details]
[PATCH] Comments addressed

[WebKit Review Bot]

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 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)]

Created attachment 104519 [details]
[PATCH] Comments addressed

[WebKit Review Bot]

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)]

Created attachment 104522 [details]
[PATCH] Compilability fixed for Mac

[WebKit Review Bot]

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)]

Created attachment 104665 [details]
[PATCH] Simplified box model parts highlighting

[WebKit Review Bot]

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 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 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 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 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)]

Created attachment 104676 [details]
[PATCH] Build fix

[Alexander Pavlov (apavlov)]

Created attachment 104677 [details]
[PATCH] Commented method removed

[WebKit Review Bot]

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 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)]

(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)]

Created attachment 104840 [details]
[PATCH] Protocol-related comments addressed

[Pavel Feldman]

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)]

Committed r93603: <http://trac.webkit.org/changeset/93603>

[Alexander Pavlov (apavlov)]

Mac build fix by pfeldman: http://trac.webkit.org/changeset/93604

[Jon Rimmer]

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]

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]

(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.