Closed Bug 199331 Opened 22 years ago Closed 21 years ago

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

Categories

(Core :: XSLT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.4final

People

(Reporter: sicking, Assigned: sicking)

Details

(Keywords: regression)

Attachments

(1 file)

With compiled stylesheets we regressed the ability to modify the DOM after
calling .importStylesheet. This should be fixed
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.4beta
Attached patch patch to fixSplinter Review
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.
Attachment #123162 - Flags: superreview?(peterv)
Attachment #123162 - Flags: review?(axel)
trying to get this in for 1.4 since I know there are users out there relies on
being able to do this.
Target Milestone: mozilla1.4beta → mozilla1.4final
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.
Attachment #123162 - Flags: review?(axel) → review+
Comment on attachment 123162 [details] [diff] [review]
patch to fix

fixed Pikes comment locally
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?
Attachment #123162 - Flags: superreview?(peterv) → superreview+
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.
Attachment #123162 - Flags: approval1.4?
Comment on attachment 123162 [details] [diff] [review]
patch to fix

a=mkaply for checkin to 1.4

This needs to go in this morning.
Attachment #123162 - Flags: approval1.4? → approval1.4+
Is this one going in?  I tried the latest nightly on Windows, and it doesn't
seem to be taking style attribute modifications..
This one was landed before the branch so it should work in 1.4, please file a
bug otherwise
Oh, and this bug is about XSLT stylesheets. This has nothing to do with CSS.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: