202035 - Cannot use XSL stylesheet as "default stylesheet" for a certain DOCTYPE

Status: RESOLVED WONTFIX

(Core :: XML, defect)

Description

[Roland Mainz]

[as discussed with heikki on IRC]
I cannot use XSL stylesheet as "default stylesheet" for a certain DOCTYPE - just
adjusting nsExpatDriver.cpp fails due additional bugs/issues in the code chain
which follows:
-- snip --
RCS file: /cvsroot/mozilla/htmlparser/src/nsExpatDriver.cpp,v
retrieving revision 3.37
diff -u -r3.37 nsExpatDriver.cpp
--- htmlparser/src/nsExpatDriver.cpp       2 Apr 2003 22:59:50 -0000       3.37
+++ htmlparser/src/nsExpatDriver.cpp       14 Apr 2003 23:31:26 -0000
@@ -207,6 +207,9 @@
  {"-//W3C//DTD XHTML 1.1 plus MathML 2.0 plus SVG 1.1//EN", "mathml.dtd",
"resource:/res/mathml.css" },
  {"-//W3C//DTD SVG 20001102//EN",              "svg.dtd",     nsnull },
  {"-//WAPFORUM//DTD XHTML Mobile 1.0//EN",     "xhtml11.dtd", nsnull },
+ {"-//OASIS//DTD DocBook XML V4.2//EN",        nsnull,
"file:///tmp/docbooktest/docbook-xsl-1.60.1/html/docbook.xsl" },
  {nsnull, nsnull, nsnull}
 };
-- snip --

ToDo (per heikki):
1. in HandleDoctypeDecl(), disable pretty printer if aCatalogData exists
2. since
http://lxr.mozilla.org/seamonkey/source/content/xml/document/src/nsXMLContentSink.cpp#1882
expects a CSS style sheet - determine if the stylesheet is XSLT and set up the
XSLT transformation, otherwise do what the code does now

Comment 1

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

So this bug is about fixing the content sink, not adding the catalog entry.
People who build custom browsers should be able to what Roland, wants though. 

I don't feel like adding the Docbook catalog entry unconditionally would be too
good an idea yet, because there is no way to disable them. If someone happens to
serve a DocBook document with their own CSS, the default XSLT would still be
applied which would suck.

Comment 2

[Roland Mainz]

heikki wrote:
> I don't feel like adding the Docbook catalog entry unconditionally would be 
> too good an idea yet, because there is no way to disable them. If someone 
> happens to serve a DocBook document with their own CSS, the default XSLT would
> still be applied which would suck.

The same argumentation could be done for MathML and SVG. What if the users want
to use their own stylesheets for these documents, too ?

Comment 3

[Roland Mainz]

Attachment: Patch for 2003-04-08-08-trunk

Comment 4

[Roland Mainz]

Attachment: DocBook XSL stylesheets (based on docbook-xsl-1.60.1) to test the patch (tar.gz; unpacks into dist/bin/res/docbook)

Comment 5

[Roland Mainz]

Comment on attachment 120520 [details] [diff] [review]
Patch for 2003-04-08-08-trunk

Requesting r= ...

Comment 6

[Roland Mainz]

Attachment: New patch for 2003-04-08-08-trunk top cover more DocBook/XML DOCTYPEs

Comment 7

[Roland Mainz]

Taking myself for now...

Comment 8

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

Comment on attachment 120528 [details] [diff] [review]
New patch for 2003-04-08-08-trunk top cover more DocBook/XML DOCTYPEs

The MathML CSS file is different, because it is required to render MathML in
Mozilla. Sure, you could use other stylesheets, but you wouldn't get the
special rendering. The SVG CSS is actually empty file (we should remove it for
now). Also you can, actually, override the CSS styles with content CSS. You
cannot do that with XSLT I believe. Btw, what happens if the docbook.xsl file
does not exist?

In the sink, we can't just look at the URL and do ad hoc guessing of the mime
type from there. nsIChannel has contentType property, SimpleMAPI code asks from
Windows registry with the extension, and uriloader has a defaultMimeEntries
table for extension mapping. We need to use some of that here IMO - I would
first try to figure out if it would be possible to do this via the channel.

Comment 9

[Roland Mainz]

Attachment: New, cleaned-up patch per comments from heikki and dmose

Comment 10

[Roland Mainz]

Heikki Toivonen wrote:
> (From update of attachment 120528 [details] [diff] [review])
> The MathML CSS file is different, because it is required to render MathML in
> Mozilla. Sure, you could use other stylesheets, but you wouldn't get the
> special rendering. The SVG CSS is actually empty file (we should remove it for
> now). Also you can, actually, override the CSS styles with content CSS. You
> cannot do that with XSLT I believe.

I doubt there is any DocBook/XML file which has inline CSS (sure, in theory you
could write one but I never saw such a thing in the wilderness and I doubt it
would make much sense).

