Bug 35003 – Web Inspector: Implement evaluate-on-hover for scripts panel.

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

[Pavel Feldman]

Hover card should allow browsing evaluation results.

[Pavel Feldman]

Created attachment 48843 [details]
[IMAGE] Screen 1

[Pavel Feldman]

Created attachment 48844 [details]
[IMAGE] Screen 2

[Pavel Feldman]

Calling for input / feedback on the screenshots!

[Timothy Hatcher]

This is a good start! The bottom area is the part that is bothering me right now.

We should attempt to smart size everything. If the whole popover can be shown without a scrollbar (within some maximum height/width) do it. Also the bottom area should not be visible if it just shows the same value you can fully see above.

I am also tempted to not have a bottom area, and have second contextual popover that you can show by arrowing Right or hovering or something. Then you just deal with the list and the more detail popover can be shown if you need to see the whole value. Heck, I think we should have that everywhere we clip/truncate values in the sidebars.

Then the bottom area wont be a huge distraction if you dont need to see everything. It wont be constently resizing, hiding, etc.

Now the rest. We need to highlight the expression in the code for context. And I would like to see an arrow on the adjacent box edge pointing to it. Then you don't need to show the expression again in the popover.

It should also have a shadow and likely no border and/or rounded.

/me opens Photoshop

[Timothy Hatcher]

Created attachment 48851 [details]
Mock Up

I am still thinking about what to do with the detail view.

[Timothy Hatcher]

Created attachment 48864 [details]
Artwork and CSS for popover with arrow

[Timothy Hatcher]

Created attachment 48877 [details]
Scrollbar art

[Timothy Hatcher]

Created attachment 48880 [details]
Scrollbar hover/active art

[Timothy Hatcher]

Created attachment 48881 [details]
Horizontal scrollbar art

[Pavel Feldman]

Created attachment 48908 [details]
[IMAGE] Work in progress image 1.

[Pavel Feldman]

Created attachment 48909 [details]
[IMAGE] Work in progress image 2.

[Pavel Feldman]

Created attachment 48910 [details]
[IMAGE] Work in progress image 3.

[Pavel Feldman]

Created attachment 48912 [details]
[PATCH] Proposed fix (work in progress, couple of nits I need to fix).

[Eric Seidel (no email)]

Sadly the patch does not apply, so the EWS bots can't tell us if it builds or not. :(

[Timothy Hatcher]

Comment on attachment 48912 [details]
[PATCH] Proposed fix (work in progress, couple of nits I need to fix).

So this popover will be used for conditional breakpoints too?

> +        preferredWidth = Math.max(preferredWidth, 100 - 2 * 23);
> +        preferredHeight = Math.max(preferredHeight, 100 - 2 * 23);
> +        var newElementPosition = { x: 0, y: 0, width: preferredWidth + 2 * 27, height: preferredHeight + 2 * 27 };
> +                newElementPosition.width += 11;
> +            newElementPosition.y += 10;
> +                newElementPosition.width += 11;
> +            newElementPosition.y -= 10;
> +            newElementPosition.x = Math.max(0, anchorBox.x) - 40;
> +            this._popupArrowElement.style.right = totalWidth - anchorBox.x - 25 - anchorBox.width + "px";
> +            this._popupArrowElement.style.left = anchorBox.x - 25 + "px";
> +        newElementPosition.height += 11;

You should put these numbers in named constants at the top of the file.

> +        this._hoverTimer = setTimeout(this._mouseHover.bind(this), 1000);

Should it be longer or better match tooltips? Put in a named constant at the top of the file.

> +            InspectorBackend.releaseWrapperObjectGroup(0, "hovercard");

Maybe "popover" instead of "hovercard" would be better. And a constant so it can be shared with the other place in this file.

> +        if (element.hasStyleClass("webkit-javascript-keyword")) {
> +            if (element.textContent === "this")
> +                this._showPopup(element, "this");
> +            return;
> +        }

I don't understand this.

> +        var offset = 0;
> +        var node = lineRow.lastChild.traverseNextTextNode(lineRow.lastChild);
> +        while (node && node !== element.firstChild) {
> +            offset += node.nodeValue.length;
> +            node = node.traverseNextTextNode(lineRow.lastChild);
> +        }

Some comments here would help.

> +        var prefix = this._textModel.line(lineNumber).substring(0, offset + element.textContent.length);
> +        var reversedPrefix = prefix.split("").reverse().join("");
> +        var match = /[a-zA-Z\x80-\xFF\_$0-9.]+/.exec(reversedPrefix);
> +        if (!match)
> +            return;
> +        var expression = match[0].split("").reverse().join("");

And here. Why reverse?


> +    this.headerElement.className = "hidden";

Should you use addStyleClass just so the previous classes are preserved? I guess it isn't an issue.

> +WebInspector.HoverPropertiesSection.prototype = {
> +    update: function()
> +    {
> +    }
> +}

I assume this is will do more later.


> +.webkit-javascript-ident {
> +    color: rgb(0, 0, 0);
> +}

Use black.

> +    z-index: 100;

Use 1000, or something higher than 100.

[Timothy Hatcher]

Maybe Popup.js should be renamed to Popover.js?

[Pavel Feldman]

Thanks, all good suggestions, will fix them.

> 
> Should it be longer or better match tooltips? Put in a named constant at the
> top of the file.
> 

I tried that, but then thought that even 1sec is too much. Rationale: 
it is only enabled while on a breakpoint, hovering over source code's identifier. It is hard to get a false positive trigger in such a context.

> > +        if (element.hasStyleClass("webkit-javascript-keyword")) {
> > +            if (element.textContent === "this")
> > +                this._showPopup(element, "this");
> > +            return;
> > +        }
> 
> I don't understand this.
> 

We apply to webkit-javascript-ident, but "this" is a webkit-javascript-keyword. We still want to show popover for this, hence the hack.
> > +            return;
> > +        var expression = match[0].split("").reverse().join("");
> 
> And here. Why reverse?
> 

Imagine the hit target is C in "foo(A.B.C.D)" what I do is I reverse the string to "C.B.A(oof" and run a regex against it that cuts out "C.B.A". Then I reverse it back and get what I need to eval: "A.B.C". I can comment.

> > +WebInspector.HoverPropertiesSection.prototype = {
> > +    update: function()
> > +    {
> > +    }
> > +}
> 
> I assume this is will do more later.
> 

Not really, section is populated in properties manually. I actually wanted to get real properties number before calculating popover size, but ended up with fixed size for "object" eval results, so I will most likely nuke this one and use simple PropertiesSection.

[Pavel Feldman]

Created attachment 49008 [details]
[IMAGE] Patch looks.

[Pavel Feldman]

Created attachment 49009 [details]
[PATCH] Proposed change.

Comments addressed and more!

[Timothy Hatcher]

Comment on attachment 49009 [details]
[PATCH] Proposed change.

> +        this.contentElement.positionAt(0, 0);
> +        document.body.appendChild(this.contentElement);
> +        var preferredWidth = preferredWidth || this.contentElement.offsetWidth;
> +        var preferredHeight = preferredHeight || this.contentElement.offsetHeight;

A comment here explaining you do this for measuement would be good.

> +        // Short tooltips are not pretty, their arrow location is not nice.
> +        preferredWidth = Math.max(preferredWidth, 50);

Don't you mean "skinny"? Short is for height, but you are padding the width.

> +        var totalWidth = window.innerWidth;
> +        var totalHeight = window.innerHeight;

const

> +        // User has 500ms to reach the popup.
> +        if (this._popup) {
> +            var self = this;
> +            this._hidePopupTimer = setTimeout(function() { self._hidePopup(); delete self._hidePopupTimer; }, 500);
> +        }

Usually tooltips stay up longer so you can just look at them without moving. 5-10 seconds even. A named function would be nice for that timeout.

> +                this._popup.show(element, 300, 250);

These could be named constants.

> -        total += element.offsetLeft;
> +        total += element.offsetLeft + element.clientLeft;

You should fix the totalOffsetTop function to match.

[Timothy Hatcher]

Comment on attachment 49009 [details]
[PATCH] Proposed change.

I think the totalOffsetLeft change might affect other code.

Any place we take totalOffsetLeft and add it to offsetWidth will be wrong if there is a border. Here are the couple places I found.

> return ((crumbs.totalOffsetLeft + crumbs.offsetWidth + rightPadding) < window.innerWidth);
> var x = root.totalOffsetLeft + root.offsetWidth - 20;
> this._dragOffset = (event.target.offsetWidth - (event.pageX - event.target.totalOffsetLeft));

Maybe we should just acount for clientLeft in the caller?

[Timothy Hatcher]

Maybe if we didn't add clientLeft for the inner element? I think adding it in
for the ancestors is correct.

[Pavel Feldman]

Landed with review comments addressed.

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	D	WebCore/inspector/front-end/Popup.js
	M	WebCore/ChangeLog
	M	WebCore/WebCore.gypi
	M	WebCore/WebCore.vcproj/WebCore.vcproj
	A	WebCore/inspector/front-end/Images/gearButtonGlyph.png
	A	WebCore/inspector/front-end/Images/popoverArrows.png
	A	WebCore/inspector/front-end/Images/popoverBackground.png
	A	WebCore/inspector/front-end/Images/thumbActiveHoriz.png
	A	WebCore/inspector/front-end/Images/thumbActiveVert.png
	A	WebCore/inspector/front-end/Images/thumbHoriz.png
	A	WebCore/inspector/front-end/Images/thumbHoverHoriz.png
	A	WebCore/inspector/front-end/Images/thumbHoverVert.png
	A	WebCore/inspector/front-end/Images/thumbVert.png
	A	WebCore/inspector/front-end/Images/trackHoriz.png
	A	WebCore/inspector/front-end/Images/trackVert.png
	A	WebCore/inspector/front-end/Popover.js
	M	WebCore/inspector/front-end/SourceFrame.js
	M	WebCore/inspector/front-end/TextEditorHighlighter.js
	M	WebCore/inspector/front-end/TextViewer.js
	M	WebCore/inspector/front-end/WebKit.qrc
	M	WebCore/inspector/front-end/inspector.css
	M	WebCore/inspector/front-end/inspector.html
	M	WebCore/inspector/front-end/inspectorSyntaxHighlight.css
	A	WebCore/inspector/front-end/popover.css
	M	WebCore/inspector/front-end/utilities.js
Committed r54962

[Peter Kasting]

The patch added "popoverArrows.png", but referenced "popoverArrow.png" in the relevant build files.  This broke the inspector_resources Chromium project build.

r54963 fixed the Qt side of this, but not the Chromium side.  I checked in an unreviewed fix to the Chromium side in r54989.