Bug 192443 – Web Inspector: REGRESSION (r238599): unable to select specific timeline

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

* STEPS TO REPRODUCE:
1. open WebInspector
2. show Timelines tab
3. click on any of the timelines in the left "sidebar"
 => timeline isn't selected

* NOTE:
This is most likely a regression of multiple selection <https://webkit.org/b/191483>, but I'm leaving it as ? as I'm not completely sure of it.

[Radar WebKit Bug Importer]

<rdar://problem/46608087>

[Matt Baker]

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

[Matt Baker]

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.

[Matt Baker]

Created attachment 357005 [details]
Patch

[Joseph Pecoraro]

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?

[Joseph Pecoraro]

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

[Matt Baker]

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

[Matt Baker]

Created attachment 357020 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 357020 [details]
Patch

Hmm, okay. r=me

[WebKit Commit Bot]

Comment on attachment 357020 [details]
Patch

Clearing flags on attachment: 357020

Committed r239066: <https://trac.webkit.org/changeset/239066>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.