Bug 192443

<rdar://problem/46608087>

Regressed in https://trac.webkit.org/changeset/238599.

https://webkit.org/b/191483 removed TreeElement's mousedown handler, and replaced it with one in TreeOutline. The new handler is failing to lookup the clicked TreeElement because TreeOutline.treeElementForEvent appears to break when the TreeOutline's scroll container is wider than its DOM element: treeElementFromEvent(event) { let scrollContainer = this.element.parentElement; // We choose this X coordinate based on the knowledge that our list // items extend at least to the trailing edge of the outer <ol> container. // In the no-word-wrap mode the outer <ol> may be wider than the tree container // (and partially hidden), in which case we are left to use only its trailing boundary. // This adjustment is useful in order to find the inner-most tree element that // lines up horizontally with the location of the event. If the mouse event // happened in the space preceding a nested tree element (in the leading indentated // space) we use this adjustment to get the nested tree element and not a tree element // from a parent / outer tree outline / tree element. // // NOTE: This can fail if there is floating content over the trailing edge of // the <li> content, since the element from point could hit that. let isRTL = WI.resolvedLayoutDirection() === WI.LayoutDirection.RTL; let trailingEdgeOffset = isRTL ? 36 : (scrollContainer.offsetWidth - 36); let x = scrollContainer.totalOffsetLeft + trailingEdgeOffset; let y = event.pageY; // Our list items have 1-pixel cracks between them vertically. We avoid // the cracks by checking slightly above and slightly below the mouse // and seeing if we hit the same element each time. let elementUnderMouse = this.treeElementFromPoint(x, y); let elementAboveMouse = this.treeElementFromPoint(x, y - 2); let element = null; if (elementUnderMouse === elementAboveMouse) element = elementUnderMouse; else element = this.treeElementFromPoint(x, y + 2); return element; } For the Timelines TreeOutline, `trailingEdgeOffset` will be a large number because `scrollContainer` is the TimelineOverview. This means the value of `x` passed to `treeElementFromPoint` is outside the bounds of the TimelineTreeElement. This bug would have been introduced when TimelineSidebarPanel was removed, which changed its scroll container from the sidebar to the overview.

Created attachment 357005 [details] Patch

Comment on attachment 357005 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357005&action=review > Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:892 > + // could be indented, in which case we can't rely on it's DOM element to be Typo: "it's" => "its" > Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:897 > + // (and partially hidden), in which case we use the edge of it's container. Typo: "it's" => "its" > Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:901 > + let scrollContainer = this.element.parentElement; > + if (scrollContainer.offsetWidth > this.element.offsetWidth) > + scrollContainer = this.element; I'm not sure I understand this logic. If the container is larger than the <ol> we switch to the <ol>? I think we bumped into this case with context menus of nested ObjectTrees (logging an array of objects of objects to the console). Did you notice any change in behavior across the console?

(In reply to Matt Baker from comment #3) > https://webkit.org/b/191483 removed TreeElement's mousedown handler, and > replaced it with one in TreeOutline. The new handler is failing to lookup > the clicked TreeElement because TreeOutline.treeElementForEvent appears to > break when the TreeOutline's scroll container is wider than its DOM element: Can you draw me some boxes. If I click here where did it actually hit test? |--------------------------------------| | | | |------------------------| | | | | | | | X | | | | | | | |------------------------| | | | |--------------------------------------|

(In reply to Joseph Pecoraro from comment #6) > (In reply to Matt Baker from comment #3) > > https://webkit.org/b/191483 removed TreeElement's mousedown handler, and > > replaced it with one in TreeOutline. The new handler is failing to lookup > > the clicked TreeElement because TreeOutline.treeElementForEvent appears to > > break when the TreeOutline's scroll container is wider than its DOM element: > > Can you draw me some boxes. If I click here where did it actually hit test? > > > |--------------------------------------| > | | > | |------------------------| | > | | | | > | | X | | > | | | | > | |------------------------| | > | | > |--------------------------------------| If the outer box is the `scrollContainer` (the TimelineOverview), and the inner box is the <ol>, then we hit test here: |--------------------------------------| | | | |------------------------| | | | | | | | | X-| | | | | | |------------------------| | | | |--------------------------------------| Where the gap '-' is 36 pixels from the trailing edge of the TimelineOverview.

Created attachment 357020 [details] Patch

Comment on attachment 357020 [details] Patch Hmm, okay. r=me

Comment on attachment 357020 [details] Patch Clearing flags on attachment: 357020 Committed r239066: <https://trac.webkit.org/changeset/239066>

All reviewed patches have been landed. Closing bug.