Bug 177477 – Web Inspector: Fix Layers tab sidebar popover.

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

[Ross Kirsling]

The Layers tab sidebar has a very buggy popover (inherited virtually as-is from the legacy sidebar on the Elements tab).

Those bugs which are specific to this sidebar (and not to popovers at large) should be fixed here, namely:

1. When a data grid row is selected but the popover has been hidden...
  a. ...going to another window and back causes an empty popover to reappear in its place.
  b. ...going to another tab and back causes and empty popover to reappear in the upper-left corner.

2. For consistency with all other data grids in Web Inspector, clicking empty space in the data grid should not change the current selection.

[Ross Kirsling]

Created attachment 321782 [details]
Patch

[Ross Kirsling]

Comment on attachment 321782 [details]
Patch

This turns out to be mostly code removal and a bit of refactoring.

Specifically:
- Remove focus/blur/click handlers from DataGrid. This is unlike anywhere else in Web Inspector, isn't desired, and causes issue #2.
- Use willDismissPopover to dispose of popover after it has been dismissed (fixes issue #1).
- Make popover setup less convoluted.

Note that this patch also removes the DOM highlighting triggered by the data grid. I will add this back in a later patch -- it needs to be added to the data grid row itself, and to avoid polluting the legacy sidebar, I will create a new LayerDataGridRow for the purpose.

[Ross Kirsling]

Note that there are some remaining issues that seem to affect popovers overall:

1. When the window is resized to force a direction change on the popover, the CSS class for the old direction isn't removed from the existing popover.

2. When moving with arrow keys, the arrow doesn't follow the target element correctly.

I don't see these as specifically related to this sidebar so I'm not addressing them here.

[Matt Baker]

(In reply to Ross Kirsling from comment #3)
> Note that there are some remaining issues that seem to affect popovers
> overall:
> 
> 1. When the window is resized to force a direction change on the popover,
> the CSS class for the old direction isn't removed from the existing popover.
> 
> 2. When moving with arrow keys, the arrow doesn't follow the target element
> correctly.

FYI:
<https://webkit.org/b/150551> Web Inspector: Selecting child layers with keyboard causes Compositing Reason popover to become misaligned

> I don't see these as specifically related to this sidebar so I'm not
> addressing them here.

[Matt Baker]

Comment on attachment 321782 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:-91
> -        this._dataGrid.element.addEventListener("click", this._dataGridClicked.bind(this), false);

I'm glad to see all this go. I think eventually we'll want to completely rethink the Layers sidebar UI. Putting important data (compositing reasons!) in popovers and forcing people to play the popover game is terrible.

> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:-151
> -    _highlightSelectedNode()

I'm not clear on why this is being removed. Calling this when the node changes works fine with your other changes, as long as the highlight is removed later.

[Ross Kirsling]

(In reply to Matt Baker from comment #4)
> FYI:
> <https://webkit.org/b/150551> Web Inspector: Selecting child layers with keyboard causes Compositing Reason popover to become misaligned

Cool. I couldn't find another place in Web Inspector that had a side-facing popover, but I believe this would happen for any such case.

(In reply to Matt Baker from comment #5)
> > Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:-91
> > -        this._dataGrid.element.addEventListener("click", this._dataGridClicked.bind(this), false);
> 
> I'm glad to see all this go. I think eventually we'll want to completely
> rethink the Layers sidebar UI. Putting important data (compositing reasons!)
> in popovers and forcing people to play the popover game is terrible.

That's fair. Just hoping to provide an adequate UX for Day 1 -- I'm not really sure how to fundamentally restructure that information, but at least one can sort by highest-impact layers, even if you have to ask "why" for each one separately.

> > Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:-151
> > -    _highlightSelectedNode()
> 
> I'm not clear on why this is being removed. Calling this when the node
> changes works fine with your other changes, as long as the highlight is
> removed later.

Sure, I can put this back and remove it in the following ticket.

[Ross Kirsling]

Created attachment 321977 [details]
Patch

[Ross Kirsling]

Comment on attachment 321977 [details]
Patch

Reverted DOM highlight behavior.

Will rework sidebar to use Table instead of DataGrid in a follow-up patch and handle DOM highlighting at that time as well.

[Matt Baker]

Comment on attachment 321977 [details]
Patch

Nice work, r=me.

[WebKit Commit Bot]

Comment on attachment 321977 [details]
Patch

Clearing flags on attachment: 321977

Committed r222566: <http://trac.webkit.org/changeset/222566>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34692908>