588014 - Format clickable output to look like being clickable

Status: VERIFIED FIXED

(DevTools :: General, defect)

Description

[Julian Viereck]

The WebConsole has some output that is click-/inspectable (e.g. PropertyPanel to inspect JS/DOM objects, NetworkPanel to inspect network requests). Currently, this kind of output looks the same like other output in the WebConsole. It should be styled to tell the user that it can be clicked.

Comment 1

[Kevin Dangoor]

This one could slip into a later beta, but given that b5 is going to be adding the clickable content it is likely best to try to get this change into b5.

Comment 3

[Kevin Dangoor]

Reprioritizing bugs. You can filter the mail on the word TEABAGS.

Comment 4

[Kevin Dangoor]

Requesting blocking status for this bug. People will have a hard time discovering that there's more information about objects and network traffic without making this change.

Comment 5

[Joe Walker [:jwalker]]

Attachment: Initial patch

Yellow is terrible.
It is the right thing to change the element type from label to description?

Comment 6

[Mihai Sucan [:msucan]]

I looked into the patch and tested it. Thoughts:
 
-      var timestampedMessage = messageObject.timestampedMessage;
-      var urlIdx = timestampedMessage.indexOf(aActivityObject.url);
-      messageObject.prefix = timestampedMessage.substring(0, urlIdx);
+      messageObject.messageNode.setAttribute("class", "hud-clickable");
+      messageObject.messageNode.setAttribute("crop", "end");

There are two problems with that change:

- messageObject.prefix is used by the network panel code, you should not remove this.

- messageNode.setAttribute("class", "hud-clickable") overwrites the current list of class names that are set on the element. I get unstyled output now, also the rest of class names are required to not be lost in the process.

In JSTerm.writeOutputJS():

     var node = this.xulElementFactory("label");
     node.setAttribute("class", "jsterm-output-line");
[...] 
+    let outputNode = this.xulElementFactory("description");
[...]
-    let textNode = this.textFactory(aOutputObject);
-    node.appendChild(textNode);
+    outputNode.setAttribute("value" , "\u27A4 " + aOutputObject);
+    outputNode.setAttribute("class", "hud-clickable");
+    outputNode.setAttribute("crop", "end");
+
+    node.appendChild(outputNode);
 
     lastGroupNode.appendChild(node);

That adds a xul:description inside a xul:label, to a xul:group. Not nice.

Why not change the xul:label to a xul:description? and just add one xul:description into the group. Should work fine. Also, make sure you set the class attribute to be "jsterm-output-line hud-clickable" (both class names).

diff --git a/toolkit/themes/pinstripe/global/webConsole.css b/toolkit/themes/pinstripe/global/webConsole.css

Please make the same changes in winstripe and gnomestripe, so we get cross-platform rendering of the hud-clickable elements.

- Running the HUDService tests shows 16 tests failing. I believe some of the above mentioned changes break tests (eg. lack of some class names, or lack of .prefix property, etc.).


Other thoughts:

- the entire output line is clickable, and the Property Panel shows up, or the Network Panel shows up. The arrow you added for the URLs make it look like only the URL is clickable, and that's confusing.

- should clicking the URL open the clicked link or open the network panel?

- clicking the output line multiple times opens the same panel multiple times. it shouldn't do this.

- right-clicking the output line opens the panel. again this shouldn't happen.

(the last two items are not a new problem, but if they are easy fixes, perhaps they should go into this patch, or make a follow-up report)

- there's bug 592309 and your patch almost fixes it. Please also update HUDConsole.sendToHUDService() to use a xul:description instead of a xul:label, and JSTerm.writeOutput() and that's all (two lines of changes only! hehe).

That's all for now.

Comment 7

[Joe Walker [:jwalker]]

Attachment: Upload 2

Things that need doing:
- I've commented out the separator code, we should perhaps remove the whole section.
- Copy the styling over to windows/linux

Comment 8

[Joe Walker [:jwalker]]

Thanks for the feedback Mihai. Almost all your comments have been addressed with the exception of the notes above.

Comment 9

[Mihai Sucan [:msucan]]

Comment on attachment 473573 [details] [diff] [review]
Upload 2

You're welcome, Joe!

Hm, given that the code which appends groups was added to show that separator, maybe keep the commented snippet there without removing. Or? Not sure, please ask ddahl or robcee.

Tests still fail:

TEST-INFO | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js | Console message: [JavaScript Error: "group.childNodes[1] is undefined" {file: "chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js" line: 225}]

This happens because the separator is missing. Please run the tests and fix them.

