Adding a textNode to an existing xul:label causes an error in the WebConsole

RESOLVED FIXED

Status

RESOLVED FIXED
8 years ago
3 months ago

People

(Reporter: julian.viereck, Assigned: pcwalton)

Tracking

Trunk
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [kd4b5] [patchclean:0826])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

8 years ago
In bug 568634 I add a xul:textNodes to an existing xul:label in the WebConsole. This new insert node makes the

  this.outputNode.addEventListener("DOMNodeInserted", function(ev) {

throw an error (taken from an unit test):

TEST-INFO | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js | Console message: [JavaScript Error: "ev.target.classList is undefined" {file: "resource://gre/modules/HUDService.jsm" line: 2265}]

TextNodes doesn't have a classList object (== undefined) and therefore the check

line 2265:  if (ev.target.classList.contains("hud-msg-node")) {

failes.

Updated

8 years ago
Whiteboard: [kd4b5]
(Assignee)

Comment 1

8 years ago
Created attachment 467949 [details] [diff] [review]
Proposed patch.

Patch attached.
Assignee: nobody → pwalton
Status: NEW → ASSIGNED
Attachment #467949 - Flags: feedback?
(Reporter)

Comment 2

8 years ago
Patrick, I haven't tried the patch but it looks good from just reading the code. This is not part of this bug, but if a new textNode is insert, the parent node of the textNode should get checked in case it matches the filtering rules or not.

Imagine a label has the text "foo" and filtering is set to "bar". The "foo"-label is not visible. Now the string "baring" is added to the "foo"-label. This now matches the search string "bar" and the "foo"-label should show up. Do you want me to fill a separate bug on that?
(Reporter)

Comment 3

8 years ago
(In reply to comment #1)
> Created attachment 467949 [details] [diff] [review]
> Proposed patch.
> 
> Patch attached.

Patrick, I included your patch into my repo without the fix so that only the unit tests run. They passed although I could see some 

  Console message: [JavaScript Error: "ev.target.classList is undefined" {file: "resource://gre/modules/HUDService.jsm" line: 3005}]

which means that the unit test doesn't work. I tried to figure out why the unit test is not working (adding the textNode after a timeout of 1 sec and see if an error occurred) but I couldn't. Let's discuss this later.
(Reporter)

Comment 4

8 years ago
Created attachment 468319 [details]
Patch I've used to test unit test

This patch is based on Patrick's submission with some modifications that should make the unit test fail - but they don't.

Patrick, can you reproduce that this isn't failing?

Updated

8 years ago
Whiteboard: [kd4b5] → [kd4b6]
(Assignee)

Updated

8 years ago
Whiteboard: [kd4b6] → [kd4b6] [patchbitrot]
(Assignee)

Comment 5

8 years ago
Created attachment 469643 [details] [diff] [review]
Proposed patch, version 2.

New patch attached. Unit tests seem to pass on my machine; try is borked due to bug 591074 so I can't be 100% confident at this point.
Attachment #467949 - Attachment is obsolete: true
Attachment #469643 - Flags: feedback?
Attachment #467949 - Flags: feedback?
(Assignee)

Updated

8 years ago
Whiteboard: [kd4b6] [patchbitrot] → [kd4b6] [patchclean:0826]

Comment 6

8 years ago
(In reply to comment #5)
> Created attachment 469643 [details] [diff] [review]
> Proposed patch, version 2.
> 
> New patch attached. Unit tests seem to pass on my machine; try is borked due to
> bug 591074 so I can't be 100% confident at this point.

Do you have a windows build and linux build to try it out on? I think we should all have them at this point:(
(Assignee)

Updated

8 years ago
Whiteboard: [kd4b6] [patchclean:0826] → [kd4b6] [patchbitrot]
(Assignee)

Comment 7

8 years ago
Created attachment 469696 [details] [diff] [review]
Proposed patch, version 2.1.

Patch rebased to trunk.
Attachment #469643 - Attachment is obsolete: true
Attachment #469696 - Flags: feedback?
Attachment #469643 - Flags: feedback?
(Assignee)

Updated

8 years ago
Whiteboard: [kd4b6] [patchbitrot] → [kd4b6] [patchclean:0826]
(Reporter)

Updated

8 years ago
Blocks: 568634

Updated

8 years ago
blocking2.0: --- → ?

Comment 8

8 years ago
This bug is blocking blockers (some even designated as beta5+)

Updated

8 years ago
Whiteboard: [kd4b6] [patchclean:0826] → [kd4b5] [patchclean:0826]
(Assignee)

Updated

8 years ago
Attachment #469696 - Flags: review?(dietrich)
Attachment #469696 - Flags: feedback?(mihai.sucan)
Attachment #469696 - Flags: feedback?
Comment on attachment 469696 [details] [diff] [review]
Proposed patch, version 2.1.

This patch looks fine to me.

As far as I know the listener in the test code does not need to be an object with the observe method. I think you can directly pass the function. (this works with the observer service - might work with the console service listeners as well)

Also, you manually get the nsIConsoleService component. You should get it much quicker with Services.console from Services.jsm.
Attachment #469696 - Flags: feedback?(mihai.sucan) → feedback+
Attachment #469696 - Flags: review?(dietrich) → review+
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
blocking2.0: ? → final+

Updated

8 years ago
Keywords: checkin-needed

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Updated

3 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.