Bug 124614 – Web Inspector: [CSS Regions] Show a list of containing regions when clicking a node that is part of a flow

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Alexandru Chiculita]

Open a page with CSS Regions. Use the inspector to find a node inside a CSS Region. Use the inspector to figure out the containting regions of the element.

[Alexandru Chiculita]

Created attachment 217346 [details]
Screenshot

[Alexandru Chiculita]

Created attachment 217607 [details]
Patch V1

[Alexandru Chiculita]

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.

[Alexandru Chiculita]

Created attachment 217608 [details]
Screenshot with region clicked

[Alexandru Chiculita]

Created attachment 217609 [details]
Screenshot with content node clicked

[Alexandru Chiculita]

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.

[Alexandru Chiculita]

Created attachment 217615 [details]
Screenshot with nested regions

[Alexandru Chiculita]

Created attachment 217622 [details]
Screenshot V2

[Alexandru Chiculita]

Created attachment 217845 [details]
Patch V2

Implemented the UI we discussed on IRC: http://tinypic.com/view.php?pic=ka01h1&s=5#.UpPqXWTXSQw

[Timothy Hatcher]

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.

[Alexandru Chiculita]

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

[Alexandru Chiculita]

Created attachment 218548 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 218548 [details]
Patch for landing

Clearing flags on attachment: 218548

Committed r160198: <http://trac.webkit.org/changeset/160198>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.