Summary: | Web Inspector: [CSS Regions] Show a list of containing regions when clicking a node that is part of a flow | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexandru Chiculita <achicu> | ||||||||||||||||||
Component: | Web Inspector | Assignee: | Alexandru Chiculita <achicu> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | commit-queue, graouts, joepeck, timothy, webkit-bug-importer, WebkitBugTracker | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||
OS: | All | ||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||
Bug Blocks: | 57312 | ||||||||||||||||||||
Attachments: |
|
Description
Alexandru Chiculita
2013-11-19 15:44:28 PST
Created attachment 217346 [details]
Screenshot
Created attachment 217607 [details]
Patch V1
Comment on attachment 217607 [details] Patch V1 View in context: https://bugs.webkit.org/attachment.cgi?id=217607&action=review > Source/WebInspectorUI/UserInterface/DetailsSection.js:33 > + this._element.classList.add(identifier); This is needed for the ".region-flow.details-section" rule above. Created attachment 217608 [details]
Screenshot with region clicked
Created attachment 217609 [details]
Screenshot with content node clicked
Things missing in this patch: 1. We want an index next to the region. The index corresponds to the index shown when you highlight the region. 2. The arrow next to the flow name should hide and show on hover, similar to the DOMTreeDataGrid below it. Created attachment 217615 [details]
Screenshot with nested regions
Created attachment 217622 [details]
Screenshot V2
Created attachment 217845 [details] Patch V2 Implemented the UI we discussed on IRC: http://tinypic.com/view.php?pic=ka01h1&s=5#.UpPqXWTXSQw Comment on attachment 217845 [details] Patch V2 View in context: https://bugs.webkit.org/attachment.cgi?id=217845&action=review > Source/WebInspectorUI/UserInterface/ComputedStyleDetailsPanel.css:34 > + -webkit-user-select: none; Why prevent selecting the flow name? The user might want to copy it. > Source/WebInspectorUI/UserInterface/ComputedStyleDetailsPanel.css:71 > +.details-section > .content > .group > .row.simple.content-flow-link > .value > div > .go-to-arrow { > + display: none; > + width: 16px; > + height: 16px; > +} > + > +.details-section > .content > .group > .row.simple.content-flow-link:hover > .value > div > .go-to-arrow { > + display: block; > +} The go-to arrow should always be visible. There is already a style for .go-to-arrow in a row in DetailsSection.css. > Source/WebInspectorUI/UserInterface/ComputedStyleDetailsPanel.js:58 > + goToRegionFlowButton.addEventListener("click", this._goToRegionFlowArrowWasClicked.bind(this), false); We omit the false in addEventListener. > Source/WebInspectorUI/UserInterface/ComputedStyleDetailsPanel.js:69 > + goToContentFlowButton.addEventListener("click", this._goToContentFlowArrowWasClicked.bind(this), false); Ditto. > Source/WebInspectorUI/UserInterface/ComputedStyleDetailsPanel.js:108 > + this._regionFlowNameRow.element.classList.toggle("hidden", !flow); > + this._flowNamesSection.element.classList.toggle("hidden", !flow && !this._contentFlow); The variable flow confused me here. I think this._regionFlow would be more readable given the use of this._contentFlow. > Source/WebInspectorUI/UserInterface/ComputedStyleDetailsPanel.js:120 > + this._contentFlowNameRow.element.classList.toggle("hidden", !flow); DetailsSectionSimpleRow auto hides if it has an empty value. The styles for the sections and groups also adjust padding around empty rows, so it would be best to use the auto hide behavior. > Source/WebInspectorUI/UserInterface/ComputedStyleDetailsPanel.js:121 > + this._flowNamesSection.element.classList.toggle("hidden", !flow && !this._regionFlow); Ditto about flow. > Source/WebInspectorUI/UserInterface/ComputedStyleDetailsPanel.js:194 > + } > + this.regionFlow = flowData.regionFlow; Newline. > Source/WebInspectorUI/UserInterface/DOMTreeDataGrid.css:70 > + content: url(Images/DOMElement.svg); Not all DOM nodes are elements. The icon should be based on the node type. This can be a FIXME. > Source/WebInspectorUI/UserInterface/RuntimeManager.js:79 > + } > + var properties = new Map; Newline. > Source/WebInspectorUI/UserInterface/RuntimeManager.js:82 > + properties.set(property.name, property); > + callback(null, properties); Newline. (In reply to comment #10) > (From update of attachment 217845 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=217845&action=review > Thanks! > > Source/WebInspectorUI/UserInterface/ComputedStyleDetailsPanel.css:34 > > + -webkit-user-select: none; > > Why prevent selecting the flow name? The user might want to copy it. I'm using this one to disable the icon selection and also avoids making webkit do 2 layers of selection. The span that contains the text has user-select: text below, so the actual text is selectable. I will add a comment explaining that. > > > Source/WebInspectorUI/UserInterface/ComputedStyleDetailsPanel.css:71 > > +.details-section > .content > .group > .row.simple.content-flow-link > .value > div > .go-to-arrow { > > + display: none; > > + width: 16px; > > + height: 16px; > > +} > > + > > +.details-section > .content > .group > .row.simple.content-flow-link:hover > .value > div > .go-to-arrow { > > + display: block; > > +} > > The go-to arrow should always be visible. There is already a style for .go-to-arrow in a row in DetailsSection.css. I wanted to make it consisntent with the layers panel and the new dom tree data grid. I didn't know there's a "go-to-array" already. It took me some time to make it show up. I've attached pictures of the two ways of having the arrow in the right panel: http://i44.tinypic.com/v5zqkm.jpg http://i41.tinypic.com/n6qipx.jpg I was following the later, just because they show up next to each other in the same panel. Created attachment 218548 [details]
Patch for landing
Comment on attachment 218548 [details] Patch for landing Clearing flags on attachment: 218548 Committed r160198: <http://trac.webkit.org/changeset/160198> All reviewed patches have been landed. Closing bug. |