Open Bug 210460 Opened 21 years ago Updated 2 years ago

xml-stylesheet PI processing doesn't use media pseudo-attributes

Categories

(Core :: XSLT, defect)

Product:
Core
Component:
XSLT
Type:
defect

Tracking

()

Milestone:
mozilla1.5beta

People

(Reporter: jarno.elovirta, Unassigned)

References

Details

Attachments

(1 file, 2 obsolete files)

v2, don't return rv in a bool function
6.84 KB, patch
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4b) Gecko/20030515 Mozilla Firebird/0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4b) Gecko/20030515 Mozilla Firebird/0.6

When an XSLT stylesheet is linked to a document using the xml-stylesheet PI
<http://www.w3.org/TR/xml-stylesheet/> and there are multiple PIs using
different media pseudo-attributes, the correct one will not be selected. Instead
the last one is used both in rendering to the screen and to print. 


Reproducible: Always

Steps to Reproduce:
1. Create XML document with PIs
   <?xml-stylesheet type="text/xsl" href="test-print.xsl" media="print"?>
   <?xml-stylesheet type="text/xsl" href="test-screen.xsl" media="screen"?>
2. Create corresponding XSLT documents.
3. Open XML document and view in Print Preview or print page

Actual Results:  
The document is transformed with stylesheet for "screen" for both normal viewing
and Print Preview/print.

Expected Results:  
The stylesheet for "print" should have been used for Print Preview/print.
confirming. Asking stylesystem and printing default owners for help.
I have no idea how CSS does that, it'd really help to know. And, how to we get
the right media in the first place, my recent search in IDLs didn't turn up 
anything obvious.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: helpwanted
nsIPresContext::GetMedium

(See also CSSRuleProcessor::GetRuleCascade for how CSS does this.)
Ok, printing works by going over the DOM and printing the *current state* of it,
using the CSS rules for media="print".
As there is no relation between the DOM that loads the XSLT stylesheet and the
DOM that gets printed, media="print" does not make sense for XSLT stylesheets.
We would have to retransform the document for printing, which would cause stuff
like data entered into textfields disappear.
Nevertheless, we probably shouldn't apply XSLT stylesheet with media="print", or
no stylesheet with any media. Or just warn to the js console if we get media,
saying that we don't.
I'd favour warning to the js-console if media is set, and not load anything but
media="foo" containing screen.
OS: Windows 2000 → All
Hardware: PC → All
taking, patch coming up
Assignee: peterv → axel
triage
Status: NEW → ASSIGNED
Keywords: helpwanted
Target Milestone: --- → mozilla1.5beta
Comment on attachment 127440 [details] [diff] [review]
just load for media="foo, screen, bar", warn to the console

requesting feedback from Heikki, this is XML after all.
Attachment #127440 - Flags: review?(heikki)
Comment on attachment 127440 [details] [diff] [review]
just load for media="foo, screen, bar", warn to the console

Sorry it took me such a long time to get to this...

I don't especially like addingt the intl dependency there (even though it
strictly does not matter since content already introduces that). This patch
also adds quite a bit of XSLT specific code to the sink (again, not a big
things since there already is XSLT stuff there). However, could you consider
moving this code to the XSLT module?

Also I am a little sceptical about media check. Are you sure there won't be any
other whitespace characters?
> I don't especially like addingt the intl dependency there (even though it
> strictly does not matter since content already introduces that).

This is to give some localizable user feedback. Note that localisation in XML
handling is supposed to improve, like getting localisation for the XML pretty
print blurb.

> This patch
> also adds quite a bit of XSLT specific code to the sink (again, not a big
> things since there already is XSLT stuff there). However, could you consider
> moving this code to the XSLT module?

The XSLT code has no concept of the underlying PI, so that stuff should go into
the content sink. I'd say that we shouldn't try to load the XSLT module, if
we do not intend to use, too.

I could add more whitespace to the logic, though.
Comment on attachment 127440 [details] [diff] [review]
just load for media="foo, screen, bar", warn to the console

Maybe reverse the logic, and instead of whitespace, you could test for
alphanumeric character instead (or whatever character can appear as part of
media name)?
Comment on attachment 127440 [details] [diff] [review]
just load for media="foo, screen, bar", warn to the console

new patch in the works, with media parsing much closer to what
CSSMedialListImpl does.
Attachment #127440 - Attachment is obsolete: true
Attachment #127440 - Flags: review?(hjtoi-bugzilla)
I changed the media check to mimic what DOMMediaListImpl::SetText does down in
nsCSSStyleSheet.cpp, and moved the code into a separate function. Early returns

make that code easier to read.
Attachment #136425 - Flags: review?(hjtoi-bugzilla)
Comment on attachment 136425 [details] [diff] [review]
mimic what DOMMediaListImpl::SetText does

>Index: document/src/jar.mn
>===================================================================
>+en-US.jar:
>+	locale/en-US/global/xml.properties

I don't know enough about jar files to say if this if correct or not...

>Index: document/src/nsXMLContentSink.cpp
>===================================================================
>+// media don't make sense for XSLT stylesheets, see bug 210460

"doesn't"

>+  nsAString::const_iterator start, iter, end;
>+  start = aMedia.BeginReading(iter);
>+  aMedia.EndReading(end);
>+  nsAutoString tmp;
>+  PRBool loadXSLT = PR_TRUE;
>+  do {
>+    if (FindCharInReadable(',', iter, end)) {
>+      tmp = Substring(start, iter);
>+      ++iter; // skip ','
>+    }
>+    else {
>+      tmp = Substring(start, iter);
>+    }
>+    tmp.CompressWhitespace();
>+    if (!tmp.IsEmpty()) {
>+      loadXSLT = PR_FALSE;
>+      if (tmp.Equals(all) || tmp.Equals(screen)) {
>+        return PR_TRUE;
>+      }
>+    }
>+    start = iter;
>+  } while (start != end);

