Bug 136268 – Web Inspector: Layout issues for popover on not legacy OS

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Saam Barati]

There is a layout issue where a popover will show up under the title bar in the Safari webpage for the Web Inspector on not legacy OSs.

There might also be other places where this is a problem too. I'll look into it.

[Radar WebKit Bug Importer]

<rdar://problem/18141133>

[Saam Barati]

I don't think there are any other problems with innerHeight in the code, because
the other uses rely on some minimum/maximum size that will never conflict with the extra 22 pixels we don't want to touch on the title bar.

here are the other places:

## Main.js (I don't think there is a problem with this, but I do notice some behavior where the console's frame might quickly jump when dragging it, but I think it only happens when resizing a window and has to do with maximally being 55% of the height)

    function dockedResizerDrag(event)
    {
        if (event.button !== 0)
            return;

        var height = window.innerHeight - event.pageY - mouseOffset;

        this._splitConsoleHeightSetting.value = height;

        this._updateSplitConsoleHeight(height);
    }

WebInspector._updateSplitConsoleHeight = function(height)
{
    const minimumHeight = 64;
    const maximumHeight = window.innerHeight * 0.55;

    height = Math.max(minimumHeight, Math.min(height, maximumHeight));

    this.splitContentBrowser.element.style.height = height + "px";
}



## CompletionSuggestionsView.js (In my testing, this is never large enough in the smallest window I can shrink the inspector to, to go over the title bar)

        var aboveHeight = anchorBounds.origin.y;
        var underHeight = window.innerHeight - anchorBounds.origin.y - anchorBounds.size.height;
        var maximumHeight = Math.min(absoluteMaximumHeight, Math.max(underHeight, aboveHeight) - margin);
        var height = Math.min(containerHeight, maximumHeight);



## QuickConsole.js (I'm not actually sure what this is, but I think that it maximally being 33% of innerHeight will prevent any problems w/ covering the title bar)
    updateLayout: function()
    {
        // A hard maximum size of 33% of the window.
        const maximumAllowedHeight = Math.round(window.innerHeight * 0.33);
        this.prompt.element.style.maxHeight = maximumAllowedHeight + "px";
    },

[Saam Barati]

Created attachment 237235 [details]
patch

Solve the problem by adding a padding of 22 for newer OS.

[Joseph Pecoraro]

Comment on attachment 237235 [details]
patch

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

> Source/WebInspectorUI/UserInterface/Views/Popover.js:200
> +        var padding = (WebInspector.Platform.isLegacyMacOS ? 0 : 22);

The condition needs to check if this is mac:

    var shouldAddTopWindowPadding = WebInspector.Platform.name === "mac" && !WebInspector.Platform.isLegacyMacOS;

Otherwise windows and linux ports would add this 22px of padding unnecessarily.

And maybe a better word is "offset" instead of padding.

[Saam Barati]

Created attachment 237236 [details]
patch

[Timothy Hatcher]

Comment on attachment 237236 [details]
patch

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

> Source/WebInspectorUI/UserInterface/Views/Popover.js:199
> +        const titleBarOffset = (WebInspector.Platform.name === "mac" && !WebInspector.Platform.isLegacyMacOS ? 22 : 0);

Could be written as:

const titleBarOffset = WebInspector.Platform.isLegacyMacOS ? 0 : 22;

Drop the () too.

[Joseph Pecoraro]

Comment on attachment 237236 [details]
patch

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

>> Source/WebInspectorUI/UserInterface/Views/Popover.js:199
>> +        const titleBarOffset = (WebInspector.Platform.name === "mac" && !WebInspector.Platform.isLegacyMacOS ? 22 : 0);
> 
> Could be written as:
> 
> const titleBarOffset = WebInspector.Platform.isLegacyMacOS ? 0 : 22;
> 
> Drop the () too.

I disagree. I think we still need to check Platform.name === "mac" to ensure we don't add unnecessary padding on linux and windows.

[Timothy Hatcher]

Comment on attachment 237236 [details]
patch

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

>>> Source/WebInspectorUI/UserInterface/Views/Popover.js:199
>>> +        const titleBarOffset = (WebInspector.Platform.name === "mac" && !WebInspector.Platform.isLegacyMacOS ? 22 : 0);
>> 
>> Could be written as:
>> 
>> const titleBarOffset = WebInspector.Platform.isLegacyMacOS ? 0 : 22;
>> 
>> Drop the () too.
> 
> I disagree. I think we still need to check Platform.name === "mac" to ensure we don't add unnecessary padding on linux and windows.

WebInspector.Platform.isLegacyMacOS is only true for Mac. It is in the property name. We do this check in many other places.

[Timothy Hatcher]

Comment on attachment 237236 [details]
patch

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

>>>> Source/WebInspectorUI/UserInterface/Views/Popover.js:199
>>>> +        const titleBarOffset = (WebInspector.Platform.name === "mac" && !WebInspector.Platform.isLegacyMacOS ? 22 : 0);
>>> 
>>> Could be written as:
>>> 
>>> const titleBarOffset = WebInspector.Platform.isLegacyMacOS ? 0 : 22;
>>> 
>>> Drop the () too.
>> 
>> I disagree. I think we still need to check Platform.name === "mac" to ensure we don't add unnecessary padding on linux and windows.
> 
> WebInspector.Platform.isLegacyMacOS is only true for Mac. It is in the property name. We do this check in many other places.

Note: I flipped the order. I agree !WebInspector.Platform.isLegacyMacOS is wrong.

[Saam Barati]

(In reply to comment #8)
> (From update of attachment 237236 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=237236&action=review
> 
> >>> Source/WebInspectorUI/UserInterface/Views/Popover.js:199
> >>> +        const titleBarOffset = (WebInspector.Platform.name === "mac" && !WebInspector.Platform.isLegacyMacOS ? 22 : 0);
> >> 
> >> Could be written as:
> >> 
> >> const titleBarOffset = WebInspector.Platform.isLegacyMacOS ? 0 : 22;
> >> 
> >> Drop the () too.
> > 
> > I disagree. I think we still need to check Platform.name === "mac" to ensure we don't add unnecessary padding on linux and windows.
> 
> WebInspector.Platform.isLegacyMacOS is only true for Mac. It is in the property name. We do this check in many other places.

Right, so on linux/windows isLegacyMacOS will be false, and we would add the padding for a UI we're not intending to.

[Saam Barati]

Created attachment 237238 [details]
patch

[WebKit Commit Bot]

Comment on attachment 237238 [details]
patch

Clearing flags on attachment: 237238

Committed r173015: <http://trac.webkit.org/changeset/173015>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.