389465 - Places should decode URLs

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P1)

Description

[Emil Ivanov]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.5) Gecko/20070713 Firefox/2.0.0.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.5) Gecko/20070713 Firefox/2.0.0.5

When the url is copied and pasted somewhere else its not encoded.
The autocomplete on location bar is not encoded.
When adding a bookmark the url is not encoded.

I think only codes like space as %20 should be shown in these cases.


Reproducible: Always

Steps to Reproduce:
1.Open the url
2.Delete the part after /wiki/
3.Look the autocomplete on the url
Actual Results:  
http://bg.wikipedia.org/wiki/%D0%9D%D0%B0%D1%87%D0%B0%D0%BB%D0%BD%D0%B0_%D1%81%D1%82%D1%80%D0%B0%D0%BD%D0%B8%D1%86%D0%B0

Expected Results:  
http://bg.wikipedia.org/wiki/Начална_страница

Comment 1

[Jesse Ruderman]

> When the url is copied and pasted somewhere else its not encoded.

There's a tradeoff for copying/pasting (see bug 105909 comment 43).  Decoded URLs are more readable, but aren't clickable in many IRC clients.  They also don't work as well for href attributes.  I think they're not even really URLs.

> The autocomplete on location bar is not encoded.
> When adding a bookmark the url is not encoded.

I agree that the URL should be displayed decoded in those places, especially now that they're displayed decoded in the URL bar (on trunk).

Comment 2

[Dão Gottwald [:dao]]

(In reply to comment #1)
> > When the url is copied and pasted somewhere else its not encoded.
> 
> There's a tradeoff for copying/pasting (see bug 105909 comment 43).  Decoded
> URLs are more readable, but aren't clickable in many IRC clients.

Not only IRC clients. It's a problem for all kinds of software -- comment 0 is a great example.

Changing the component and the summary, as the copy/paste-thingy is INVALID and the rest doesn't apply to the autocomplete menu only.

Comment 4

[Erwan Loisant]

Attachment: Unescape URIs in the URL bar search results

Just calling decodeURI before displaying the history/bookmark URI in the search results.

Comment 5

[Dietrich Ayala (:dietrich)]

Hey Erwan, I'm not a full toolkit peer, just Places. From looking at the module owners list (http://www.mozilla.org/owners.html), I'd say ask Gavin or Mano for review.

Comment 6

[Dão Gottwald [:dao]]

(In reply to comment #4)
> Created an attachment (id=298626) [details]
> Unescape URIs in the URL bar search results
> 
> Just calling decodeURI before displaying the history/bookmark URI in the search
> results.

There's another problem here. Bookmarking http://bg.wikipedia.org/wiki/Начална_страница puts http://bg.wikipedia.org/wiki/%D0%9D... in the DB, thus you're not going to find this bookmark when entering "Начална" in the location bar.

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 298626 [details] [diff] [review]
Unescape URIs in the URL bar search results

Put comment 6 aside, decodeURI can throw which needs to be handled. E.g. try decodeURI("https://foo.bar/%e2").

Comment 8

[Erwan Loisant]

Dão,
OK for comment 7. 
Regarding comment 6, my opinion (for what it's worth, i.e. not much) is that it's better to keep the encoded form in the DB and decode for display and search. In this way we make sure that low-level layers work with an encoded URI (for better compatibility). The decoded version is just for the user.

I'm not sure where you stand. Do you think that we should store the decoded version in the DB? That any patch that addresses one issue (display of search results) should also address all other issues (search itself)? That nothing should be done in this bug?

If I can know how this bug is supposed to be fixed, I'd be glad to help.

Comment 9

[Dão Gottwald [:dao]]

I don't know if decoding URIs on the fly when searching is possible; another option might be to add a second, encoded search term for each entered word. Either way Edward Lee should know more about this.

Comment 10

[Ed Lee :Mardak]

It would be possible to decode URIs on the fly with a custom SQL function which would just call a decodeURI that's implemented elsewhere. But that would require a decodeURI on each url checked.

Like Dão pointed out, we could search on the original term as well as an encoded search.. but that could double the execution time if both the title/url don't match either.

This bug was filed about the autocomplete dropdown not showing decoded URIs, so something along the lines of the original patch would work. There's the separate issue of not being able to match the encoded url with the (decoded) search term, so a separate bug can be filed if we want this fix to land sooner. (Things aren't that bad in this case anyway because the title gets matched..)

Comment 11

[Erwan Loisant]

Attachment: decodeURI on the url for search results - and catch exceptions

Comment 12

[Erwan Loisant]

I opened bug 413784 for the search problem.

Comment 13

[Erwan Loisant]

Attachment: decodeURI on the url for search results

Now using the same code as browser.js.

Gavin: I've thought about moving that decoding logic to a lower layer, but since decoded URLs can cause problems (see the comments on bug 389465) I think it's safer to decode in the UI. In this way, we're working with encoded URLs most of the time and the decoded version is only used for display.

Comment 14

[Dão Gottwald [:dao]]

Is the address that you're decoding the very same string that will be put in the location bar when you select that item? If not, going through the pain of bug 397815 (and bug 412458 as a result of that) is probably not worth it.

Comment 15

[Ed Lee :Mardak]

Perhaps a separate issue is that selecting entries (by pressing down) will fill the location bar with the encoded url.

Comment 16

[Ed Lee :Mardak]

(In reply to comment #15)
> Perhaps a separate issue is that selecting entries (by pressing down) will fill
> the location bar with the encoded url.
Filed bug 415187.

Comment 17

[Ed Lee :Mardak]

Thanks for looking into this Erwan and the patches here and for bug 413784.

Comment 18

[Ed Lee :Mardak]

This should be fixed by bug 407974.

Comment 19

[Emil Ivanov]

The decoding is not working in the Library. The URLs are not decoded there.

Comment 20

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

(In reply to comment #19)
> The decoding is not working in the Library. The URLs are not decoded there.

Keeping this open to track the issue from comment 19, above. URLs decode in the location bar (bug 407974) now, so while I think this is an annoyance, I don't think it blocks Beta 3 - setting blocking on Beta 4, though.

Comment 21

[Dietrich Ayala (:dietrich)]

I've opened bug 415460 for fixing this in the places query system. That will fix the library, and all other places views, both internally and in extension consumers.

Comment 22

[Reed Loden [:reed]]

Then I'm closing this one, as it's not needed anymore.

Comment 23

[Dão Gottwald [:dao]]

Bug 415460 is not a blocker (yet), so I'd say let's keep this open for blocker tracking.

Comment 24

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

That bug has a blocking request, so it won't be forgotten.

Comment 25

[Dão Gottwald [:dao]]

(In reply to comment #24)
> bug has a blocking request, so it won't be forgotten.

That's an interesting assumption.

Comment 26

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

All blocking flags are triaged regularly, and they'll certainly be dealt with one way or the other before the final release, at the very least. I'm not sure why you would think otherwise.

Comment 27

[Dão Gottwald [:dao]]

Either triage for Firefox doesn't happen often enough or it doesn't lead to a decision on all bugs. Beltzner already made a decision in comment 20, so I don't see why we should enter that circle again. Right before the final release, bugs that should have been blocking but don't cause a security hazard or make Firefox absolutely unusable will just be minused.

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3) Gecko/2008020511 Firefox/3.0b3 ID:2008020511

Comment 29

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv