199331 - modifying the DOM of a stylesheet after calling .importStylesheet does nothing

Status: RESOLVED FIXED

(Core :: XSLT, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

With compiled stylesheets we regressed the ability to modify the DOM after
calling .importStylesheet. This should be fixed

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: patch to fix

This patch makes us attach an nsIDocumentObserver to the stylesheet-document.
The patch tries to not hold owning references to the document, since if noone
else cares about the document there's no point in us keeping it alive. I was
however not able to do that for embedded stylesheets since I couldn't come up
with a good way of knowing when/if the element dies.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

trying to get this in for 1.4 since I know there are users out there relies on
being able to do this.

Comment 3

[Axel Hecht]

Comment on attachment 123162 [details] [diff] [review]
patch to fix

+    NS_ENSURE_TRUE(!mStylesheetDocument && !mStylesheet,
+		    NS_ERROR_NOT_IMPLEMENTED);

add a comment that this says that we don't support multiple imports per js
interface yet.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 123162 [details] [diff] [review]
patch to fix

fixed Pikes comment locally

Comment 5

[Peter Van der Beken [:peterv]]

Comment on attachment 123162 [details] [diff] [review]
patch to fix

>Index: source/xslt/txMozillaXSLTProcessor.cpp
>===================================================================

> NS_IMETHODIMP
> txMozillaXSLTProcessor::Reset()
> {
>+    if (mStylesheetDocument) {
>+        mStylesheetDocument->RemoveObserver(this);
>+    }
>     mStylesheet = nsnull;
>+    mStylesheetDocument = nsnull;
>+    mEmbeddedStylesheetRoot = nsnull;

Do you want to reset mCompileResult here?

>+nsresult
>+txMozillaXSLTProcessor::ensureStylesheet()
>+{

>+    nsCOMPtr<nsIDOMNode> style;
>+    if (mEmbeddedStylesheetRoot) {
>+        style = do_QueryInterface(mEmbeddedStylesheetRoot);
>+    }
>+    else {
>+        style = do_QueryInterface(mStylesheetDocument);
>+    }

I'd write this as 

    nsCOMPtr<nsIDOMNode> style = do_QueryInterface(mEmbeddedStylesheetRoot);
    if (!style) {
	style = do_QueryInterface(mStylesheetDocument);
    }

No biggie though.

>+    nsresult rv = TX_CompileStylesheet(style, getter_AddRefs(mStylesheet));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    return NS_OK;

Just return the result of TX_CompileStylesheet

>+NS_IMETHODIMP
>+txMozillaXSLTProcessor::DocumentWillBeDestroyed(nsIDocument* aDocument)
>+{
>+    if (NS_FAILED(mCompileResult)) {
>+        return NS_OK;
>+    }

Shouldn't this just be an assertion since you RemoveObserver below?

Comment 6

[Peter Van der Beken [:peterv]]

Comment on attachment 123162 [details] [diff] [review]
patch to fix

Asking approval on behalf of sicking. This is a low risk patch which makes us
handle mutations of the stylesheet document before using it in a transformation
again. This only affects XSLT transformations from JS, so it affects a small
number of pages but those mostly rely on this behavior.

Comment 7

[Mike Kaply [:mkaply]]

Comment on attachment 123162 [details] [diff] [review]
patch to fix

a=mkaply for checkin to 1.4

This needs to go in this morning.

Comment 8

[Marcus G. Daniels]

Is this one going in?  I tried the latest nightly on Windows, and it doesn't
seem to be taking style attribute modifications..

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This one was landed before the branch so it should work in 1.4, please file a
bug otherwise

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oh, and this bug is about XSLT stylesheets. This has nothing to do with CSS.