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.
Created attachment 321782 [details] Patch
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.
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.
(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.
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.
(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.
Created attachment 321977 [details] Patch
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.
Comment on attachment 321977 [details] Patch Nice work, r=me.
Comment on attachment 321977 [details] Patch Clearing flags on attachment: 321977 Committed r222566: <http://trac.webkit.org/changeset/222566>
All reviewed patches have been landed. Closing bug.
<rdar://problem/34692908>