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
Created attachment 363617 [details] [PATCH] Proposed Fix
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 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
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
(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 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.
(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 on attachment 363617 [details] [PATCH] Proposed Fix Clearing flags on attachment: 363617 Committed r242510: <https://trac.webkit.org/changeset/242510>
All reviewed patches have been landed. Closing bug.
<rdar://problem/48613407>
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.