Bug 195319 – Web Inspector: TimelineOverview clicks do not always behave as expected

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Created attachment 363618 [details]
[PATCH] Proposed Fix

[Devin Rousso]

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);

[EWS Watchlist]

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

[EWS Watchlist]

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

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Created attachment 363785 [details]
[PATCH] Proposed Fix

[Devin Rousso]

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?

[Joseph Pecoraro]

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

[Joseph Pecoraro]

And because this snippet was specific to CPU handling I'll move it to the other patch so it is grouped appropriately with that.

[Joseph Pecoraro]

https://trac.webkit.org/r242566

[Radar WebKit Bug Importer]

<rdar://problem/48719185>