> Btw, what happens if the docbook.xsl file does not exist?

Well, then there is no stylesheet to load and I _guess_ the default behaviour
will then "kick in".

I know that the current patch is not perfect (in theory XPI/JARs should be able
to register their stuff instead of using the hardcoded |kCatalogTable| table in
htmlparser/src/nsExpatDriver.cpp, the stylesheet type detection could be better
(I am using the mimetype service for that now, anything beyond that ends in
major surgery in the code which should be _offloaded_ to seperate follow-up
bugs) and I see lots of stuff which could be "done" better) - however it's a
very nice feature which should go "in" now and then we can extend and enhanche
it step-by-step...

Comment 11

[Roland Mainz]

Attachment: New patch for 2003-04-08-08-trunk

OK, I fixed the issue that l10n.xsl could not be loaded because local XSL
stylesheets could not be used by XML documents from the net.

Comment 12

[Roland Mainz]

BTW: l10n.xsl error was:
-- snip --
Security Error: Content at http://snurse-l.org/acllc-c++/faq.xml may not load or
link to
file:///shared/bigtmp2/mozilla/2003-04-08-08-trunk_cvs/objdir_ws7_gtk/dist/bin/res/docbook/common/l10n.xml.
-- snip --
fixed by adding |nsresult nsXMLContentSink::LoadXSLStyleSheet(nsIURI* aUrl,
PRBool ignoreReferrer)| (see attachment 120592 [details] [diff] [review]) ...

Comment 13

[Roland Mainz]

Comment on attachment 120592 [details] [diff] [review]
New patch for 2003-04-08-08-trunk

Requesting r= ...

The patch has one minor flaw. If "resource:/res/docbook/xhtml/docbook.xsl" is
missing the XML PrettyPrinter does not kick-in (however the number of people
who may be affacted by this is close to zero).
I tend to forward this and other nasty nits to one of the follow-up bugs... :)

Comment 14

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

Comment on attachment 120592 [details] [diff] [review]
New patch for 2003-04-08-08-trunk

>Index: htmlparser/src/nsExpatDriver.cpp
>===================================================================

These changes are tricky. Currently if the files don't exist the display ends
up in bad state - old document seems to stay there but the content area does
not accept events. peterv is working on fixing this, but once it is done it
will put up an error message on the content area which is not suitable in our
case because we would like to proceed as if we never even tried to apply the
stylesheet.

The docbook.xsl files should be an extension at best to not burden people who
don't want them. At the moment I am more inclided towards an external XPI from
mozdev.org, for example. The XPI solution might be required if the docbook
stylesheets cannot be released with MPL compatible license.

>Index: content/xml/document/src/nsXMLContentSink.cpp
>===================================================================
>+static PRLogModuleInfo *nsXMLContentSinkLM = PR_NewLogModule("nsXMLContentSink");

Does all logging code "leak" like this? Can it be made not to leak?

>+  return mXSLTProcessor->LoadStyleSheet(aUrl, loadGroup, (ignoreReferrer)?(nsnull):(mDocumentURL));

Long line, need spaces around ? and :, no parenthesis around nsnull,
mDocumentURL.

>+      PRBool isXSL = PR_FALSE,
>+             isCSS = PR_FALSE;

I think you could get rid of isXSL, and assume it is XSLT if !isCSS. That would
change the code below slightly. We do similar assumptions else where in the
sink.

>+        /* Ignore referrer to get rid of the security checks - the XSLT files
>+         * from the XML catalogs are guranteed to be local */

This is scary - there aren't really any guarantees as it stands now. This code
here assumes nobody will ever add a catalog to an arbitrary URL. A hacky
solution would be to check the URLs scheme and only allow chrome, resource and
file, but I am also concerned about some of these local stylesheets including
remote styles (what happens then?).

I am not sure what we should do here...

We could get by the bad display state (or error message in the future) by
checking if the URL points to an existing local file and try to load the style
in that case only. I think this is really needed.

Comment 15

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

Do you guys have any thoughts on this? Am I being overly paranoid about the
security etc.?

Comment 16

[Peter Van der Beken [:peterv]]

Yes, there's potential security problems here. However, since we're not talking
about including the docbook stylesheets by default we could allow loading of
catalog sheets and then the user can decide if he wants to install those XSLT
stylesheets in the first place when installing the xpi or something. There
absolutely needs to be a way to disable catalog XSLT stylesheets, CSS is
different because of the cascade.
This patch has plenty of problems. I don't think we should force on logging in
the sink. The new LoadXSLStyleSheet function is completely unnecessary, just add
an argument to the existing one. I don't like the new code in HandleDoctypeDecl,
there should be a way of specifying the mimetype for the stylesheet in the
catalog stylesheet list, don't use the MIME service for that.

