Last Comment Bug 339350 - js: innerHTML removes 2 last symbols "]]"
: js: innerHTML removes 2 last symbols "]]"
Status: RESOLVED FIXED
: fixed1.8.1.1
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: mozilla1.9alpha1
Assigned To: Robert Sayre
:
Mentors:
http://intertwingly.net/blog/2006/10/...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-05-26 11:17 PDT by Constantin
Modified: 2008-07-31 02:44 PDT (History)
8 users (show)
sayrer: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
The code to reproduce the bug. (562 bytes, text/html)
2006-06-03 05:00 PDT, yspam88-bugzillamozilla
no flags Details
The code to reproduce the bug. (521 bytes, application/xhtml+xml)
2006-06-03 05:33 PDT, Constantin
no flags Details
another test case (969 bytes, application/xhtml+xml)
2006-10-03 09:03 PDT, Robert Sayre
no flags Details
add a piece of endtag to the fragment (1.73 KB, patch)
2006-10-03 14:55 PDT, Robert Sayre
no flags Details | Diff | Review
if there's a tagStack, add the first bit of end tag to the source buffer (97.30 KB, patch)
2006-10-03 16:34 PDT, Robert Sayre
no flags Details | Diff | Review
if there's a tagStack, add the first bit of end tag to the source buffer (3.16 KB, patch)
2006-10-03 16:35 PDT, Robert Sayre
mrbkap: review+
Details | Diff | Review
fix mrbkap's nits (3.08 KB, patch)
2006-10-05 10:01 PDT, Robert Sayre
jonas: superreview+
dveditz: approval1.8.1.1+
Details | Diff | Review
move the unit test into parser/htmlparser/ (6.06 KB, patch)
2007-02-19 11:36 PST, Robert Sayre
mrbkap: review+
Details | Diff | Review

Description Constantin 2006-05-26 11:17:18 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ru; rv:1.8.0.3) Gecko/20060426 Firefox/1.5.0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; ru; rv:1.8.0.3) Gecko/20060426 Firefox/1.5.0.3

html-code:

<div id="someBlock" onclick="javascript:updateMe();">someText</div>

js-code:

function updateMe(){
    someBlock = document.getElementById("someBlock");
    someBlock.innerHTML = "[[someNewText]]";
}

result:

