Bug 192443 - Web Inspector: REGRESSION (r238599): unable to select specific timeline
Summary: Web Inspector: REGRESSION (r238599): unable to select specific timeline
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: Matt Baker
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-12-05 18:53 PST by Devin Rousso
Modified: 2018-12-10 19:50 PST (History)
5 users (show)

See Also:


Attachments
Patch (2.53 KB, patch)
2018-12-10 15:18 PST, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (2.52 KB, patch)
2018-12-10 17:01 PST, Matt Baker
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2018-12-05 18:53:40 PST
* 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.
Comment 1 Radar WebKit Bug Importer 2018-12-10 14:16:09 PST
<rdar://problem/46608087>
Comment 2 Matt Baker 2018-12-10 14:16:19 PST
Regressed in https://trac.webkit.org/changeset/238599.
Comment 3 Matt Baker 2018-12-10 14:39:09 PST
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.
Comment 4 Matt Baker 2018-12-10 15:18:41 PST
Created attachment 357005 [details]
Patch
Comment 5 Joseph Pecoraro 2018-12-10 16:50:06 PST
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?
Comment 6 Joseph Pecoraro 2018-12-10 16:53:25 PST
(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                     |          |
  |  |                        |          |
  |  |------------------------|          |
  |                                      |
  |--------------------------------------|
Comment 7 Matt Baker 2018-12-10 16:57:10 PST
(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.
Comment 8 Matt Baker 2018-12-10 17:01:20 PST
Created attachment 357020 [details]
Patch
Comment 9 Joseph Pecoraro 2018-12-10 18:22:14 PST
Comment on attachment 357020 [details]
Patch

Hmm, okay. r=me
Comment 10 WebKit Commit Bot 2018-12-10 19:50:33 PST
Comment on attachment 357020 [details]
Patch

Clearing flags on attachment: 357020

Committed r239066: <https://trac.webkit.org/changeset/239066>
Comment 11 WebKit Commit Bot 2018-12-10 19:50:35 PST
All reviewed patches have been landed.  Closing bug.