316184 - Long URIs break External Protocol Request dialog box

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Tom Graves]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051107 Firefox/1.5
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051107 Firefox/1.5

If a link to a song in iTunes from an iTunes partner is passed through Firefox 1.5rc2, the External Protocol dialog box asking if you want to launch an external application pops up, but it is broken. The box, due to the long url, is the width of my widescreen PowerBook. The Launch Application and Cancel Buttons are missing, although the remember my choice checkoff is present. This dialog box is not broken in Camino, Mozilla, or Netscape.

Reproducible: Always

Steps to Reproduce:
1. For demonstration purposes, get Mobster for OS X 10.4.x (http://www.musicmobs.com/mobster).
2. Download any playlist from Musicmobs (http://www.musicmobs.com/mobster). The file will be in xspf format.
3. Open the xspf file in Mobster and double-click on any entry with a red dot.
4. If Firefox is the default browser, you should see the behavior as described in Details.

Actual Results:  
A broken External Protocol dialog box appeared with no Launch Application or Cancel buttons.

Expected Results:  
A normal External Protocol dialog box appeared with Launch Application and Cancel buttons.

PowerBook 1 Ghx, 1 gb Ram.
OS X 10.4.3.
No themes.
Following extensions installed: Adblock, All-in-One Sidebar, BBCode, DOM Inspector, FoxyTunes, Tabbrowser Preferences, Talkback.

Comment 1

[Tom Graves]

Attachment: Screenshot of broken External Protocol Request dialog box

Comment 2

[Reed Loden [:reed]]

*** This bug has been marked as a duplicate of 260292 ***

Comment 3

[Reed Loden [:reed]]

Actually, no. This is INVALID as that is the way the External Protocol Request dialog box works now.

Comment 4

[Reed Loden [:reed]]

well, I forgot about WONTFIX. Using it instead of INVALID.

Comment 5

[Reed Loden [:reed]]

I didn't know that only module owners set WONTFIX. Sorry!
Reopening to DUPE it back against 260292.

Comment 6

[Reed Loden [:reed]]

*** This bug has been marked as a duplicate of 260292 ***

Comment 7

[Asaf Romano (gone)]

Neither a dupe, nor a wontfix (at least on mac).

Comment 8

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

why is this not a duplicate?

Comment 9

[Asaf Romano (gone)]

Bug 260292 seems to report the opposite behavior.

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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

Comment 11

[Eric Bailey]

This bug is reproducible for a lot of users of the iTunes U service.  I have been able to reproduce it using Firefox 1.5.0.4 and Firefox 1.5.0.6.

Build identifier:
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4

This bug is also effected by the preferences network.protocol-handler.external and network.protocol-handler.warn-external .  If a user has already clicked on an itms:// or itmss:// URL, seen this panel, checked the checkbox for "Remember my choice for all links of this type." then they will no longer be presented this panel.

Comment 12

[Marcia Knous [:marcia]]

I am able to reproduce this quite easily using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1b2) Gecko/20060818 BonEcho/2.0b2. Screenshot coming.

I followed these steps, which are similar to the reporter's:

Steps to Reproduce:
1. For demonstration purposes, get Mobster for OS X 10.4.x
(http://www.musicmobs.com/mobster). You will need to create an account first.
2. Login to Mobster with your user name and password.
2. Download any playlist from Musicmobs (http://www.musicmobs.com/mobster). The
file will be in xspf format.
3. Open the xspf file in Mobster and double-click on any entry with a red dot.
4. If Firefox is the default browser, you should see the behavior as described
in Details.

Comment 13

[Marcia Knous [:marcia]]

Attachment: Screenshot of external protocol window using Bon Echo Nightly build

Comment 14

[Marcia Knous [:marcia]]

nominating. Considering the popularity of this type of operation, I think this should work when we ship FF 2.0.

Comment 15

[Josh Aas]

I tried fixing this by only posting the host (e.g. "itms://phobos.apple.com/") but that doesn't work because nsIURI won't give us a host for itms URIs. Which brings us to a more general issue of what to do if there isn't a host (a chrome or file URI?).

If we have to fix this in a more complicated way - say by putting the URI in a multiline text box, then we can't use the common dialog code any more and we need to design a special dialog for this.

Comment 16

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #15)
> I tried fixing this by only posting the host (e.g. "itms://phobos.apple.com/")
> but that doesn't work because nsIURI won't give us a host for itms URIs. Which
> brings us to a more general issue of what to do if there isn't a host (a 
> chrome or file URI?).

Could we perhaps trim the URI spec if it's greater than some specified length (f.e. 80 characters), inserting an ellipsis to signify the trimmed characters?

Comment 17

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch that replaces the middle of the URL with an ellipsis

Here's a simple patch that replaces the middle of the URL with an ellipsis surrounded by spaces if the URL is more than 100 characters long.  I added spaces around the ellipsis to make it clear that the ellipsis replaces a portion of the URL rather than being part of the URL itself.

Note that the dialog actually wraps the URL at the ellipsis, probably because of the spaces around it.  So another, potentially better solution to this problem is to insert zero-width spaces into the URL every 50-100 characters, so the dialog can break the URL across multiple lines, and we don't have to remove a portion of the URL when displaying the dialog.

Comment 18

[Myk Melez [:myk] [@mykmelez]]

Attachment: inserts zero-width space every 75 characters

Here's a better fix that inserts a zero-width space every 75 characters so the dialog breaks the URL at those points.  It doesn't make long URLs look especially pretty, but it does ensure that the dialog remains usable when displaying a particularly long URL.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 236148 [details] [diff] [review]
inserts zero-width space every 75 characters

This would break if the URI contains non-plane-0 characters (which are encoded as pairs of PRUnichars).

Also, once the URI wraps, don't we run into the same problem but with the buttons being too low (and hence outside the dialog)?  I seem to recall this happening.

Finally, this would really not work very well in other consumers of this code (e.g. embedding clients, where the zero-width spaces are likely to do weird things, imo).

I would prefer that the fixes for this, if any, are in the dialog code itself, not in the back end here.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Hmm... Although the problem is that this is not the helper app dialog, just a prompt with ConfirmEx...  that kinda sucks.  :(  The problem remains where a long URI will just give you a window that's so tall the buttons are off screen.  I we're willing to live with that, I guess something like this is OK as wallpaper if it's fixed to deal with non-plane-0 chars.  And we should file a followup bug for a better interface for this prompt or something.

Comment 21

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

a nicer fix would probably an appropriately placed crop=middle in the XUL file, if we could do that...

Comment 22

[Boris Zbarsky [:bzbarsky]]

It's a standard prompt, is the problem....

Comment 23

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

it doesn't have to be :)

Comment 24

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #23)
> it doesn't have to be :)

Indeed.  Unfortunately, the description element doesn't crop multiline content, so we couldn't stick the entire text into a single string, as we're currently doing, and pass it to a custom dialog.  We'd instead have to pass the URL, and the strings before and after it, separately, which is a string change we can't take on the 1.8 branch.

So, as far as I can tell, our two options (for the branch, anyway) are to fix the description element or wrap/crop the text ourselves.  Not sure which is smaller and lower-risk, but the latter seems like it, at first glance anyway.

Comment 25

[Myk Melez [:myk] [@mykmelez]]

Attachment: testcase: page with links to long URLs

Here's a page with some links to long URLs that you can use to test the patch I'm about to attach.  Note: to test on Linux, add the following (where "/bin/sh" can be any existing program as long as you don't actually click the "Launch Application" button in the dialog) to prefs.js:

user_pref("network.protocol-handler.app.itms", "/bin/sh");

Comment 26

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: accommodates plane 1 chars and crops to prevent overtall dialog

Here's a patch that takes plane 1 characters into account and crops very long URLs to prevent overtall dialogs whose buttons spill off the bottom of the page.

To accommodate plane 1 characters, I check for their presence before inserting or removing text and adjust the insertion/crop point accordingly.  Note that such characters appear to be URL-escaped before they ever reach this function, so these checks aren't strictly necessary.  But better to be safe than sorry.

To handle overtall dialogs, I define a maximum height (in lines) for the URL in addition to the maximum width and then crop it to fit that height.

I also modify whitespace in the externalProtocolPrompt property to make the dialog clearer.  According to beltzner, whitespace changes aren't l10n changes, so it's possible to take this on the branch during the l10n freeze.

I'll be the first to admit that this isn't the ideal solution.  But it's the only fix I can think of that doesn't require l10n changes or XUL platform hacking, and since this is a Firefox 2 blocker, it seems worth taking this fix.

Comment 27

[Myk Melez [:myk] [@mykmelez]]

Attachment: screenshot: short URL w/fix applied

Comment 28

[Myk Melez [:myk] [@mykmelez]]

Attachment: screenshot: medium URL w/fix applied

Comment 29

[Myk Melez [:myk] [@mykmelez]]

Attachment: screenshot: long URL w/fix applied

Comment 30

[Myk Melez [:myk] [@mykmelez]]

There's no "uireview" flag in this component, but this bug has UI impact.  Mike, can you take a look at the post-fix screenshots to see if this meets your expectations?

attachment 236860 [details] - a short URL (no wrapping or cropping)
attachment 236861 [details] - a medium-length URL (wrapping, no cropping)
attachment 236862 [details] - a long URL (both wrapping and cropping)

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 236859 [details] [diff] [review]
patch v3: accommodates plane 1 chars and crops to prevent overtall dialog

>Index: uriloader/exthandler/nsExternalHelperAppService.cpp

>+        const nsString ellipsis = NS_LITERAL_STRING("\n...\n");

  NS_NAMED_LITERAL_STRING(ellipsis, "\n...\n");

>+        uri.Insert(PRUnichar(0x200B), charIdx);

Add a comment that 0x200B is zero-width breakable space?

r=bzbarsky with those nits.  Thanks!

Comment 32

[Myk Melez [:myk] [@mykmelez]]

Patch checked in to the trunk with the two nits from comment 31 fixed.

Comment 33

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v4: the version checked into the trunk

This is the patch that was checked into the trunk.  It's the same as patch v3 (attachment 236859 [details] [diff] [review]), which has review and super-review from bzbarsky, but with his nits from comment 31 fixed.

Notes for drivers considering this approval request:

This patch wraps and crops long URLs in the "External Protocol Request" dialog to prevent them from causing the dialog to render buttons offscreen (and thus break the dialog).

It includes some code to do the actual wrapping/cropping (see screenshots listed in comment 30; testcases are in attachment 236857 [details]) as well as some whitespace-only improvements to the "externalProtocolPrompt" locale properties, which beltzner says aren't string changes.

It has a low risk of regression and was checked into the trunk on Monday, September 11.

Comment 34

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 237732 [details] [diff] [review]
patch v4: the version checked into the trunk

a=beltzner on behalf of 181drivers