Places should decode URLs

VERIFIED FIXED in Firefox 3 beta3

Status

()

P1
normal
VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: stream, Assigned: Mardak)

Tracking

({intl})

Trunk
Firefox 3 beta3
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

11 years ago
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/Начална_страница
(Reporter)

Updated

11 years ago
Version: unspecified → Trunk

Comment 1

11 years ago
> 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).
OS: Windows XP → All
Hardware: PC → All

Comment 2

11 years ago
(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.
Status: UNCONFIRMED → NEW
Component: Location Bar and Autocomplete → Places
Ever confirmed: true
QA Contact: location.bar → places
Summary: Location bar encoding → Places should decode URLs

Updated

11 years ago
Blocks: 391175

Updated

11 years ago
Duplicate of this bug: 408741

Comment 4

11 years ago
Created attachment 298626 [details] [diff] [review]
Unescape URIs in the URL bar search results

Just calling decodeURI before displaying the history/bookmark URI in the search results.
Attachment #298626 - Flags: review?(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

11 years ago
(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.
Keywords: intl
Assignee: nobody → erwan

Comment 7

11 years ago
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").
Attachment #298626 - Flags: review?(dietrich) → review-

Comment 8

11 years ago
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

11 years ago
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.
(Assignee)

Comment 10

11 years ago
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

11 years ago
Created attachment 298859 [details] [diff] [review]
decodeURI on the url for search results - and catch exceptions
Attachment #298626 - Attachment is obsolete: true
Attachment #298859 - Flags: review?

Updated

11 years ago
Attachment #298859 - Attachment description: decodeURI on the url for search results - and catches exceptions → decodeURI on the url for search results - and catch exceptions

Comment 12

11 years ago
I opened bug 413784 for the search problem.

Updated

11 years ago
Attachment #298859 - Flags: review? → review?(gavin.sharp)

Comment 13

11 years ago
Created attachment 300485 [details] [diff] [review]
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.
Attachment #298859 - Attachment is obsolete: true
Attachment #300485 - Flags: review?(gavin.sharp)
Attachment #298859 - Flags: review?(gavin.sharp)
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.
(Assignee)

Comment 15

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

Comment 16

11 years ago
(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.
(Assignee)

Updated

11 years ago
Attachment #300485 - Flags: review?(gavin.sharp)
(Assignee)

Comment 17

11 years ago
Thanks for looking into this Erwan and the patches here and for bug 413784.
Assignee: erwan → edilee
(Assignee)

Comment 18

11 years ago
This should be fixed by bug 407974.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Depends on: 407974
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 beta3
(Reporter)

Comment 19

11 years ago
The decoding is not working in the Library. The URLs are not decoded there.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Updated

11 years ago
Flags: blocking-firefox3?
(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.
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P1
Target Milestone: Firefox 3 beta3 → Firefox 3 beta4
Depends on: 415460
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.
Then I'm closing this one, as it's not needed anymore.
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 3 beta4 → Firefox 3 beta3
Bug 415460 is not a blocker (yet), so I'd say let's keep this open for blocker tracking.
Status: RESOLVED → REOPENED
Keywords: meta
Resolution: FIXED → ---
Target Milestone: Firefox 3 beta3 → Firefox 3 beta4
That bug has a blocking request, so it won't be forgotten.
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 3 beta4 → Firefox 3 beta3
Keywords: meta
(In reply to comment #24)
> bug has a blocking request, so it won't be forgotten.

That's an interesting assumption.
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.
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.
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
Status: RESOLVED → VERIFIED
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.