Bug 94213 – Web Inspector: hovering over an image link in Timeline popup kills popup

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Pavel Feldman]

Reproducible on the image resource load records...

It sounds like we are trying to show popup from popup. We should include image preview in the original timeline popup instead.

[Alexander Pavlov (apavlov)]

Created attachment 158815 [details]
[IMAGE] Screenshot with the patch applied

[Alexander Pavlov (apavlov)]

Created attachment 158819 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 158819 [details]
Patch

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

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:739
> +    _buildImagePreviewElementAndContinue: function(callback)
> +    {
> +        WebInspector.buildImagePreviewContents(null, null, this.url, false, returnContent);
> +
> +        function returnContent(contentElement)
> +        {
> +            callback(contentElement);
> +        }
>      },

I'd replace a call to this with just WebInspector.buildImagePreviewContents(null, null, this.url, false, callback);

[Alexander Pavlov (apavlov)]

Created attachment 158825 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 158825 [details]
Patch

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

> Source/WebCore/inspector/front-end/UIUtils.js:1100
> +        if (naturalWidth > naturalHeight) {
> +            if (naturalWidth > maxImageWidth) {
> +                imageElement.style.width = maxImageWidth + "px";
> +                imageElement.style.height = (naturalHeight * maxImageWidth / naturalWidth) + "px";
> +            }
> +        } else {
> +            if (naturalHeight > maxImageHeight) {
> +                imageElement.style.width = (naturalWidth * maxImageHeight / naturalHeight) + "px";
> +                imageElement.style.height = maxImageHeight + "px";
> +            }
> +        }

Can we do this with CSS, using max-width & max-height instead?

[Alexander Pavlov (apavlov)]

Created attachment 158829 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 158829 [details]
Patch

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

> Source/WebCore/inspector/front-end/ElementsPanel.js:434
> +            var listItem = relatedObject.enclosingNodeOrSelfWithNodeNameInArray(["li"]);

enclosingNodeOrSelfWithNodeName()?

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:641
> +    generatePopupContent: function(callback)
> +    {
> +        WebInspector.buildImagePreviewContents(null, null, this.url, false, this._generatePopupContentWithImagePreview.bind(this, callback));
> +    },

I think we should only do this for the record types where we expect images and where URL is valid (i.e. network events etc.)

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:643
> +    _generatePopupContentWithImagePreview: function(callback, previewElement)

Please annotate.

> Source/WebCore/inspector/front-end/UIUtils.js:1041
> +WebInspector.buildImagePreviewContents = function(relatedObject, dimensionsProvider, imageURL, showDimensions, userCallback)

Please annotate.
I'd suggest we also drop showDimensions parameter and use presence of dimenstionsProvider instead.
Actually, given that we invoke dimensionsProvider() first thing, I'd suggest we leave this to the caller and let him invoke this with the dimensions, provided caller has them -- this would simplify control flow a bit in my view.

[Pavel Feldman]

Comment on attachment 158829 [details]
Patch

r- as per caseq's recommendations.

[Alexander Pavlov (apavlov)]

Created attachment 158838 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 158838 [details]
Patch

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

> Source/WebCore/inspector/front-end/ElementsPanel.js:434
> +            WebInspector.buildImagePreviewContents.bind(WebInspector, anchor.href, true, showPopover);

Is this intentional?

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:877
> +    this.record = record;

We also use PopupContentHelper for frames that are not records, so it should not be aware of the record. Can we have this logic elsewhere?

> Source/WebCore/inspector/front-end/UIUtils.js:1047
> + * @param {Object=} precomputedDimensions
> + */
> +WebInspector.buildImagePreviewContents = function(imageURL, showDimensions, userCallback, precomputedDimensions)

Drop precomputed, just dimensions -- it's cleaner.

> Source/WebCore/inspector/front-end/UIUtils.js:1068
> +    imageElement.addEventListener("load", buildContent.bind(null, imageElement, precomputedDimensions), false);
> +    imageElement.addEventListener("error", buildContent.bind(null, null, null), false);
> +    resource.populateImageSource(imageElement);
> +
> +    /**
> +     * @param {Object=} nodeDimensions
> +     */
> +    function buildContent(imageElement, nodeDimensions)
> +    {
> +        if (!imageElement) {
> +            userCallback();
> +            return;
> +        }

So we only invoke buildContent() with a null imageElement if we had an error -- I'd suggest we directly invoke userCallback on error instead and remove this check. We could also remove both parameters, as these would always be visible in the outer lexical context. Hence we would not have to bind() as well.

[Alexander Pavlov (apavlov)]

Created attachment 159086 [details]
Patch

[Alexander Pavlov (apavlov)]

(In reply to comment #10)
> (From update of attachment 158838 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158838&action=review
> 
> > Source/WebCore/inspector/front-end/ElementsPanel.js:434
> > +            WebInspector.buildImagePreviewContents.bind(WebInspector, anchor.href, true, showPopover);
> 
> Is this intentional?

It's an unfortunate copy-paste...

> > Source/WebCore/inspector/front-end/TimelinePresentationModel.js:877
> > +    this.record = record;
> 
> We also use PopupContentHelper for frames that are not records, so it should not be aware of the record. Can we have this logic elsewhere?

Fixed the way we had agreed offline.

> > Source/WebCore/inspector/front-end/UIUtils.js:1047
> > + * @param {Object=} precomputedDimensions
> > + */
> > +WebInspector.buildImagePreviewContents = function(imageURL, showDimensions, userCallback, precomputedDimensions)
> 
> Drop precomputed, just dimensions -- it's cleaner.

As agreed offline, this will make the reader think that the "showDimensions" is related to this parameter (i.e. the former can be superfluous if the latter is null), while this is not the case.

> > Source/WebCore/inspector/front-end/UIUtils.js:1068
> > +    imageElement.addEventListener("load", buildContent.bind(null, imageElement, precomputedDimensions), false);
> > +    imageElement.addEventListener("error", buildContent.bind(null, null, null), false);
> > +    resource.populateImageSource(imageElement);
> > +
> > +    /**
> > +     * @param {Object=} nodeDimensions
> > +     */
> > +    function buildContent(imageElement, nodeDimensions)
> > +    {
> > +        if (!imageElement) {
> > +            userCallback();
> > +            return;
> > +        }
> 
> So we only invoke buildContent() with a null imageElement if we had an error -- I'd suggest we directly invoke userCallback on error instead and remove this check. We could also remove both parameters, as these would always be visible in the outer lexical context. Hence we would not have to bind() as well.

This will result in the userCalback() invocation with the ErrorEvent object as the first argument, which is wrong. Fixed by using an intermediate callback.

[Andrey Kosyakov]

Comment on attachment 159086 [details]
Patch

LGTM

[Alexander Pavlov (apavlov)]

Committed r125882: <http://trac.webkit.org/changeset/125882>