Web Inspector: popover can overlap target frame

• Component: Web Inspector
• Status: RESOLVED FIXED

[Antoine Quint]

While https://bugs.webkit.org/show_bug.cgi?id=124350 did fix the issue of having popovers smaller than there was room for them to be, it did introduce a regression where by offsetting the computed frame of the popover regardless of the target edge we could possibly overlap the target frame. See the attached screenshot for an illustration of the issue, the popover was offset to fit within the window bounds in the y-axis, but this should only happen if the target edge is either to the left or right or the frame (x-axis).

[Antoine Quint]

Created attachment 218154 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 218154 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Popover.js:404
> +        if (edge === WebInspector.RectEdge.MIN_X || edge === WebInspector.RectEdge.MAX_X) {
> +            if (y < containerFrame.minY())
> +                y = containerFrame.minY();
> +            if (y + height > containerFrame.maxY())
> +                y += containerFrame.maxY() - (y + height);
> +        } else {
> +            if (x < containerFrame.minX())
> +                x = containerFrame.minX(); 
> +            if (x + width > containerFrame.maxX())
> +                x += containerFrame.maxX() - (x + width);
> +        }

I don't understand this section:

    if (popover is above or below) {
        if (y < min)
          y = min;
        if (y + height > max)
          y = y + max - y + height;
    }

My confusion is with the second case.

1. The adjustment is basically "y = max - height". Is there any guarantee that after that we will still have "y > min"? If so, should we clamp height, or would that cause other problems?
2. The "+=" statement makes this more confusing then it needs to be. Just make it an assignment and drop the inner y.

I'm fine as long as (2) is handled. I assume (1) is not actually a problem. So I'd like to see that last statement be just: (likewise simplifying the "x" case)

    y = containerFrame.maxY() - height;

[Antoine Quint]

(In reply to comment #2)
> (From update of attachment 218154 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218154&action=review
> 
> > Source/WebInspectorUI/UserInterface/Popover.js:404
> > +        if (edge === WebInspector.RectEdge.MIN_X || edge === WebInspector.RectEdge.MAX_X) {
> > +            if (y < containerFrame.minY())
> > +                y = containerFrame.minY();
> > +            if (y + height > containerFrame.maxY())
> > +                y += containerFrame.maxY() - (y + height);
> > +        } else {
> > +            if (x < containerFrame.minX())
> > +                x = containerFrame.minX(); 
> > +            if (x + width > containerFrame.maxX())
> > +                x += containerFrame.maxX() - (x + width);
> > +        }
> 
> I don't understand this section:
> 
>     if (popover is above or below) {

It's actually "to the left or right" in the case where we adjust y, but that doesn't change the validity of your comment.

>         if (y < min)
>           y = min;
>         if (y + height > max)
>           y = y + max - y + height;
>     }
> 
> My confusion is with the second case.
> 
> 1. The adjustment is basically "y = max - height". Is there any guarantee that after that we will still have "y > min"? If so, should we clamp height, or would that cause other problems?

It's clamped to fit the container frame right after this block of code like so:

        var preferredFrame = new WebInspector.Rect(x, y, width, height);
        var bestFrame = preferredFrame.intersectionWithRect(containerFrame);

> 2. The "+=" statement makes this more confusing then it needs to be. Just make it an assignment and drop the inner y.

Good point. Will make that change upon landing (same with x-axis).

[Antoine Quint]

Created attachment 218199 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 218199 [details]
Patch for landing

Clearing flags on attachment: 218199

Committed r159952: <http://trac.webkit.org/changeset/159952>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.