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.
<rdar://problem/15143910>
<rdar://problem/15143909>
Created attachment 213294 [details] Patch V1
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.
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; }
> 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 =)
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.
Created attachment 213623 [details] Patch V2
Created attachment 213625 [details] Patch V3
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.
Created attachment 213837 [details] Patch for landing
Comment on attachment 213837 [details] Patch for landing Clearing flags on attachment: 213837 Committed r157199: <http://trac.webkit.org/changeset/157199>
All reviewed patches have been landed. Closing bug.