Bug 133818 – Web Inspector: Selected DOM element highlights invisible near bottom of the viewport (topContentInset?)

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

[Jonathan Wells]

Created attachment 232979 [details]
[Screenshot] normal highlight

When inspecting the DOM, highlights do not occur in about a 20px tall zone at the bottom of the window. To test this:

1. Open the 501nodes.html file attached.
2. Position a specific box at the bottom of the window.
3. Inspect and highlight that box.

[Radar WebKit Bug Importer]

<rdar://problem/17291737>

[Jonathan Wells]

Created attachment 232981 [details]
[Screenshot] Abnormal highlight

[Jonathan Wells]

Created attachment 232983 [details]
[Test] Nodes test

[Joseph Pecoraro]

Created attachment 234130 [details]
[PATCH] Proposed Fix

Dealing with ScrollView / FrameView sizes is not my normal area of expertise. Simon / Benjamin, is there a better way to do this? In fact, the whole InspectorOverlay page approach could use an audit by someone who knows more about the page sizing. This fixes the issue in what I considered a safe way if there is no topContentInset.

[Joseph Pecoraro]

Beth, you were recommended as a topContentInset expert by a few others. Does this patch look dirty to you, is there a better way, or some cleanup we can do in general here?

[Benjamin Poulain]

Comment on attachment 234130 [details]
[PATCH] Proposed Fix

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

> Source/WebCore/inspector/InspectorOverlay.cpp:350
> +    frameViewFullSize.setHeight(frameViewFullSize.height() + view->topContentInset());

It this is for iOS, that makes no sense.
If this is for OS X, that may make sense :)

[Joseph Pecoraro]

(In reply to comment #6)
> (From update of attachment 234130 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234130&action=review
> 
> > Source/WebCore/inspector/InspectorOverlay.cpp:350
> > +    frameViewFullSize.setHeight(frameViewFullSize.height() + view->topContentInset());
> 
> It this is for iOS, that makes no sense.
> If this is for OS X, that may make sense :)

This is OS X code right now. Node highlighting goes a different path for iOS.

[Timothy Hatcher]

Comment on attachment 234130 [details]
[PATCH] Proposed Fix

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

>> Source/WebCore/inspector/InspectorOverlay.cpp:350
>> +    frameViewFullSize.setHeight(frameViewFullSize.height() + view->topContentInset());
> 
> It this is for iOS, that makes no sense.
> If this is for OS X, that may make sense :)

This code is only used on OS X right now.

[Beth Dakin]

Comment on attachment 234130 [details]
[PATCH] Proposed Fix

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

Hi Joe! Generally this concept behind this patch seems sound. But there are slightly better functions you can call to make it a little cleaner.

> Source/WebCore/inspector/InspectorOverlay.cpp:348
> +    viewportSize.setHeight(viewportSize.height() + view->topContentInset());

Instead of fetching visibleContentRect and manually adding in the content inset, you can just call unobscuredContentRect()

> Source/WebCore/inspector/InspectorOverlay.cpp:349
>      IntSize frameViewFullSize = view->visibleContentRectIncludingScrollbars().size();

And here instead of calling visibleContentRectIncludingScrollbars() and manually adding in the inset, you can call unobscuredContentRect(IncludeScrollbars)

[Joseph Pecoraro]

(In reply to comment #9)
> (From update of attachment 234130 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234130&action=review
> 
> Hi Joe! Generally this concept behind this patch seems sound. But there are slightly better functions you can call to make it a little cleaner.
> 
> > Source/WebCore/inspector/InspectorOverlay.cpp:348
> > +    viewportSize.setHeight(viewportSize.height() + view->topContentInset());
> 
> Instead of fetching visibleContentRect and manually adding in the content inset, you can just call unobscuredContentRect()
> 
> > Source/WebCore/inspector/InspectorOverlay.cpp:349
> >      IntSize frameViewFullSize = view->visibleContentRectIncludingScrollbars().size();
> 
> And here instead of calling visibleContentRectIncludingScrollbars() and manually adding in the inset, you can call unobscuredContentRect(IncludeScrollbars)

Going with these versions I end up with slightly smaller rects, and the highlights do get clipped:

(lldb) p viewportSize // unobscuredContentRect
(WebCore::IntSize) $16 = (m_width = 1218, m_height = 669)
(lldb) p viewportSize1// visible + topContentInset
(WebCore::IntSize) $17 = (m_width = 1218, m_height = 707)

(lldb) p frameViewFullSize // unobscuredContentRect(ScrollableArea::IncludeScrollbars)
(WebCore::IntSize) $18 = (m_width = 1218, m_height = 669)
(lldb) p frameViewFullSize1 // visibleWithScrollbars + topContentInset
(WebCore::IntSize) $19 = (m_width = 1218, m_height = 707)

Is there something else that I may be missing?

[Beth Dakin]

> Going with these versions I end up with slightly smaller rects, and the highlights do get clipped:
> 
> (lldb) p viewportSize // unobscuredContentRect
> (WebCore::IntSize) $16 = (m_width = 1218, m_height = 669)
> (lldb) p viewportSize1// visible + topContentInset
> (WebCore::IntSize) $17 = (m_width = 1218, m_height = 707)
> 
> (lldb) p frameViewFullSize // unobscuredContentRect(ScrollableArea::IncludeScrollbars)
> (WebCore::IntSize) $18 = (m_width = 1218, m_height = 669)
> (lldb) p frameViewFullSize1 // visibleWithScrollbars + topContentInset
> (WebCore::IntSize) $19 = (m_width = 1218, m_height = 707)
> 
> Is there something else that I may be missing?

Oh jeez, the state of these functions is such a mess right now. Try: 

unscaledUnobscuredVisibleContentSize()
and
unscaledUnobscuredVisibleContentSize((ScrollableArea::IncludeScrollbars)

[Joseph Pecoraro]

Different sizes, but still gets clipped:

(lldb) p viewportSize // unscaledUnobscuredVisibleContentSize()
(WebCore::IntSize) $14 = (m_width = 1218, m_height = 667)

(lldb) p frameViewFullSize // unscaledUnobscuredVisibleContentSize((ScrollableArea::IncludeScrollbars)
(WebCore::IntSize) $16 = (m_width = 1218, m_height = 667)

[Beth Dakin]

(In reply to comment #12)
> Different sizes, but still gets clipped:
> 
> (lldb) p viewportSize // unscaledUnobscuredVisibleContentSize()
> (WebCore::IntSize) $14 = (m_width = 1218, m_height = 667)
> 
> (lldb) p frameViewFullSize // unscaledUnobscuredVisibleContentSize((ScrollableArea::IncludeScrollbars)
> (WebCore::IntSize) $16 = (m_width = 1218, m_height = 667)

I'm sorry; I'm getting myself confused. Of course we don't want the unobscured rects. Please try:

unscaledTotalVisibleContentSize()
unscaledTotalVisibleContentSize(ScrollableArea::IncludeScrollbars))

[Joseph Pecoraro]

Created attachment 234191 [details]
[PATCH] Proposed Fix

Perfect!

[WebKit Commit Bot]

Comment on attachment 234191 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 234191

Committed r170665: <http://trac.webkit.org/changeset/170665>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.