> If "resource:/res/docbook/xhtml/docbook.xsl" is missing the XML PrettyPrinter
> does not kick-in (however the number of people who may be affacted by this is
> close to zero).
> I tend to forward this and other nasty nits to one of the follow-up bugs... :)

IMHO known regressions like this should be fixed before landing.

Comment 17

[Roland Mainz]

Heikki Toivonen wrote:
> The docbook.xsl files should be an extension at best to not burden people who
> don't want them. At the moment I am more inclided towards an external XPI from
> mozdev.org, for example.

This will not work, future versions of this stuff will include new code in
layout/docbook/ to handle the DocBook stuff which can't be handled by HTML/CSS.
the usage of the XSL transformation stylesheets is just an intermediate
solution; next steps would be to rip-out some of the transformation stuff and
replace it step-by-step with CSS and usage of the special DocBook code in
layout/ and extensions/dsssl/ ...

Comment 18

[Peter Van der Beken [:peterv]]

I personally think we should not put those stylesheets in the default build. As
for your future plans, I think modifying the layout engine for Docbook shouldn't
be done unless it's done in a modular way. I don't think our layout engine is
ready for that. And adding DSSSL support to Mozilla, even if it's in
mozilla/extensions, lets not go there. CC'ing some of the people from the XSL:FO
bug (bug 95959), they might have something to say about DSSSL support :-).

Comment 19

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

I thought about adding a parameter that would explicitly state the mime type of
the URL, but that won't work once we implement real XML Catalogs/"entityrc"
files (because you can't specify content type there) so I think Roland's
approach to use the MIME service is the correct one. I also think it is better
to put that MIME service call here, because otherwise every caller would need to
do it. Since we do this in very rare occasions, and only once per document,
performance should not generally be an issue.

Comment 20

[Peter Van der Beken [:peterv]]

Since the MIME service doesn't do sniffing, I still think it's a bad idea. A
misconfigured mimetype on a local file is not uncommon in my experience. Loading
that stylesheet through a xml-stylesheet PI would work, but through the catalog
wouldn't.

Comment 21

[Hixie (not reading bugmail)]

It is unclear to me what this bug is trying to fix.

If it is about DOCBOOK support, then that should be done in a very modular way,
and preferably using a CSS stylesheet not an XSLT one.

All the problems with XSLT and CSS not being equivalent are merely symptoms of
an underlying problem with the way XSLT was implemented -- there should be one
place in the code that can take a stylesheet and dispatch it to the right style
system, be it CSS, XSLT, JSSS, or whatever. bz and I have spoken about this in
the past. It is unlikely to happen any time soon as I understand it.

In any case, none of this dispatching should be based on the DOCTYPE. It should
all be triggered by the namespaces. The only thing the catalog should be used
for is linking in a DTD for entities. (Again, if this is specifically about
DOCBOOK, I have been in contact with the DOCBOOK guy, and he is aware of (and
willing to try to solve) the problem with DOCBOOK not having a namespace.)

Comment 22

[Henri Sivonen (:hsivonen) (on vacation)]

This looks like adding doctype sniffing to XML, which would be dirty. This stuff
really should be namespace or MIME type-triggered. To me it looks wrong that
MathML gets some additional styles is a particular public id is present.

Comment 23

[Hixie (not reading bugmail)]

I thought MathML got its additional styles from the namespace, not the DOCTYPE.
All I thought it got from the DOCTYPE is the entities, which would be correct.

Comment 24

[charles_belov]

I'm not sure from reading this bug whether my problem is a manifestation of this
bug, or if I need to make a new bug.  In Mozilla milestone 1.3, the following
XML file http://www.ecommercetoday.com.au/ecom/news/rss/rss-news.xml displays
with the message "This XML file does not appear to have any style information
associated with it. The document tree is shown below." even though it starts
with the code

<?xml:stylesheet
type="text/xsl"
href="http://www.ecommercetoday.com.au/ecom/news/rss/rss-news-summary.xsl"
?>

I verified with an XML validator that
http://www.ecommercetoday.com.au/ecom/news/rss/rss-news-summary.xsl has
well-formed XML code.

Does this situation fall under bug 202740, or do I need to file a separate bug?

Comment 25

[charles_belov]

Typo. S/b does this apply to 202035. Sorry.

Comment 26

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

Charles, that is a different issue. The reason the stylesheet is not applied in
your case is invalid processing instruction, replace ':' with '-'.

Comment 27

[RNicoletto]

Attachment: Testase result using Mozilla/5.0 (Windows; Windows NT 5.1; en-US; rv:2.0b2pre) Gecko/20100716 Minefield/4.0b2pre

Comment 28

[Henri Sivonen (:hsivonen) (on vacation)]

I think this should be WONTFIXed, because binding behaviors other than DTD processing to the XML doctype is bogus. See: http://hsivonen.iki.fi/doctype/#xml

Comment 29

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

Agree. Though if someone feels strongly otherwise, feel free to reopen.