[FIX]HTMLStyleElement.innerHTML is horked

VERIFIED FIXED in mozilla1.1beta

Status

()

defect
P1
normal
VERIFIED FIXED
18 years ago
2 months ago

People

(Reporter: bc, Assigned: bzbarsky)

Tracking

({testcase})

Trunk
mozilla1.1beta
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

Reporter

Description

18 years ago
Setting the innerHTML property of an HTMLStyleELement to 'foo' results in
innerHTML containing '<style></style>foo'
Reporter

Comment 1

18 years ago
Posted file testcase
Reporter

Updated

18 years ago
Keywords: testcase
I think the most reasonable course of action is to drop the old sheet if any and
treat the new "html" as just stylesheet data...  Will look into doing that.

Comment 3

18 years ago
In IE,innerHTML is read only on some elements.
STYLE is one of those elements.
Even having it as read only does not make much sense on STYLE
Yeah, but this will be simple to fix once bug 34849 is fixed.  :)  Marking
dependent, pushing out to 1.0
Depends on: 34849
OS: Windows 2000 → All
Priority: -- → P3
Hardware: PC → All
Target Milestone: --- → mozilla1.0
I'm going out of town in two days and not getting back till after 1.0 freeze. 
Setting realistic milestones for all 1.0 bugs that depend on other bugs or on
possible interface work.

Any help on getting these fixed would be much appreciated...
Keywords: helpwanted
Target Milestone: mozilla1.0 → mozilla1.1alpha
Bug 34849 didn't fix this. The content model looks wrong though. After the
script has run I see

       STYLE
         |
         +--STYLE
         |
         +--textnode with the style rule

That second STYLE element shouldn't be there I think. If we can fix that, the
style will be applied.
1.1alpha is frozen.  Unsetting milestone and will retriage in a few days when I
can make a realistic assessment of the situation.
Target Milestone: mozilla1.1alpha → ---
Looks like an issue in nsHTMLFragmentContentSink::AddLeaf at first glance...
OK. So there's a number of issues here:

1)  <style>, <textarea>, <xmp>, <script>, <plaintext> are considered _leaf_
    nodes. So the fragment sink unconditionally appends them....
2)  The <endnote> mess used to denote the location of the context.

So the parser parses something like "<html><head><style><endnote>foo" (the
original testcase).  It ends up calling AddLeaf() on the "<style>" and then
OpenContainer() on the <endnote> and AddLeaf() on the "foo", since apparently
the <endnote> causes the <style> to be closed out!

The <textarea> case is similar, except there <endnote> does not close out the
textarea but is instead treated as text inside the textarea.

So what do we want to do?  We can disable setting innerHTML on the elements in
question, possibly (the container elements treated as leaves by the parser). 
But createContextualFragment would still be broken for such elements.... Do we
want to try and fix that?  Ccing akkana, since that's where the CVS blame points
for the initial ParseFragment() impl.

It looks like editor's cut/paste is looks like the only real user of
CreateContextualFragment.... Therefore, simply disabling it for the elements in
question should be OK, since no one will be pasting into any of the elements in
question in our editor, as far as I can tell (though not so sure about <xmp> and
<plaintext>).

Thoughts?
Posted patch Proposed fix v 1.0 (obsolete) — Splinter Review
Sets up the future XMLFragmentSink stuff a bit too...
Comment on attachment 88281 [details] [diff] [review]
Proposed fix v 1.0

This breaks some copy/paste stuff in composer.... Will tweak to fix that.
Attachment #88281 - Flags: needs-work+
Posted patch Proposed patch v 2.0 (obsolete) — Splinter Review
This should be much better
Attachment #88281 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Keywords: helpwanted
Priority: P3 → P1
Target Milestone: --- → mozilla1.1beta
Comment on attachment 89390 [details] [diff] [review]
Proposed patch v 2.0

