Bug 195318 - Web Inspector: Attempting to select records in the bottom 16px of the timeline overview graph fails
Summary: Web Inspector: Attempting to select records in the bottom 16px of the timelin...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-05 01:01 PST by Joseph Pecoraro
Modified: 2019-03-06 14:30 PST (History)
7 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (3.34 KB, patch)
2019-03-05 01:03 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (3.15 MB, application/zip)
2019-03-05 03:10 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2019-03-05 01:01:56 PST
Attempting to select records in the bottom 16px of the timeline overview graph fails

Steps to Reproduce:
1. Record timeline with records
2. Attempt to click a record in the bottom 16 px of the overview graph when scrollbars are not showing
  => Click doesn't work
Comment 1 Joseph Pecoraro 2019-03-05 01:03:17 PST
Created attachment 363617 [details]
[PATCH] Proposed Fix
Comment 2 Devin Rousso 2019-03-05 02:11:25 PST
Comment on attachment 363617 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/TimelineOverview.js:541
> +        this.element.classList.toggle("has-scrollbar", this._scrollContainerElement.clientHeight <= 1);

Am I misreading this, or should we flip the `<=` to be a `>`?  We only `has-scrollbar` when the height is `<= 1`?  Wouldn't the scrollbar make the height larger, or am I thinking backwards?
Comment 3 Build Bot 2019-03-05 03:10:05 PST
Comment on attachment 363617 [details]
[PATCH] Proposed Fix

Attachment 363617 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11375615

New failing tests:
fast/scrolling/ios/hit-testing-iframe-004.html
fast/scrolling/ios/hit-testing-iframe-002.html
fast/scrolling/ios/mixing-user-and-programmatic-scroll-006.html
fast/scrolling/ios/hit-testing-iframe-003.html
Comment 4 Build Bot 2019-03-05 03:10:07 PST
Created attachment 363630 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 5 Joseph Pecoraro 2019-03-05 10:35:24 PST
(In reply to Devin Rousso from comment #2)
> Comment on attachment 363617 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=363617&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/TimelineOverview.js:541
> > +        this.element.classList.toggle("has-scrollbar", this._scrollContainerElement.clientHeight <= 1);
> 
> Am I misreading this, or should we flip the `<=` to be a `>`?  We only
> `has-scrollbar` when the height is `<= 1`?  Wouldn't the scrollbar make the
> height larger, or am I thinking backwards?

When there is no scrollbar the height is 16.
When there is a scrollbar the height is 1.

This was tested with overlay scrollbars always on.
Comment 6 Devin Rousso 2019-03-05 13:25:26 PST
Comment on attachment 363617 [details]
[PATCH] Proposed Fix

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

rs=me

>>> Source/WebInspectorUI/UserInterface/Views/TimelineOverview.js:541
>>> +        this.element.classList.toggle("has-scrollbar", this._scrollContainerElement.clientHeight <= 1);
>> 
>> Am I misreading this, or should we flip the `<=` to be a `>`?  We only `has-scrollbar` when the height is `<= 1`?  Wouldn't the scrollbar make the height larger, or am I thinking backwards?
> 
> When there is no scrollbar the height is 16.
> When there is a scrollbar the height is 1.
> 
> This was tested with overlay scrollbars always on.

Rather than have this be repeated in multiple places, I'd rather you have a separate function (possibly in Utilities.js) that does this logic.
Comment 7 Joseph Pecoraro 2019-03-05 13:55:59 PST
(In reply to Devin Rousso from comment #6)
> Comment on attachment 363617 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=363617&action=review
> 
> rs=me
> 
> >>> Source/WebInspectorUI/UserInterface/Views/TimelineOverview.js:541
> >>> +        this.element.classList.toggle("has-scrollbar", this._scrollContainerElement.clientHeight <= 1);
> >> 
> >> Am I misreading this, or should we flip the `<=` to be a `>`?  We only `has-scrollbar` when the height is `<= 1`?  Wouldn't the scrollbar make the height larger, or am I thinking backwards?
> > 
> > When there is no scrollbar the height is 16.
> > When there is a scrollbar the height is 1.
> > 
> > This was tested with overlay scrollbars always on.
> 
> Rather than have this be repeated in multiple places, I'd rather you have a
> separate function (possibly in Utilities.js) that does this logic.

This is specific to this _scrollContainerElement. I'm not sure if it would work in other places.
Comment 8 WebKit Commit Bot 2019-03-05 14:01:03 PST
Comment on attachment 363617 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 363617

Committed r242510: <https://trac.webkit.org/changeset/242510>
Comment 9 WebKit Commit Bot 2019-03-05 14:01:04 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2019-03-05 14:02:45 PST
<rdar://problem/48613407>
Comment 11 Nikita Vasilyev 2019-03-06 14:30:15 PST
Comment on attachment 363617 [details]
[PATCH] Proposed Fix

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

>>>>> Source/WebInspectorUI/UserInterface/Views/TimelineOverview.js:541
>>>>> +        this.element.classList.toggle("has-scrollbar", this._scrollContainerElement.clientHeight <= 1);
>>>> 
>>>> Am I misreading this, or should we flip the `<=` to be a `>`?  We only `has-scrollbar` when the height is `<= 1`?  Wouldn't the scrollbar make the height larger, or am I thinking backwards?
>>> 
>>> When there is no scrollbar the height is 16.
>>> When there is a scrollbar the height is 1.
>>> 
>>> This was tested with overlay scrollbars always on.
>> 
>> Rather than have this be repeated in multiple places, I'd rather you have a separate function (possibly in Utilities.js) that does this logic.
> 
> This is specific to this _scrollContainerElement. I'm not sure if it would work in other places.

If scrollbar height decreases in some future version of OS, this would break. How about:

    this.element.classList.toggle("has-scrollbar", this._scrollContainerElement.clientHeight < 16);

Also, instead of hardcoding 16 and making sure it matches CSS, we could set it as an inline style in JS.