<div id="someBlock" onclick="javascript:updateMe();">[[someNewText</div> 

-- without "]]"

Reproducible: Always

Steps to Reproduce:
1.Create a page sush as in Details
2.Run in Firefox created page
3.Click on div "someText"
4.See result

Actual Results:  
[[someNewText

Expected Results:  
[[someNewText]]

May be that the problem is in the CDATA process
Comment 1 Zook Valem 2006-05-26 14:34:00 PDT
A familiar pattern to the resolved fixed bug on the different javascript's object (function) somewhere but don't remember what it was.  That was almost a year ago to my knowledge.
Comment 2 yspam88-bugzillamozilla 2006-06-03 04:56:25 PDT
I think this is fixed now...
Comment 3 yspam88-bugzillamozilla 2006-06-03 05:00:05 PDT
Created attachment 224303 [details]
The code to reproduce the bug.
Comment 4 Constantin 2006-06-03 05:33:03 PDT
Created attachment 224304 [details]
The code to reproduce the bug.

I changed resolution and document type to xhtml
Comment 5 Robert Sayre 2006-10-03 09:03:46 PDT
Created attachment 241071 [details]
another test case

from: http://intertwingly.net/blog/2006/10/03/Firefox-XHTML-innerHTML-quirk

    * Displays properly in Opera 9.02 when served as application/xhtml+xml

    * Displays properly in IE when served as text/html

    * Displays properly in Firefox 1.5.0.7 when served as text/html

    * Omits trailing closing bracket in innerHTML on Firefox 1.5.0.7 when served as application/xhtml+xml

Adding more brackets to the tail end of the string results in the last two being removed.  Adding a space at the end makes the brackets show up.
Comment 6 Robert Sayre 2006-10-03 09:28:02 PDT
Looks like we are buggy in nsXMLContentSink::HandleCharacterData or nsXMLContentSink::AddText. expat sends two character data events:

"foo [bar]"
 --------   <-- aLength is 8

"]</div></td></tr></table></body>..."
 -          <-- aLength is 1
Comment 7 Robert Sayre 2006-10-03 12:54:42 PDT
ok, so the content stack is something like

>>>> tr->td->div->#document-fragment


the first character chunk is getting added to 

>>>> tr->td->div->#document-fragment

second character chunk is getting added to 

>>>> tr->td->div

and we return the nsIDOMDocumentFragment to InnerHTML, which doesn't contain the second chunk.
Comment 8 Robert Sayre 2006-10-03 13:40:27 PDT
Looks like we're calling DidBuildContent with the closing ']' still available in nsParser::ParseFragment.
Comment 9 Robert Sayre 2006-10-03 14:55:59 PDT
Created attachment 241112 [details] [diff] [review]
add a piece of endtag to the fragment

I'm not proud of this, but it does fix the bug. Not sure we should mess with the tokenizer.
Comment 10 Robert Sayre 2006-10-03 14:58:19 PDT
Comment on attachment 241112 [details] [diff] [review]
add a piece of endtag to the fragment

hmm, that is going to break XML with no context stack. hrmph.
Comment 11 Robert Sayre 2006-10-03 16:34:11 PDT
Created attachment 241127 [details] [diff] [review]
if there's a tagStack, add the first bit of end tag to the source buffer

Looks like mrbkap's patch for bug 263053 (remove <endnote>) regressed this. Where can I add a unit test?
Comment 12 Robert Sayre 2006-10-03 16:35:52 PDT
Created attachment 241129 [details] [diff] [review]
if there's a tagStack, add the first bit of end tag to the source buffer
Comment 13 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-10-03 17:32:26 PDT
Comment on attachment 241129 [details] [diff] [review]
if there's a tagStack, add the first bit of end tag to the source buffer

>+  } else {
>+

Nit: Remove this empty line.

>+        if (theIndex > 0)
>+          endContext.AppendLiteral("</");

Nit: Add braces to this if statement and the if/else below.

>+        nsAutoString thisTag( (PRUnichar*)aTagStack.ElementAt(theIndex) );
>+        PRInt32 endOfTag = thisTag.FindChar(PRUnichar(' '));  // was there an xmlns=?

Nits: Since you're here, could you remove those odd spaces before and after the parens? Also, wrap these lines at 80 chars.

Looks good otherwise. r=mrbkap
Comment 14 Robert Sayre 2006-10-05 10:01:11 PDT
Created attachment 241343 [details] [diff] [review]
fix mrbkap's nits
Comment 15 Jonas Sicking (:sicking) PTO Until July 5th 2006-10-10 13:39:14 PDT
Comment on attachment 241343 [details] [diff] [review]
fix mrbkap's nits

>+        if (endOfTag == -1) {
>+          endContext.Append(thisTag);
>+        } else {
>+          endContext.Append(Substring(thisTag,0,endOfTag));

You can use StringHead(thisTag, endOfTag)

>+      result = Parse(endContext, (void*)&theContext, aMimeType,
>+                     PR_FALSE, PR_TRUE, aMode);

there should be no need for the cast to (void*)

sr=sicking with that
Comment 16 Robert Sayre 2006-10-10 14:30:46 PDT
Checking in nsParser.cpp;
/cvsroot/mozilla/parser/htmlparser/src/nsParser.cpp,v  <--  nsParser.cpp
new revision: 3.394; previous revision: 3.393
done
Comment 17 Robert Sayre 2006-10-21 14:11:21 PDT
(In reply to comment #11)
>
> Where can I add a unit test?

Nowhere, so I set up my own server :)

http://people.mozilla.com/~sayrer/Mozilla_DHTML/tests/

 
bug 357523
Comment 18 Daniel Veditz [:dveditz] 2006-11-29 11:50:30 PST
Comment on attachment 241343 [details] [diff] [review]
fix mrbkap's nits

approved for 1.8 branch, a=dveditz for drivers
Comment 19 Robert Sayre 2006-11-30 09:53:56 PST
Checking in nsParser.cpp;
/cvsroot/mozilla/parser/htmlparser/src/nsParser.cpp,v  <--  nsParser.cpp
new revision: 3.370.4.6; previous revision: 3.370.4.5
done
Comment 20 Robert Sayre 2006-12-05 17:33:00 PST
Test case here:

http://intertwingly.net/blog/2006/10/03/Firefox-XHTML-innerHTML-quirk
Comment 21 Robert Sayre 2006-12-27 12:02:16 PST
this is in mochitest now, so marking in-testsuite+
Comment 22 Robert Sayre 2007-02-19 11:36:20 PST
Created attachment 255709 [details] [diff] [review]
move the unit test into parser/htmlparser/
Comment 23 Blake Kaplan (:mrbkap) (please use needinfo!) 2007-02-19 11:40:11 PST
Comment on attachment 255709 [details] [diff] [review]
move the unit test into parser/htmlparser/

Neat!

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