WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
[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-
Details
Formatted Diff
Diff
[IMAGE] Screenshot of the new tooltip
(101.53 KB, image/png)
2011-08-17 10:08 PDT
,
Alexander Pavlov (apavlov)
no flags
Details
[PATCH] Compilability fixed
(15.93 KB, patch)
2011-08-17 10:44 PDT
,
Alexander Pavlov (apavlov)
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
[PATCH] Global initializers fix attempt
(16.17 KB, patch)
2011-08-18 01:04 PDT
,
Alexander Pavlov (apavlov)
pfeldman
: review-
Details
Formatted Diff
Diff
[PATCH] Comments addressed
(44.66 KB, patch)
2011-08-19 05:14 PDT
,
Alexander Pavlov (apavlov)
pfeldman
: review-
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
[PATCH] Comments addressed
(43.76 KB, patch)
2011-08-19 10:14 PDT
,
Alexander Pavlov (apavlov)
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
[PATCH] Compilability fixed for Mac
(43.76 KB, patch)
2011-08-19 10:28 PDT
,
Alexander Pavlov (apavlov)
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
[PATCH] Simplified box model parts highlighting
(45.46 KB, patch)
2011-08-22 05:59 PDT
,
Alexander Pavlov (apavlov)
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
[PATCH] Build fix
(46.12 KB, patch)
2011-08-22 07:24 PDT
,
Alexander Pavlov (apavlov)
no flags
Details
Formatted Diff
Diff
[PATCH] Commented method removed
(45.28 KB, patch)
2011-08-22 07:26 PDT
,
Alexander Pavlov (apavlov)
pfeldman
: review-
Details
Formatted Diff
Diff
[PATCH] Protocol-related comments addressed
(51.75 KB, patch)
2011-08-23 06:51 PDT
,
Alexander Pavlov (apavlov)
pfeldman
: review+
Details
Formatted Diff
Diff
Show Obsolete
(10)
View All
Add attachment
proposed patch, testcase, etc.
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
Committed
r93603
: <
http://trac.webkit.org/changeset/93603
>
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.
Top of Page
Format For Printing
XML
Clone This Bug