192443 2018-12-05 18:53:40 -0800 Web Inspector: REGRESSION (r238599): unable to select specific timeline 2018-12-10 19:50:35 -0800 1 1 1 Unclassified WebKit Web Inspector WebKit Nightly Build All All RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=191483 InRadar P2 Normal --- 1 hi mattbaker commit-queue hi inspector-bugzilla-changes joepeck webkit-bug-importer oldest_to_newest 1485545 0 hi 2018-12-05 18:53:40 -0800 * 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. 1486737 1 webkit-bug-importer 2018-12-10 14:16:09 -0800 <rdar://problem/46608087> 1486738 2 mattbaker 2018-12-10 14:16:19 -0800 Regressed in https://trac.webkit.org/changeset/238599. 1486748 3 mattbaker 2018-12-10 14:39:09 -0800 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. 1486770 4 357005 mattbaker 2018-12-10 15:18:41 -0800 Created attachment 357005 Patch 1486818 5 357005 joepeck 2018-12-10 16:50:06 -0800 Comment on attachment 357005 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? 1486821 6 joepeck 2018-12-10 16:53:25 -0800 (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 | | | | | | | |------------------------| | | | |--------------------------------------| 1486823 7 mattbaker 2018-12-10 16:57:10 -0800 (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. 1486824 8 357020 mattbaker 2018-12-10 17:01:20 -0800 Created attachment 357020 Patch 1486842 9 357020 joepeck 2018-12-10 18:22:14 -0800 Comment on attachment 357020 Patch Hmm, okay. r=me 1486864 10 357020 commit-queue 2018-12-10 19:50:33 -0800 Comment on attachment 357020 Patch Clearing flags on attachment: 357020 Committed r239066: <https://trac.webkit.org/changeset/239066> 1486865 11 commit-queue 2018-12-10 19:50:35 -0800 All reviewed patches have been landed. Closing bug. 357005 2018-12-10 15:18:41 -0800 2018-12-10 17:01:18 -0800 Patch bug-192443-20181210151828.patch text/plain 2587 mattbaker U3VidmVyc2lvbiBSZXZpc2lvbjogMjM5MDUwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViSW5zcGVj dG9yVUkvQ2hhbmdlTG9nIGIvU291cmNlL1dlYkluc3BlY3RvclVJL0NoYW5nZUxvZwppbmRleCA1 OTM3Y2FmZDJkZWRmN2ZlZWIwNzVmZTIzYTJhNzFjMDAyODBlYjRiLi4yODE0M2FjNjlhZDZhZWE5 YmI0ODYyYTIzMTNkNzg5MWU4MTc2ZmRkIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViSW5zcGVjdG9y VUkvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxOCBAQAorMjAxOC0xMi0xMCAgTWF0dCBCYWtlciAgPG1hdHRiYWtlckBhcHBsZS5jb20+ CisKKyAgICAgICAgV2ViIEluc3BlY3RvcjogUkVHUkVTU0lPTiAocjIzODU5OSk6IHVuYWJsZSB0 byBzZWxlY3Qgc3BlY2lmaWMgdGltZWxpbmUKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5v cmcvc2hvd19idWcuY2dpP2lkPTE5MjQ0MworICAgICAgICA8cmRhcjovL3Byb2JsZW0vNDY2MDgw ODc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgV2hl biBkZXRlcm1pbmluZyB0aGUgdHJhaWxpbmcgaG9yaXpvbnRhbCBlZGdlIGZvciBUcmVlRWxlbWVu dCBtb3VzZQorICAgICAgICBldmVudHMsIFRyZWVPdXRsaW5lIHNob3VsZCBub3QgYXNzdW1lIHRo YXQgaXRzIGNvbnRhaW5pbmcgRE9NIGVsZW1lbnQKKyAgICAgICAgaXMgb25seSBhcyB3aWRlIGFz IGl0cyA8b2w+IGVsZW1lbnQuCisKKyAgICAgICAgKiBVc2VySW50ZXJmYWNlL1ZpZXdzL1RyZWVP dXRsaW5lLmpzOgorICAgICAgICAoV0kuVHJlZU91dGxpbmUucHJvdG90eXBlLnRyZWVFbGVtZW50 RnJvbUV2ZW50KToKKwogMjAxOC0xMi0xMCAgRGVhbiBKYWNrc29uICA8ZGlub0BhcHBsZS5jb20+ CiAKICAgICAgICAgVXNlIHRleHQvamF2YXNjcmlwdCBhcyByZWNvbW1lbmRlZCBieSB0aGUgSFRN TCBzcGVjaWZpY2F0aW9uCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViSW5zcGVjdG9yVUkvVXNlcklu dGVyZmFjZS9WaWV3cy9UcmVlT3V0bGluZS5qcyBiL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9Vc2Vy SW50ZXJmYWNlL1ZpZXdzL1RyZWVPdXRsaW5lLmpzCmluZGV4IDcyZjJjZmRhZjE3ZTYyNjZiNDc1 YWU5ZDQxNzA1NGQzMGVkYjMzZGMuLjJmYjYyMjdkMjIxODc0MTFjZmJkYzNlY2ZkZWRiZTNkZmVh YWExODYgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9Vc2VySW50ZXJmYWNlL1Zp ZXdzL1RyZWVPdXRsaW5lLmpzCisrKyBiL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9Vc2VySW50ZXJm YWNlL1ZpZXdzL1RyZWVPdXRsaW5lLmpzCkBAIC04ODgsMTIgKzg4OCwxOCBAQCBXSS5UcmVlT3V0 bGluZSA9IGNsYXNzIFRyZWVPdXRsaW5lIGV4dGVuZHMgV0kuT2JqZWN0CiAKICAgICB0cmVlRWxl bWVudEZyb21FdmVudChldmVudCkKICAgICB7Ci0gICAgICAgIGxldCBzY3JvbGxDb250YWluZXIg PSB0aGlzLmVsZW1lbnQucGFyZW50RWxlbWVudDsKLQorICAgICAgICAvLyBXZSBjYW4ndCB0YWtl IGV2ZW50LnBhZ2VYIHRvIGJlIG91ciBYIGNvb3JkaW5hdGUsIHNpbmNlIHRoZSBUcmVlRWxlbWVu dAorICAgICAgICAvLyBjb3VsZCBiZSBpbmRlbnRlZCwgaW4gd2hpY2ggY2FzZSB3ZSBjYW4ndCBy ZWx5IG9uIGl0J3MgRE9NIGVsZW1lbnQgdG8gYmUKKyAgICAgICAgLy8gdW5kZXIgdGhlIG1vdXNl LgogICAgICAgICAvLyBXZSBjaG9vc2UgdGhpcyBYIGNvb3JkaW5hdGUgYmFzZWQgb24gdGhlIGtu b3dsZWRnZSB0aGF0IG91ciBsaXN0CiAgICAgICAgIC8vIGl0ZW1zIGV4dGVuZCBhdCBsZWFzdCB0 byB0aGUgdHJhaWxpbmcgZWRnZSBvZiB0aGUgb3V0ZXIgPG9sPiBjb250YWluZXIuCiAgICAgICAg IC8vIEluIHRoZSBuby13b3JkLXdyYXAgbW9kZSB0aGUgb3V0ZXIgPG9sPiBtYXkgYmUgd2lkZXIg dGhhbiB0aGUgdHJlZSBjb250YWluZXIKLSAgICAgICAgLy8gKGFuZCBwYXJ0aWFsbHkgaGlkZGVu KSwgaW4gd2hpY2ggY2FzZSB3ZSBhcmUgbGVmdCB0byB1c2Ugb25seSBpdHMgdHJhaWxpbmcgYm91 bmRhcnkuCisgICAgICAgIC8vIChhbmQgcGFydGlhbGx5IGhpZGRlbiksIGluIHdoaWNoIGNhc2Ug d2UgdXNlIHRoZSBlZGdlIG9mIGl0J3MgY29udGFpbmVyLgorCisgICAgICAgIGxldCBzY3JvbGxD b250YWluZXIgPSB0aGlzLmVsZW1lbnQucGFyZW50RWxlbWVudDsKKyAgICAgICAgaWYgKHNjcm9s bENvbnRhaW5lci5vZmZzZXRXaWR0aCA+IHRoaXMuZWxlbWVudC5vZmZzZXRXaWR0aCkKKyAgICAg ICAgICAgIHNjcm9sbENvbnRhaW5lciA9IHRoaXMuZWxlbWVudDsKKwogICAgICAgICAvLyBUaGlz IGFkanVzdG1lbnQgaXMgdXNlZnVsIGluIG9yZGVyIHRvIGZpbmQgdGhlIGlubmVyLW1vc3QgdHJl ZSBlbGVtZW50IHRoYXQKICAgICAgICAgLy8gbGluZXMgdXAgaG9yaXpvbnRhbGx5IHdpdGggdGhl IGxvY2F0aW9uIG9mIHRoZSBldmVudC4gSWYgdGhlIG1vdXNlIGV2ZW50CiAgICAgICAgIC8vIGhh cHBlbmVkIGluIHRoZSBzcGFjZSBwcmVjZWRpbmcgYSBuZXN0ZWQgdHJlZSBlbGVtZW50IChpbiB0 aGUgbGVhZGluZyBpbmRlbnRhdGVkCg== 357020 2018-12-10 17:01:20 -0800 2018-12-10 19:50:33 -0800 Patch bug-192443-20181210170107.patch text/plain 2585 mattbaker U3VidmVyc2lvbiBSZXZpc2lvbjogMjM5MDUwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViSW5zcGVj dG9yVUkvQ2hhbmdlTG9nIGIvU291cmNlL1dlYkluc3BlY3RvclVJL0NoYW5nZUxvZwppbmRleCA1 OTM3Y2FmZDJkZWRmN2ZlZWIwNzVmZTIzYTJhNzFjMDAyODBlYjRiLi4yODE0M2FjNjlhZDZhZWE5 YmI0ODYyYTIzMTNkNzg5MWU4MTc2ZmRkIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViSW5zcGVjdG9y VUkvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxOCBAQAorMjAxOC0xMi0xMCAgTWF0dCBCYWtlciAgPG1hdHRiYWtlckBhcHBsZS5jb20+ CisKKyAgICAgICAgV2ViIEluc3BlY3RvcjogUkVHUkVTU0lPTiAocjIzODU5OSk6IHVuYWJsZSB0 byBzZWxlY3Qgc3BlY2lmaWMgdGltZWxpbmUKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5v cmcvc2hvd19idWcuY2dpP2lkPTE5MjQ0MworICAgICAgICA8cmRhcjovL3Byb2JsZW0vNDY2MDgw ODc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgV2hl biBkZXRlcm1pbmluZyB0aGUgdHJhaWxpbmcgaG9yaXpvbnRhbCBlZGdlIGZvciBUcmVlRWxlbWVu dCBtb3VzZQorICAgICAgICBldmVudHMsIFRyZWVPdXRsaW5lIHNob3VsZCBub3QgYXNzdW1lIHRo YXQgaXRzIGNvbnRhaW5pbmcgRE9NIGVsZW1lbnQKKyAgICAgICAgaXMgb25seSBhcyB3aWRlIGFz IGl0cyA8b2w+IGVsZW1lbnQuCisKKyAgICAgICAgKiBVc2VySW50ZXJmYWNlL1ZpZXdzL1RyZWVP dXRsaW5lLmpzOgorICAgICAgICAoV0kuVHJlZU91dGxpbmUucHJvdG90eXBlLnRyZWVFbGVtZW50 RnJvbUV2ZW50KToKKwogMjAxOC0xMi0xMCAgRGVhbiBKYWNrc29uICA8ZGlub0BhcHBsZS5jb20+ CiAKICAgICAgICAgVXNlIHRleHQvamF2YXNjcmlwdCBhcyByZWNvbW1lbmRlZCBieSB0aGUgSFRN TCBzcGVjaWZpY2F0aW9uCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViSW5zcGVjdG9yVUkvVXNlcklu dGVyZmFjZS9WaWV3cy9UcmVlT3V0bGluZS5qcyBiL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9Vc2Vy SW50ZXJmYWNlL1ZpZXdzL1RyZWVPdXRsaW5lLmpzCmluZGV4IDcyZjJjZmRhZjE3ZTYyNjZiNDc1 YWU5ZDQxNzA1NGQzMGVkYjMzZGMuLmJhZWE1NWRkZmJiNThhMGI0MTA1MWEzMjY1MjM1ZjRlY2Q2 ZDI3MTMgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9Vc2VySW50ZXJmYWNlL1Zp ZXdzL1RyZWVPdXRsaW5lLmpzCisrKyBiL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9Vc2VySW50ZXJm YWNlL1ZpZXdzL1RyZWVPdXRsaW5lLmpzCkBAIC04ODgsMTIgKzg4OCwxOCBAQCBXSS5UcmVlT3V0 bGluZSA9IGNsYXNzIFRyZWVPdXRsaW5lIGV4dGVuZHMgV0kuT2JqZWN0CiAKICAgICB0cmVlRWxl bWVudEZyb21FdmVudChldmVudCkKICAgICB7Ci0gICAgICAgIGxldCBzY3JvbGxDb250YWluZXIg PSB0aGlzLmVsZW1lbnQucGFyZW50RWxlbWVudDsKLQorICAgICAgICAvLyBXZSBjYW4ndCB0YWtl IGV2ZW50LnBhZ2VYIHRvIGJlIG91ciBYIGNvb3JkaW5hdGUsIHNpbmNlIHRoZSBUcmVlRWxlbWVu dAorICAgICAgICAvLyBjb3VsZCBiZSBpbmRlbnRlZCwgaW4gd2hpY2ggY2FzZSB3ZSBjYW4ndCBy ZWx5IG9uIGl0cyBET00gZWxlbWVudCB0byBiZQorICAgICAgICAvLyB1bmRlciB0aGUgbW91c2Uu CiAgICAgICAgIC8vIFdlIGNob29zZSB0aGlzIFggY29vcmRpbmF0ZSBiYXNlZCBvbiB0aGUga25v d2xlZGdlIHRoYXQgb3VyIGxpc3QKICAgICAgICAgLy8gaXRlbXMgZXh0ZW5kIGF0IGxlYXN0IHRv IHRoZSB0cmFpbGluZyBlZGdlIG9mIHRoZSBvdXRlciA8b2w+IGNvbnRhaW5lci4KICAgICAgICAg Ly8gSW4gdGhlIG5vLXdvcmQtd3JhcCBtb2RlIHRoZSBvdXRlciA8b2w+IG1heSBiZSB3aWRlciB0 aGFuIHRoZSB0cmVlIGNvbnRhaW5lcgotICAgICAgICAvLyAoYW5kIHBhcnRpYWxseSBoaWRkZW4p LCBpbiB3aGljaCBjYXNlIHdlIGFyZSBsZWZ0IHRvIHVzZSBvbmx5IGl0cyB0cmFpbGluZyBib3Vu ZGFyeS4KKyAgICAgICAgLy8gKGFuZCBwYXJ0aWFsbHkgaGlkZGVuKSwgaW4gd2hpY2ggY2FzZSB3 ZSB1c2UgdGhlIGVkZ2Ugb2YgaXRzIGNvbnRhaW5lci4KKworICAgICAgICBsZXQgc2Nyb2xsQ29u dGFpbmVyID0gdGhpcy5lbGVtZW50LnBhcmVudEVsZW1lbnQ7CisgICAgICAgIGlmIChzY3JvbGxD b250YWluZXIub2Zmc2V0V2lkdGggPiB0aGlzLmVsZW1lbnQub2Zmc2V0V2lkdGgpCisgICAgICAg ICAgICBzY3JvbGxDb250YWluZXIgPSB0aGlzLmVsZW1lbnQ7CisKICAgICAgICAgLy8gVGhpcyBh ZGp1c3RtZW50IGlzIHVzZWZ1bCBpbiBvcmRlciB0byBmaW5kIHRoZSBpbm5lci1tb3N0IHRyZWUg ZWxlbWVudCB0aGF0CiAgICAgICAgIC8vIGxpbmVzIHVwIGhvcml6b250YWxseSB3aXRoIHRoZSBs b2NhdGlvbiBvZiB0aGUgZXZlbnQuIElmIHRoZSBtb3VzZSBldmVudAogICAgICAgICAvLyBoYXBw ZW5lZCBpbiB0aGUgc3BhY2UgcHJlY2VkaW5nIGEgbmVzdGVkIHRyZWUgZWxlbWVudCAoaW4gdGhl IGxlYWRpbmcgaW5kZW50YXRlZAo=