Bug 160321

Summary:

Web Inspector: Popover for Color Picker should update position when window resizes

Product:

WebKit

Reporter:

Joseph Pecoraro <joepeck>

Component:

Web Inspector

Assignee:

Joseph Pecoraro <joepeck>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bburg, joepeck, mattbaker, nvasilyev, simon.fraser, timothy, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Attachments:

Description	Flags
[PATCH] Proposed Fix	none
[PATCH] Proposed Fix	bburg: review+

Description Joseph Pecoraro 2016-07-28 15:24:56 PDT

Summary:
Popover for Color Picker should update position when window resizes.

Steps to Reproduce:
1. Inspect <body> on webkit.org
2. Show Elements Tab
3. Show Styles - Rules sidebar
4. Click on a Color swatch to show Color Picker Popover
5. Horizontally resize the inspector window
  => Popover does not update its position to track the element it is for

Notes:
- This is a common problem. We solved it for the LayerDetailsSidebarPanel. We should solve it here and more generally.

Comment 1 Radar WebKit Bug Importer 2016-07-28 15:25:10 PDT

<rdar://problem/27598358>

Comment 2 Joseph Pecoraro 2016-07-28 15:41:31 PDT

Created attachment 284824 [details]
[PATCH] Proposed Fix

This fixes color swatch case, and moves all the older custom implementations over to the newer, simpler version.

Comment 3 Joseph Pecoraro 2016-07-28 15:53:33 PDT

Comment on attachment 284824 [details]
[PATCH] Proposed Fix

I'll address a few other popovers that aren't updating for window resizes.

Comment 4 Joseph Pecoraro 2016-07-28 16:07:41 PDT

Created attachment 284828 [details]
[PATCH] Proposed Fix

This updates popover repositioning on window resize for most popovers.

It removes custom code for:

  - layer details sidebar
  - timeline datagrid
  - watch expressions

Adds resize handling for:

  - color swatches
  - probe expressions
  - visual sidebar help text
  - heap snapshot popovers

Does not change: (Right now these do not need adjustments with window resize... The top+left is enough. But They may in the future though)

  - debugger popovers
  - breakpoint popovers

Comment 5 BJ Burg 2016-07-29 10:32:56 PDT

Comment on attachment 284828 [details]
[PATCH] Proposed Fix

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

r=me

> Source/WebInspectorUI/ChangeLog:16
> +        * UserInterface/Views/HeapSnapshotDataGridTree.js:

Nit: pls newline

> Source/WebInspectorUI/UserInterface/Views/HeapSnapshotDataGridTree.js:75
> +    set popoverNode(x) { this._popoverNode = x; }

Nit: popoverGridNode

> Source/WebInspectorUI/UserInterface/Views/HeapSnapshotDataGridTree.js:83
> +            this._popover.windowResizeHandler = () => {

If the popover is not visible when the resize handler is called, will this make it pop up?

> Source/WebInspectorUI/UserInterface/Views/Popover.js:152
> +        window.removeEventListener("resize", this, true);

I think it's worth mentioning in the change log that the resize handler is only called while the Popover is being presented.

> Source/WebInspectorUI/UserInterface/Views/ProbeSetDetailsSection.js:135
> +            let target = WebInspector.Rect.rectFromClientRect(event.target.element.getBoundingClientRect());

Nit: I would call this resizedTarget to emphasize we can't reuse `target` from above which is probably out of date.

> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:-373
> -        window.addEventListener("resize", this._windowResizeListener);

Nice.

Comment 6 Joseph Pecoraro 2016-08-08 12:15:20 PDT

(In reply to comment #5)
> Comment on attachment 284828 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284828&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/HeapSnapshotDataGridTree.js:83
> > +            this._popover.windowResizeHandler = () => {
> 
> If the popover is not visible when the resize handler is called, will this
> make it pop up?

The resize handler can only be called when the popover is visible. Since now Popover.js handles adding/removing the listener when the popover is shown/hidden.

> > Source/WebInspectorUI/UserInterface/Views/Popover.js:152
> > +        window.removeEventListener("resize", this, true);
> 
> I think it's worth mentioning in the change log that the resize handler is
> only called while the Popover is being presented.

Okay.

Comment 7 Joseph Pecoraro 2016-08-08 13:58:37 PDT

<https://trac.webkit.org/changeset/204264>