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
:
: Andrew Overholt [:overholt]
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 | Splinter 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 | Splinter 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 | Splinter 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 | Splinter Review
move the unit test into parser/htmlparser/ (6.06 KB, patch)
2007-02-19 11:36 PST, Robert Sayre
mrbkap: review+
Details | Diff | Splinter Review

Description User image 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 User image 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 User image yspam88-bugzillamozilla 2006-06-03 04:56:25 PDT
I think this is fixed now...
Comment 3 User image yspam88-bugzillamozilla 2006-06-03 05:00:05 PDT
Created attachment 224303 [details]
The code to reproduce the bug.
Comment 4 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image Blake Kaplan (:mrbkap) 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 User image Robert Sayre 2006-10-05 10:01:11 PDT
Created attachment 241343 [details] [diff] [review]
fix mrbkap's nits
Comment 15 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image Robert Sayre 2006-12-27 12:02:16 PST
this is in mochitest now, so marking in-testsuite+
Comment 22 User image Robert Sayre 2007-02-19 11:36:20 PST
Created attachment 255709 [details] [diff] [review]
move the unit test into parser/htmlparser/
Comment 23 User image Blake Kaplan (:mrbkap) 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.