Summary: Sidebar should always reveal active call frame when hitting a breakpoint. DebuggerSidebarPanel should reveal the TreeElement for the active call frame when handling WI.DebuggerManager.Event.CallFramesDidChange, after refreshing the target ThreadTreeElement. Note 1: Refreshing the target ThreadTreeElement will cause the active call frame TreeElement to be selected. It just needs to be revealed. Note 2: The call stack's DetailsSection header has "position: sticky", and may cover the revealed TreeElement even after calling scrollIntoViewIfNeeded on it's DOM node. NavigationSidebarPanel should compensate for this whenever one of its TreeElements is revealed. Note 3: The sticky header also covers the selected TreeElement if it was scrolled in from the top edge, using the up arrow key. This patch will also address this.
<rdar://problem/46719447>
Created attachment 370582 [details] Patch
Comment on attachment 370582 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370582&action=review > Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:639 > + if (parent.__detailsSection) { I'm not sure I like this approach. I agree that it's the most "widespread" and straightforward solution, but it's very weird to have this relationship between `WI.TreeOutline` and `WI.DetailsSection` be managed by yet a third party, `WI.NavigationSidebarPanel`. It also means that every time you reveal a `WI.TreeElement` in ANY navigation sidebar, it does all this work, which isn't useful for most (if not all) of the navigation sidebars in other tabs. What about instead having `WI.TreeOutline.set calculateStickyHeaderSize` (or some other name) that you can use to provide a callback (or just a hardcoded value) that the `WI.TreeOutline` (or more specific `WI.TreeElement`) can use to determine how much to offset?
Comment on attachment 370582 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370582&action=review >> Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:639 >> + if (parent.__detailsSection) { > > I'm not sure I like this approach. I agree that it's the most "widespread" and straightforward solution, but it's very weird to have this relationship between `WI.TreeOutline` and `WI.DetailsSection` be managed by yet a third party, `WI.NavigationSidebarPanel`. It also means that every time you reveal a `WI.TreeElement` in ANY navigation sidebar, it does all this work, which isn't useful for most (if not all) of the navigation sidebars in other tabs. > > What about instead having `WI.TreeOutline.set calculateStickyHeaderSize` (or some other name) that you can use to provide a callback (or just a hardcoded value) that the `WI.TreeOutline` (or more specific `WI.TreeElement`) can use to determine how much to offset? It's a bit hacky, but TreeOutline shouldn't need to know about sticky headers or anything related to the context in which the control is being used. NavigationSidebarPanel currently seems like the best level to handle this, since it owns DetailsSections, and already has a factory method used by subclasses to create TreeOutlines. I agree that locating the DetailsSection for every revealed TreeElement isn't great. I'll see if I can write this another way.
Created attachment 370982 [details] Patch
Comment on attachment 370982 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370982&action=review I think this is a much better approach than before (no expando 😃). I'd like to try it out a bit before reviewing more. > Source/WebInspectorUI/ChangeLog:18 > + * UserInterface/Views/DebuggerSidebarPanel.js: Can you also make this change for `SourcesNavigationSidebarPanel`, as I'd imagine that that has the same issue too. > Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:332 > + createContentTreeOutline({ignoreCookieRestoration, suppressFiltering} = {}) I'd just call this `options = {}` (or even `...args`) since you aren't actually doing anything with the arguments, just passing them along. > Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:338 > + let treeOutline = treeElement.treeOutline; This variable is unused. Please remove it. > Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:352 > + let treeElementRect = treeElement.listItemElement.getBoundingClientRect(); NIT: you could just use `treeElement.listItemElement.totalOffsetTop` (which is a utility we add that does the same thing). > Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:358 > + let treeOutline = super.createContentTreeOutline({ignoreCookieRestoration, suppressFiltering}) Ditto (>332). > Source/WebInspectorUI/UserInterface/Views/DetailsSection.js:88 > + get headerElement() Style: simple getters should be on one line. > Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:263 > + set treeElementRevealedHandler(revealHandler) Rather than have a `set`er for some callback function, I think this makes more sense to have as an event that get's fired. That matches the existing pattern of other things like this.
Comment on attachment 370982 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370982&action=review r-, as this throws an error when I try to switch to the Debugger tab: TypeError: undefined is not an object (evaluating 'detailsSection.element') (at DebuggerSidebarPanel.js:343:89) ? @ DebuggerSidebarPanel.js:343:89 find @ [native code] treeElementRevealed @ DebuggerSidebarPanel.js:343:54 didRevealTreeElement @ TreeOutline.js:900:45 reveal @ TreeElement.js:493:50 revealAndSelect @ TreeElement.js:535:20 _revealAndSelectRepresentedObject @ ContentBrowserTabContentView.js:319:40 _contentBrowserCurrentContentViewDidChange @ ContentBrowserTabContentView.js:301:47 dispatch @ Object.js:165:30 dispatchEventToListeners @ Object.js:172:17 _currentContentViewDidChange @ ContentBrowser.js:520:38 dispatch @ Object.js:165:30 dispatchEventToListeners @ Object.js:172:17 showBackForwardEntryForIndex @ ContentViewContainer.js:170:38 showContentView @ ContentViewContainer.js:142:42 showDefaultContentViewForTreeElement @ NavigationSidebarPanel.js:202:82 _checkElementsForPendingViewStateCookie @ NavigationSidebarPanel.js:724:79 _checkOutlinesForPendingViewStateCookie @ NavigationSidebarPanel.js:680:53 restoreStateFromCookie @ NavigationSidebarPanel.js:245:53 restoreStateFromCookie @ DebuggerSidebarPanel.js:422:41 restoreStateFromCookie @ TabContentView.js:146:63 shown @ TabContentView.js:125:40 shown @ ContentBrowserTabContentView.js:106:20 prepareToShow @ BackForwardEntry.js:84:35 _showEntry @ ContentViewContainer.js:450:28 showBackForwardEntryForIndex @ ContentViewContainer.js:166:28 showContentView @ ContentViewContainer.js:142:42 _tabBarItemSelected @ TabBrowser.js:238:55 dispatch @ Object.js:165:30 dispatchEventToListeners @ Object.js:172:17 selectedTabBarItem @ LegacyTabBar.js:386:38 _handleMouseDown @ LegacyTabBar.js:635:13 _handleMouseDown @ [native code] > Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:334 > + let detailsSections = [this._pauseReasonSection, this._callStackSection, this._breakpointsSection, this._scriptsSection]; You should move the instantiation of this inside `treeElementRevealed`, since they could be null when `createContentTreeOutline` is called (it's called in the constructor of the superclass `WI.NavigationSidebarPanel`). > Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:336 > + function treeElementRevealed(treeElement) Style: this could be inlined.
Comment on attachment 370982 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370982&action=review >> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:334 >> + let detailsSections = [this._pauseReasonSection, this._callStackSection, this._breakpointsSection, this._scriptsSection]; > > You should move the instantiation of this inside `treeElementRevealed`, since they could be null when `createContentTreeOutline` is called (it's called in the constructor of the superclass `WI.NavigationSidebarPanel`). Nice catch! >> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:263 >> + set treeElementRevealedHandler(revealHandler) > > Rather than have a `set`er for some callback function, I think this makes more sense to have as an event that get's fired. That matches the existing pattern of other things like this. I'll put back the WI.TreeOutline.Event.ElementRevealed event from the initial patch.
Created attachment 371162 [details] Patch
Comment on attachment 371162 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371162&action=review r=me, but I haven't been able to apply/test locally because my build is broken :/ > Source/WebInspectorUI/ChangeLog:23 > + (WI.DetailsSection.prototype.get element): NIT: since you're not really "editing" this function, I'd remove it from the ChangeLog. > Source/WebInspectorUI/ChangeLog:24 > + (WI.DetailsSection.prototype.get headerElement): NIT: I like to add an "Added." suffix to functions that are new :) > Source/WebInspectorUI/ChangeLog:25 > + (WI.DetailsSection.prototype.get identifier): Ditto (>23). > Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:348 > + let topOffset = headerRect.bottom - treeElement.listItemElement.totalOffsetTop; It's interesting how we have a `totalOffset*` for everything but "bottom". Perhaps you could add one (e.g. `Element.prototype.totalOffsetBottom`)? > Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:493 > + let topOffset = headerRect.bottom - treeElement.listItemElement.totalOffsetTop; Ditto (>DebuggerSidebarPanel.js:348). > Source/WebInspectorUI/UserInterface/Views/TreeElement.js:493 > + this.treeOutline.didRevealTreeElement(this); It's fine to just inline this function here. We do similar things elsewhere for `TreeElement`. ``` if (this.treeOutline) this.treeOutline.dispatchEventToListeners(WI.TreeOutline.Event.ElementRevealed, {treeElement: this}); ``` > Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:1169 > + ElementRevealed: Symbol("element-revealed"), NIT: I know we used `Symbol`s in the past, but I personally prefer a more verbose string (e.g. `tree-outline-element-revealed`) so that it's globally unique/searchable.
Comment on attachment 371162 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371162&action=review >> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:348 >> + let topOffset = headerRect.bottom - treeElement.listItemElement.totalOffsetTop; > > It's interesting how we have a `totalOffset*` for everything but "bottom". Perhaps you could add one (e.g. `Element.prototype.totalOffsetBottom`)? Will do. >> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:1169 >> + ElementRevealed: Symbol("element-revealed"), > > NIT: I know we used `Symbol`s in the past, but I personally prefer a more verbose string (e.g. `tree-outline-element-revealed`) so that it's globally unique/searchable. I want to be consistent with the surrounding code. If we agree on strings over Symbols, we should just change the whole code base in one go and update our style guidelines.
Created attachment 371165 [details] Patch for landing
Created attachment 371166 [details] Patch for landing
The commit-queue encountered the following flaky tests while processing attachment 371166 [details]: inspector/canvas/recording.html bug 198470 (authors: drousso@apple.com and mattbaker@apple.com) The commit-queue is continuing to process your patch.
The commit-queue encountered the following flaky tests while processing attachment 371166 [details]: fetch/fetch-worker-crash.html bug 187257 (author: youennf@gmail.com) The commit-queue is continuing to process your patch.
Comment on attachment 371166 [details] Patch for landing Clearing flags on attachment: 371166 Committed r246026: <https://trac.webkit.org/changeset/246026>
All reviewed patches have been landed. Closing bug.