Bug 195318 – Web Inspector: Attempting to select records in the bottom 16px of the timeline overview graph fails

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

[Joseph Pecoraro]

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

[Joseph Pecoraro]

Created attachment 363617 [details]
[PATCH] Proposed Fix

[Devin Rousso]

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?

[EWS Watchlist]

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

[EWS Watchlist]

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

[Joseph Pecoraro]

(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.

[Devin Rousso]

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.

[Joseph Pecoraro]

(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.

[WebKit Commit Bot]

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

Clearing flags on attachment: 363617

Committed r242510: <https://trac.webkit.org/changeset/242510>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48613407>

[Nikita Vasilyev]

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.