133827 - createContextualFragment only works for HTML

Status: RESOLVED FIXED

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

Description

[Patty Gilbert]

I'm using createContextualFragment to create an HTML fragment and
appendChild to insert that fragment into a <description> tag.  Before
I append the new child to the <description> tag, I remove any existing
children.  This process works fine the first time it is called, but if
I attempt to delete and replace the contents of the tag a second time,
I get the following asserts and the text does not get either deleted
or replaced:

###!!! ASSERTION: Content has no document.: 'doc', file
V:\mozilla\layout\html\base\src\nsTextFrame.
cpp, line 5078
WARNING: Reflow of frame failed in nsLineLayout, file
V:\mozilla\layout\html\base\src\nsLineLayout.c
pp, line 1088
WARNING: Reflow of frame failed in nsLineLayout, file
V:\mozilla\layout\html\base\src\nsLineLayout.c
pp, line 1088
###!!! ASSERTION: Content has no document.: 'doc', file
V:\mozilla\layout\html\base\src\nsTextFrame.
cpp, line 5078
WARNING: Reflow of frame failed in nsLineLayout, file
V:\mozilla\layout\html\base\src\nsLineLayout.c
pp, line 1088
WARNING: Reflow of frame failed in nsLineLayout, file
V:\mozilla\layout\html\base\src\nsLineLayout.c
pp, line 1088
###!!! ASSERTION: reflow dirty lines failed: 'NS_SUCCEEDED(rv)', file
V:\mozilla\layout\html\base\sr
c\nsBlockFrame.cpp, line 825

I've attached a simplified example that produces the error.  The first time
this page appears, the old text has been deleted and the new text
appears.  No asserts.  But, if I click on the button, causing the
contents of the <description> tag to be removed and reinserted, the
asserts occur and the old text remains.  

I'm using the 0.9.7 Mozilla release, but will be upgrading to a newer version 
soon.

Comment 1

[Patty Gilbert]

Attachment: Load this XUL file and click on the button.

Comment 2

[rbs]

testcase is crashing in a latest build from the tip. Stack trace below. See also 
bug 130023.
===

XML_UnblockParser(void * 0x00000000) line 753 + 3 bytes
nsExpatDriver::ConsumeToken(nsExpatDriver * const 0x03a21744, nsScanner & {...}, 
int & 0) line 762 + 12 bytes
nsParser::Tokenize(int 0) line 2507 + 26 bytes
nsParser::ResumeParse(int 0, int 0, int 0) line 1730 + 12 bytes
nsParser::Parse(const nsAString & {...}, void * 0x0012d1c8, const nsACString & 
{...}, int 0, int 1, nsDTDMode eDTDMode_autodetect) line 1620 + 17 bytes
nsParser::ParseFragment(const nsAString & {...}, void * 0x00000000, nsVoidArray 
& {...}, unsigned int 0, const nsACString & {...}, nsDTDMode 
eDTDMode_autodetect) line 1667 + 63 bytes
nsRange::CreateContextualFragment(nsRange * const 0x03a177ec, const nsAString & 
{...}, nsIDOMDocumentFragment * * 0x0012d6f0) line 2817 + 57 bytes
XPTC_InvokeByIndex(nsISupports * 0x03a177ec, unsigned int 3, unsigned int 2, 
nsXPTCVariant * 0x0012d6e0) line 106
XPCWrappedNative::CallMethod(XPCCallContext & {...}, XPCWrappedNative::CallMode 
CALL_METHOD) line 2025 + 42 bytes
XPC_WN_CallMethod(JSContext * 0x037d6170, JSObject * 0x038d69c8, unsigned int 1, 
long * 0x038055cc, long * 0x0012d9bc) line 1266 + 14 bytes
js_Invoke(JSContext * 0x037d6170, unsigned int 1, unsigned int 0) line 788 + 23 
bytes
js_Interpret(JSContext * 0x037d6170, long * 0x0012e7fc) line 2745 + 15 bytes
js_Invoke(JSContext * 0x037d6170, unsigned int 1, unsigned int 2) line 805 + 13 
bytes
js_InternalInvoke(JSContext * 0x037d6170, JSObject * 0x02f60da0, long 52058960, 
unsigned int 0, unsigned int 1, long * 0x0012ea54, long * 0x0012e924) line 880 + 
20 bytes
JS_CallFunctionValue(JSContext * 0x037d6170, JSObject * 0x02f60da0, long 
52058960, unsigned int 1, long * 0x0012ea54, long * 0x0012e924) line 3412 + 31 
bytes
nsJSContext::CallEventHandler(nsJSContext * const 0x037d5fd8, void * 0x02f60da0, 
void * 0x031a5b50, unsigned int 1, void * 0x0012ea54, int * 0x0012ea58, int 0) 
line 1016 + 33 bytes
nsJSEventListener::HandleEvent(nsJSEventListener * const 0x032c6740, nsIDOMEvent 
* 0x037c4218) line 182 + 77 bytes
nsEventListenerManager::HandleEventSubType(nsListenerStruct * 0x03bc3528, 
nsIDOMEvent * 0x037c4218, nsIDOMEventTarget * 0x037d5ed0, unsigned int 1, 
unsigned int 7) line 1219 + 20 bytes
nsEventListenerManager::HandleEvent(nsEventListenerManager * const 0x0390aad0, 
nsIPresContext * 0x03b2b1f8, nsEvent * 0x0012f254, nsIDOMEvent * * 0x0012f1b0, 
nsIDOMEventTarget * 0x037d5ed0, unsigned int 7, nsEventStatus * 0x0012f27c) line 
1892 + 36 bytes
GlobalWindowImpl::HandleDOMEvent(GlobalWindowImpl * const 0x037d5ec0, 
nsIPresContext * 0x03b2b1f8, nsEvent * 0x0012f254, nsIDOMEvent * * 0x0012f1b0, 
unsigned int 1, nsEventStatus * 0x0012f27c) line 742
DocumentViewerImpl::LoadComplete(DocumentViewerImpl * const 0x03ab4750, unsigned 
int 0) line 1508 + 56 bytes
nsDocShell::EndPageLoad(nsIWebProgress * 0x037ec8c4, nsIChannel * 0x038ea3c0, 
unsigned int 0) line 3958
nsWebShell::EndPageLoad(nsIWebProgress * 0x037ec8c4, nsIChannel * 0x038ea3c0, 
unsigned int 0) line 731
nsDocShell::OnStateChange(nsDocShell * const 0x037d5a54, nsIWebProgress * 
0x037ec8c4, nsIRequest * 0x038ea3c0, int 131088, unsigned int 0) line 3875
nsDocLoaderImpl::FireOnStateChange(nsIWebProgress * 0x037ec8c4, nsIRequest * 
0x038ea3c0, int 131088, unsigned int 0) line 1105
nsDocLoaderImpl::doStopDocumentLoad(nsIRequest * 0x038ea3c0, unsigned int 0) 
line 744
nsDocLoaderImpl::DocLoaderIsEmpty() line 642
nsDocLoaderImpl::OnStopRequest(nsDocLoaderImpl * const 0x037ec8b4, nsIRequest * 
0x038ea3c0, nsISupports * 0x00000000, unsigned int 0) line 573
nsLoadGroup::RemoveRequest(nsLoadGroup * const 0x037ec988, nsIRequest * 
0x038ea3c0, nsISupports * 0x00000000, unsigned int 0) line 526 + 35 bytes
nsHttpChannel::OnStopRequest(nsHttpChannel * const 0x038ea3c4, nsIRequest * 
0x038c3e54, nsISupports * 0x00000000, unsigned int 0) line 2904
nsOnStopRequestEvent::HandleEvent() line 213
nsARequestObserverEvent::HandlePLEvent(PLEvent * 0x03bf21e4) line 116
PL_HandleEvent(PLEvent * 0x03bf21e4) line 596 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x002f9248) line 526 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x018a0bce, unsigned int 49488, unsigned int 0, 
long 3117640) line 1077 + 9 bytes
USER32! 77e148dc()
USER32! 77e14aa7()
USER32! 77e266fd()
nsAppShellService::Run(nsAppShellService * const 0x01740978) line 309
main1(int 1, char * * 0x00304e70, nsISupports * 0x00000000) line 1414 + 32 bytes
main(int 1, char * * 0x00304e70) line 1762 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77e992a6()

