662807 - Error bubbles for multiline messages in console look silly

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Attachment: Screenshot

See screenshot.

Comment 1

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

Attachment: silly-bubbles

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 546113 [details] [diff] [review]
silly-bubbles

The right solution here is to put the label in a container with align="start".

Comment 3

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

d'oh. I was hoping something simple like this would be sufficient. Adding a container (a div? any suggestions on which container I should use?) will require modifying some of the HUDService.jsm.

Comment 4

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

Attachment: [checked-in] silly-bubbles-2

here's a second attempt using an hbox container aligned:start.

I think this might need a little padding on the right as some of the unittests seem to have the repeat bubble crushed right against the sourcelink at the right, but need to do a little more testing.

Comment 5

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

ping for review?

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 546442 [details] [diff] [review]
[checked-in] silly-bubbles-2

>+    let repeatContainer = aDocument.createElementNS(XUL_NS, "xul:hbox");

s/xul:hbox/hbox/

There's a bug filed on fixing this throughout this code, but nobody's working on it. Anyway, we shouldn't add more broken code for consistency with other broken code.

>   mergeFilteredMessageNode:
>   function ConsoleUtils_mergeFilteredMessageNode(aOriginal, aFiltered) {
>-    // childNodes[3] is the node containing the number of repetitions of a node.
>-    let repeatNode = aOriginal.childNodes[3];
>+    // childNodes[3].firstChild is the node containing the number of repetitions
>+    // of a node.
>+    let repeatNode = aOriginal.childNodes[3].firstChild;
>     if (!repeatNode) {
>       return aOriginal; // no repeat node, return early.
>     }
> 
>     let occurrences = parseInt(repeatNode.getAttribute("value")) + 1;
>     repeatNode.setAttribute("value", occurrences);
>   },

Sigh. All this would be so much cleaner (e.g. aOriginal.setAttribute("repeat", parseInt(aOriginal.getAttribute("repeat")) + 1)) with the messages' internal markup hidden in a binding.

Comment 7

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

Thanks for the reviews, Dão.

(In reply to comment #6)
> Comment on attachment 546442 [details] [diff] [review] [review]
> silly-bubbles-2
> 
> >+    let repeatContainer = aDocument.createElementNS(XUL_NS, "xul:hbox");
> 
> s/xul:hbox/hbox/
> 
> There's a bug filed on fixing this throughout this code, but nobody's
> working on it. Anyway, we shouldn't add more broken code for consistency
> with other broken code.

ok, will do. I'll dig that bug up and take care of it.

> >   mergeFilteredMessageNode:
> >   function ConsoleUtils_mergeFilteredMessageNode(aOriginal, aFiltered) {
> >-    // childNodes[3] is the node containing the number of repetitions of a node.
> >-    let repeatNode = aOriginal.childNodes[3];
> >+    // childNodes[3].firstChild is the node containing the number of repetitions
> >+    // of a node.
> >+    let repeatNode = aOriginal.childNodes[3].firstChild;
> >     if (!repeatNode) {
> >       return aOriginal; // no repeat node, return early.
> >     }
> > 
> >     let occurrences = parseInt(repeatNode.getAttribute("value")) + 1;
> >     repeatNode.setAttribute("value", occurrences);
> >   },
> 
> Sigh. All this would be so much cleaner (e.g.
> aOriginal.setAttribute("repeat", parseInt(aOriginal.getAttribute("repeat"))
> + 1)) with the messages' internal markup hidden in a binding.

It would be. I'm not overly fond of these childNodes[n] lookups for this stuff.

I'll file a follow-up to convert our messages to XBL.

Comment 8

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

Comment on attachment 546442 [details] [diff] [review]
[checked-in] silly-bubbles-2

http://hg.mozilla.org/mozilla-central/rev/a6ae7b82861d