175074 - URN:ISBN link converted to URN://ISBN

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Mario Mueller]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.2b) Gecko/20021016
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.2b) Gecko/20021016

Mozilla can't handle URNs with ISBNs defined in RFC 3187. Here is an example:

<blockquote cite="URN:ISBN:3-89432-379-5" title="Die Schweinswale"><p>Some text
cite form a book.</blockquote>

When I click right on the citation and choose Properties, Mozilla shows
following text as URN:

Info: urn://isbn:3/


Reproducible: Always

Steps to Reproduce:
1. Go to the example page
2. Click right on the upper citation in the gray box beginning with "Manche
dieser Bezeichnungen..."
3. Choose Properties

Actual Results:  
Info: urn://isbn:3/

Expected Results:  
Info: ISBN: 3-89432-379-5

Comment 1

[benc]

fixed summary for readability, set to default owners.

dsirnapalli: please use the "Reassign bug to owner and QA contact of selected
component" selection. You cleared the QA field somehow.

 

Comment 2

[dsirnapalli]

-- Reassiging but to owner and QA contact of selected component.

Comment 3

[benc]

per bug 181135: all bugs owned by previous default owner that were not futured
are being reassigned to default owner.

Comment 4

[benc]

+clean report,
no found dupes, although bug 178133 might block.

Comment 5

[Robin Lionheart]

Setting all platforms, enhancement.

Comment 6

[Boris Zbarsky [:bzbarsky]]

This worksforme in a current build.

Comment 7

[Boris Zbarsky [:bzbarsky]]

biesi pointed out why this worked in a testcase but not on that page...

The bug is in the getAbsoluteURL function in metadata.js (both versions).  It
shouldn't be setting .spec, but rather assigning to the URI object altogether. 
See what getAbsoluteURL does in pageInfo.js.

Comment 8

[Boris Zbarsky [:bzbarsky]]

I don't understand this.  Getting the absolute URL from a node and a URI string
should be a two-step operation:

1)  Get the base URI for the node (DOM3 gives us this)
2)  newURI on the IOService.

so all 4 impls of getAbsoluteURL we have in the tree are way more complicated
than they need to be, and wrong.

Comment 9

[[e-mail]]

Remember that we didn't have those handy DOM3 methods when page info was first
written. Over to db48x so he can decide what to do, because he was switching
over to using properties which now return absolute urls anyway.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Again, page info gets this right.  It's item properties that's broken...

Comment 11

[[e-mail]]

Ah, I keep confusing the two... anyway, I hope db48x can fix metadata.js in the
same way as pageInfo.js where he is removing getAbsoluteURL altogether.

Comment 12

[Christian Schmidt]

Attachment: Fix

This patch gets rid of getAbsoluteURL. Now that bug 72524 has been fixed,
blockquote.cite, a.href etc. all return absolute URIs. So only the XLink part
needs special care. I have added the additional try/catch as mentioned over in
bug 242924.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 177656 [details] [diff] [review]
Fix

The patch looks fine, but I'm not a peer for browser/ code (and note that we
really do want this fixed in the xpfe version of metaData.js as well, given
that that's what the bug is filed on).

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 177656 [details] [diff] [review]
Fix

+	 var baseURI = ioService.newURI(elem.baseURI, null, null);
+	 url = ioService.newURI(href, null, baseURI).spec;

please pass a character set to newURI (second argument); for example
elem.ownerDocument.characterSet

Comment 15

[Christian Schmidt]

Attachment: Updated patch that addresses comment 14

I added elem.ownerDocument.characterSet to both calls to newURI (I hope that
makes sense).

Patch for xpfe coming up.

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #15)
> I added elem.ownerDocument.characterSet to both calls to newURI (I hope that
> makes sense).

yes, thanks!

Comment 17

[Christian Schmidt]

Attachment: Identical patch for xpfe

Comment 18

[:deanis74]

URIs are not a strongpoint of mine.  I don't feel comfortable giving a review.

Comment 19

[[e-mail]]

Comment on attachment 177780 [details] [diff] [review]
Identical patch for xpfe

db48x would probably like to look at this.

I wouldn't have bothered with separate url and href variables.

Comment 20

[Daniel Brooks [:db48x]]

Comment on attachment 177780 [details] [diff] [review]
Identical patch for xpfe

Yea, it looks good. I also would recommend using a single variable instead of
both url and href, but that's not really a very big deal

Comment 21

[Christian Schmidt]

Attachment: Updated patch for Firefox that uses only one variable

Comment 22

[Christian Schmidt]

Attachment: Updated patch for xpfe that uses only one variable [Checked in: Comment 29]

I don't have check-in priviledges, so I need somebody to help me get these in,
when they pass reviews.

(I assume that I don't need new review from db48x)

Comment 23

[[e-mail]]

Comment on attachment 177846 [details] [diff] [review]
Updated patch for xpfe that uses only one variable
[Checked in: Comment 29]

I'll check this in.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 177845 [details] [diff] [review]
Updated patch for Firefox that uses only one variable

This code doesn't need sr, and I wouldn't be able to get to this for at least
several weeks anyway... so if you do want an sr, please pick someone else.

Comment 25

[Christian Schmidt]

I don't want a super-review, if it isn't necessary.

Where should I look if I want to find out whether I should seek super-review for
a patch or not? I looked at <http://www.mozilla.org/hacking/reviewers.html>, but
that didn't make it clear to me why I don't need a super-review for this patch.

Comment 26

[Boris Zbarsky [:bzbarsky]]

It's the module owner's call, basically...  I'm not sure that the /browser
module review policy is actually coherently written down anywhere on the website.

Comment 27

[Marc Schumann [:Wurblzap]]

*** Bug 288416 has been marked as a duplicate of this bug. ***

Comment 28

[[e-mail]]

*** Bug 242924 has been marked as a duplicate of this bug. ***

Comment 29

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 177846 [details] [diff] [review]
Updated patch for xpfe that uses only one variable
[Checked in: Comment 29]

the xpfe patch was checked in 2005-03-18 02:38

Comment 30

[Hermann Schwab]

Steps to Reproduce from comment 0 are wfm on Seamonkey, as a fix has been checked in.
They are not working however on Firefox, checkin there is still needed, but nobody seems to notice, as this is filed as Seamonkey bug.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Does the Firefox patch even still apply?

Comment 32

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #31)
> Does the Firefox patch even still apply?

Yes, it does. I checked it in on the trunk. Hermann, if you could verify that'd be great.

mozilla/browser/base/content/metaData.js; new revision: 1.8;

Comment 33

[Hermann Schwab]

verifying
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20060212 Firefox/1.6a1
nightly: urn://isbn:3/
tinderbox-build with fix: urn:ISBN:3-89432-379-5

Comment 34

[:Gavin Sharp [email: [e-mail]]]

Thanks Hermann. I checked this in on the 1.8 branch as well.
mozilla/browser/base/content/metaData.js; new revision: 1.6.10.1;

Comment 35

[Hermann Schwab]

verifying
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8) Gecko/20060212 Firefox/1.5
urn:ISBN:3-89432-379-5

tested using tinderbox build:
WINNT 5.2 pacifica Depend Fx-Release
Started 21:55, finished 22:29

WindowsExplorer Properties gives wrong BuildID: Bug 305233
1.8: 2006021203
12. Februar 2006 22:29:12