Comment 3

[anthonyd]

since I don't own range anymroe (I think kin does, but I may be wrong)
reassigning to core owner.

Comment 4

[anthonyd]

another one for the kin man.

dont hate me.

anthonyd

Comment 5

[Boris Zbarsky [:bzbarsky]]

OK.  This simply does not work now (crashes on the branch, throws an exception
on the trunk).  The reason is very simple:

CreateContextualFragment uses an nsIHTMLFragmentContentSink implementation
(nsHTMLFragmentContentSink) as the sink.  This sink does not implement
nsIExpatSink.  The document in question has an XML type, so it's passed to the
nsExpatDriver DTD to parse.  This DTD returns an error from WillBuildModel,
since the sink is not usable (mSink is null).

Now WillBuildModel is called by nsParser::ResumeParse.  On the trunk, the return
value is checked and an error is returned.  Hence the exception.  On the branch,
the return value is ignored and the parser goes on to tokenize the input stream
while the DTD has a null sink.  This crashes, probably because the mExpatParser
being passed around is null (WillBuildModel never got around to creating it).

I'll attach a patch to bug 147225 to fix the crash....

Comment 6

[Boris Zbarsky [:bzbarsky]]

So it seems like making this work will take two steps:

1)  Creating a fragment sink that implements nsIExpatSink (the
    nsHTMLFragmentSink could be coerced into doing this, perhaps, but that
    seems like a bad idea).
2)  Making sure that createContextualFragment uses the right sink based on the
    content type.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I might as well take this; we'll need something like this for Load/Save anyway.
 kin, feel free to take back if you actually plan to work on this.

Comment 8

[Boris Zbarsky [:bzbarsky]]

*** Bug 158307 has been marked as a duplicate of this bug. ***

Comment 9

[Boris Zbarsky [:bzbarsky]]

*** Bug 190665 has been marked as a duplicate of this bug. ***

Comment 10

[Boris Zbarsky [:bzbarsky]]

To default owner; I don't have time to work on this in the near future...

Comment 11

[Boris Zbarsky [:bzbarsky]]

*** Bug 208953 has been marked as a duplicate of this bug. ***

Comment 12

[Andrew Hagen]

This is creating a problem with the popular blogging tool 'Nucleus'. 

http://www.nucleuscms.org/documentation/index.html#known

Comment 13

[Boris Zbarsky [:bzbarsky]]

Any fix for this would be far too involved to go into anything but an alpha
milestone....  The tool you cite uses .innerHTML in XHTML content, which is just
asking for trouble.

Comment 14

[steve.swanson]

While working on MathML in Composer (bug 78128), I built a 
nsXMLFragmentContentSink. It's certainly not perfect, but it works in simple 
testcases in my Composer.

I recently noticed, however, that switching from the source tab in Composer 
uses CreateContextualFragment and that fails in my MathML-enabled Composer.  I 
will investigate (soon), and may post a patch.  I expect the heavy lifting 
should be done by nsXMLFragmentContentSink and only minimal plumbing changes 
are needed in CreateContextualFragment.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Steve, the only change that should be needed in CreateContextualFragment is to
change the following code

2456     nsCOMPtr<nsIHTMLFragmentContentSink> sink;
2457 
2458     result = NS_NewHTMLFragmentContentSink(getter_AddRefs(sink));
2459     if (NS_SUCCEEDED(result)) {
2460       sink->SetTargetDocument(document);
2461       parser->SetContentSink(sink);
2462       nsCOMPtr<nsIDOMNSDocument> domnsDocument(do_QueryInterface(document));
2463       if (domnsDocument) {
2464         nsAutoString buf;
2465         domnsDocument->GetContentType(buf);
2466         CopyUCS2toASCII(buf, contentType);
2467       }
2468       else {
2469         // Who're we kidding. This only works for html.
2470         contentType = NS_LITERAL_CSTRING("text/html");
2471       }

to use nsIFragmentContentSink (which will have the GetFragment method) and to
create the sink using a function that depends on the content type (basically,
use the XML one for anything that's not HTML).  So you're right -- the heavy
lifting is in the sink.

Comment 16

[steve.swanson]

I have this working.  I want to spend a bit more time cleaning up and testing 
before I submit the patch.

Actually, I had it working several days ago.  But then I realized my tree had 
gone stale and updating my tree broke my nsXMLFragmentContentSink.

Comment 17

[steve.swanson]

Attachment: testcase using MathML

Click anywhere and a MathML formula should be appended to the second <p>.

Comment 18

[steve.swanson]

Attachment: rough patch

There is no such thing as a nsIFragmentContentSink, so the change to nsRange is
not as pretty as it could be.

There are two new files and I don't know how to add these to patches, so just
stuck them at the top of the diff.

The bulk of the patch is in nsXMLFragmentContentSink.cpp.  Much of this file is
directly copied from nsXMLContentSink.cpp, so it would be good to refactor this
eventually.  It would also be nice to combine functionality with the HTML
versions of these files.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> There is no such thing as a nsIFragmentContentSink

Why not create it?  I see no reason why the nsIHTMLFragmentContentSink methods
are HTML-specific... you'll need to change the inheritance to inherit from
nsISupports and change some code to QI to nsIContentSink as needed, but....

Comment 20

[steve.swanson]

I'm just chicken.  But with your encouragement, I'll give it a go.

Comment 21

[Heikki Toivonen (remove -bugzilla when emailing directly)]

To make new files appear in a diff, you need to cvws add them first (but not
commit!). Then, add -N option to the cvs diff and it will include new files.

There may be some difficulties with doing cvs add if you don't have cvs write
access, but give it a try. In that case it might be possible to fool CVS by
manually changing CVS/Entries file in the correct directory, but you'd need to
read some CVS docs on what the values mean.

Comment 22

[steve.swanson]

Attachment: slightly better patch

OK, that wasn't so bad. (I wasn't able to fool CVS about files not in the
repository.)

nsIFragmentContentSink.h replaces nsIHTMLFragmentContentSink.h.  This interface
is factored out, which makes the code in nsRange::CreateContextualFragment()
much cleaner.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Steve, is the code similar enough to nsXMLContentSink that inheriting from it
would make sense?  Or would you just have to override all the methods?

Comment 24

[steve.swanson]

Yes, inheritance would be a good idea here.  Most of the work is done by 
methods in nsXMLContentSink.  

The main differences are in the handling of scripts, character sets, XML 
declarations, etc.  I think none of these make sense for fragments, and 
overriding the relevant methods on nsXMLContentSink will be easy. 
nsXMLContentSink derives from nsITransformObserver -- I assume this won't be a 
problem.

I'm taking this as a hint to try to make the inherit work.  Will do.

I'm not taking this as a hint to do the same work with 
nsHTMLContentSink/nsHTMLFragmentContentSink.  Yet.

Comment 25

[Boris Zbarsky [:bzbarsky]]

> I assume this won't be a problem.

Shouldn't be if you disable XSLT stylesheet loading in your subclass (which you
should).

> I'm taking this as a hint to try to make the inherit work.  Will do.

If you can, that would be great -- less code and easier to review if nothing else.

> I'm not taking this as a hint to do the same work with
> nsHTMLContentSink/nsHTMLFragmentContentSink.

Wise man.  ;)  That would be _very_ hard.

Comment 26

[steve.swanson]

Attachment: much better patch

The new nsXMLFragmentContentSink looks great.  Less filling.

Pursuing the idea of minimizing code duplication, I had to add a few functions
to nsXMLContentSink (so I could override them).

The XUL testcase no longer loads on my system -- the debug window says
"waaah!".   I don't think this is because of anything I've done.  The MathML
testcase works fine.

Comment 27

[Boris Zbarsky [:bzbarsky]]

