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
lensco
2010-04-18 00:56:33 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. 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 Created attachment 104180 [details]
[PATCH] Suggested solution
Created attachment 104186 [details]
[PATCH] Altered hover backgrounds for the Metrics pane sections to follow the new highlight
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 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 (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... Created attachment 104191 [details]
[PATCH] Compilability fixed
Comment on attachment 104191 [details] [PATCH] Compilability fixed Attachment 104191 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9404986 Created attachment 104312 [details]
[PATCH] Global initializers fix attempt
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). Created attachment 104493 [details]
[PATCH] Comments addressed
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 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. Created attachment 104519 [details]
[PATCH] Comments addressed
Comment on attachment 104519 [details] [PATCH] Comments addressed Attachment 104519 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9438368 Created attachment 104522 [details]
[PATCH] Compilability fixed for Mac
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 Created attachment 104665 [details]
[PATCH] Simplified box model parts highlighting
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 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 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 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 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 Created attachment 104676 [details]
[PATCH] Build fix
Created attachment 104677 [details]
[PATCH] Commented method removed
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 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? (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. Created attachment 104840 [details]
[PATCH] Protocol-related comments addressed
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 Committed r93603: <http://trac.webkit.org/changeset/93603> Mac build fix by pfeldman: http://trac.webkit.org/changeset/93604 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? 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?
(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. |