>+nsresult
>+nsGenericHTMLContainerElement::ReplaceContentsWithText(const nsAString& aText,
>+                                                       PRBool aNotify) {
>+  PRInt32 children;
>+  nsresult rv = ChildCount(children);
>+  if (NS_SUCCEEDED(rv)){

Do NS_ENSURE_SUCCESS(rv, rv) instead.

>+    PRInt32 i;
>+    for (i = children - 1; i >= 0; --i) {
>+      RemoveChildAt(i, aNotify);
>+    }
>+  }
>+    
>+  if (!aText.IsEmpty()) {
>+    nsCOMPtr<nsIContent> text;
>+    rv = NS_NewTextNode(getter_AddRefs(text));
>+    if (NS_SUCCEEDED(rv)) {

use NS_ENSURE_TRUE here too.

>+      nsCOMPtr<nsIDOMText> tc = do_QueryInterface(text);
>+      if (tc) {

IMHO you don't need the if(tc) check.


It might be nice to make nsHTMLScriptElement::(S|G)etText and
nsHTMLTextAreaElement::(S|G)etDefaultValue use your new functions. Have a look
at the implementation of nsHTMLTextAreaElement::SetDefaultValue which is a bit
more optimized then your implementation.


We could also use GetContentsAsText in nsStyleLinkElement, which doesn't
inherit anything except a couple of interfaces. So if that function lived as a
static in some helperclass then we'd be able to use it there too. Feel free to
do as you please.


>+nsresult
>+nsGenericHTMLContainerElement::GetContentsAsText(nsAString& aText)
>+{
>+  aText.Truncate();
>+  PRInt32 children;
>+  nsresult rv = ChildCount(children);
>+  nsCOMPtr<nsIDOMText> tc;
>+  nsCOMPtr<nsIContent> child;
>+  nsAutoString textData;
>+  if (NS_SUCCEEDED(rv)){

NS_ENSURE_TRUE :)

>+    PRInt32 i;
>+    for (i = 0; i < children; ++i) {
>+      ChildAt(i, *getter_AddRefs(child));
>+      tc = do_QueryInterface(child);
>+      // XXX we should really consider asserting that tc is non-null here..

IMHO no, someone could add a non-textchild using the DOM, we don't want to
assert in that case. assertions should be for fatal conditions and we defenetly
don't wanna be fatal then.

>+protected:
>+  nsresult ReplaceContentsWithText(const nsAString& aText, PRBool aNotify);
>+  nsresult GetContentsAsText(nsAString& aText);

please add a comment to the functions. Especially comment on the fact that
GetContentsAsText doesn't do a deep walk.

>+NS_IMETHODIMP
>+nsHTMLScriptElement::GetInnerHTML(nsAString& aInnerHTML)
>+{
>+  return GetText(aInnerHTML);
>+}
>+
>+NS_IMETHODIMP
>+nsHTMLScriptElement::SetInnerHTML(const nsAString& aInnerHTML)
>+{
>+  return SetText(aInnerHTML);
>+}

For the sake of consistency it might be nice to use
GetContentsAsText/ReplaceContentsWithText (same thing in textarea)
Posted patch Address those comments (obsolete) — Splinter Review
Attachment #89390 - Attachment is obsolete: true
Attachment #89859 - Attachment is obsolete: true
Comment on attachment 89867 [details] [diff] [review]
Pre-address some more comments

r=me
Attachment #89867 - Flags: review+
Summary: HTMLStyleElement.innerHTML is horked → [FIX]HTMLStyleElement.innerHTML is horked
Comment on attachment 89867 [details] [diff] [review]
Pre-address some more comments

- In nsGenericHTMLContainerElement::ReplaceContentsWithText():

+  if (!textChild) {
...
+  } else {
+    rv = textChild->SetData(aText);

This ignores the aNotify flag, do you want to QI to nsITextContent and call
SetText() on that so that you can pass in the aNotify flag?

Other than that, sr=jst
Attachment #89867 - Flags: superreview+
Per discussion with jst, leaving that part as-is and checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
Also per discussion filed bug 155723

Updated

17 years ago
QA Contact: lchiang → ian
v per testcase
Status: RESOLVED → VERIFIED
Component: DOM: Mozilla Extensions → DOM
Product: Core → Core
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.