The default bug view has changed. See this FAQ.

Error bubbles for multiline messages in console look silly

RESOLVED FIXED in Firefox 8

Status

()

Firefox
Developer Tools
--
trivial
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jrmuizel, Assigned: rc)

Tracking

unspecified
Firefox 8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Created attachment 538024 [details]
Screenshot

See screenshot.
(Assignee)

Comment 1

6 years ago
Created attachment 546113 [details] [diff] [review]
silly-bubbles
Assignee: nobody → rcampbell
Status: NEW → ASSIGNED
Attachment #546113 - Flags: review?(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".
Attachment #546113 - Flags: review?(dao) → review-
(Assignee)

Comment 3

6 years ago
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.
(Assignee)

Comment 4

6 years ago
Created attachment 546442 [details] [diff] [review]
[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.
Attachment #546113 - Attachment is obsolete: true
Attachment #546442 - Flags: review?(dao)
(Assignee)

Comment 5

6 years ago
ping for review?
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.
Attachment #546442 - Flags: review?(dao) → review+

Updated

6 years ago
Severity: normal → trivial
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 7

6 years ago
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.
(Assignee)

Comment 8

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

http://hg.mozilla.org/mozilla-central/rev/a6ae7b82861d
Attachment #546442 - Attachment description: silly-bubbles-2 → [checked-in] silly-bubbles-2
(Assignee)

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 8
You need to log in before you can comment on or make changes to this bug.