What if media ended in a comma, wouldn't iter then equal end, and you would
advance iter beyond end, therebody ending up in an endless loop?

>Index: document/src/nsXMLContentSink.h
>===================================================================
>+  PRPackedBool mDidWarnXSLTMedia;

Somebody should find the time to make these into bits and stuff them into a
single 32-bit unsigned. There is already more than 4 so you are not making this
any bigger, so I leave it up to you. In any case, this is a really minor issue.

So, if can fix the iter loop (or show that it will always terminate as is) you
have r=heikki.
Attachment #136425 - Flags: review?(hjtoi-bugzilla) → review+
arn't you using |start| before giving it a value?
>Index: document/src/nsXMLContentSink.cpp
>>===================================================================
>>+// media don't make sense for XSLT stylesheets, see bug 210460

> "doesn't"

media is plural, so in this context I'd like to keep it as "don't". As I don't
talk about the attribute in particular.

> What if media ended in a comma, wouldn't iter then equal end, and you would
> advance iter beyond end, therebody ending up in an endless loop?
iter points to the ',', end points to the end. So this is allright, and I 
tested it, too.

I did a new attachement, because I had 
return rv;
inside a bool function. That's stupid. I forked the message stuff into a 
separate void function, so that I can use early returns for failures. I choose
to ignore failures in there, as it is only a convenience thing and it shouldn't

change anything in the code path.
Attachment #136425 - Attachment is obsolete: true
Comment on attachment 139185 [details] [diff] [review]
v2, don't return rv in a bool function

requesting new review
Attachment #139185 - Flags: review?(hjtoi-bugzilla)
Comment on attachment 139185 [details] [diff] [review]
v2, don't return rv in a bool function

Sorry that I have neglected this, maybe you should get a review from someone
who has more time...

But one word about packaging: since the xml.properties contains only one item
and it is XSLT specific, I would advice putting it in the XSLT extension
instead.
Attachment #139185 - Flags: review?(hjtoi-bugzilla)
This issue is important not just for media="print" vs. media="screen". I have an XML file which I transform into XHTML using stylesheetA.xsl. I also have stylesheetB.xsl which transforms my XML file into an Excel XML document.
 
As both Excel and most new browsers have a built in XSTL engine, I decided to add the stylesheet information to the original XML file. Like this: 

<?xml version="1.0" encoding="utf-8" standalone="yes"?> 
<?xml-stylesheet href="stylesheetB.xsl"  type="text/xsl" media="screen"?> 
<?xml-stylesheet href="stylesheetA.xsl" type="text/xsl" media="excel"?> 
<blahblah> 
... etc ... 
</blahblah> 

When I open the XML file with Excel, it asks me which of the two 
stylesheets to use, which is great. Internet Explorer and Safari grab the first stylesheet which is OK too. Firefox 1.5.0.1 selects the wrong stylesheet (stylesheetA.xls) without asking. It always seems to choose the second one.

So, if I am to embbed the stylesheet information, I need to do it one 
way for Firefox, and in a different way for Internet Explorer and 
Safari. 

It seems to me that Firefox should use the first stylesheet, like Intenet Explorer and 
Safari. Or even better use the stylesheet with the media="screen" attribute.
Yeah, we should probably only use ones where we recognize the media as one we want to use for screen.
*** Bug 338107 has been marked as a duplicate of this bug. ***
(In reply to comment #3)
> [...]
> Nevertheless, we probably shouldn't apply XSLT stylesheet with media="print",
> or
> no stylesheet with any media. Or just warn to the js console if we get media,
> saying that we don't.
> I'd favour warning to the js-console if media is set, and not load anything but
> media="foo" containing screen.
> 

I don't completely agree with that. Your statement implies that CSS styles never affect the DOM structure of a document, whereas XSLT transformations might well pump out a completely different DOM structure that that of the transformed document.
This is, however, only partly true. Even in CSS, I can logically add text nodes by use of the "content:" property.

Furthermore, I don't see a problem in retransforming the XML for printing. Due to the dual-staged nature of most XML-styling strategies used with  browser-based document viewing (stage1: XML -> XSLT -> HTML, stage2: HTML -> CSS --> Screen), there is no problem letting the author decide whether to branch the styles for screen and print at the XSLT level, thus loosing e.g. the contents of input fields, or at the CSS level, simply changing minor appearance aspects.
Sophisticated navigation menus, for example, just don't make sense on print media, but might be needed on screen.
IMHO it would be a great improvement to make Firefox interpret the "media" pseudo attribute correctly even for XSL stylesheets, with the given limitation that data added to the document after the XML-to-screen transformation will be lost in the printout.
I’m going to tack this on here as it seems to be related;  if you’d rather I raise a separate report then I can.

The xml-stylesheet “alternate” pseudo-attribute seems not to be honoured;  I’d expect alternate XML stylesheets to appear in the View/Page-Style [Firefox] menu like alternate CSS styelsheets:

<?xml-stylesheet type="text/xsl" href="test2.xsl" alternate="yes" title="Other"?>
<?xml-stylesheet type="text/xsl" href="test.xsl"?>
QA Contact: keith → xslt
Assignee: axel → nobody
Status: ASSIGNED → NEW
Bug 210460 is similar. The differences:
1. Media type (text/css)
2. Actual result (CSS stylesheet for printing is completely ignored)
I meant bug 314622...
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: