Closed Bug 194387 Opened 18 years ago Closed 15 years ago

support parameter-passing in PI transformations

Categories

(Core :: XSLT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: sicking, Assigned: sicking)

References

()

Details

(Keywords: fixed1.8.1)

Attachments

(1 file, 4 obsolete files)

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" ... ?>
seing that you would be able to pass more then one param "moz:params" would 
probably be a better name
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.
our grand module owner granted me permission to note that HE HIMSELF favours
having a separate PI for params.
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.
<?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.
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.
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"

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?
Attached patch Patch to fix (obsolete) — Splinter Review
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.
Attachment #215603 - Flags: superreview?(peterv)
Attachment #215603 - Flags: review?(peterv)
I put up more details on the wiki so we can take comments there from other browser devs.
Comment on attachment 215603 [details] [diff] [review]
Patch to fix

I hear there's a new patch coming.
Attachment #215603 - Flags: superreview?(peterv)
Attachment #215603 - Flags: review?(peterv)
Attachment #215603 - Flags: review-
Attached patch address review comments (obsolete) — Splinter Review
Assignee: peterv → bugmail
Attachment #215603 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #218510 - Flags: superreview?(peterv)
Attachment #218510 - Flags: review?(peterv)
Attached patch full patch (obsolete) — Splinter Review
Attachment #219418 - Flags: superreview?
Attachment #219418 - Flags: review?(peterv)
Attachment #219418 - Flags: superreview? → superreview?(peterv)
Comment on attachment 218510 [details] [diff] [review]
address review comments

Oops, this patch didn't contain all the changed files. Full patch attached.
Attachment #218510 - Attachment is obsolete: true
Attachment #218510 - Flags: superreview?(peterv)
Attachment #218510 - Flags: review?(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 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
Attachment #219418 - Flags: superreview?(peterv)
Attachment #219418 - Flags: superreview+
Attachment #219418 - Flags: review?(peterv)
Attachment #219418 - Flags: review+
> >+    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.
Attached patch Support undeclaring namespaces (obsolete) — Splinter Review
Attachment #219418 - Attachment is obsolete: true
Attachment #222587 - Flags: superreview?(peterv)
Attachment #222587 - Flags: review?(peterv)
Attachment #222587 - Flags: approval-branch-1.8.1?(peterv)
Attachment #222587 - Attachment is obsolete: true
Attachment #222603 - Flags: superreview?(peterv)
Attachment #222603 - Flags: review?(peterv)
Attachment #222587 - Flags: superreview?(peterv)
Attachment #222587 - Flags: review?(peterv)
Attachment #222587 - Flags: approval-branch-1.8.1?(peterv)
Attachment #222603 - Flags: superreview?(peterv)
Attachment #222603 - Flags: superreview+
Attachment #222603 - Flags: review?(peterv)
Attachment #222603 - Flags: review+
Attachment #222603 - Flags: approval-branch-1.8.1+
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Checked in on trunk and 1.8 branch
You need to log in before you can comment on or make changes to this bug.