Similar to XHR breakpoints, we should have the ability to set a breakpoint on all "click" events such that each time a registered event listener for "click" is about to fire, we break.
Created attachment 334593 [details] Patch
Created attachment 334594 [details] [Image] After Patch is applied - Pause
Comment on attachment 334593 [details] Patch Attachment 334593 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/6671567 New failing tests: inspector/dom-debugger/event-listener-breakpoints.html
Created attachment 334596 [details] Archive of layout-test-results from ews102 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 334593 [details] Patch Attachment 334593 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/6671581 New failing tests: inspector/dom-debugger/event-listener-breakpoints.html
Created attachment 334597 [details] Archive of layout-test-results from ews105 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 334593 [details] Patch Attachment 334593 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/6672818 New failing tests: inspector/dom-debugger/event-listener-breakpoints.html
Created attachment 334603 [details] Archive of layout-test-results from ews113 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 334633 [details] Patch
Comment on attachment 334633 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334633&action=review > Source/WebCore/ChangeLog:8 > + Test: inspector/dom-debugger/event-listener-breakpoints.html This is a cool patch, but I want to nail down the use cases/behavior of the new feature before getting into the weeds with code comments. It's not obvious from the screenshot how this will be used by a developer. Is it to pause on an event to see if it fires at all prior to adding an event listener? an easy way to set breakpoints on event listeners which may be scattered throughout the web app? As it is written, this patch will only pause when an event listener is about to run for the specified event type. I think it'd be useful for the backend to support pausing for *any* event regardless of whether it has a listener in the code right now or not. One option, which may be preferable anyway, is for Inspector to inject its own event listener so that the existing code path "just works" even if there is no listener in the page's code. If we don't do something like that, certain events such as wheel/scroll events will never actually be dispatched on the main thread if there are no listeners on the page. > Source/WebInspectorUI/ChangeLog:7 > + Please include high-level description of the design/implementation.
Comment on attachment 334633 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334633&action=review >> Source/WebCore/ChangeLog:8 >> + Test: inspector/dom-debugger/event-listener-breakpoints.html > > This is a cool patch, but I want to nail down the use cases/behavior of the new feature before getting into the weeds with code comments. > > It's not obvious from the screenshot how this will be used by a developer. Is it to pause on an event to see if it fires at all prior to adding an event listener? an easy way to set breakpoints on event listeners which may be scattered throughout the web app? > As it is written, this patch will only pause when an event listener is about to run for the specified event type. I think it'd be useful for the backend to support pausing for *any* event regardless of whether it has a listener in the code right now or not. > > One option, which may be preferable anyway, is for Inspector to inject its own event listener so that the existing code path "just works" even if there is no listener in the page's code. If we don't do something like that, certain events such as wheel/scroll events will never actually be dispatched on the main thread if there are no listeners on the page. My "use case" is to debug complex pages where an unexpected `event.preventDefault()` is getting called somewhere, preventing the desired event listener from being run. As far as pausing for *any* event, I was thinking that that would be a followup of sorts. This patch is more of a "proof of concept". My "vision" for doing that would be to add a checkbox to the EventListenerBreakpointPopover that says something like "Force Pause if no Event Listeners are registered". The "followup" patch to this <https://webkit.org/b/183138> uses the same instrumentation point to pause the next time the given event is fired for the given listener on the given element (basically trying to be as specific as possible), since a callback might be used for multiple event listeners or the element might have multiple callbacks for the same event. Just so that it's also written down somewhere, I plan to add global breakpoints similar to XHR's "All Requests" for `setTimeout`, `setInterval`, and `requestAnimationFrame`. In the future, some of this logic could also probably play into breaking whenever a Promise resolves/rejects, but that probably would require additional instrumentation points. >> Source/WebInspectorUI/ChangeLog:7 >> + > > Please include high-level description of the design/implementation. The vast majority of this is UI, but I can see how it could be confusing. I'll give an explanation of how it all ties together.
Screen shots showing the UI that allows the user to actually add, for example, a "click" event breakpoint on an element would be helpful.
(In reply to Matt Baker from comment #12) > Screen shots showing the UI that allows the user to actually add, for > example, a "click" event breakpoint on an element would be helpful. Right now the UI is just a popover with an input field. There's nothing "special" about it. I'll do a build overnight and post something tomorrow. :)
Created attachment 335486 [details] [Image] After Patch is applied - Popover
Created attachment 335762 [details] Patch
<rdar://problem/40430640>
Created attachment 346100 [details] [Image] Xcode Symbolic Breakpoint - UI Inspiration
Created attachment 346101 [details] [Image] EventBreakpoint.svg
Comment on attachment 335762 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335762&action=review This looks great to me! But the patch no longer applies. Rebaseline it, and address my few comments above, and lets get this in! I may have some additional comments on the next round after I get to play with this. > Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:213 > + if (this._eventListenerBreakpoints.includes(breakpoint)) > + return; > + > + if (this._eventListenerBreakpoints.some((item) => item.eventName === breakpoint.eventName)) > + return; Given we always do the bottom expression I guess we can drop the includes check, because it is just redundant. > Source/WebInspectorUI/UserInterface/Controllers/EventListenerBreakpointTreeController.js:26 > +WI.EventListenerBreakpointTreeController = class EventListenerBreakpointTreeController extends WI.Object May not need to extend WI.Object if you don't dispatch events. > Source/WebInspectorUI/UserInterface/Models/EventListenerBreakpoint.js:31 > + Nit: I like throwing validation lines in here, as it'll show what arguments are required and their expected types. For example here I would probably just do at least: console.assert(typeof eventName === "string") > Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:1237 > + popover.show(event.target.element, [WI.RectEdge.MAX_Y, WI.RectEdge.MIN_Y, WI.RectEdge.MAX_X]); After reading this, and pointing my finger in various directions like a complete fool, I wonder if it would be easier to have aliases of these: WI.Popover.Present.Above WI.Popover.Present.Below WI.Popover.Present.After (which would be Left or Right depending on RTL) WI.Popover.Present.Before (which would be Left or Right depending on RTL) > Source/WebInspectorUI/UserInterface/Views/EventListenerBreakpointPopover.js:57 > + label.textContent = WI.UIString("Break on listeners for event with name:"); UI: In the screenshot the <div> and <input> look a little too close together. We may want to run this past someone with UI chops. This label also feels too long. Take a look at the Xcode UI for adding Symbolic breakpoints. I think we could do something simpler like that. > Source/WebInspectorUI/UserInterface/Views/EventListenerBreakpointPopover.js:59 > + this._inputElement = contentElement.appendChild(document.createElement("input")); I'd like to see a placeholder for this. Perhaps just: this._inputElement.placeholder = "click" Would be enough for users to know what to type in here. We may even be able to include autocompletion for a bunch of event names. WI.ScriptTimelineRecord.EventType.displayName has 146 of them, but some look out of date. We could do better and autogenerate from EventNames.h. > Source/WebInspectorUI/UserInterface/Views/EventListenerBreakpointTreeElement.js:33 > + if (!className) > + className = WI.BreakpointTreeElement.GenericLineIconStyleClassName; UI: I'd like the default to be a blue [E] icon. The core ideas being: • [E] means Event / Event Listener in other places in our UI • We pretty consistently use blue [#] icons to mean breakpoints Basically modify EventListener.svg to have the color of Exception.svg. NOTE: I just did that to produce the attached EventBreakpoint.svg. > LayoutTests/inspector/dom-debugger/event-listener-breakpoints-expected.txt:6 > +Adding Breakpoint... This would read better if you included the event name in the test. Perhaps something like: Adding Breakpoint (click)... Adding "click" Event Breakpoint... > LayoutTests/inspector/dom-debugger/event-listener-breakpoints-expected.txt:35 > +-- Running test teardown. I would like to see another test for a custom event name and dispatch: (it could be a separate test file if you want to keep this fundamental) https://developer.mozilla.org/en-US/docs/Web/Guide/Events/Creating_and_triggering_events If I understand things correctly if code on the page dispatches a custom event "TestEvent", and we have a breakpoint on that name, we could potentially show the dispatchEvent(...) in the backtrace, which would be pretty cool. This also brings to mind that we should have a `$event` exposed to the console when breaking inside of an event listener, like we do `$exception` when breaking inside of an exception handler (catch block). That can be a separate enhancement, but we should file a bug report on that. Technically this could be different from `window.event` but maybe that is good enough for an initial implementation. > LayoutTests/inspector/dom-debugger/event-listener-breakpoints.html:37 > + return new Promise((resolve, reject) => { > + InspectorTest.evaluateInPage(`clickBody()`, (error) => { > + if (error) > + reject(error); > + else > + resolve(); > + }); > + }); > + } This ma be the same as just: return InspectorTest.evaluateInPage(`clickBody()`);
Created attachment 346363 [details] Patch This patch is mainly a rebase with a few style changes. My build is currently broken, so I was unable to test it. :/
Comment on attachment 346363 [details] Patch Attachment 346363 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/8735706 New failing tests: inspector/dom-debugger/event-listener-breakpoints.html
Created attachment 346367 [details] Archive of layout-test-results from ews103 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 346363 [details] Patch Attachment 346363 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/8735713 New failing tests: inspector/dom-debugger/event-listener-breakpoints.html
Created attachment 346368 [details] Archive of layout-test-results from ews104 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 346363 [details] Patch Attachment 346363 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/8735760 New failing tests: inspector/dom-debugger/event-listener-breakpoints.html
Created attachment 346369 [details] Archive of layout-test-results from ews116 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 346384 [details] Patch Oops. Missed one test result.
Created attachment 347172 [details] Patch
Comment on attachment 347172 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347172&action=review I have a few remaining comments / questions. 1. How does this behave when we have multiple event listeners for a single event? Does it pause in each handler, just the first handler? Test Page: document.body.addEventListener("click", () => { console.log(1); }); document.body.addEventListener("click", () => { console.log(2); }); Scenario: 1. Inspect test page 2. Add Event breakpoint for "click" 3. Click the body of the page => What happens? Do we pause in just (1), or both (1) and (2)? 2. Do event breakpoints properly persist across page loads? Test Page: document.body.addEventListener("click", () => { console.log(1); }); Scenario: 1. Inspect test page 2. Add Event breakpoint for "click" 3. Reload the page 4. Click the body => Do we pause? This could have a test. I expect this to work, but this is a backend implementation detail that is testable and should be tested. > Source/WebInspectorUI/ChangeLog:15 > + Event listener breakpoints are created in the Debugger tab in a new "Event Listener Breakpoints" Should we just call these "Event Breakpoints" instead of "Event Listener Breakpoints"? > Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:36 > + this._eventListenerBreakpointSetting = new WI.Setting("event-listener-breakpoint", []); I feel like this should be plural: `this._eventListenerBreakpointsSettings` > Source/WebInspectorUI/UserInterface/Views/EventListenerBreakpointPopover.js:60 > + this._inputElement = contentElement.appendChild(document.createElement("input")); > + this._inputElement.placeholder = "click"; Pretty much everywhere we create input elements we end up doing: this._inputElement.spellcheck = false; So we will probably want to do that here as well!
Comment on attachment 347172 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347172&action=review (In reply to Joseph Pecoraro from comment #29) > 1. How does this behave when we have multiple event listeners for a single > event? Does it pause in each handler, just the first handler? > > Test Page: > document.body.addEventListener("click", () => { console.log(1); }); > document.body.addEventListener("click", () => { console.log(2); }); > > Scenario: > 1. Inspect test page > 2. Add Event breakpoint for "click" > 3. Click the body of the page > => What happens? > > Do we pause in just (1), or both (1) and (2)? Both (1) and (2). > 2. Do event breakpoints properly persist across page loads? > > Test Page: > document.body.addEventListener("click", () => { console.log(1); }); > > Scenario: > 1. Inspect test page > 2. Add Event breakpoint for "click" > 3. Reload the page > 4. Click the body > => Do we pause? Yes, it persists across page loads and navigations. One thing to note, however, is that the breakpoint is global (not specific to a url/domain) and will persist, similar to "All XHR" or "Uncaught Exception" breakpoints. > This could have a test. I expect this to work, but this is a backend > implementation detail that is testable and should be tested. I've spoken with Brian in the past about this, and he suggested that I avoid writing tests that involve a reload as they tend to not work :/ >> Source/WebInspectorUI/ChangeLog:15 >> + Event listener breakpoints are created in the Debugger tab in a new "Event Listener Breakpoints" > > Should we just call these "Event Breakpoints" instead of "Event Listener Breakpoints"? I remember having a reason for calling this "Event Listener Breakpoints", but I don't actually remember what it was. :| Event Breakpoints just sounds cleaner (plus we can "expand" this to more "events", like `requestAnimationFrame` or `setTimeout`). >> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:36 >> + this._eventListenerBreakpointSetting = new WI.Setting("event-listener-breakpoint", []); > > I feel like this should be plural: `this._eventListenerBreakpointsSettings` Yup. My mistake. `this._eventListenerBreakpointsSetting`
(In reply to Devin Rousso from comment #30) > Comment on attachment 347172 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=347172&action=review > > (In reply to Joseph Pecoraro from comment #29) > > 1. How does this behave when we have multiple event listeners for a single > > event? Does it pause in each handler, just the first handler? > > > > Test Page: > > document.body.addEventListener("click", () => { console.log(1); }); > > document.body.addEventListener("click", () => { console.log(2); }); > > > > Scenario: > > 1. Inspect test page > > 2. Add Event breakpoint for "click" > > 3. Click the body of the page > > => What happens? > > > > Do we pause in just (1), or both (1) and (2)? > > Both (1) and (2). Fantastic! > > 2. Do event breakpoints properly persist across page loads? > > > > Test Page: > > document.body.addEventListener("click", () => { console.log(1); }); > > > > Scenario: > > 1. Inspect test page > > 2. Add Event breakpoint for "click" > > 3. Reload the page > > 4. Click the body > > => Do we pause? > > Yes, it persists across page loads and navigations. One thing to note, > however, is that the breakpoint is global (not specific to a url/domain) and > will persist, similar to "All XHR" or "Uncaught Exception" breakpoints. > > > This could have a test. I expect this to work, but this is a backend > > implementation detail that is testable and should be tested. > > I've spoken with Brian in the past about this, and he suggested that I avoid > writing tests that involve a reload as they tend to not work :/ I had also had issues with this in the past but I think we've solved them. `InspectorTest.reloadPage()` returns a Promise that should be good enough to use. FWIW I recommend writing such a test in its own file, not in this same file. That way if this does somehow become flakey we won't lose all test coverage. Something like this (pseudocode) seems reasonable. I like the idea of testing the `load` event because its so tricky otherwise, but it could be something more basic like `click`. async test() { await setEventBreakpoint("load"); InspectorTest.reloadPage(); await WI.debuggerManager.awaitEvent(WI.DebuggerManager.Event.Paused); InspectorTest.pass("Pause after page reloaded."); // Verify its a load event... WI.debuggerManager.resume(); }
> > > Do we pause in just (1), or both (1) and (2)? > > > > Both (1) and (2). > > Fantastic! Oh and this can be tested as well. We probably should. It seems pretty straightforward.
Comment on attachment 347172 [details] Patch r=me, but lets try to add those additional tests.
Created attachment 347331 [details] Patch
Comment on attachment 347331 [details] Patch Clearing flags on attachment: 347331 Committed r234974: <https://trac.webkit.org/changeset/234974>
All reviewed patches have been landed. Closing bug.
Comment on attachment 347331 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347331&action=review Thanks for adding the additional tests! > LayoutTests/inspector/dom-debugger/event-breakpoint-with-navigation-expected.txt:7 > +Reloading WebInspector... This is reloading the inspected page, not WebInspector. > LayoutTests/inspector/dom-debugger/event-breakpoint-with-navigation.html:16 > + name: `DOMDebugger.EventWithNavigation.AddBreakpoint`, Weird name. I'd probably go with DOMDebugger.EventBreakpoint.TriggerAfterNavigation or some such. Doesn't really matter though. > LayoutTests/inspector/dom-debugger/event-breakpoint-with-navigation.html:60 > + teardown(resolve, reject) { This teardown isn't really needed unless we were to add other tests. The Test WebInspector frontend doesn't persist settings. > LayoutTests/inspector/dom-debugger/event-breakpoints.html:201 > + name: `DOMDebugger.Event.AddMultipleBreakpoints`, This is a poor name. It's not adding multiple breakpoints, its pausing on multiple listeners. Or pausing multiple times for the same event.
Looks like the new test inspector/dom-debugger/event-breakpoint-with-navigation.html is a flakey timeout. Test History: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Fdom-debugger%2Fevent-breakpoint-with-navigation.html also in the trac log the test is called: inspector/dom-debugger/event-breakpoints-with-navigation.html: Added. ^ has event-breakpoints while the test file name is event-breakpoint.
(In reply to Truitt Savell from comment #38) > Looks like the new test > inspector/dom-debugger/event-breakpoint-with-navigation.html is a flakey > timeout. > > Test History: > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard. > html#showAllRuns=true&tests=inspector%2Fdom-debugger%2Fevent-breakpoint-with- > navigation.html Marked as flaky in: https://trac.webkit.org/changeset/234996/webkit