Bug 133818 - Web Inspector: Selected DOM element highlights invisible near bottom of the viewport (topContentInset?)
Summary: Web Inspector: Selected DOM element highlights invisible near bottom of the v...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-06-12 13:36 PDT by Jonathan Wells
Modified: 2014-07-01 14:12 PDT (History)
13 users (show)

See Also:


Attachments
[Screenshot] normal highlight (106.02 KB, image/png)
2014-06-12 13:36 PDT, Jonathan Wells
no flags Details
[Screenshot] Abnormal highlight (109.45 KB, image/png)
2014-06-12 13:37 PDT, Jonathan Wells
no flags Details
[Test] Nodes test (9.68 KB, text/html)
2014-06-12 13:38 PDT, Jonathan Wells
no flags Details
[PATCH] Proposed Fix (2.43 KB, patch)
2014-06-30 17:48 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (2.50 KB, patch)
2014-07-01 12:47 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Wells 2014-06-12 13:36:54 PDT
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.
Comment 1 Radar WebKit Bug Importer 2014-06-12 13:37:31 PDT
<rdar://problem/17291737>
Comment 2 Jonathan Wells 2014-06-12 13:37:55 PDT
Created attachment 232981 [details]
[Screenshot] Abnormal highlight
Comment 3 Jonathan Wells 2014-06-12 13:38:23 PDT
Created attachment 232983 [details]
[Test] Nodes test
Comment 4 Joseph Pecoraro 2014-06-30 17:48:56 PDT
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.
Comment 5 Joseph Pecoraro 2014-06-30 17:55:18 PDT
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?
Comment 6 Benjamin Poulain 2014-06-30 18:20:03 PDT
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 :)
Comment 7 Joseph Pecoraro 2014-07-01 10:35:01 PDT
(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.
Comment 8 Timothy Hatcher 2014-07-01 11:09:15 PDT
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.
Comment 9 Beth Dakin 2014-07-01 11:31:41 PDT
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)
Comment 10 Joseph Pecoraro 2014-07-01 12:15:55 PDT
(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?
Comment 11 Beth Dakin 2014-07-01 12:18:36 PDT
> 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)
Comment 12 Joseph Pecoraro 2014-07-01 12:29:43 PDT
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)
Comment 13 Beth Dakin 2014-07-01 12:42:30 PDT
(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))
Comment 14 Joseph Pecoraro 2014-07-01 12:47:39 PDT
Created attachment 234191 [details]
[PATCH] Proposed Fix

Perfect!
Comment 15 WebKit Commit Bot 2014-07-01 14:12:32 PDT
Comment on attachment 234191 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 234191

Committed r170665: <http://trac.webkit.org/changeset/170665>
Comment 16 WebKit Commit Bot 2014-07-01 14:12:39 PDT
All reviewed patches have been landed.  Closing bug.