Bug 176824 - Web Inspector: Timeline should show when events preventDefault() was called on an event or not
Summary: Web Inspector: Timeline should show when events preventDefault() was called o...
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: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-09-12 21:34 PDT by Joseph Pecoraro
Modified: 2022-09-07 08:27 PDT (History)
16 users (show)

See Also:


Attachments
[PATCH] Proposed Fix - Lacking Tests (20.36 KB, patch)
2017-09-12 21:40 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[IMAGE] Event Display (124.83 KB, image/png)
2017-09-12 21:40 PDT, Joseph Pecoraro
no flags Details
[PATCH] Proposed Fix (58.77 KB, patch)
2017-09-13 15:26 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (58.75 KB, patch)
2017-09-13 15:29 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] For Landing (58.71 KB, patch)
2017-09-13 21:07 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] For Landing (33.47 KB, patch)
2017-09-14 10:52 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
Patch (55.44 KB, patch)
2019-03-17 20:57 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (55.41 KB, patch)
2019-03-20 13:34 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2017-09-12 21:34:42 PDT
Timeline should show when events preventDefault() was called on an event or not

* Test
<button id="b1">Default Not Prevent</button>
<button id="b2">Default Prevented</button>
<button onclick="sleep(); return false">Default Prevented Two</button>
<script>
function sleep() {
    let now = Date.now();
    while ((Date.now() - now) < 100);
}
document.getElementById("b1").addEventListener("click", (event) => { sleep(); });
document.getElementById("b2").addEventListener("click", (event) => { sleep(); event.preventDefault(); });
</script>

* Steps to reproduce:
1. Inspect test page
2. Start a timeline
3. Click each button
4. Stop recording
  => Should see which events had default prevented
Comment 1 Joseph Pecoraro 2017-09-12 21:34:53 PDT
<rdar://problem/34290931>
Comment 2 Joseph Pecoraro 2017-09-12 21:40:15 PDT
Created attachment 320617 [details]
[PATCH] Proposed Fix - Lacking Tests
Comment 3 Joseph Pecoraro 2017-09-12 21:40:43 PDT
Created attachment 320618 [details]
[IMAGE] Event Display
Comment 4 Simon Fraser (smfr) 2017-09-13 10:58:37 PDT
Noice. No spaces around em dash tho.
Comment 5 Joseph Pecoraro 2017-09-13 11:24:45 PDT
(In reply to Simon Fraser (smfr) from comment #4)
> Noice. No spaces around em dash tho.

I'm use the default styles for subtitles in tree elements.

For example Resources has the same styles for "Name – domain".
Comment 6 Joseph Pecoraro 2017-09-13 15:14:57 PDT
Comment on attachment 320617 [details]
[PATCH] Proposed Fix - Lacking Tests

I'll shortly include a patch with tests.
Comment 7 Joseph Pecoraro 2017-09-13 15:26:59 PDT
Created attachment 320696 [details]
[PATCH] Proposed Fix

Now with tests!
Comment 8 Joseph Pecoraro 2017-09-13 15:27:50 PDT
Comment on attachment 320696 [details]
[PATCH] Proposed Fix

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

> LayoutTests/inspector/timeline/resources/timeline-event-utilities.js:21
> +function finishRecording(data = undefined) {

I suppose at this point the = undefined is redundant.
Comment 9 Joseph Pecoraro 2017-09-13 15:29:16 PDT
Created attachment 320697 [details]
[PATCH] Proposed Fix
Comment 10 Devin Rousso 2017-09-13 15:52:12 PDT
Comment on attachment 320697 [details]
[PATCH] Proposed Fix

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

r=me

> LayoutTests/inspector/timeline/resources/timeline-event-utilities.js:8
> +        InspectorTest.log("Starting Capture...");
> +        const newRecording = true;

Style: I'd add a newline.

> LayoutTests/inspector/timeline/resources/timeline-event-utilities.js:10
> +        InspectorTest.evaluateInPage(expression);

I would prefer you call this after adding the listener.

> LayoutTests/inspector/timeline/timeline-event-EventDispatch.html:16
> +    button.dispatchEvent(new MouseEvent("click", {bubbles: true, cancelable: true}));

Can you just call `button.click();`?

> LayoutTests/inspector/timeline/timeline-event-EventDispatch.html:21
> +    button.dispatchEvent(new MouseEvent("click", {bubbles: true, cancelable: true}));

Ditto.

> LayoutTests/inspector/timeline/timeline-event-TimerFire.html:20
> +            clearTimeout(setIntervalIdentifier);

NIT: for consistency, I think `clearInterval(setIntervalIdentifier);` is preferred.

> LayoutTests/inspector/timeline/timeline-event-TimerInstall.html:20
> +            clearTimeout(setIntervalIdentifier);

Ditto

> LayoutTests/inspector/timeline/timeline-event-TimerRemove.html:19
> +            clearTimeout(setIntervalIdentifier);

Ditto

> LayoutTests/inspector/timeline/timeline-event-TimerRemove.html:71
> +        name: "SanityCheck",

lol

> Source/WebInspectorUI/UserInterface/Test/Test.js:42
> +    InspectorBackend.registerDOMDispatcher(new WI.DOMObserver);

Why did this move?
Comment 11 Joseph Pecoraro 2017-09-13 16:42:43 PDT
Comment on attachment 320697 [details]
[PATCH] Proposed Fix

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

>> LayoutTests/inspector/timeline/resources/timeline-event-utilities.js:10
>> +        InspectorTest.evaluateInPage(expression);
> 
> I would prefer you call this after adding the listener.

I mostly find that approach harder to read. In practice these commands trigger async operations and are serial and so the events below cannot happen before. So I like to write it this way to logically group things. If we somehow rewrote tests to use a synchronous backend we'd encounter all kinds of issues.

>> LayoutTests/inspector/timeline/timeline-event-TimerFire.html:20
>> +            clearTimeout(setIntervalIdentifier);
> 
> NIT: for consistency, I think `clearInterval(setIntervalIdentifier);` is preferred.

Hah, I can't believe I totally forgot about `clearInterval`. Done!

>> Source/WebInspectorUI/UserInterface/Test/Test.js:42
>> +    InspectorBackend.registerDOMDispatcher(new WI.DOMObserver);
> 
> Why did this move?

To match the order of operations in Main.js. It isn't important, just consistency.
Comment 12 Joseph Pecoraro 2017-09-13 21:07:41 PDT
Created attachment 320724 [details]
[PATCH] For Landing
Comment 13 WebKit Commit Bot 2017-09-14 00:57:50 PDT
Comment on attachment 320724 [details]
[PATCH] For Landing

Clearing flags on attachment: 320724

Committed r222015: <http://trac.webkit.org/changeset/222015>
Comment 14 Ryan Haddad 2017-09-14 09:37:33 PDT
(In reply to WebKit Commit Bot from comment #13)
> Comment on attachment 320724 [details]
> [PATCH] For Landing
> 
> Clearing flags on attachment: 320724
> 
> Committed r222015: <http://trac.webkit.org/changeset/222015>
Three of the tests added with this change are failing on macOS Debug WK1 bots:

inspector/timeline/timeline-event-FireAnimationFrame.html
inspector/timeline/timeline-event-TimerFire.html
inspector/timeline/timeline-event-TimerRemove.html

https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK1%20(Tests)/r222016%20(3594)/results.html
Comment 16 Ryan Haddad 2017-09-14 10:17:19 PDT
Reverted r222015 for reason:

The LayoutTests added with this change are flaky.

Committed r222030: <http://trac.webkit.org/changeset/222030>
Comment 17 Joseph Pecoraro 2017-09-14 10:49:49 PDT
Okay, I will avoid adding the Timer tests to this patch (they are kind of unrelated anyways) and investigate them separately. It doesn't appear the EventDispatch test was flakey like the timer ones.
Comment 18 Joseph Pecoraro 2017-09-14 10:52:29 PDT
Created attachment 320789 [details]
[PATCH] For Landing
Comment 19 WebKit Commit Bot 2017-09-14 11:09:12 PDT
Comment on attachment 320789 [details]
[PATCH] For Landing

Clearing flags on attachment: 320789

Committed r222036: <http://trac.webkit.org/changeset/222036>
Comment 21 Joseph Pecoraro 2017-09-14 16:51:16 PDT
Rolled out in: https://trac.webkit.org/changeset/222064/webkit

It appears we have an existing bug in Timelines, that will have to be hunted down in order to test this.

Likewise it may be more common in Debug, since running this test 50 times in release didn't exhibit any issue on my machines before I submitted it.
Comment 22 Devin Rousso 2019-03-17 20:57:54 PDT
Created attachment 364999 [details]
Patch
Comment 23 WebKit Commit Bot 2019-03-20 12:53:30 PDT Comment hidden (obsolete)
Comment 24 Devin Rousso 2019-03-20 13:34:21 PDT
Created attachment 365390 [details]
Patch
Comment 25 Simon Fraser (smfr) 2019-03-20 17:20:40 PDT
Nice!
Comment 26 WebKit Commit Bot 2019-03-20 17:55:45 PDT
Comment on attachment 365390 [details]
Patch

Clearing flags on attachment: 365390

Committed r243269: <https://trac.webkit.org/changeset/243269>
Comment 27 WebKit Commit Bot 2019-03-20 17:55:48 PDT
All reviewed patches have been landed.  Closing bug.
Comment 28 Truitt Savell 2019-03-21 10:15:17 PDT
Looks like all of the tests added in https://trac.webkit.org/changeset/243269/webkit

are flaky:
inspector/timeline/timeline-event-CancelAnimationFrame.html
inspector/timeline/timeline-event-EventDispatch.html
inspector/timeline/timeline-event-FireAnimationFrame.html
inspector/timeline/timeline-event-RequestAnimationFrame.html
inspector/timeline/timeline-event-TimerFire.html
inspector/timeline/timeline-event-TimerInstall.html
inspector/timeline/timeline-event-TimerRemove.html

Combined History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Ftimeline%2Ftimeline-event-CancelAnimationFrame.html%20inspector%2Ftimeline%2Ftimeline-event-EventDispatch.html%20inspector%2Ftimeline%2Ftimeline-event-FireAnimationFrame.html%20inspector%2Ftimeline%2Ftimeline-event-RequestAnimationFrame.html%20inspector%2Ftimeline%2Ftimeline-event-TimerFire.html%20inspector%2Ftimeline%2Ftimeline-event-TimerInstall.html%20inspector%2Ftimeline%2Ftimeline-event-TimerRemove.html
Comment 29 Truitt Savell 2019-03-21 10:18:00 PDT
Looks like this also broke inspector/timeline/line-column.html again.

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Ftimeline%2Fline-column.html
Comment 30 Devin Rousso 2019-03-21 10:47:46 PDT
(In reply to Truitt Savell from comment #28)
> Looks like all of the tests added in
> https://trac.webkit.org/changeset/243269/webkit
> 
> are flaky:
> inspector/timeline/timeline-event-CancelAnimationFrame.html
> inspector/timeline/timeline-event-EventDispatch.html
> inspector/timeline/timeline-event-FireAnimationFrame.html
> inspector/timeline/timeline-event-RequestAnimationFrame.html
> inspector/timeline/timeline-event-TimerFire.html
> inspector/timeline/timeline-event-TimerInstall.html
> inspector/timeline/timeline-event-TimerRemove.html
> 
> Combined History:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=inspector%2Ftimeline%2Ftimeline-event-
> CancelAnimationFrame.html%20inspector%2Ftimeline%2Ftimeline-event-
> EventDispatch.html%20inspector%2Ftimeline%2Ftimeline-event-
> FireAnimationFrame.html%20inspector%2Ftimeline%2Ftimeline-event-
> RequestAnimationFrame.html%20inspector%2Ftimeline%2Ftimeline-event-TimerFire.
> html%20inspector%2Ftimeline%2Ftimeline-event-TimerInstall.
> html%20inspector%2Ftimeline%2Ftimeline-event-TimerRemove.html

(In reply to Truitt Savell from comment #29)
> Looks like this also broke inspector/timeline/line-column.html again.
> 
> History:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=inspector%2Ftimeline%2Fline-column.html

(╯°□°)╯︵ ┻━┻
Comment 31 Devin Rousso 2019-03-21 12:56:29 PDT
(In reply to Truitt Savell from comment #29)
> Looks like this also broke inspector/timeline/line-column.html again.
> 
> History:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=inspector%2Ftimeline%2Fline-column.html
Fixed in r243317 <https://trac.webkit.org/changeset/243317>.

Still investigating the other failures.
Comment 32 Devin Rousso 2019-03-22 16:35:59 PDT
(In reply to Truitt Savell from comment #28)
> Looks like all of the tests added in
> https://trac.webkit.org/changeset/243269/webkit
> 
> are flaky:
> inspector/timeline/timeline-event-CancelAnimationFrame.html
> inspector/timeline/timeline-event-EventDispatch.html
> inspector/timeline/timeline-event-FireAnimationFrame.html
> inspector/timeline/timeline-event-RequestAnimationFrame.html
> inspector/timeline/timeline-event-TimerFire.html
> inspector/timeline/timeline-event-TimerInstall.html
> inspector/timeline/timeline-event-TimerRemove.html
> 
> Combined History:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=inspector%2Ftimeline%2Ftimeline-event-
> CancelAnimationFrame.html%20inspector%2Ftimeline%2Ftimeline-event-
> EventDispatch.html%20inspector%2Ftimeline%2Ftimeline-event-
> FireAnimationFrame.html%20inspector%2Ftimeline%2Ftimeline-event-
> RequestAnimationFrame.html%20inspector%2Ftimeline%2Ftimeline-event-TimerFire.
> html%20inspector%2Ftimeline%2Ftimeline-event-TimerInstall.
> html%20inspector%2Ftimeline%2Ftimeline-event-TimerRemove.html
Fixed in r243405 <http://trac.webkit.org/changeset/243405>.