Closed Bug 434749 Opened 11 years ago Closed 11 years ago

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

Categories

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

3.0 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 3

People

(Reporter: tomer, Assigned: mano)

References

(Blocks 1 open bug)

Details

(Keywords: rtl, Whiteboard: [RC2+])

Attachments

(2 files)

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?
Flags: blocking-firefox3?
Blocks: fx35-l10n-fa
Keywords: rtl
Yehuda have published some video captures to show the scenario on Mac:

English: http://screencast.com/t/SnhViCMJl 
Hebrew: http://screencast.com/t/P0YCvhVLhqt
Component: Bookmarks → Places
QA Contact: bookmarks → places
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.
Mano's fix for bug 218528 never got ported to Places, apparently.
Whiteboard: [RC2?]
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 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.
Priority: -- → P2
Whiteboard: [RC2?] → [RC2?][RC2+
Whiteboard: [RC2?][RC2+ → [RC2?][RC2+]
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 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.
Attachment #322002 - Flags: approval1.9+
Flags: blocking1.9.0.1+
Comment on attachment 322002 [details] [diff] [review]
Patch for chrome://browser/content/places/toolbar.xml

removing approval to wait for code review.
Attachment #322002 - Flags: approval1.9+
Assignee: nobody → alice0775
Status: NEW → ASSIGNED
Comment on attachment 322002 [details] [diff] [review]
Patch for chrome://browser/content/places/toolbar.xml

will test and review tomorrow
Attachment #322002 - Flags: review?
Comment on attachment 322002 [details] [diff] [review]
Patch for chrome://browser/content/places/toolbar.xml

Mano, can you look at this?
Attachment #322002 - Flags: review? → review?(mano)
Comment on attachment 322002 [details] [diff] [review]
Patch for chrome://browser/content/places/toolbar.xml

See Simon's comment about the code duplication.
Attachment #322002 - Flags: review?(mano) → review-
Attached patch patchSplinter Review
Assignee: alice0775 → mano
Attachment #322563 - Flags: review?(mconnor)
Flags: blocking-firefox3? → blocking-firefox3-
Keywords: relnote
Whiteboard: [RC2?][RC2+] → [RC2?]
Comment on attachment 322563 [details] [diff] [review]
patch

that works
Attachment #322563 - Attachment description: untested → patch
Target Milestone: --- → Firefox 3
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.
Attachment #322563 - Flags: review?(mconnor)
Attachment #322563 - Flags: review+
Attachment #322563 - Flags: approval1.9?
mconnor: What about comment 14?  Should we leave it off to a followup bug?  Or should it be addressed here?
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.
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 on attachment 322563 [details] [diff] [review]
patch

a=shaver, please land _immediately_ for FF3 inclusion.
Attachment #322563 - Flags: approval1.9? → approval1.9+
Whiteboard: [RC2?] → [RC2+]
mozilla/browser/components/places/content/toolbar.xml 	1.157 
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
Flags: blocking1.9.0.1+
No longer blocks: fx35-l10n-fa
Blocks: Persian
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.