Closed Bug 347481 Opened 19 years ago Closed 18 years ago

Microsummaries are not picked up when the page is served/parsed as XML

Categories

(Firefox Graveyard :: Microsummaries, defect)

Product:
Firefox Graveyard
Component:
Microsummaries
Version:
2.0 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 2

People

(Reporter: rflint, Assigned: Dolske)

Details

(Keywords: fixed1.8.1)

Attachments

(1 file)

Simple patch
1.15 KB, patch
myk
: review+
mtschrep
: approval1.8.1+
Details | Diff | Splinter Review
When serving a documents using an XML dialect as the MIME type (e.g. application/xml, application/xhtml+xml, etc), available microsummaries are not shown. Myk suspects two things may be going on here: 1) We're not checking for the microsummary processing instruction. 2) We're not checking for the link tag because we're treating the file as XML. The following documents use the share the same content and microsummary, but are served with different MIME types. Served as text/html: http://www.screwedbydesign.com/mozilla/testcases/microsummaries/html.php Served as application/xhtml+xml: http://www.screwedbydesign.com/mozilla/testcases/microsummaries/xhtml.php
The problem is that when MicrosummarySet::extractFromPage() calls the DOM getElementsByTagName("LINK") function, it's not getting any elements back. If I call this function in DOMi from the top 'html' element instead of '#document', I get 1 instead of zero. Specifically, it's a case-sensitivity issue. If you ask for 'link' elements instead of 'LINK' elements, it works fine. XML is case sensitive, and XHTML requires tags to be lower case. So, changing to code to look for 'link' should be good, although I wonder if we should additionally be using getElementsByTagNameNS with the XHTML NS to be especially selective (to avoid other XML 'link' tags?), although if there are other XML namespaces microsummaries needs to work in that might be too restrictive.
Status: NEW → ASSIGNED
Assignee: nobody → dolske
Status: ASSIGNED → NEW
Attached patch Simple patch — — Splinter Review
This fixes the reported problem. However, the generator is producing a blank microsummary. :-( I've verified that we're feeding the same info into _processTemplate() as we do in the HTML case, and can get static text if I put it into the XSLT template. No errors or messages logged. So, I kinda suspect that the generator itself is wrong, but my XPath-fu is weak -- does it treat xhtml differently?
Attachment #237153 - Flags: review?(myk)
Comment on attachment 237153 [details] [diff] [review] Simple patch > XML is case sensitive, and XHTML requires tags to be lower case. So, changing > to code to look for 'link' should be good Yup, looks like it works fine. > although I wonder if we should additionally be using getElementsByTagNameNS > with the XHTML NS to be especially selective (to avoid other XML 'link' > tags?) We probably should. Besides the problem of avoiding other XML 'link' tags, there's also the issue of finding XHTML 'link' tags specified with an explicit namespace prefix (i.e. not in the default namespace), for example: <html xhtml:foo="http://www.w3.org/1999/xhtml"> <foo:link ... /> </html> Currently we don't handle such tags, even with this fix, because our getElementsByTagName call only retrieves 'link' tags in the default namespace. But we should take this fix in the meantime, since it fixes a common case and is very low-risk. > although if there are other XML namespaces microsummaries needs to work in that might be too > restrictive. It should be possible for any XML document to provide a microsummary. But for non-XHTML XML documents, it probably makes more sense to define an XML processing instruction: <?microsummary type="application/x.microsummary+xml" href="microsummary.xml"?> Alternately, we could define a tag in the microsummaries namespace that XML documents could include: <microsummary xmlns="http://www.mozilla.org/microsummaries/0.1" href="microsummary.xml" /> I suspect that the processing instruction is the better solution. > However, the generator is producing a blank microsummary. :-( I've verified > that we're feeding the same info into _processTemplate() as we do in the HTML > case, and can get static text if I put it into the XSLT template. No errors > or messages logged. So, I kinda suspect that the generator itself is wrong, > but my XPath-fu is weak -- does it treat xhtml differently? Indeed, the problem is in the generator. It uses an XPath expression containing selectors that have no explicit namespace, but in XSLT 1.0 such selectors will only match tags without a namespace (they won't match tags in the default namespace, although that may change in XSLT 2.0), and the tags in the XHTML version of the document are all namespaced. For the generator to work with the XHTML document, it has to specify the namespace of the tags, i.e. instead of: <transform xmlns="http://www.w3.org/1999/XSL/Transform" version="1.0"> <output method="text"/> <template match="/"> <value-of select="/html/body/span[@id='pickup-text']/text()"/> </template> </transform> the generator must do something like: <transform xmlns="http://www.w3.org/1999/XSL/Transform" version="1.0" xmlns:xhtml="http://www.w3.org/1999/xhtml"> <output method="text"/> <template match="/"> <value-of select="/xhtml:html/xhtml:body/xhtml:span[@id='pickup-text']/text()"/> </template> </transform> or, if it wants to work with both the HTML and the XHTML versions of the document, something like: <transform xmlns="http://www.w3.org/1999/XSL/Transform" version="1.0" xmlns:xhtml="http://www.w3.org/1999/xhtml"> <output method="text"/> <template match="/"> <value-of select="/html/body/span[@id='pickup-text']/text()"/> <value-of select="/xhtml:html/xhtml:body/xhtml:span[@id='pickup-text']/text()"/> </template> </transform> Alternately, it could use the selector *[local-name()='foo'] to match a tag named foo in any namespace. But that'll get unwieldly quickly.
Attachment #237153 - Flags: review?(myk) → review+
Fix checked in to the trunk. We should take this on the branch as well.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment on attachment 237153 [details] [diff] [review] Simple patch Notes for drivers considering this approval request: This patch improves microsummary support for XHTML by looking for lowercase "link" tags specifying microsummaries. Although it doesn't fix all cases in which XHTML specifies microsummaries, it fixes a common case with very little risk of regression. It was checked into the trunk on Monday, September 11.
Attachment #237153 - Flags: approval1.8.1?
Comment on attachment 237153 [details] [diff] [review] Simple patch a=schrep for drivers.
Attachment #237153 - Flags: approval1.8.1? → approval1.8.1+
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: