Bug 122291 – Web Inspector: [CSS Regions] Display the correct fragment boxes for content inside flow threads

• Component: Web Inspector
• Status: RESOLVED FIXED

[Alexandru Chiculita]

Currently the inspector overlay will try to map the boxes using localToAbsoluteQuad. However, that is not working correctly for elements inside flow threads as they can be split across multiple regions. Currently, localToAbsoluteQuad will try to use the region that has the center of the box ending up with bad looking results.

Also, fragments inside regions can end up having different widths and margins, because width: 100% might mean something else when you flow in a smaller region.

[Radar WebKit Bug Importer]

<rdar://problem/15143910>

[Radar WebKit Bug Importer]

<rdar://problem/15143909>

[Alexandru Chiculita]

Created attachment 213294 [details]
Patch V1

[Alexandru Chiculita]

If this is hard to review in the current state, I can split it up in smaller refactor patches first. 
I just went with the flow and tried to see how it would look like in the end.

[Joseph Pecoraro]

Comment on attachment 213294 [details]
Patch V1

View in context: https://bugs.webkit.org/attachment.cgi?id=213294&action=review

r=me, Just style nits in the C++ but a suggestion for a change in the JavaScript. Feel free to put up another patch if you want me to look at the JS once changed.

> Source/WebCore/inspector/InspectorOverlay.cpp:270
> +        buildNodeHighlight(m_highlightNode.get(), 0, m_nodeHighlightConfig, highlight);

Style: nullptr instead of 0.

Its a rather recent style change WebKit is going though with C++11 I think. This ensures a little extra compile time safety (0 would implicitly work for either pointer or a int / other primitive types, nullptr would only work for pointer types).

> Source/WebCore/inspector/InspectorOverlay.cpp:463
> +        return 0;

Style: nullptr

> Source/WebCore/inspector/InspectorOverlay.cpp:496
> +        buildRendererHighlight(renderer, 0, config, &highlight);

Style: nullptr

> Source/WebCore/inspector/InspectorOverlay.cpp:501
> +        RenderRegion* startRegion = 0;
> +        RenderRegion* endRegion = 0;

Style: nullptr