> NS_IMPL_ISUPPORTS_INHERITED5(nsXMLFragmentContentSink,
>                             nsContentSink,

Should that second line not be nsXMLContentSink?  And you should really only
have to impl QI to nsIFragmentContentSink -- the others will be handled by
nsXMLContentSink's QI method.

Why can't fragments have processing instructions?  Those are allowed anywhere in
a document, iirc.

I skimmed the code, and I'm not seeing anything that should break XUL... which
exact "waaah" are you triggering?  Is there a line/file?

Comment 28

[steve.swanson]

I didn't understand NS_IMPL_ISUPPORTS_INHERITED(), so just hacked on the one 
in nsXMLContentSink until it worked.  It now reads
NS_IMPL_ISUPPORTS_INHERITED1(nsXMLFragmentContentSink,
                             nsXMLContentSink,
                             nsIFragmentContentSink)
and works fine.

Maybe processing instructions should be allowed.  But I found the stylesheet 
handling in nsXMLContentSink::HandleProcessingInstruction to be scary.  Maybe 
the right thing to do is put the pi into the fragment, but not do any loading.

The XUL error is in nsXULPDGlobalObject::GetDocShell():
WARNING: waaah!, file 
c:/Mozilla/mozilla/content/xul/document/src/nsXULPrototypeDocument.cpp, line 
898

Comment 29

[Boris Zbarsky [:bzbarsky]]

> Maybe the right thing to do is put the pi into the fragment, but not do any
> loading.

Exactly.  If someone ever inserts them into a document they'll do the loading
thing on their own as needed.

The docshell thing probably shouldn't be affecting this.... anyway, worth fixing
this and then debugging the XUL issue.

I'd probably ask for r=peterv sr=jst once you're happy with the patch.

Comment 30

[Heikki Toivonen (remove -bugzilla when emailing directly)]

xml-stylesheet processing instructions should cause stylesheet loads only when
they appear in the prolog of an XML document (i.e., before the root element). We
do the right thing on normal document loads. (Then there is the XHTML link
element which we let load stylesheets anywhere in the document.)

The xml-stylesheet spec is here: http://www.w3.org/TR/xml-stylesheet/

Comment 31

[steve.swanson]

Attachment: a bit cleaner patch

Fixed the NS_IMPL_ISUPPORTS_INHERITED.

Changed processing instruction handling.  I don't have a testcase to cover
this.  In fact, it would be a good idea to build another testcase to exercise
all the datapaths.

(Fragments are always contained in the root, so xml-stylesheets should never be
active.  This is already handled in nsXMLContentSink.)

Included forgotten diff from nsHTMLDataTransfer.cpp.

The XUL error happens out in the trunk as well.  I will look into that, but I'm
currently convinced the problem has nothing to do with this patch.

The only other issue is the comment at the bottom of
nsXMLFragmentContentSink.cpp.  In particular, I don't understand how an
"autoloading link" should behave in a fragment.  (Maybe mDocShell never gets
set for the fragment, so the question is moot.)


I'm off to the Mozilla developer day, so will probably be off the air until
next week.

Comment 32

[Boris Zbarsky [:bzbarsky]]

> In particular, I don't understand how an "autoloading link" should behave in a
> fragment.

It should do absolutely nothing, in my opinion.

Comment 33

[steve.swanson]

mDocShell is only set in Init(), and I don't think Init() is ever called for 
fragments.  I could override Init() just to prevent any misunderstanding.

Comment 34

[steve.swanson]

Attachment: more extensive XHTML testcase

The various buttons cover the various branches in the code.  The fragments are
added to the document, though many are not visible.

Comment 35

[steve.swanson]

Attachment: improved patch covers more test cases

This new patch has various small improvements.

1. Previous version (usually) lost text which was not contained in an element.

2. Previous version lost <script> tags.

3. Attempting to insert an XML declaration or a DOCTYPE would destroy your
document.

Comment 36

[steve.swanson]

The XUL testcase is working again, after I refreshed my tree.  (It still 
goes "Waaah!" on loading.)

Comment 37

[steve.swanson]

Is there any interest in resolving this bug?  I can rebuild the patch, but 
maybe there are other issues I'm unaware of which are blocking the changes.

Comment 38

[Boris Zbarsky [:bzbarsky]]

Steve, if you can make this work, I believe there is absolutely interest in
resolving it....  Once you have the patch in a state you consider acceptable,
you should request reviews from people familiar with this code (jst, peterv,
maybe myself).

Comment 39

[steve.swanson]

Attachment: updated patch to reflect parser reorg

Comment 40

[Boris Zbarsky [:bzbarsky]]

Note also that nsIContentSink is likely to change a bit in the near future (bug
144072).  Should be a one-line change to that patch....

Comment 41

[steve.swanson]

Attachment: same patch with wider diff context

(I don't yet see the connection with bug 144072.  Will look into that some
more.)

Comment 42

[Boris Zbarsky [:bzbarsky]]

Oh, nevermind.  Since you're inheriting from nsXMLContentSink, you're not
implementing the method that's getting changed....

Comment 43

[steve.swanson]

Attachment: Fixed patch for recent deCOM changes

Comment 44

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 151893 [details] [diff] [review]
Fixed patch for recent deCOM changes

>? content/xml/document/src/nsXMLFragmentContentSink.cpp

> * The Original Code is Mozilla Communicator client code.

no, the original code is Mozilla.org code or something...

> * The Initial Developer of the Original Code is
> * Netscape Communications Corporation.

No, that would be Steve Swanson

> * Portions created by the Initial Developer are Copyright (C) 1998

2004, not 1998.

>class nsXMLFragmentContentSink : public nsXMLContentSink,

>  // nsIExpatSink
>  NS_IMETHOD HandleStartElement(const PRUnichar *aName,
>                                const PRUnichar **aAtts,
>                                PRUint32 aAttsCount,
>                                PRInt32 aIndex,
>                                PRUint32 aLineNumber);
>  NS_IMETHOD HandleEndElement(const PRUnichar *aName);
>  NS_IMETHOD HandleDoctypeDecl(const nsAString & aSubset, 
>                               const nsAString & aName, 
>                               const nsAString & aSystemId, 
>                               const nsAString & aPublicId,
>                               nsISupports* aCatalogData);
>  NS_IMETHOD HandleProcessingInstruction(const PRUnichar *aTarget, 
>                                         const PRUnichar *aData);
>  NS_IMETHOD HandleXMLDeclaration(const PRUnichar *aData, 
>                                  PRUint32 aLength);
>  NS_IMETHOD ReportError(const PRUnichar* aErrorText, 
>                         const PRUnichar* aSourceText);
>
>  // nsIContentSink
>  NS_IMETHOD WillBuildModel(void);
>  NS_IMETHOD DidBuildModel();
>  NS_IMETHOD SetDocumentCharset(nsACString& aCharset);
>
>  // nsIXMLContentSink
>
>  // nsIFragmentContentSink
>  NS_IMETHOD GetFragment(nsIDOMDocumentFragment** aFragment);
>  NS_IMETHOD SetTargetDocument(nsIDocument* aDocument);
>
>protected:
>  virtual PRBool OnOpenContainer(nsIAtom* aTagName);
>  virtual PRBool SetDocElement(PRInt32 aNameSpaceID, 
>                               nsIAtom *aTagName,
>                               nsIContent *aContent);
>  virtual nsresult CloseElement(nsIContent* aContent, PRBool* aAppendContent);
>  virtual nsIContent* GetCurrentContent();
>
>  // nsContentSink overrides
>  virtual nsresult ProcessStyleLink(nsIContent* aElement,
>                                    const nsAString& aHref,
>                                    PRBool aAlternate,
>                                    const nsAString& aTitle,
>                                    const nsAString& aType,
>                                    const nsAString& aMedia);
>  nsresult LoadXSLStyleSheet(nsIURI* aUrl);
>  void StartLayout();
>
>  PRBool mHitSentinel;

>  nsIContent* mRoot;

Is there a reason that's not an nsCOMPtr<nsIContent> ?

>class nsXMLFragmentContentSink2 : public nsXMLFragmentContentSink 

This seems pretty unnecessary (I know the HTML fragment sink uses this setup,
and it's unnecessary there too).  I'd say make the nsXMLFragmentContentSink
constructor take a boolean that says whether mSentinel is true or false.  Then
call the constructor appropriately from the NS_NewXMLFragmentContentSink
functions (which could then share all their code; just call a function that
actually constructs and pass it true or false).

>nsresult
>NS_NewXMLFragmentContentSink2(nsIFragmentContentSink** aResult)

>  nsCOMPtr<nsIFragmentContentSink> kungFuDeathGrip = it;
>  
>  return CallQueryInterface(it, aResult);

You can replace those two lines with:

  NS_ADDREF(*aResult = it);
  return NS_OK;

>NS_NewXMLFragmentContentSink(nsIFragmentContentSink** aResult)

Same (and again, these two functions' bodies should just be combined into a
single function).

>nsXMLFragmentContentSink::nsXMLFragmentContentSink()
>{
>  mHitSentinel = PR_FALSE;
>  mRoot = nsnull;
>}

Please use C++ initializer syntax to initialize members:

  nsXMLFragmentContentSink::nsXMLFragmentContentSink()
   : mHitSentinel(PR_FALSE),
     mRoot(nsnull)
  {}

>nsXMLFragmentContentSink::OnOpenContainer(nsIAtom* aTagName)

>  if (aTagName->Equals(NS_LITERAL_STRING("endnote"))) {    // see 

This would be better as:

  aTagName->EqualsUTF8(NS_LITERAL_CSTRING("endnote"))) {

and better yet as an atom comparison (toss "endnote" in nsHTMLAtomList or
something, for example...

>  return PR_FALSE;

Is this really desirable?  That will pop the namespace decls off the stack; it
makes it look like that element never existed.

If that's what we're doing, what's "contextual" about this fragment?

This part needs a bit more thought, in my opinion.

>nsXMLFragmentContentSink::CloseElement(nsIContent* aContent, PRBool* aAppendContent)

>  if (tagAtom == nsHTMLAtoms::script) {
>    rv = ProcessEndSCRIPTTag(aContent);

Is _that_ desirable?  This will try to block the parser as it loads the
script... (and do we want to load the script?)

Or are we disabling script loading somewhere?

For that matter, are we disabling stylesheet loading somewhere?

Perhaps we shouldn't be setting mDocument and should store the document
elsewhere?  (We _do_ want the right nodeinfo manager, though).

>nsXMLFragmentContentSink::HandleStartElement(const PRUnichar *aName,

>  if (!OnOpenContainer(tagAtom))
>    return NS_OK;
>  else
>    return nsXMLContentSink::HandleStartElement(aName, aAtts, aAttsCount, aIndex, aLineNumber);

Won't this call SetDocument() on the element with our target document, making
it think it's actually in the DOM?  That seems wrong....

>nsXMLFragmentContentSink::HandleProcessingInstruction(const 

>  if (NS_OK == result) {

NS_SUCCEEDED(result)

>? parser/htmlparser/public/nsIFragmentContentSink.h

Here you do want to leave the old license, since this is just copied from the
old file.

>// used to be #define NS_IHTML_FRAGMENT_CONTENT_SINK_IID
>#define NS_I_FRAGMENT_CONTENT_SINK_IID \
> {0xa6cf9102, 0x15b3, 0x11d2,              \
> {0x93, 0x2e, 0x00, 0x80, 0x5f, 0x8a, 0xdd, 0x32}}

Please change the iid, since you changed the interface (eg no longer inherits
from nsIHTMLContentSink).

>#define NS_HTMLFRAGMENTSINK_CONTRACTID "@mozilla.org/layout/htmlfragmentsink;1"
>#define NS_HTMLFRAGMENTSINK2_CONTRACTID "@mozilla.org/layout/htmlfragmentsink;2"
>
>#define NS_XMLFRAGMENTSINK_CONTRACTID "@mozilla.org/layout/xmlfragmentsink;1"
>#define NS_XMLFRAGMENTSINK2_CONTRACTID "@mozilla.org/layout/xmlfragmentsink;2"

Document the differences between the various contractids?

>Index: content/base/src/nsRange.cpp
>+    nsCOMPtr<nsIDOMNSDocument> domnsDocument(do_QueryInterface(document));
>+    nsCOMPtr<nsIHTMLDocument> htmlDoc(do_QueryInterface(domDocument));
>+    if (domnsDocument) {
>+      nsAutoString buf;
>+      domnsDocument->GetContentType(buf);
>+      CopyUCS2toASCII(buf, contentType);
>+    }
>+    else {
>+      // choose a default content type
>+      if (htmlDoc) {

Any document we have will QI to nsIDOMNSDocument... so if that first if is
false, this one will be too.

>+    PRBool bXHTML = contentType.Equals(NS_LITERAL_CSTRING("application/xhtml+xml"));

You could use nsIDocument::IsCaseSensitive() instead... in case the HTML WG
makes up more content types that should get HTML documents (XHTML2?).

>Index: content/html/document/src/nsHTMLFragmentContentSink.cpp

Same comments about the two nested classes.

>Index: content/xml/document/src/nsXMLContentSink.cpp

>+    if (SetDocElement(nameSpaceID,tagAtom,content)) {
>+      ; // work was done
>     }

If all callers of SetDocElement ignore the return value, why does it not return
void?

>Index: content/xml/document/src/nsXMLContentSink.h

>+  virtual PRBool SetDocElement(PRInt32 aNameSpaceID, 
>+                               nsIAtom *aTagName,
>+                               nsIContent *aContent);

Document what this does?

>Index: parser/htmlparser/src/nsParser.cpp

It looks like this is not in fact saving the namespace decls of the parents in
the namespace case....	Should it be? What's the behavior we want?

r-, but this is looking like it's nearly there.  My apologies for taking so
long on the review; I was out of the country for a month and have been trying
to catch up to things since... Next review should be much quicker, I hope.

Comment 45

[steve.swanson]

Just some remarks on your comments.  I should have a new patch by the end of 
the day.

As for questions of style, most of the code was copied from 
nsHTMLFragmentContentSink or nsXMLContentSink.  If that worked, I just left it.


>>nsXMLFragmentContentSink::OnOpenContainer(nsIAtom* aTagName)
>>  return PR_FALSE;
> Is this really desirable?

I think this is the right thing to do here.  The parser will see the namespace 
decls come in, so can appropriately label the nodes we do use.  Or am I 
missing something here?  However, see comment below about stuff in nsParser.

I was always a little puzzled by the "contextual" aspect of the fragment.  My 
current understanding is that the content of the fragment is built in the 
context of the node in the DOM at the start of the Range.  I have yet to make 
a testcase where the context is relevant, but I can see how a <tr> "should" be 
created in a <table> and, hence, the function name createContextualFragment 
makes sense.

In the XML case, you also need DTD information (as part of the context) to: a) 
validate, b) resolve character entities.  I haven't looked into how hard it 
would be to move the documents DTD information to the parser used for the 
fragment...could be very hard.  (The HTML case is much simpler, since it 
doesn't validate and the DTD is fixed anyway.)


>>Index: parser/htmlparser/src/nsParser.cpp
>It looks like this is not in fact saving the namespace decls of the parents

Very true, and this needs to be fixed.  Again, a testcase would be nice and 
seems easy to conceive in the MathML case.  (The XUL case is possibly 
different, since it's my understanding that XUL elements only work inside of 
XUL documents.)


>>    return nsXMLContentSink::HandleStartElement(aName, aAtts, aAttsCount, 
>>aIndex, aLineNumber);
>Won't this call SetDocument() on the element with our target document, making
>it think it's actually in the DOM?  That seems wrong....

I think this is right.  The element will live in a fragment which is part of 
the DOM.


>>nsXMLFragmentContentSink::CloseElement(nsIContent* aContent, PRBool* 
>>aAppendContent)
>>  if (tagAtom == nsHTMLAtoms::script) {
>>    rv = ProcessEndSCRIPTTag(aContent);
>Is _that_ desirable?  This will try to block the parser as it loads ...

I used to think this is right.  The script should be loaded, since I don't see 
any other place where it would get loaded.

nsXMLFragmentContentSink::ProcessStyleLink() blocks stylesheet loading.  Is 
this a contradiction?

nsHTMLFragmentContentSink seems to just collect the text in the script/style 
tags without taking any other actions.  Maybe that's what we should do.


>>Index: content/xml/document/src/nsXMLContentSink.cpp
>>+    if (SetDocElement(nameSpaceID,tagAtom,content)) {
>>+      ; // work was done
>>     }
>If all callers of SetDocElement ignore the return value, why does it not 
>return void?

The return value is only ignored when it's true, something happens in the 
false case.


>>#define NS_I_FRAGMENT_CONTENT_SINK_IID \
>> {0xa6cf9102, 0x15b3, 0x11d2,              \
>> {0x93, 0x2e, 0x00, 0x80, 0x5f, 0x8a, 0xdd, 0x32}}
>Please change the iid

Good idea, since that GUID is also used in editor/idl/nsICiter.idl :-)

Comment 46

[steve.swanson]

Argh, you are right.  The namespace stack is maintained by the content sink, 
not the parser.  I need to fix the behavior surrounding OnOpenContainer() so 
we don't lose namespaces in the context.

Comment 47

[Boris Zbarsky [:bzbarsky]]

> As for questions of style, most of the code was copied from 
> nsHTMLFragmentContentSink or nsXMLContentSink.  If

I realize that.  The style comments still apply -- the code in
nsHTMLFragmentContentSink is really bad in places.

> The parser will see the namespace decls come in, so can appropriately label the
> nodes we do use.

No.  If you return false here those decls are popped off the namespace stack
immediately.  See
http://lxr.mozilla.org/seamonkey/source/content/xml/document/src/nsXMLContentSink.cpp#958

> My current understanding is that the content of the fragment is built in the 
> context of the node in the DOM at the start of the Range.

That's correct.  In some cases with HTML that matters...

> In the XML case, you also need DTD information (as part of the context) to: a) 
> validate, b) resolve character entities.

Mozilla does not use a validating parser, but yes, resolving character entities
could be an issue...

> I think this is right.  The element will live in a fragment which is part of 
> the DOM.

In Mozilla, having a non-null mDocument implies that walking the parent node
chain will end up in a Document node.  In this case, this is not the case, so
the nodes in question should not be ending up with a non-null mDocument.

> I used to think this is right.  The script should be loaded, since I don't see 
> any other place where it would get loaded.

How about nsHTMLScriptElement::SetDocument?

> nsXMLFragmentContentSink::ProcessStyleLink() blocks stylesheet loading. 

No, it asserts that it's never called (which is not true, as far as I can see).
 Also, inline style should not be getting parsed in fragments.

> nsHTMLFragmentContentSink seems to just collect the text in the script/style 
> tags without taking any other actions.  Maybe that's what we should do.

That's exactly what we should do.  All the nodes in question will load their
resources when finally inserted into a document.

> The return value is only ignored when it's true, something happens in the 
> false case.

Ah, true.  In that case I think it makes more sense to do:

if (!SetDocElement(nameSpaceID, tagAtom, content) && appendContent)  {
  // stuff
}

> Good idea, since that GUID is also used in editor/idl/nsICiter.idl :-)

Really?  Yikes.  Someone must have been doing too much copy/paste.  :(

Comment 48

[steve.swanson]

Attachment: revised patch

This new patch addresses most of the issues raised in the review and still runs
all the testcases.  I would like to construct a few more testcases.

If fixed the problem with losing namespaces off the stack by not removing them
(and then cleaning up in the destructor).  This is not such a great solution.
In particular, it causes nsXBLContentSink to leave namespaces on the stack when
it didn't do that before.  (I didn't realize nsXBLContentSink had its own
OnOpenContainer().)

The only other option I see is to load the whole string
<...context...><endnote><...fragment...></endnote></...context...> into the
fragment, then use DOM surgery to remove all the stuff containing the fragment.

Comment 49

[Boris Zbarsky [:bzbarsky]]

> If fixed the problem with losing namespaces off the stack by not removing them
> (and then cleaning up in the destructor).  This is not such a great solution.

You're right.  It will lead to incorrect behavior in XBL, as you noted. Please
don't do that.

> The only other option I see is to load the whole string

I like that option a _lot_ more.

It looks like you still lose namespaces off the context....

And I'm not sure I'm so happy with the solution for SetDocument().  Does the XML
sink really depend on having an mDocument enough that you can't leave it null?

Comment 50

[steve.swanson]

I backed out the XBL botch and the ShouldSetDocument (only have to protect one 
use of mDocument). And I was able to avoid the override of GetCurrentContent() 
by pre-loading the content stack.

Still working on the DOM surgery part.  Can't decide whether filtering or 
creation then surgery will work best.

Comment 51

[steve.swanson]

Attachment: Revised patch again, should deal with namespace better

I've worked out the DOM surgery business, I think.  Does the right thing on the
testcases.  Some issues:

1. After loading the MathML testcase, I get an ASSERT on shutdown that the
shared pref handler is leaking.  (Only started after I refreshed my tree.)  I
don't think any of my code is misbehaving, but it would be good to track this
down.

2. nsRange::CreateContextualFragment() doesn't add namespace information to the
tagstack it sends in.  Hence, I haven't actually tested the behavior WRT
namespaces.  I guess I should try to fix this, but it looks tricky.

And I still want to create a few more testcases.

Comment 52

[steve.swanson]

Attachment: testcase for namespace handling

The key test creates a string of MathML without namespace declarations and then
calls CreateContextualFragment inside a MathML node (which has the right
namespace declarations).

Current behavior (with the patch) is that MathML is not being created (where it
should be).

Comment 53

[steve.swanson]

Attachment: testcase for namespace handling using XUL

Again (using the patch), XUL elements are not created unless the input string
has the XUL NS declaration.

Comment 54

[steve.swanson]

Attachment: testcase using a namespace prefix

This document uses a mml prefix for the MathML namespace.  Fragments should use
this prefix (and there are various buttons which attempt to load bits of MathML
marked up in various ways).

Comment 55

[steve.swanson]

Attachment: testcase where MathML namespace has global scope

MathML namespace is set on doc element, whereas XHTML namespace uses x prefix.
A little strange, but shows that MathML elements will be created without
further specifying the namespace.

Comment 56

[steve.swanson]

Attachment: new patch handles namespaces in the context

This patch correctly deals with all the testcases.  And I was able to clean up
the nsXMLFragmentContentSink a bit.

I had to add quite a bit of code in nsParser for serializing the context.  In
particular, there's some strange behavior of XUL elements which seem to gobble
the NS declarations.

Unfortunately, the ASSERT about leaking the shared pref handling is still there
after you try one of the MathML examples.  The patch doesn't do anything with
prefs, nor do I see a mechanism in the MathML code for this to happen, so it
remains a mystery.

Comment 57

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 155044 [details] [diff] [review]
new patch handles namespaces in the context

>? content/xml/document/src/nsXMLFragmentContentSink.cpp

>  nsCOMPtr<nsIContent>  mRoot;
>  nsCOMPtr<nsIContent>  mEndnote;
>
>  PRBool mAllContent;
>  nsCOMPtr<nsIDocument> mTargetDocument;

Would be nice to document the members (especially mAllContent; it's really not
clear what that does).

I'm a little confused by the aContext/mAllContent thing.  mAllContent is set
when we do NOT have a context, right?  That's what
NS_NewXMLFragmentContentSink/Sink2 seem to end up doing.  But when passed to
the constructor and NewXMLFragmentContentSinkHelper it's called aContext, which
has exactly the opposite meaning.  Did you just forget to rename those args?

>NewXMLFragmentContentSinkHelper(PRBool aContext, 

>  if (nsnull == aResult) {
>    return NS_ERROR_NULL_POINTER;
>  }

Don't bother with this.  People violating the XPCOM out param rules deserve
what they  get.

>  if (nsnull == it) {

if (!it) {

>NS_NewXMLFragmentContentSink2(nsIFragmentContentSink** aResult)

>  return NewXMLFragmentContentSinkHelper(PR_TRUE,aResult);

Space after comma.

>  return NewXMLFragmentContentSinkHelper(PR_FALSE,aResult);

Same.

>nsXMLFragmentContentSink::WillBuildModel(void)

>  NS_ENSURE_SUCCESS(rv, rv);
>
>  mRoot = do_QueryInterface(frag);
>
>  if (NS_SUCCEEDED(rv))

You've already ensured rv is a success code.  Did you mean to set it in the
do_QI call and be checking that here?

>      for (PRUint32 child = mEndnote->GetChildCount(); child > 0; child--) {
>        mRoot->AppendChildTo( mEndnote->GetChildAt(0), PR_FALSE, PR_FALSE );
>        mEndnote->RemoveChildAt( 0, PR_FALSE );
>      }

This isn't quite right.  When you RemoveChildAt(), it'll set the parent of the
child to null, which is not what we want here...

I'd say you want something more like:

  nsCOMPtr<nsIContent> child = mEndnote->GetChildAt(0);
  mEndnote->RemoveChildAt(0, PR_FALSE);
  mRoot->AppendChildTo(child, PR_FALSE, PR_FALSE);

>nsXMLFragmentContentSink::CreateElement(const PRUnichar** aAtts, 
>  if (NS_SUCCEEDED(rv) && aNodeInfo->Equals(nsHTMLAtoms::endnote))

Hmmm...  I just realized that this runs reasonable risk of colliding with
someone's random XML.  Perhaps file a followup bug (and cc me) to make the
endnote tagname less collision-prone?  Don't bother with that for this patch.

One other thing here.  Don't you want to make sure that *aAppendContent is
true?  Otherwise, some nodes (eg scripts) will not get appended here and won't
get appended in CloseElement either...

>nsXMLFragmentContentSink::HandleDoctypeDecl(const nsAString & 
>  FlushText();
>
>  NS_NOTREACHED("fragments shouldn't have doctype declarations");

No reason to flush if its NOTREACHED.  :)

>nsXMLFragmentContentSink::HandleXMLDeclaration(const PRUnichar *aData, 

Same.

>? parser/htmlparser/public/nsIFragmentContentSink.h

>// used to be #define NS_IHTML_FRAGMENT_CONTENT_SINK_IID \
>//             {0xa6cf9102, 0x15b3, 0x11d2,              \
>//             {0x93, 0x2e, 0x00, 0x80, 0x5f, 0x8a, 0xdd, 0x32}}

Is that comment really needed?

>   * If aDocument is null or has no nodeinfo manager, the sink will
>   * create a brand-new nodeinfo manager.

That doesn't match what the XML fragment sink actually does when aDocument is
null....  For that matter, it doesn't match what the HTML sink does before or
after this patch.  Just comment here that the document must not be null.

>Index: content/base/src/nsRange.cpp

>+              tagName.Append(NS_LITERAL_STRING(":"));

How about PRUnichar(':') instead?

>+            tagName.Append(NS_LITERAL_STRING("=\""));
>+            tagName.Append(uriStr);
>+            tagName.Append(NS_LITERAL_STRING("\""));

  tagName.Append(NS_LITERAL_STRING("=\"") + uriStr +
		 NS_LITERAL_STRING("\""));

perhaps?

>+      // XUL overrides attribute handling?  (I don't get the XUL NS in the above loop)

This is a bug in the XUL content sink... compare the code at
http://lxr.mozilla.org/seamonkey/source/content/xul/document/src/nsXULContentSi
nk.cpp#715  to the code at
http://lxr.mozilla.org/seamonkey/source/content/xml/document/src/nsXMLContentSi
nk.cpp#1409

It's OK to leave the XUL hack here for now if you want, but file a followup bug
to fix the XUL sink and remove the hack, ok?  cc me on that.

>+      // choose a default content type
>+      if (htmlDoc) {

Like I said in my first set of comments, if |document| is false, htmlDoc will
not be true.  So this branch can just safely assign text/xml, I would say.

That said, since we're assuming XML if |document| is null, we should be
assuming bCaseSensitive is true, not false.

>Index: content/html/document/src/nsHTMLFragmentContentSink.cpp

This has the same naming weirdness where aContext means that there will in fact
not be any context and all the style nit comments on the NS_New* functions and
the helper from the XML sink apply here.

>+  const char *sentinelStr;
>+  nsHTMLAtoms::endnote->GetUTF8String(&sentinelStr);
>+  if (tag.EqualsIgnoreCase(sentinelStr)) {

<endnote> will never be uppercase, so why not just:

  if (nsHTMLAtoms::endnote->Equals(tag)) {

>Index: content/xml/document/src/nsXMLContentSink.h

>+  virtual PRBool SetDocElement(PRInt32 aNameSpaceID, 
>+                               nsIAtom *aTagName,
>+                               nsIContent *aContent);

Please document what this does (especially what the return value means).

>+  NS_IMETHOD ReportErrorFrom(const PRUnichar* aErrorText, 
>+                             const PRUnichar* aSourceText,
>+                             nsIDOMNode* node);

Please document what this does.

The rest looks good.

Still marking r- because of the two substantive issues (tag appending, and the
DOM surgery bit), but fixing the comments shouldn't be too bad.  :)

Comment 58

[steve.swanson]

Thanks for the careful comments.  I don't foresee any problems cleaning things 
up, but a new patch will have to wait until I return from a week on the road.

Comment 59

[steve.swanson]

Attachment: clean up, comment, revise the patch

I believe I've addressed all the issues in the previous patch.

I'll try to make some testcases for the XUL NS problem and the <endnote> in XML
conflict.

Comment 60

[steve.swanson]

Attachment: testcase for XUL NS attribute

The XUL NS declaration should live in the XMLNS NS, but doesn't.

I think this is a result of bug 75870.

Comment 61

[steve.swanson]

Attachment: testcase where <endnote> is part of fragment stream

An XHTML document, but I added endnote and sidenote tags.  With the current
patch, creating fragments containing the endnote tag fail.

The current behavior is to take the contents of the last seen <endnote>.  I
tried to structure the testcase so it would also fail if the code accepted the
first seen <endnote>  (<endnote> might be in the context).

Comment 62

[Boris Zbarsky [:bzbarsky]]

> I think this is a result of bug 75870.

Maybe the other way around, actually.... and maybe it's completely unrelated to
bug 75870.  Just file a bug on the XUL NS issue, please?

Comment 63

[Boris Zbarsky [:bzbarsky]]

I'll try to look at this in the next day or two, but my net connection is
unpredictable at best right now...

Comment 64

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 156295 [details] [diff] [review]
clean up, comment, revise the patch

>? content/xml/document/src/nsXMLFragmentContentSink.cpp
>NewXMLFragmentContentSinkHelper(PRBool aContext, nsIFragmentContentSink** aResult)

Should aContext be aAllContent here?  It looks like you forgot to fix that....

>Index: content/base/src/nsRange.cpp

>+      // XUL overrides attribute handling?  (I don't get the XUL NS in the above loop)

Mention the bug# of the XUL bug you filed in this comment?

>Index: content/html/document/src/nsHTMLFragmentContentSink.cpp
>+NewHTMLFragmentContentSinkHelper(PRBool aContext, nsIFragmentContentSink** aResult)

Again s/aContext/aAllContent/?

>Index: content/xml/document/src/nsXMLContentSink.h

>-  nsresult ProcessBASETag(nsIContent* aContent);
>+  nsresult ProcessBASETag(nsIContent* aContent);                            

That looks like you just added whitespace.  Don't do that.  :)

r=bzbarsky with those nits fixed.

Peter, you seem like the best sr for this; care to take a look?

Comment 65

[steve.swanson]

Attachment: patch with nits removed

Comment 66

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 157629 [details] [diff] [review]
patch with nits removed

peterv says he may not get to this soon... jst, could you sr?

Comment 67

[Boris Zbarsky [:bzbarsky]]

Note that with bug 256165 fixed it should be possible to remove the XUL xmlns hack.

Comment 68

[steve.swanson]

Sorry, I don't see how to edit the patch attachment.

Remove the following lines from nsRange.cpp in the patch

      // XUL overrides attribute handling?  (bug 256165)
      content->GetNameSpaceID(&namespaceID);
      if (namespaceID == kNameSpaceID_XUL) {
        result =
nsContentUtils::GetNSManagerWeakRef()->GetNameSpaceURI(kNameSpaceID_XUL, uriStr);
        if (NS_SUCCEEDED(result)) {
          tagName.Append(NS_LITERAL_STRING(" xmlns=\"") + uriStr +
NS_LITERAL_STRING("\""));
        }
      }

I did check that all the testcases run correctly after this change.

Comment 69

[Boris Zbarsky [:bzbarsky]]

Steve, patches can't be edited once they're posted...  If jst is OK with the
patch, I'll remove those lines before checking in.

Comment 70

[Johnny Stenback (:jst)]

Comment on attachment 157629 [details] [diff] [review]
patch with nits removed

sr=jst, sorry for it taking so long for me to get to this.

Comment 71

[Boris Zbarsky [:bzbarsky]]

Checked in to trunk for 1.8a5.  Leaving it to Steve to resolve this if it's
fixed (it looks like there should be nothing else to do here, but it's his show,
so... ;) )

Steve, thanks a ton for the patch!

Comment 72

[steve.swanson]

Marking FIXED since current trunk build passes all relevant testcases.

The problem mentioned in comment 61 remains:  strings of XML containing
<endnote> tags are not correctly handled.

Comment 73

[Boris Zbarsky [:bzbarsky]]

Steve, is there a bug filed on that?

Comment 74

[steve.swanson]

Now there is, bug 263053, reusing the testcase from comment 61.

Comment 75

[Marc Diethelm]

If send a PHP header("Content-type: application/xhtml+xml");
and try an innerHTML testcase in

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.12) Gecko/20050915
Firefox/1.0.7

I get an exception:

Error: uncaught exception: [Exception... "Component returned failure code:
0x80004005 (NS_ERROR_FAILURE) [nsIDOMNSHTMLElement.innerHTML]"	nsresult:
"0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame ::
http://path/testcase.innerHTML_bug.php :: changeText :: line 9"  data: no]

(see comment[4]+testcase in bug 313049)
https://bugzilla.mozilla.org/attachment.cgi?id=200449

If I don't send the header (good ol' text/html) it works of course. Can someone
test this in the 1.5 beta or a nightly? I will, but can't do it now.

Comment 76

[Boris Zbarsky [:bzbarsky]]

1.0.x uses code from April 2004 (yes, 4, not 5).  This bug was fixed on 2004-09-30.  So please don't waste time testing with 1.0.x.  This is fixed in 1.5.

Comment 77

[Henri Sivonen (:hsivonen)]

(In reply to comment #57)
> Comment on attachment 155044 [details] [diff] [review]
> new patch handles namespaces in the context
> 
> >? content/xml/document/src/nsXMLFragmentContentSink.cpp
> 
> >  nsCOMPtr<nsIContent>  mRoot;
> >  nsCOMPtr<nsIContent>  mEndnote;
> >
> >  PRBool mAllContent;
> >  nsCOMPtr<nsIDocument> mTargetDocument;
> 
> Would be nice to document the members (especially mAllContent; it's really not
> clear what that does).

Indeed. Can anyone remember what problem this was solving? Does editor want to see an implied body (what about implied head) when there's no context node? (When is there no context node in editor?)

Should the case where mAllContent is true simply map to using "html" in the HTML namespace as the context for the purposes of the HTML5 fragment parsing algorithm, or are the requirements more complex here?

Comment 78

[Henri Sivonen (:hsivonen)]

(In reply to comment #77)
> Indeed. Can anyone remember what problem this was solving? Does editor want to
> see an implied body (what about implied head) when there's no context node?
> (When is there no context node in editor?)

Never mind. I think I'll deal with this by changing the editor code more broadly that I was expecting.