TimelineOverview clicks do not always behave as expected - Very short clicks sometimes produce 1px drags - __timelineRecordBarClick doesn't propagate as expected Do a little better at differentiating between a drag and a click.
Created attachment 363618 [details] [PATCH] Proposed Fix
Comment on attachment 363618 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=363618&action=review > Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:759 > + if (pixels <= 4) If the user has moved the mouse more than 4px (e.g. they jerk their wrist), should that be considered a pass? > Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:781 > + let subEvent = new event.constructor(event.type, event); I'd name this `newEvent` (it matches `newTarget`). > Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:784 > + event.stop(); Do we want to `stop`, or just set `__timelineRecordClick` on `newEvent` if it had previously been set on `event`? Also, considering that we're trying to dispatch the event on a new target, shouldn't we `stop` the old event regardless? let newEvent = new event.constructor(event.type, event); if (event.__timlineRecordBarClick) newEvent.__timelineRecordBarClick = event.__timelineRecordBarClick; event.stop(); newTargete.dispatchEvent(newEvent);
Comment on attachment 363618 [details] [PATCH] Proposed Fix Attachment 363618 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/11375690 New failing tests: fast/events/beforeunload-alert-user-interaction2.html fast/events/clipboard-event-constructor.html fast/events/click-handler-on-body-simple.html fast/forms/datalist/datalist-textinput-suggestions-order.html
Created attachment 363632 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 363618 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=363618&action=review >> Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:759 >> + if (pixels <= 4) > > If the user has moved the mouse more than 4px (e.g. they jerk their wrist), should that be considered a pass? If the jerk took less than 25ms then it would be ignored. But otherwise I'd consider it a real drag. That 25 is probably not meaningful so I may drop it. >> Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:784 >> + event.stop(); > > Do we want to `stop`, or just set `__timelineRecordClick` on `newEvent` if it had previously been set on `event`? Also, considering that we're trying to dispatch the event on a new target, shouldn't we `stop` the old event regardless? > > let newEvent = new event.constructor(event.type, event); > if (event.__timlineRecordBarClick) > newEvent.__timelineRecordBarClick = event.__timelineRecordBarClick; > > event.stop(); > > newTargete.dispatchEvent(newEvent); I'll try just propagating the __timelineRecordClick. I don't think we want to always stop(), because then the TimelineOverview would never get the click event even if something down below didn't want it. Also it is the dispatchEvent that triggers adding the __property, it would not be set prior to the dispatchEvent because there is only a single one followed by a return.
Comment on attachment 363618 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=363618&action=review >>> Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:759 >>> + if (pixels <= 4) >> >> If the user has moved the mouse more than 4px (e.g. they jerk their wrist), should that be considered a pass? > > If the jerk took less than 25ms then it would be ignored. But otherwise I'd consider it a real drag. That 25 is probably not meaningful so I may drop it. I've removed this. 25ms is so short anyways I'm not sure that is possible with most hardware! >>> Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:784 >>> + event.stop(); >> >> Do we want to `stop`, or just set `__timelineRecordClick` on `newEvent` if it had previously been set on `event`? Also, considering that we're trying to dispatch the event on a new target, shouldn't we `stop` the old event regardless? >> >> let newEvent = new event.constructor(event.type, event); >> if (event.__timlineRecordBarClick) >> newEvent.__timelineRecordBarClick = event.__timelineRecordBarClick; >> >> event.stop(); >> >> newTargete.dispatchEvent(newEvent); > > I'll try just propagating the __timelineRecordClick. > > I don't think we want to always stop(), because then the TimelineOverview would never get the click event even if something down below didn't want it. > > Also it is the dispatchEvent that triggers adding the __property, it would not be set prior to the dispatchEvent because there is only a single one followed by a return. The stop() seems necessary for the CPU graph case. I'm not sure how this works for the recordBar cases today. I suspect it has to do with the weird z-indexes of the record bars today (which prevent timeline range selection drags on them, an issue the CPU overview graph does not have). Logically the stop() is what I would expect once something has decided to handle the event. What I was seeing in the CPU case was if you had a column already selected and then selected a different column, the first click would appear to remove all selection and the 2nd click would select the new bar. With this change the first click selects the new bar. In code I was seeing the first click actually set the selectedRecord correctly but later the TimelineOverview's click handler `_handleGraphsContainerClick` immediately clears the selectedRecord.
Created attachment 363785 [details] [PATCH] Proposed Fix
Comment on attachment 363785 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=363785&action=review r=me > Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:755 > + let pixels = Math.abs(event.pageX - this._mouseStartX); > + if (pixels <= 4) NIT: this is obvious enough that it is a pixel value, so I'd just inline it. > Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:780 > + if (newEvent.__timelineRecordBarClick) > + event.stop(); I don't think we really want to be calling `stop`. I think we should instead always be setting `__timelineRecordBarClick` on `newEvent` before it is dispatched if `event. __timelineRecordBarClick` had already been set. Calling `event.stop()` won't prevent us from iterating over `elements` and dispatching, meaning that each `newEvent` won't have the property set. `dispatchEvent` doesn't propagate the event up the DOM, which is why we use this loop (which basically emulates it). Worst case, we set an extra property on an event that is never used. For what it's worth, the only place that I've even been able to get this event to fire is to click on empty space or one of the records that is "behind" the ruler (e.g. CPU and Memory timelines). I know you added support for clicking CPU "columns" in <https://webkit.org/b/195321>, so is this directly to support that, or is it even necessary once that lands with the `z-index` change?
> > Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:780 > > + if (newEvent.__timelineRecordBarClick) > > + event.stop(); > > I don't think we really want to be calling `stop`. I think we should > instead always be setting `__timelineRecordBarClick` on `newEvent` before it > is dispatched if `event. __timelineRecordBarClick` had already been set. > Calling `event.stop()` won't prevent us from iterating over `elements` and > dispatching, meaning that each `newEvent` won't have the property set. > `dispatchEvent` doesn't propagate the event up the DOM, which is why we use > this loop (which basically emulates it). Worst case, we set an extra > property on an event that is never used. > > For what it's worth, the only place that I've even been able to get this > event to fire is to click on empty space or one of the records that is > "behind" the ruler (e.g. CPU and Memory timelines). I know you added > support for clicking CPU "columns" in <https://webkit.org/b/195321>, so is > this directly to support that, or is it even necessary once that lands with > the `z-index` change? Correct, the TimelineRuler path doesn't even happen when clicking record bars, so __timelineRecordBarClick would never have been set. The micro clicks part is generic, but this case here does seem to be specifically in support of CPU (I had thought it was generic when I put up this patch). I'll rename this specific case to something generic, like __timelineRecordClickHandled for this generic case. That z-index can be removed. The z-index didn't seem to help and was something I had tried (but it of course changed how the markers appear.
And because this snippet was specific to CPU handling I'll move it to the other patch so it is grouped appropriately with that.
https://trac.webkit.org/r242566
<rdar://problem/48719185>