Closed Bug 1037446 Opened 10 years ago Closed 10 years ago

[rule view] Adding new rules fails when document.head is undefined

Categories

(DevTools :: Inspector, defect)

Product:
DevTools
Component:
Inspector
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 33

People

(Reporter: bgrins, Assigned: gl)

References

Details

Attachments

(1 file, 1 obsolete file)

1037446.patch
1.07 KB, patch
gl
: review+
Details | Diff | Splinter Review 
Open: chrome://browser/content/devtools/scratchpad.xul
Select an element
Right click -> Add rule

TypeError: document.head is undefined
Assignee: nobody → gabriel.luong
Attachment #8454727 - Flags: review?(fayearthur) → review+
Keywords: checkin-needed
Comment on attachment 8454727 [details] [diff] [review]
1037446.patch

Review of attachment 8454727 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/actors/styles.js
@@ +563,2 @@
>        style.setAttribute("type", "text/css");
> +      document.documentElement.appendChild(style);

Drive by: make sure to add r=harth on the end of the commit message.

Also, I'm not sure what the best thing to do here is, but it seems this element shows up in the inspector, so *if* document.head is available maybe it should still go in there to prevent confusion (seeing a new <style> tag below <body>)?  Then again, there is a benefit to putting it below everything in that it will have higher priority than even style tags added inside the body.  Just a thought, any opinions?
Attached patch 1037446.patch — — Splinter Review 
Updated commit message.

(In reply to Brian Grinstead [:bgrins] from comment #2)
> Comment on attachment 8454727 [details] [diff] [review]
> 1037446.patch
> 
> Review of attachment 8454727 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/server/actors/styles.js
> @@ +563,2 @@
> >        style.setAttribute("type", "text/css");
> > +      document.documentElement.appendChild(style);
> 
> Drive by: make sure to add r=harth on the end of the commit message.
> 
> Also, I'm not sure what the best thing to do here is, but it seems this
> element shows up in the inspector, so *if* document.head is available maybe
> it should still go in there to prevent confusion (seeing a new <style> tag
> below <body>)?  Then again, there is a benefit to putting it below
> everything in that it will have higher priority than even style tags added
> inside the body.  Just a thought, any opinions?

I considered doing a check to put it in the head if it's available. I decided to append it in documentElement since it had a visibility benefit (being after the body). I assume there would be confusion if we placed it inside of the head as well, but there is a chance the developer can see the new <style> tag added in the inspector. I didn't know about the higher priority, but I think that is also a plus.
Attachment #8454727 - Attachment is obsolete: true
Attachment #8454835 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/92f3431bd29d
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: