Closed Bug 588014 Opened 12 years ago Closed 12 years ago

Format clickable output to look like being clickable

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(blocking2.0 -)

VERIFIED FIXED
Tracking Status
blocking2.0 --- -

People

(Reporter: julian.viereck, Assigned: jwalker)

References

Details

(Whiteboard: [Web-Console-Testday])

Attachments

(1 file, 3 obsolete files)

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.
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.
Whiteboard: [kd4b5]
Duplicate of this bug: 576316
Assignee: nobody → jviereck
Whiteboard: [kd4b5] → [kd4b6]
Assignee: jviereck → pwalton
Severity: normal → blocker
Reprioritizing bugs. You can filter the mail on the word TEABAGS.
Severity: blocker → normal
Assignee: pwalton → jwalker
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.
blocking2.0: --- → ?
Blocks: devtools4b7
Attached patch Initial patch (obsolete) — Splinter Review
Yellow is terrible.
It is the right thing to change the element type from label to description?
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.
Attached patch Upload 2 (obsolete) — Splinter Review
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
Attachment #473037 - Attachment is obsolete: true
Attachment #473573 - Flags: feedback?(mihai.sucan)
Thanks for the feedback Mihai. Almost all your comments have been addressed with the exception of the notes above.
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.
Attachment #473573 - Flags: feedback?(mihai.sucan) → feedback+
Attached patch upload 3 (obsolete) — Splinter Review
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
Attachment #473573 - Attachment is obsolete: true
Attachment #473696 - Flags: feedback?(jwalker)
Attachment #473696 - Flags: review?(dietrich)
Attachment #473696 - Flags: feedback?(jwalker)
Attachment #473696 - Flags: feedback+
Thanks Mihai, that looks fine.
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.
Attachment #473696 - Flags: review?(dietrich) → review+
Comments removed. Thanks.
Attachment #473696 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [kd4b6]
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.
Attachment #474004 - Flags: review+
Attachment #474004 - Flags: approval2.0?
Attachment #474004 - Flags: approval2.0? → approval2.0+
Comment on attachment 474004 [details] [diff] [review]
[checked-in] Upload 4 (r=dietrich)

http://hg.mozilla.org/mozilla-central/rev/7654e8418f86
Attachment #474004 - Attachment description: Upload 4 (r=dietrich) → [checked-in] Upload 4 (r=dietrich)
Note, there is still 'commented out stuff' in there, which was requested by comment 12 to be removed.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Wouldn't hold the release for this, blocking-.
blocking2.0: ? → -
(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?
(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.
Filed bug 596177 on removing that comment.
Depends on: 596177
No longer depends on: 596177
Status: RESOLVED → VERIFIED
Whiteboard: [Web-Console-Testday]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.