Tomer Cohen :tomer Reporter
	Description • 16 years ago

For some reasons, ancient bugs come alive. We are having the same drag&drop problem we has with Firefox 1.x. 

Steps to reproduce:
1. Install Hebrew Firefox (Arabic/Farsi - Please check if this also affect your locales).

Actual result: 
You get the bookmark moved to the wrong location.

Expected result:
Same as in English UI - The bookmark should be placed where the mouse cursor is released. 

Can be reproduced on Linux and Mac. Tsahi, can you please check it on Windows?

	Tomer Cohen :tomer Reporter
	Comment 1 • 16 years ago

Yehuda have published some video captures to show the scenario on Mac:

English: http://screencast.com/t/SnhViCMJl 
Hebrew: http://screencast.com/t/P0YCvhVLhqt

	Tsahi Asher
	Comment 2 • 16 years ago

to be precice, the bookmark will always land on the right (start) edge of the bookmarks toolbar, regardless of where you drop it. i get this on windows, RC1.

	Simon Montagu :smontagu
	Comment 3 • 16 years ago

Mano's fix for bug 218528 never got ported to Places, apparently.

	Alice0775 White
	Comment 4 • 16 years ago

I made a patch for Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008052106 Minefield/3.0pre.
I have checked on Windows XP only.

	Simon Montagu :smontagu
	Comment 5 • 16 years ago

Comment on attachment 322002 [details] [diff] [review]
Patch for chrome://browser/content/places/toolbar.xml

Thank you for patching this. I tested on Linux, and the only problem I found was in the case where the toolbar was empty: the indicator appeared at the far left of the toolbar instead of the right.

Some code comments, even thought I'm not a browser peer and can't review this:

Instead of duplicating the whole loop in _getDropPoint, I think it's better to do something like:

          var isRTL = document.defaultView.getComputedStyle(this._self.parentNode, "").direction == "rtl";
          for (var i = 0; i < this._self.childNodes.length; i++) {
.
.
	      if ((isRTL && event.clientX > xulNode.boxObject.x + (xulNode.boxObject.width * 0.75)) ||
		  (!isRTL && event.clientX < xulNode.boxObject.x + (xulNode.boxObject.width * 0.25))) {

etc., and change the comments to use "before" and "after" instead of "to the left" and "to the right"

>+              if (this._self.childNodes.length == 0){
>+                ind.style.marginRight = this._self.boxObject.width + 'px';
>+              }else if (dropPoint.beforeIndex == -1){

Nit: local style in the file puts the 'else' on a new line.

	(no longer active)
	Comment 6 • 16 years ago

Thanks for the patch, Alice.  Are you going to ask someone for review?  Here's the list of peers you can ask for review: <http://www.mozilla.org/projects/firefox/review.html>

	Mike Schroepfer
	Comment 7 • 16 years ago

Comment on attachment 322002 [details] [diff] [review]
Patch for chrome://browser/content/places/toolbar.xml

a+ schrep for 3.0.1 or RC2 please land on cvs trunk.

	Mike Schroepfer
	Comment 8 • 16 years ago

Comment on attachment 322002 [details] [diff] [review]
Patch for chrome://browser/content/places/toolbar.xml

removing approval to wait for code review.

	Mike Connor [:mconnor]
	Comment 9 • 16 years ago

Comment on attachment 322002 [details] [diff] [review]
Patch for chrome://browser/content/places/toolbar.xml

will test and review tomorrow

	Mike Connor [:mconnor]
	Comment 10 • 16 years ago

Comment on attachment 322002 [details] [diff] [review]
Patch for chrome://browser/content/places/toolbar.xml

Mano, can you look at this?

	Asaf Romano (gone) Assignee
	Comment 11 • 16 years ago

Comment on attachment 322002 [details] [diff] [review]
Patch for chrome://browser/content/places/toolbar.xml

See Simon's comment about the code duplication.

	Asaf Romano (gone) Assignee
	Comment 13 • 16 years ago

Comment on attachment 322563 [details] [diff] [review]
patch

that works

	Simon Montagu :smontagu
	Comment 14 • 16 years ago

Comment on attachment 322563 [details] [diff] [review]
patch

This has the same bug as the previous patch that I mentioned in comment 5: when dropping a bookmark on an empty folder the indicator appears at the far left instead of the far right.

	(no longer active)
	Comment 15 • 16 years ago

mconnor: What about comment 14?  Should we leave it off to a followup bug?  Or should it be addressed here?

	Mike Connor [:mconnor]
	Comment 16 • 16 years ago

yes, we should leave that for a followup bug.  I think a completely empty toolbar is an edgecase I'm ok with being mildly broken.

	Simon Montagu :smontagu
	Comment 17 • 16 years ago

I agree with comment 16, especially since Asaf and I can't agree exactly what is the correct position for the indicator in an empty toolbar (exactly on the edge of the toolbar or shifted inwards slightly).

However if we have RC2 we should take this since it is a serious loss of functionality for rtl localizations.

	Mike Shaver (:shaver -- probably not reading bugmail closely)
	Comment 18 • 16 years ago

Comment on attachment 322563 [details] [diff] [review]
patch

a=shaver, please land _immediately_ for FF3 inclusion.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 19 • 16 years ago

mozilla/browser/components/places/content/toolbar.xml 	1.157 

	Hasham
	Comment 20 • 16 years ago

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9) Gecko/2008053008 Firefox/3.0

Mozilla/5.0 (Windows; U; Windows NT 6.0; he; rv:1.9) Gecko/2008052906 Firefox/3.0

I checked out RC2 for this on OS-X and Vista using both the en-US and he versions. Works properly for both.

	Gervase Markham [:gerv]
	Comment 21 • 15 years ago

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