251627 - Icon incorrectly placed when dropping an icon from the Customize Toolbar windows to the Toolbars when interface is RTL

Status: RESOLVED FIXED

(Toolkit :: Toolbars and Toolbar Customization, defect, P1)

Description

[Reuven Gonen]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; he-IL; rv:1.7) Gecko/20040626 Firefox/0.9.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; he-IL; rv:1.7) Gecko/20040626 Firefox/0.9.1

When interface is in RTL mode as in Hebrew or Arabic, When Dragging an Icon from
the Costumize Toolbar window to the Navigation Toolbar, the icon is placed one
place to the right from the place you drop the icon. Also the mouse becomes
instable when trying to place the icon and moves right and left.

Reproducible: Always
Steps to Reproduce:
1.Add this code to the userChrome.css file To chanage the direction to RTL(it's
under the chrome folder inside the user profile.

/*make UI RTL */

window,dialog,wizard,page { direction: rtl; }

menu { direction: rtl; }

/*************************************
**  Fix address bar directionality  **
*************************************/

/*make address bar remain LTR */

#urlbar { direction: ltr !important; }

/***** End address bar fix ******/

/* Align text in menulist pupop items to right  */

menulist > menupopup > menuitem {
  padding: 1px 2px 1px 30px !important;
}

/* Align autocomplete popups items in urlbar to left in firefox */

popup[type="autocomplete"],
.autocomplete-history-popup {
 direction: ltr !important;
}

2. Go to View->Toolbars->Customize... and drag the bookmarks button to the
navigation toolbar (i used the bookmarks button for the example, you can use any
button).

3. Try to place the button between the stop and the refresh button. were you
successful?

Actual Results:  
The bookmarks button appears between the stop and the forward button.

Expected Results:  
The bookmark button should appear between the stop and the refresh button.

Comment 1

[Reuven Gonen]

Attachment: Placing the Icon on the Toolbar

Comment 2

[Reuven Gonen]

Attachment: The incorrect place of the bookmark icon

Comment 3

[Prognathous]

Confirmed using the customized userChrome.css and Firefox-0.9.1/20040714/WinXP.

Prog.

Comment 4

[Asaf Romano (gone)]

argh, drag is also partly broken.

Comment 5

[Asaf Romano (gone)]

Attachment: patch

Comment 6

[Mike Connor [:mconnor]]

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

>   var node = aItem;
>-  var value = "left";
>+  var direction = window.getComputedStyle(aItem.parentNode, null).direction;
>+  var value = direction == "ltr"? "left" : "right";
>   if (aItem.localName == "toolbar") {
>     node = aItem.lastChild;
>-    value = "right";
>+    var value = direction == "ltr"? "right" : "left";
>   }

You're redeclaring value here.	Lets not add more JS strict errors. ;)

>   
>   if (!node)
>     return;
>   
>   if (aValue) {
>     if (!node.hasAttribute("dragover"))
>       node.setAttribute("dragover", value);
>@@ -779,17 +780,20 @@
>     
>     if (dropTarget.localName == "toolbar") {
>       gCurrentDragOverItem = dropTarget;
>     } else {
>       var dropTargetWidth = dropTarget.boxObject.width;
>       var dropTargetX = dropTarget.boxObject.x;
> 
>       gCurrentDragOverItem = null;
>-      if (aEvent.clientX > (dropTargetX + (dropTargetWidth / 2))) {
>+      var direction = window.getComputedStyle(dropTarget.parentNode, null).direction;
>+      var dragAfter = direction == "ltr" ? aEvent.clientX > (dropTargetX + (dropTargetWidth / 2)) :
>+                      aEvent.clientX < (dropTargetX + (dropTargetWidth / 2))

Make this a normal if/else statement, please, for simple bits like the bits
earlier in the patch its ok, but for something like this its not the most
readable code.	Possibly in the name of code readability you might use a var
like dropTargetCenter = dropTargetX + (dropTargetWidth / 2) so its obvious what
you're comparing here, plus it's looks cleaner that way.  Or add a comment
explaining what were comparing here, at the least.

>@@ -976,9 +980,8 @@
>     if (!this._flavourSet) {
>       this._flavourSet = new FlavourSet();
>       var documentId = gToolboxDocument.documentElement.id;
>       this._flavourSet.appendFlavour("text/toolbarwrapper-id/"+documentId);
>     }
>     return this._flavourSet;
>   }
> }
>-

Please don't do this.

Comment 7

[Asaf Romano (gone)]

Attachment: patch

Comment 8

[Mike Connor [:mconnor]]

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

That's much better.  Thanks!

Comment 9

[Asaf Romano (gone)]

Checking in toolkit/content/customizeToolbar.js;
/cvsroot/mozilla/toolkit/content/customizeToolbar.js,v  <--  customizeToolbar.js
new revision: 1.25; previous revision: 1.24
done

leaving open for thunderbird

Comment 10

[Asaf Romano (gone)]

Attachment: parallel patch for thunderbird

Comment 11

[Asaf Romano (gone)]

mscott: care to review the tbird version? the toolkit version is already on the
trunk.

Comment 12

[Asaf Romano (gone)]

Thanks Scott.

Checking in customizeToolbar.js;
/cvsroot/mozilla/mail/base/content/customizeToolbar.js,v  <--  customizeToolbar.js
new revision: 1.7; previous revision: 1.6
done

Comment 13

[(no longer active)]

Mass-assigning the new rtl keyword to RTL-related (see bug 349193).