Closed Bug 251627 Opened 16 years ago Closed 15 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.8beta2

People

(Reporter: linxspider, Assigned: mano)

References

(Blocks 1 open bug)

Details

(Keywords: intl, rtl)

Attachments

(4 files, 1 obsolete file)

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.
Confirmed using the customized userChrome.css and Firefox-0.9.1/20040714/WinXP.

Prog.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: bugs → bugs.mano
Priority: -- → P3
Target Milestone: --- → Firefox1.0beta
Keywords: intl
Priority: P3 → P1
Hardware: PC → All
Summary: Icon incorrectly placed when dragging an icon from the Costumise Toolbar winodws to the Navigation Toolbar when interface is RTL → [Firefox & Thunderbird] Icon incorrectly placed when dropping an icon from the Customize Toolbar windows to the Toolbars when interface is RTL
Target Milestone: Firefox1.0beta → Firefox1.0
argh, drag is also partly broken.
Target Milestone: Firefox1.0 → After Firefox 1.0
Component: Toolbars → Toolbars and Toolbar Customization
Product: Firefox → Toolkit
Summary: [Firefox & Thunderbird] Icon incorrectly placed when dropping an icon from the Customize Toolbar windows to the Toolbars when interface is RTL → Icon incorrectly placed when dropping an icon from the Customize Toolbar windows to the Toolbars when interface is RTL
Target Milestone: Future → ---
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.8beta2
Attached patch patch (obsolete) — Splinter Review
Attachment #176268 - Flags: first-review?(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.
Attachment #176268 - Flags: first-review?(mconnor) → first-review-
Attached patch patchSplinter Review
Attachment #176268 - Attachment is obsolete: true
Attachment #176289 - Flags: first-review?(mconnor)
Comment on attachment 176289 [details] [diff] [review]
patch

That's much better.  Thanks!
Attachment #176289 - Flags: first-review?(mconnor) → first-review+
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
mscott: care to review the tbird version? the toolkit version is already on the
trunk.
Attachment #176299 - Flags: first-review?(mscott) → first-review+
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
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Blocks: 306980
QA Contact: bugzilla → toolbars
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl
Depends on: 485407
You need to log in before you can comment on or make changes to this bug.