I believe you forgot to address the issue of mixing xul:label and xul:description elements in the output node. Please change:

    let messageNode = hud.makeXULNode("label");
to
    let messageNode = hud.makeXULNode("description");
(in HUDConsole.sendToHUDService())

Please do this similarly for JSTerm.writeOutput() as well. With your patch we add both xul:description and xul:label elements into the output node, not a really good mix I believe. It's easy to make them all xul:description elements.

In NodeFactory:

+    throw new Error('NodeFactory: Unknown factory type: ' + aFactoryType);

Please change that to double quotes, as all other code uses double quotes, not single quotes. Also, I am not sure if we want to throw. I think it's better to just call Cu.reportError(...).


That's all for now.

Comment 10

[Mihai Sucan [:msucan]]

Attachment: upload 3

Updated patch with the changes I suggested in my feedback (as discussed on IRC).

Changes:

- switched the xul:description elements to xul:label elements because I just found that the filtering mechanism breaks if we switch to descriptions.

- fixed all tests. Offsets in tests that checked the exact positioning in the DOM for each of the output nodes became wrong now that the xul:separator went missing, hehe. One thought: if we ever want to add the separator back, we must do that without an element - we can do it with CSS.

- added the style changes from pinstripe (mac) to winstripe (windows) and gnomestripe (linux).

For context-awareness, for Joe, here's the diff from upload 2 (attachment 473573 [details] [diff] [review]) to this new patch:

http://hg.mozilla.org/users/mihai.sucan_gmail.com/devtools-work/rev/7d605a0c1c6d

Comment 11

[Joe Walker [:jwalker]]

Thanks Mihai, that looks fine.

Comment 12

[Dietrich Ayala (:dietrich)]

Comment on attachment 473696 [details] [diff] [review]
upload 3


> 
>+    /*
>     let separatorNode = chromeDocument.createElement("separator");
>     separatorNode.setAttribute("class", "groove hud-divider");
>     separatorNode.setAttribute("orient", "horizontal");
>     groupNode.appendChild(separatorNode);
>+    */

remove

> .hud-msg-node {    
>     width: 100%;
>     margin-top: 0.3em; 
>     margin-bottom: 0.3em; 
>     padding-left: 0.3em;
>-    border-bottom: 1px solid #eee;   
>+    /* border-bottom: 1px solid #eee; */
> }


> .hud-msg-node {    
>     width: 100%;
>     margin-top: 0.3em; 
>     margin-bottom: 0.3em; 
>     padding-left: 0.3em;
>-    border-bottom: 1px solid #eee;   
>+    /* border-bottom: 1px solid #eee; */
> }


> .hud-msg-node {    
>     width: 100%;
>     margin-top: 0.3em; 
>     margin-bottom: 0.3em; 
>     padding-left: 0.3em;
>-    border-bottom: 1px solid #eee;   
>+    /* border-bottom: 1px solid #eee; */
> }

r=me on a patch with all the commented out stuff removed. for future, please clean up patches prior to asking for review.

Comment 13

[Joe Walker [:jwalker]]

Attachment: [checked-in] Upload 4 (r=dietrich)

Comments removed. Thanks.

Comment 14

[Kevin Dangoor]

Comment on attachment 474004 [details] [diff] [review]
[checked-in] Upload 4 (r=dietrich)

(review carried forward). requesting approval for this because the UI is not very discoverable without it.

Comment 15

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 474004 [details] [diff] [review]
[checked-in] Upload 4 (r=dietrich)

http://hg.mozilla.org/mozilla-central/rev/7654e8418f86

Comment 16

[Alfred Kayser]

Note, there is still 'commented out stuff' in there, which was requested by comment 12 to be removed.

Comment 17

[Dietrich Ayala (:dietrich)]

Wouldn't hold the release for this, blocking-.

Comment 18

[Michael Lefevre]

(In reply to comment #17)
> Wouldn't hold the release for this, blocking-.

Er... this bug was already fixed.

But there is still that stray commented out line of code which should not have been checked in. Does anyone care enough to remove it? Should I file a new bug?

Comment 19

[David Dahl :ddahl]

(In reply to comment #18)
> (In reply to comment #17)
> > Wouldn't hold the release for this, blocking-.
> 
> Er... this bug was already fixed.
> 
> But there is still that stray commented out line of code which should not have
> been checked in. Does anyone care enough to remove it? Should I file a new bug?

yes, file a new bug. thanks.

Comment 20

[Michael Lefevre]

Filed bug 596177 on removing that comment.