Web Inspector: Popover's arrow is misplaced

[Nikita Vasilyev]

Created attachment 265454 [details]
[Image] Bug

Steps:
1. Set a breakpoint
2. Close the left sidebar
3. Right click on the breakpoint, select "Edit Breakpoint..."

[Radar WebKit Bug Importer]

<rdar://problem/23527296>

[Matt Baker]

I wonder if this is related to https://bugs.webkit.org/show_bug.cgi?id=150551.

[Matt Baker]

Created attachment 285172 [details]
[Patch] Proposed Fix

[Matt Baker]

(In reply to comment #2)
> I wonder if this is related to
> https://bugs.webkit.org/show_bug.cgi?id=150551.

Not related.

[Matt Baker]

Created attachment 285183 [details]
[Image] Adjusted arrow placement

[BJ Burg]

Comment on attachment 285172 [details]
[Patch] Proposed Fix

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

r=me but please address nits. Thanks Matt!

> Source/WebInspectorUI/UserInterface/Views/Popover.js:498
>          var arrowHalfLength = WebInspector.Popover.AnchorSize.width / 2;

Nit: let's improve variable names:

r-> cornerRadius
arrowHalfLength -> arrowWidth (and multiply by 0.5)

> Source/WebInspectorUI/UserInterface/Views/Popover.js:499
>          var anchorPoint = this._anchorPoint;

Nit: 'let' it be

> Source/WebInspectorUI/UserInterface/Views/Popover.js:502
> +        let anchorPointBounds = bounds.pad(-(r + arrowHalfLength));

It took me a little bit to understand this. So basically, we make a smaller bounds where we could correctly draw the arrow attached to any point of the bound. The earlier point we could attach the arrow is cornerRadius + 0.5 * arrowWidth away from any corner. Then we clamp the anchor point's x, y.

Maybe we could come up with better names, like adjustedAnchorBounds or something. Or maybe it's just a messy bit of code.
I think using Number.constrain would make it clearer:

anchorPoint.x = Number.constrain(anchorPoint.x, anchorPointBounds.minX(), anchorPointBounds.maxX())
...

[Matt Baker]

Comment on attachment 285172 [details]
[Patch] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Views/Popover.js:502
>> +        let anchorPointBounds = bounds.pad(-(r + arrowHalfLength));
> 
> It took me a little bit to understand this. So basically, we make a smaller bounds where we could correctly draw the arrow attached to any point of the bound. The earlier point we could attach the arrow is cornerRadius + 0.5 * arrowWidth away from any corner. Then we clamp the anchor point's x, y.
> 
> Maybe we could come up with better names, like adjustedAnchorBounds or something. Or maybe it's just a messy bit of code.
> I think using Number.constrain would make it clearer:
> 
> anchorPoint.x = Number.constrain(anchorPoint.x, anchorPointBounds.minX(), anchorPointBounds.maxX())
> ...

I like this better. Have a bounds rectangle is also misleading, since the arrow is really just bound on one axis, not both.

[Matt Baker]

Comment on attachment 285172 [details]
[Patch] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Views/Popover.js:498
>>          var arrowHalfLength = WebInspector.Popover.AnchorSize.width / 2;
> 
> Nit: let's improve variable names:
> 
> r-> cornerRadius
> arrowHalfLength -> arrowWidth (and multiply by 0.5)

I don't think arrowWidth is an improvement.

[Matt Baker]

Created attachment 285350 [details]
[Patch] For Landing

[WebKit Commit Bot]

Comment on attachment 285350 [details]
[Patch] For Landing

Clearing flags on attachment: 285350

Committed r204136: <http://trac.webkit.org/changeset/204136>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.