<?xml-stylesheet?> that's outside of prolog is applied

VERIFIED FIXED in mozilla0.9.4

Status

()

Core
XML
P3
normal
VERIFIED FIXED
17 years ago
16 years ago

People

(Reporter: Chris Petersen, Assigned: Heikki Toivonen (remove -bugzilla when emailing directly))

Tracking

Trunk
mozilla0.9.4
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments)

(Reporter)

Description

17 years ago
Build: 2001060713
Platform: All
Expected Results: Stylesheet PI should be ignored if outside of the document's
prolog.
What I got: Stylesheet is applied to the document

Steps to reproduce:

1) Go to  any of these tests:

http://mozilla.org/quality/browser/standards/xml/pi_in_root_element.xml
http://mozilla.org/quality/browser/standards/xml/pi_in_child_element.xml
http://mozilla.org/quality/browser/standards/xml/pi_after_root_element.xml

2) The stylesheet PI is not located in the document's prolog but it style
applied to the text.
(Reporter)

Comment 1

17 years ago
Changing platform to All
Hardware: DEC → All
Peter, I thought you might be interested to learn about this bug. We are a bit
too forgiving with our handling of the xml-stylesheet PI. This is kinda
unfortunate because people are already learning to depend on this incorrect
behavior.

This is probably easy to fix. In the sink we have the prolog state, but that
does not solve the whole problem. If one adds the PI with script after the
document is loaded one needs to check that the PI has no parent elements and no
elements before it.
Keywords: correctness
Heikki, I agree. I was very surprised that no check was done either for <link>
elements (to see if they're within head). However, I didn't want to change the
code as part of my patch for bug 7515, which turned out to cause enough
regressions as is :(.
We do need the check in the content sink so it applies to XSLT stylesheets too.
And we could add a check to nsStyleLinkElement::UpdateStylesheet, calling a new
function in nsIStyleSheetLinkingElement that checks if the node is in the right
"spot". We then need to implement that function in the three stylesheet linking
elements: HTMLLink, HTMLStyle and XMLPI.
*** Bug 86058 has been marked as a duplicate of this bug. ***
Priority: -- → P3
Target Milestone: --- → mozilla0.9.3

Comment 5

17 years ago
Doesn't look like this is getting fixed before the freeze tomorrow night.
Pushing out a milestone.  Please correct if I'm mistaken.
Target Milestone: mozilla0.9.3 → mozilla0.9.4
Created attachment 46111 [details]
zip file of testcases
Created attachment 46112 [details] [diff] [review]
Proposed fix 1
The zip of testfiles contains 5 XML files and 2 CSS files. The first three tests
have the PI somewhere in the prolog, which should cause the document text to be
green. p4-p5 have the PI in the document and in the epilog, and the text color
should NOT be red.

Note that IE 5.x fails test p2 where the PI is in the DOCTYPE, but it passes all
of the other tests.

The fix went into the PI element code. I have not yet tested with XSLT to see if
this is enough. I might need to add a little check in the XML content sink or
something.
Status: NEW → ASSIGNED
Whiteboard: [fixinhand]
Created attachment 46116 [details] [diff] [review]
Proposed fix 2 (take care of XSLT in XML content sink)
Turns out I need to check in the XML content sink if we are in the prolog. I
will still check with scripts adding stuff, but I think/hope this is enough
(I'll attach more tests). 

Peter, can you review?
r=peterv.
Created attachment 46168 [details]
zip with JS samples and one XSLT (JS) sample
With some more testing it seems things work perfectly with CSS. Things also work
in XSLT expect when the PI is added by script (in which case we don't seem to do
the XSLT transformation and I did not change that behaviour). Peter, is there a
bug on that?
Looks ok to me, sr=jst.

One style nit:

-    if (target.EqualsWithConversion(kStyleSheetPI) &&
!type.EqualsIgnoreCase("text/css")) {
+    if (mState == eXMLContentSinkState_InProlog && +
       target.EqualsWithConversion(kStyleSheetPI) && +
       !type.EqualsIgnoreCase("text/css")) {

shouldn't the two last lines here be indented to line up with mState?
Fixed, time for wine.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
Whiteboard: [fixinhand]
(Reporter)

Comment 16

16 years ago
Marking verified in the Sept 05 th build.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.