> Source/WebCore/inspector/InspectorOverlay.cpp:505
> +        if (!startRegion) {
> +            ASSERT_NOT_REACHED();
> +            return 0;

Style: nullptr

This would also read easier without braces. For example:

    ASSERT(startRegion);
    if (!startRegion)
        return nullptr;

> Source/WebCore/inspector/InspectorOverlay.cpp:516
> +                // Compute the cliping area of the region.

Typo: "cliping" => "clipping"

> Source/WebCore/inspector/InspectorOverlay.cpp:531
> +        return 0;

Style: nullptr

> Source/WebCore/inspector/InspectorOverlay.cpp:576
> +    if (containingFlowThread) {
> +        if (containingFlowThread && containingFlowThread->isRenderNamedFlowThread()) {

Nit: You can remove the outer if.

> Source/WebCore/inspector/InspectorOverlay.cpp:594
> +        return 0;
> +
> +    Node* node = m_highlightNode.get();
> +    RenderObject* renderer = node->renderer();
> +    if (!renderer)
> +        return 0;

Style: nullptr

> Source/WebCore/inspector/InspectorOverlayPage.js:266
> +    var elementTitleTemplate = document.getElementById("element-title-template");
> +    var elementTitle = elementTitleTemplate.cloneNode(true);
> +    // Remove the template's ID.
> +    elementTitle.id = "";

It would probably be better to just create the elements rather then cloneNode, since inevitably with cloning we need to querySelector to find nodes. Building it ourselves we can both create only the elements we need and not have to search for elements. You would also avoid the temporary situation of having multiple elements with the same id.

For example (untested):

    function _createElementTitle(elementInfo)
    {
        var elementTitleElement = document.createElement("div");
        elementTitleElement.className = "element-title";

        var tagNameElement = elementTitle.appendChild(document.createElement("span"));
        tagNameElement.className = "tag-name";
        tagNameElement.textContent = elementInfo.tagName;

        if (elementInfo.idValue) {
            var idElement = elementTitle.appendChild(document.createElement("span"));
            idElement.className = "node-id";
            idElement.textContent = "#" + elementInfo.idValue;
        }

        if (elementInfo.className) {
            var className = elementTitle.className.length > 50 ? elementTitle.className.substring(0, 50) + "\u2026" : elementTitle.className;
            var classNameElement = elementTitle.appendChild(document.createElement("span"));
            classNameElement.className = "class-name";
            classNameElement.textContent = className;
        }

        …

        return elementTitleElement;
    }

[Joseph Pecoraro]

>         if (elementInfo.className) {
>             var className = elementTitle.className.length > 50 ? elementTitle.className.substring(0, 50) + "\u2026" : elementTitle.className;

See what I mean by untested? This would have been elementInfo.className, not elementTitle.className =)

[Alexandru Chiculita]

Comment on attachment 213294 [details]
Patch V1

View in context: https://bugs.webkit.org/attachment.cgi?id=213294&action=review

>> Source/WebCore/inspector/InspectorOverlay.cpp:576
>> +        if (containingFlowThread && containingFlowThread->isRenderNamedFlowThread()) {
> 
> Nit: You can remove the outer if.

Nice catch. :)

Thanks for your review! I will upload another patch where I added the code for the title element. Let me know how that looks to you.

[Alexandru Chiculita]

Created attachment 213623 [details]
Patch V2

[Alexandru Chiculita]

Created attachment 213625 [details]
Patch V3

[Timothy Hatcher]

Comment on attachment 213625 [details]
Patch V3

View in context: https://bugs.webkit.org/attachment.cgi?id=213625&action=review

> Source/WebCore/inspector/InspectorOverlayPage.css:84
> +    /* Add a space to separate the property name from the value. */
> +    content: " ";

This isn't accessibility friendly. Can it be added in the DOM instead? (Not that the overlay is friendly to accessibility already, but less barriers the better.)

> Source/WebCore/inspector/InspectorOverlayPage.js:281
> +    new DOMBuilder("div", className)
> +        .appendTo(this.element)
> +        .appendSpan("css-property", propertyName)
> +        .appendSpan("value", value);

The chaining here confused me. It would be clearer to do: var builder = new DOMBuilder… and not chain all the calls.

> Source/WebCore/inspector/InspectorOverlayPage.js:292
> +    return (value && value.length > maxLength) ? value.substring(0, 50) + "\u2026" : value;

No need for the ( ) around the condition.

> Source/WebCore/inspector/InspectorOverlayPage.js:311
> +    var titleBuilder = new DOMBuilder("div", "element-title")
> +        .appendTo(document.getElementById("element-title-container"))
> +
> +        .appendSpanIfNotNull("tag-name", elementInfo.tagName)
> +        .appendSpanIfNotNull("node-id", elementInfo.idValue, "#")
> +        .appendSpanIfNotNull("class-name", _truncateString(elementInfo.className, 50))
> +
> +        .appendSpan("node-width", elementInfo.nodeWidth)
> +        .appendSpan("px", "px \xd7 ") // \xd7 is the code for the × HTML entity.
> +        .appendSpan("node-height", elementInfo.nodeHeight)
> +        .appendSpan("px", "px")
> +
> +        .appendPropertyIfNotNull("region-flow-name", "Region Flow", elementInfo.regionFlowInfo ? elementInfo.regionFlowInfo.name : null)
> +        .appendPropertyIfNotNull("content-flow-name", "Content Flow", elementInfo.contentFlowInfo ? elementInfo.contentFlowInfo.name : null);
> +

Ditto here about the extreme chaining. I think it is less clear to read.

> Source/WebCore/inspector/InspectorOverlayPage.js:503
> -function drawNodeHighlight(highlight)
> -{
> +function _drawFragmentHighlight(highlight) {

The { should be on the next line like it was before.

> Source/WebCore/inspector/InspectorOverlayPage.js:514
> +        var regionClipQuad = highlight.region.quad;
> +        quadToPath(regionClipQuad).clip();

Could be one line. Not much is gained by making a variable.

[Alexandru Chiculita]

Created attachment 213837 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 213837 [details]
Patch for landing

Clearing flags on attachment: 213837

Committed r157199: <http://trac.webkit.org/changeset/157199>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.