434749 - [RTL] Drag&Drop bookmarks in Firefox 3 not usable

Status: VERIFIED FIXED

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

Description

[Tomer Cohen :tomer]

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?

Comment 1

[Tomer Cohen :tomer]

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

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

Comment 2

[Tsahi Asher]

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.

Comment 3

[Simon Montagu :smontagu]

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

Comment 4

[Alice0775 White]

Attachment: Patch for chrome://browser/content/places/toolbar.xml

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.

Comment 5

[Simon Montagu :smontagu]

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.

Comment 6

[(no longer active)]

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>

Comment 7

[Mike Schroepfer]

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.

Comment 8

[Mike Schroepfer]

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

removing approval to wait for code review.

Comment 9

[Mike Connor [:mconnor]]

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

will test and review tomorrow

Comment 10

[Mike Connor [:mconnor]]

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

Mano, can you look at this?

Comment 11

[Asaf Romano (gone)]

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

See Simon's comment about the code duplication.

Comment 12

[Asaf Romano (gone)]

Attachment: patch

Comment 13

[Asaf Romano (gone)]

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

that works

Comment 14

[Simon Montagu :smontagu]

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.

Comment 15

[(no longer active)]

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

Comment 16

[Mike Connor [:mconnor]]

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.

Comment 17

[Simon Montagu :smontagu]

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.

Comment 18

[Mike Shaver (:shaver emeritus)]

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

a=shaver, please land _immediately_ for FF3 inclusion.

Comment 19

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

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

Comment 20

[Hasham]

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.

Comment 21

[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