Bug 160321 - Web Inspector: Popover for Color Picker should update position when window resizes
Summary: Web Inspector: Popover for Color Picker should update position when window re...
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: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-07-28 15:24 PDT by Joseph Pecoraro
Modified: 2016-08-08 13:58 PDT (History)
7 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (10.78 KB, patch)
2016-07-28 15:41 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (16.79 KB, patch)
2016-07-28 16:07 PDT, Joseph Pecoraro
bburg: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>