Closed Bug 1368077 Opened 2 years ago Closed 2 years ago

Dragging URLs should strip page titles from the URL on the pasteboard

Categories

(Core :: Widget: Cocoa, defect)

All
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla55
Tracking Status
firefox54 --- verified
firefox55 --- verified

People

(Reporter: spohl, Assigned: spohl)

References

Details

Attachments

(1 file, 2 obsolete files)

Bug 1358075 restored the ability to drag URLs to Finder and have the resulting .webloc files named based on the URL's title. However, it no longer stripped the page title from the URL on the pasteboard, resulting in incorrect URLs being stored in the .webloc file. For example, dragging and dropping https://www.google.com/ on Finder would result in a .webloc file that would open https://www.google.com/%0AGoogle. Depending on the page title and/or URL, this could also cause the drop of the URL to fail completely, as was reported in bug 1358075 comment 21.
Attached patch Patch (obsolete) — Splinter Review
Patch 1 in bug 1358075 (attachment 8867833 [details] [diff] [review]) moved the escaping of the URL and assignment of nativeURL ahead of the truncating of the URL in case a new line was found. This was done because we needed nativeURL inside the if (newlinePos >= 0) block to set urlsAndTitles on the pasteboard, but it accidentally resulted in urls+titles being stored as URL. This patch reorganizes this code to restore proper functionality and does away with an alloc/release.
Assignee: nobody → spohl.mozilla.bugs
Status: NEW → ASSIGNED
Attachment #8871777 - Flags: review?(mstange)
Attachment #8871777 - Flags: review?(mstange)
Attached patch Patch (obsolete) — Splinter Review
Forgot to qrefresh the previous patch. This patch only sets public.url-name and WebURLsWithTitlesPboardType if we actually have a title, which was the previous behavior.
Attachment #8871777 - Attachment is obsolete: true
Attachment #8871781 - Flags: review?(mstange)
Comment on attachment 8871781 [details] [diff] [review]
Patch

Review of attachment 8871781 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/cocoa/nsClipboard.mm
@@ +690,5 @@
>        urlObject->GetData(url);
>  
> +      NSString* nativeTitle = nil;
> +
> +      // A newline embedded in the URL means that the form is actually URL + title.

Maybe add a comment that those combined strings are assembled in nsDragService::GetData.
Attachment #8871781 - Flags: review?(mstange) → review+
Attached patch PatchSplinter Review
Thanks, Markus! Added comment, carrying over r+.
Attachment #8871781 - Attachment is obsolete: true
Attachment #8871786 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b9b12d91081fbbe98b82b2501834c6877366011
Bug 1368077: Strip page titles from URLs on the pasteboard when dragging on OSX/macOS. r=mstange
Comment on attachment 8871786 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: bug 1358075
[User impact if declined]: When URLs are dragged to Finder, the resulting .webloc file will have an incorrect URL or the drop will fail.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: Yes. Same steps as bug 1358075, but we should also verify:
1. that the issues reported in bug 1358075 comment 21 no longer occur, and
2. that when the .webloc file is opened, the correct URL is opened in the browser (no page title at the end of the URL).
[List of other uplifts needed for the feature/fix]: bug 1358075, bug 1330470, bug 1361116
[Is the change risky?]: no
[Why is the change risky/not risky?]: This restores the behavior that existed prior to the patches in bug 1358075 landing.
[String changes made/needed]: none
Attachment #8871786 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/4b9b12d91081
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment on attachment 8871786 [details] [diff] [review]
Patch

regression from bug 1358075, beta54+
Attachment #8871786 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I have managed to reproduce the issue described in comment 6 using Firefox 53.0.3 (Build Id:20170518000419).

This issue is verified fixed on Firefox 54.0 (Build Id:20170605134926) and Firefox 55.0a1 (Build Id:20170607030206) using macOS 10.12.1 and macOS 10.11.6.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
https://hg.mozilla.org/projects/jamun/rev/3f0075d62b3675ae8c8e25552336a8091f08cf3e
Bug 1368077 - Strip page titles from URLs on the pasteboard when dragging on OSX/macOS. r=mstange, a=jcristau
You need to log in before you can comment on or make changes to this bug.