Bug 194254 - Web Inspector: REGRESSION (r240947): Resources tab: can't select main frame after refreshing page
Summary: Web Inspector: REGRESSION (r240947): Resources tab: can't select main frame a...
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: 2019-02-04 16:36 PST by Matt Baker
Modified: 2019-02-05 15:33 PST (History)
4 users (show)

See Also:


Attachments
Patch (2.31 KB, patch)
2019-02-04 17:47 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 Matt Baker 2019-02-04 16:36:20 PST
Summary:
Can't select main frame after refreshing page.

Steps to Reproduce:
1. Inspect webkit.org
2. Open Resource tab
3. In the sidebar, select a child of the main resource
4. Reload page
5. The child should be re-selected after reload
6. Try to select the main resource using the mouse
  => Cannot click to select

Note:
Hitting the up arrow key to select the main resource from the child works fine.
Comment 1 Radar WebKit Bug Importer 2019-02-04 16:36:49 PST
<rdar://problem/47805023>
Comment 2 Matt Baker 2019-02-04 17:16:16 PST
Regressed in https://trac.webkit.org/changeset/240947.
Comment 3 Matt Baker 2019-02-04 17:47:33 PST
Created attachment 361142 [details]
Patch
Comment 4 Matt Baker 2019-02-04 17:48:28 PST
(In reply to Matt Baker from comment #2)
> Regressed in https://trac.webkit.org/changeset/240947.

This patch actually just allowed the bug to manifest. It was a defect lurking in TreeOutline from before this change was made.
Comment 5 Devin Rousso 2019-02-04 18:34:20 PST
Comment on attachment 361142 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=361142&action=review

r-, can you write some sort of test for this?  I think we've long passed the point where we need regression tests for our common UI classes.

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:825
> +                const omitFocus = true;

This doesn't seem right.  If we try to select a `WI.TreeElement` via `_handleMouseDown`, we'd never focus the parent.  Either provide a justification for this, or maybe be a bit more "limiting" (e.g. only `omitFocus` when there is more than one `selectedItems`).
Comment 6 Matt Baker 2019-02-04 18:43:03 PST
Comment on attachment 361142 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=361142&action=review

>> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:825
>> +                const omitFocus = true;
> 
> This doesn't seem right.  If we try to select a `WI.TreeElement` via `_handleMouseDown`, we'd never focus the parent.  Either provide a justification for this, or maybe be a bit more "limiting" (e.g. only `omitFocus` when there is more than one `selectedItems`).

Actually that's not the case. The tree receives the focus by virtue of being clicked; the awful `omitFocus` flag is only for forcing the focus when selecting elements programmatically.
Comment 7 Matt Baker 2019-02-04 19:32:25 PST
(In reply to Devin Rousso from comment #5)
> Comment on attachment 361142 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361142&action=review
> 
> r-, can you write some sort of test for this?  I think we've long passed the
> point where we need regression tests for our common UI classes.

Added a comment to https://bugs.webkit.org/show_bug.cgi?id=192044.
Comment 8 Devin Rousso 2019-02-05 14:01:28 PST
Comment on attachment 361142 [details]
Patch

rs=me, please don't forget to write tests :)
Comment 9 WebKit Commit Bot 2019-02-05 15:33:11 PST
Comment on attachment 361142 [details]
Patch

Clearing flags on attachment: 361142

Committed r240997: <https://trac.webkit.org/changeset/240997>
Comment 10 WebKit Commit Bot 2019-02-05 15:33:12 PST
All reviewed patches have been landed.  Closing bug.