Closed Bug 125746 Opened 23 years ago Closed 22 years ago

[FIX]HTMLStyleElement.innerHTML is horked

Categories

(Core :: DOM: Core & HTML, defect, P1)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.1beta

People

(Reporter: bc, Assigned: bzbarsky)

References

Details

(Keywords: testcase)

Attachments

(4 files, 3 obsolete files)

testcase
785 bytes, text/html
Details
HTMLTextareaElement has the same problem....
495 bytes, text/html
Details
xmp/plaintext as well, and I could write a testcase for <script> if people care
701 bytes, text/html
Details
Pre-address some more comments
16.42 KB, patch
sicking
: review+
jst
: superreview+
Details | Diff | Splinter Review 
Setting the innerHTML property of an HTMLStyleELement to 'foo' results in
innerHTML containing '<style></style>foo'
Attached file testcase — 

    
  
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.

    
    

    
  
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?

    
    

    
  

      
      
      
      

        Attached 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+

    
    

    
  

      
      
      
      

        Attached 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)

    
    

    
  

      
      
      
      

        Attached 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
Closed: 22 years ago
Resolution: --- → FIXED

    
    

    
  
Also per discussion filed bug 155723

    
  
QA Contact: lchiang → ian

    
    

    
  
v per testcase
Status: RESOLVED → VERIFIED
Component: DOM: Mozilla Extensions → DOM
Component: DOM → DOM: Core & HTML

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: