The default bug view has changed. See this FAQ.

Can't drag proxy icons from tabs to location bar if focus is outside location bar

RESOLVED FIXED in Camino1.5

Status

Camino Graveyard
Drag & Drop
--
minor
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Chris Lawson (gone), Assigned: Sean Murphy)

Tracking

({fixed1.8.1, regression})

unspecified
Camino1.5
PowerPC
Mac OS X
fixed1.8.1, regression

Details

(Whiteboard: [good first bug])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

11 years ago
Dragging the favicon from an extant tab to the location bar only works if the location bar currently has focus.

It's not entirely clear to me whether the patch in bug 162791 was intended to fix this, but if it did, it's broken again.
(Reporter)

Updated

11 years ago
Whiteboard: [good first bug]
Ew, we should try to fix this for 1.1.
Keywords: regression
(Assignee)

Comment 2

11 years ago
This is probably caused by the Cocoa text system's field editor.  One of the major sacrifices of the field editor's optimization trick is that dropped objects are ignored unless the text field is selected, because otherwise there's no NSTextView actually there:

"The field editor is a single NSTextView object that is shared among all the controls, including text fields, in a window."
From: http://developer.apple.com/documentation/Cocoa/Conceptual/TextEditing/Tasks/FieldEditor.html

The AutoCompleteTextField doesn't handle any aspect of this particular kind of drag (It's performDragOperation: is never called).

It should be possible to modify this behavior and allow the AutoCompleteTextField to recognize and respond to a drag of a tab's proxy icon.  Our preferred drag and drop action is to not load the URL on drop - just to insert the URL at the cursor location and select it.  We'll have to check and make sure the proxy dragging pasteboard type is something the AutoCompleteTextField can deal with.

I'll look into this..
(Assignee)

Comment 3

11 years ago
Created attachment 239610 [details] [diff] [review]
Patch for AutoCompleteTextField Drag Destination Issues

Short Explanation: Drags finishing inside the AutoCompleteTextField do not work correctly if it has focus (is first responder).

Long Explanation:
There was some inconsistent behavior occurring when proxy icons were being dropped onto the AutoCompleteTextField (ACTF).  If the ACTF was selected, then a drag originating from a proxy icon or bookmark button and ending inside the ACTF, (while the field retained focus during the entire drag session) the URL for the proxy/bookmark would be inserted somewhere in-between the old URL value at the cursor's location, failing to replace the old URL value.

I think dragging proxy icons and bookmark buttons should have the same behavior as dragging a link displayed in the browser view.  This kind of drag will un-select the ACTF as soon as any drag session is started, so drags always end the correct way.  To accomplish this, I've added some code to the tab view item and bookmark item objects which ensures no text fields (mainly the ACTF) are the window's first responder.

Note: Proxy Icons and Bookmark Buttons were the only two types of drags I could find which didn't force the ACTF to give up focus.  Are there any others?

Also, I made another minor change to keep the dragging of proxies consistent with all other URLs.  Dragging a link (or even a BookmarkButton, which works correctly in this case) into the ACTF should display the "plus" cursor, which is a nice obvious indicator that this is a valid destination for the drag.  I removed a check against the draggingSourceOperatingMask from inside AutoCompleteTextField.mm which failed to show a correct indicator in the case of a proxy icon drag.  Now, as long as the pasteboard contains anything we can form into a URL, the plus cursor (NSDragOperationCopy) will be displayed.

I hope this long-winded explanation didn't confuse everyone, but I wanted to make it clear why I made each change :)
Assignee: nobody → camino

Updated

11 years ago
Attachment #239610 - Flags: review?(hwaara)

Comment 4

11 years ago
Comment on attachment 239610 [details] [diff] [review]
Patch for AutoCompleteTextField Drag Destination Issues

r- (stealing review, sorry hwarra)

Unless it's impossible for some reason for the drag source to be allowing copy, having the destination just ignore what is allowed seems like the wrong way to go.

Also, having starting a drag change the keyboard focus is almost definitely going to be undesireable behavior. Isn't AutoCompleteTextField using a custom editor already?  Overriding drop behavior there would be a better way to go.
Attachment #239610 - Flags: review?(hwaara) → review-
(Assignee)

Comment 5

11 years ago
(In reply to comment #4)
> Unless it's impossible for some reason for the drag source to be allowing copy,
> having the destination just ignore what is allowed seems like the wrong way to
> go.
> 
> Also, having starting a drag change the keyboard focus is almost definitely
> going to be undesireable behavior. Isn't AutoCompleteTextField using a custom
> editor already?  Overriding drop behavior there would be a better way to go.
> 

Yeah, you're definitely right.  I was approaching each issue from the wrong direction.

Like you mentioned on IRC, I fixed the NSDragOperationCopy problem by changing the drag source (BrowserTabViewItem) instead of the destination (ACTF).  The ACTF is back to checking the sourceDragMask.

As for the keyboard focus issue, we can choose to provide a custom field editor for the ACTF, which is the only other way to handle drag and drop correctly.  This will avoid the hack which changes keyboard focus.

Thanks for pointing me in a better direction, and sorry both of my solutions were more like hacks or workarounds.
(Assignee)

Comment 6

11 years ago
Created attachment 239702 [details] [diff] [review]
Proxy Icon Dragging, Field Editor Patch

I changed the proxy icon object to allow NSDragOperationCopy, which now properly displays the "plus" cursor when hovering over the AutoCompleteTextField without modifying the way the text field checks the drag object's source mask.

Yeah, I should have noticed we were already using a field editor on the AutoCompleteTextField.  Those 'workarounds' I posted were attempts to avoid specially creating a field editor just to handle drag and drop appropriately.  Since we've already got one, letting it handle the drags is for sure the right way to go.  So, I added drag support to the AutoCompleteFieldEditor (which is actually declared inside BrowserWindowController.mm).

Additional notes: The #import of NSPasteboard+Utils.h in BrowserWindowController is so the field editor can access the global NSString kCorePasteboardFlavorType_url.
Attachment #239610 - Attachment is obsolete: true
(Assignee)

Comment 7

11 years ago
Created attachment 239703 [details] [diff] [review]
This patch is the same as above, with an addition to fix a compiler warning not related to this bug

I noticed a compiler warning which occurred in BrowserWindowController.mm when working on the AutoCompleteTextFieldEditor.  There was a comparison between a signed and unsigned integer (i compared to [menuArray count]).  

Not sure if you wanted or needed this fixed, and also if it should be done under another bug report, but I've attached my previous patch with this fix included in the dragging modifications.
Comment on attachment 239703 [details] [diff] [review]
This patch is the same as above, with an addition to fix a compiler warning not related to this bug

Seeing if Stuart will look at the new iteration....
Attachment #239703 - Flags: review?(stuart.morgan)

Comment 9

11 years ago
Comment on attachment 239703 [details] [diff] [review]
This patch is the same as above, with an addition to fix a compiler warning not related to this bug

Looks good, r=me.  Thanks!  As an added bonus, fixing the drag source makes favicon drops to the empty tab bar area show a plus, which matches the actual behavior.

Just one nit: move the #import "NSPasteboard+Utils.h" up to group it with the other NSFoo+Utils imports.
Attachment #239703 - Flags: superreview?(sfraser_bugs)
Attachment #239703 - Flags: review?(stuart.morgan)
Attachment #239703 - Flags: review+
(Assignee)

Comment 10

11 years ago
Created attachment 239867 [details] [diff] [review]
Same patch, just moved around the #import.

Ok, this is the same patch (including the unsigned comparison fix) except I moved up the #import of NSPasteboard+Utils.h so it's grouped with similar imports.

I didn't obsolete the old patch yet, however, since that one has the review status attributed to it..

Thanks for looking it over Stuart.
Comment on attachment 239867 [details] [diff] [review]
Same patch, just moved around the #import.

Carrying over review...

(Typically if there's just a small change to make, you can carry over the review from the previous patch.)
Attachment #239867 - Flags: superreview?(sfraser_bugs)
Attachment #239867 - Flags: review+
Attachment #239703 - Attachment is obsolete: true
Attachment #239703 - Flags: superreview?(sfraser_bugs)

Comment 12

11 years ago
Comment on attachment 239867 [details] [diff] [review]
Same patch, just moved around the #import.

Looks good but please remove the spaces from inside the () in performDragOperation.
Attachment #239867 - Flags: superreview?(sfraser_bugs) → superreview+

Comment 13

11 years ago
Checked in on 1.8branch and trunk.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.