Bug 35003 - Web Inspector: Implement evaluate-on-hover for scripts panel.
Summary: Web Inspector: Implement evaluate-on-hover for scripts panel.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Pavel Feldman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-16 15:35 PST by Pavel Feldman
Modified: 2010-02-18 14:41 PST (History)
6 users (show)

See Also:


Attachments
[IMAGE] Screen 1 (159.45 KB, image/png)
2010-02-16 15:42 PST, Pavel Feldman
no flags Details
[IMAGE] Screen 2 (163.01 KB, image/png)
2010-02-16 15:42 PST, Pavel Feldman
no flags Details
Mock Up (164.09 KB, image/png)
2010-02-16 17:35 PST, Timothy Hatcher
no flags Details
Artwork and CSS for popover with arrow (4.74 KB, application/zip)
2010-02-16 23:14 PST, Timothy Hatcher
no flags Details
Scrollbar art (1.84 KB, application/zip)
2010-02-17 02:29 PST, Timothy Hatcher
no flags Details
Scrollbar hover/active art (2.03 KB, application/zip)
2010-02-17 02:43 PST, Timothy Hatcher
no flags Details
Horizontal scrollbar art (4.12 KB, application/zip)
2010-02-17 03:01 PST, Timothy Hatcher
no flags Details
[IMAGE] Work in progress image 1. (102.03 KB, image/png)
2010-02-17 10:34 PST, Pavel Feldman
no flags Details
[IMAGE] Work in progress image 2. (103.95 KB, image/png)
2010-02-17 10:34 PST, Pavel Feldman
no flags Details
[IMAGE] Work in progress image 3. (106.40 KB, image/png)
2010-02-17 10:38 PST, Pavel Feldman
no flags Details
[PATCH] Proposed fix (work in progress, couple of nits I need to fix). (35.13 KB, patch)
2010-02-17 10:51 PST, Pavel Feldman
no flags Details | Formatted Diff | Diff
[IMAGE] Patch looks. (100.53 KB, image/png)
2010-02-18 06:29 PST, Pavel Feldman
no flags Details
[PATCH] Proposed change. (43.81 KB, patch)
2010-02-18 06:30 PST, Pavel Feldman
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Feldman 2010-02-16 15:35:47 PST
Hover card should allow browsing evaluation results.
Comment 1 Pavel Feldman 2010-02-16 15:42:10 PST
Created attachment 48843 [details]
[IMAGE] Screen 1
Comment 2 Pavel Feldman 2010-02-16 15:42:47 PST
Created attachment 48844 [details]
[IMAGE] Screen 2
Comment 3 Pavel Feldman 2010-02-16 15:43:39 PST
Calling for input / feedback on the screenshots!
Comment 4 Timothy Hatcher 2010-02-16 16:17:19 PST
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
Comment 5 Timothy Hatcher 2010-02-16 17:35:13 PST
Created attachment 48851 [details]
Mock Up

I am still thinking about what to do with the detail view.
Comment 6 Timothy Hatcher 2010-02-16 23:14:43 PST
Created attachment 48864 [details]
Artwork and CSS for popover with arrow
Comment 7 Timothy Hatcher 2010-02-17 02:29:47 PST
Created attachment 48877 [details]
Scrollbar art
Comment 8 Timothy Hatcher 2010-02-17 02:43:17 PST
Created attachment 48880 [details]
Scrollbar hover/active art
Comment 9 Timothy Hatcher 2010-02-17 03:01:06 PST
Created attachment 48881 [details]
Horizontal scrollbar art
Comment 10 Pavel Feldman 2010-02-17 10:34:20 PST
Created attachment 48908 [details]
[IMAGE] Work in progress image 1.
Comment 11 Pavel Feldman 2010-02-17 10:34:45 PST
Created attachment 48909 [details]
[IMAGE] Work in progress image 2.
Comment 12 Pavel Feldman 2010-02-17 10:38:20 PST
Created attachment 48910 [details]
[IMAGE] Work in progress image 3.
Comment 13 Pavel Feldman 2010-02-17 10:51:55 PST
Created attachment 48912 [details]
[PATCH] Proposed fix (work in progress, couple of nits I need to fix).
Comment 14 Eric Seidel (no email) 2010-02-17 16:39:49 PST
Sadly the patch does not apply, so the EWS bots can't tell us if it builds or not. :(
Comment 15 Timothy Hatcher 2010-02-17 17:34:15 PST
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.
Comment 16 Timothy Hatcher 2010-02-17 17:35:37 PST
Maybe Popup.js should be renamed to Popover.js?
Comment 17 Pavel Feldman 2010-02-17 22:26:36 PST
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.
Comment 18 Pavel Feldman 2010-02-18 06:29:17 PST
Created attachment 49008 [details]
[IMAGE] Patch looks.
Comment 19 Pavel Feldman 2010-02-18 06:30:18 PST
Created attachment 49009 [details]
[PATCH] Proposed change.

Comments addressed and more!
Comment 20 Timothy Hatcher 2010-02-18 07:54:49 PST
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.
Comment 21 Timothy Hatcher 2010-02-18 08:00:14 PST
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?
Comment 22 Timothy Hatcher 2010-02-18 08:02:59 PST
Maybe if we didn't add clientLeft for the inner element? I think adding it in
for the ancestors is correct.
Comment 23 Pavel Feldman 2010-02-18 08:13:48 PST
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
Comment 24 Peter Kasting 2010-02-18 14:41:14 PST
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.