194387 - support parameter-passing in PI transformations

Status: RESOLVED FIXED

(Core :: XSLT, defect)

Description

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

It would be usefull to be able to pass parameters to the XSLT stylesheet when 
transforming using a PI.

It has been suggested (and IE possibly supports) something like:

<?xml-stylesheet href="mystylesheet.xsl?param=value" ... ?>

however those parameters are part of the uri and addressed at the *server* so 
i'm very reluctant to do this. Same goes for using ';' rather then '?'.

I suggest something like

<?xml-stylesheet href="mystylesheet.xsl" moz:param="param=value" ... ?>

Note that the 'moz' is not a namespace but a prefix, much like the -moz- prefix 
in css. Not entierly sure what multiple parameters would look like, probably

<?xml-stylesheet href="mystylesheet.xsl"
                 moz:param="param=value,param2=value2" ... ?>

There's also the question of what datatypes to support. If we only support 
strings we could just do 

<?xml-stylesheet href="mystylesheet.xsl"
                 moz:param="theme=green" ... ?>

but if we want bools and numbers then maybe

<?xml-stylesheet href="mystylesheet.xsl"
                 moz:param="columns=2" ... ?>

The latter probably works better for multiple-parameter passing:

<?xml-stylesheet href="mystylesheet.xsl"
                 moz:param="theme='green', columns=2, toc=true" ... ?>

Comment 1

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

seing that you would be able to pass more then one param "moz:params" would 
probably be a better name

Comment 2

[Pete Ryland]

This sounds like a nice idea, but this too is problematic, since this is
non-standard xml, and will probably break a parser that doesn't know about the
attribute.  It should be noted that passing parameters to xsl is not a bug in
mozilla as such, but rather a bug in the web standards to not provide a
mechanism for this at all.

Comment 3

[Axel Hecht]

our grand module owner granted me permission to note that HE HIMSELF favours
having a separate PI for params.

Comment 4

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

Reading the <?xml-stylesheet?> spec suggests that a complient parser should 
*not* break even if you add a parameter.

However I do like the idea of having a spearate PI for the parameters. This 
fits especially fine with multiple <?xml-stylesheet?>s in a document.

So then it's just a matter for someone to come up with a syntax for such a 
separate PI. Any suggestions? I do think we should keep the "attribute" 
approach since that's what most other PIs use.

Comment 5

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

<?xslt-params name1="value1" name2="value2"?>

is one way. However it doesn't support any other parameter-types then strings.

<?xslt-param name="name1" value="value1"?>
<?xslt-param name="name2" value="value2"?>

is another slightly more verbose way. The advantage with this is that it nicely
supports namespaced parameters

<?xslt-param name="name1" namespace="myNamespace" value="value1"?>

It could also be extended to support several datatypes either using

<?xslt-param name="name1" value="value1" datatype="string"?>
<?xslt-param name="name2" value="2" datatype="number"?>

or using

<?xslt-param name="name1" string-value="value1"?>
<?xslt-param name="name2" number-value="2"?>


yet another way is of course

<?xslt-param name="name1" value="'value1'"?>
<?xslt-param name="name2" value="2"?>

though i fear that many people would forget the inner quotes.

I can't say that i think any of these are perfect, but I think i like the
<?xslt-param name="name1" value="value1" datatype="string"?>
<?xslt-param name="name2" value="2" datatype="number"?>
way best where datatype would default to "string" and namespace to empty-string.

Comment 6

[Axel Hecht]

I don't like the datatype thing. first, conversion of types is pretty good within
XPath.
What about 
<?moz-xslt-param name="name1" select="value1"?>
which, as regular top-level params would be evaluated with the source document
as context? Calling the value select should make it obvious that folks are
dealing with an expression here.

Comment 7

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

ok, so we talked on IRC and this is what we agreed on

To set the param 'theme' (in the null namespace) to the string "blue":
<?moz-xslt-param name="theme" value="blue"?>

To set the param 'theme' in the 'myNamespace' namespace to the string "green":
<?moz-xslt-param name="theme" namespace="myNamespace" value="green"?>

To set the param 'foo' to a nodeset containing all 'book' elements:
<?moz-xslt-param name="foo" select="//book"?>

To set the param 'columns' to the number 2:
<?moz-xslt-param name="columns" select="2"?>

To set the param 'descending' to false:
<?moz-xslt-param name="descending" select="false()"?>


"name" is the name of the parameter.

"namespace" is the namespace of the parameter, it is optional and defaults to ""
which means the null namespace

"value" sets a string-value of the parameter

"select" is an expression which is evaluated with the same context as top-level
parameters. If both "select" and "value" is present then the "value" attribute
is ignored.


There were no strong oppinions on if the PI should be called "moz-xslt-param" or
"xslt-param"

Comment 8

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

Reading my own old comments i found a problem with the above proposal. The
problem is that for stuff like:

<?moz-xslt-param name="foo" select="//book"?>

you can only use the null-namespace for 'book'. What if you want to select all
books in the 'library' namespace? One solution is:

<?moz-xslt-param name="foo" select="//lib:book" xmlns:lib="library"?>

Another is:

<?moz-xslt-param-namespace prefix="lib" namespace="library"?>
<?moz-xslt-param name="foo" select="//lib:book"?>

This works extra well with multiple params like
<?moz-xslt-param-namespace prefix="lib" namespace="library"?>
<?moz-xslt-param name="books" select="//lib:book"?>
<?moz-xslt-param name="authors" select="//lib:authors"?>

Oppinions?

Comment 9

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

Attachment: Patch to fix

This should take care of it. I went with the <?xslt-param-namespace?> approach mostly because of ease for multiple parameters.

I didn't use the "moz-" prefix on the PI names because I think that we can get this adopted by other browsers if we don't include it.

Comment 10

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

I put up more details on the wiki so we can take comments there from other browser devs.

Comment 11

[Peter Van der Beken [:peterv]]

Comment on attachment 215603 [details] [diff] [review]
Patch to fix

I hear there's a new patch coming.

Comment 12

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

Attachment: address review comments

Comment 13

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

Attachment: full patch

Comment 14

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

Comment on attachment 218510 [details] [diff] [review]
address review comments

Oops, this patch didn't contain all the changed files. Full patch attached.

Comment 15

[Peter Van der Beken [:peterv]]

(In reply to comment #14)
> (From update of attachment 218510 [details] [diff] [review] [edit])
> Oops, this patch didn't contain all the changed files.

Yeah, I noticed that while reviewing it during my two-hour train trip yesterday :-).

Comment 16

[Peter Van der Beken [:peterv]]

Comment on attachment 219418 [details] [diff] [review]
full patch

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

>+CheckXSLTParamPI(nsIDOMProcessingInstruction* aPi,
>+                 nsIDocumentTransformer* aProcessor,
>+                 nsIDocument* aDocument)
>+{
>+  nsresult rv = NS_OK;

Unused, drop it.

>+    if (!prefix.IsEmpty() && !namespaceAttr.IsEmpty()) {

Do we want to allow empty namespace to undeclare the namespace?

>Index: content/xslt/src/xslt/txMozillaXSLTProcessor.cpp
>===================================================================

>+        // Set up context
>+        nsAutoPtr<txXPathNode> contextNode =
>+          txXPathNativeNode::createXPathNode(aContext);

Indent with four spaces (so add two).

>+    txVariable* var = (txVariable*)mVariables.get(varName);

NS_STATIC_CAST

Comment 17

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

> >+    if (!prefix.IsEmpty() && !namespaceAttr.IsEmpty()) {
> Do we want to allow empty namespace to undeclare the namespace?

I can't say I feel strongly either way, but I like to keep things simple and don't see a pressing need to be able to undeclare namespaces.

Comment 18

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

Attachment: Support undeclaring namespaces

Comment 19

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

Attachment: Fix review comments

Comment 20

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

Checked in on trunk and 1.8 branch