240536 - resizer direction is to the right, when interface is RTL

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Reuven Gonen]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; he-IL; rv:1.6) Gecko/20040206 Firefox/0.8
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; he-IL; rv:1.6) Gecko/20040206 Firefox/0.8

When the interface is RTL the window resizer resizes the window to the right,
inetead of to the left. This bug is common to Mozilla, Firefox, and Thounderbird.

Reproducible: Always
Steps to Reproduce:
1.Add the following code to the userChrome.css file(it's under the chrome
directory in the profile folder) to change interface direction to RTL:

/*make UI RTL */

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

menu { direction: rtl; }

outliner { direction: rtl; }

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

/*make address bar remain LTR */

#urlbar { direction: ltr !important; }

/* leave search from address bar in RTL */

#urlbar .autocomplete-search-engine {
direction: rtl !important;
}

2. open the program(Mozilla, Firefox, Thounderbird)
3.restore the window using the "restore" button
4.try to resize the winddow, using the resizer at the bottom left corner of the
window.
Actual Results:  
The window resizes to the right

Expected Results:  
The window should resize to the left

this bug appears in all windows versions. (Not sure about linux and mac)

Comment 1

[Reuven Gonen]

Attachment: the bookmark manager window in mozilla

Comment 2

[Reuven Gonen]

Attachment: resizer mouse cursor

Comment 3

[Asaf Romano (gone)]

Confirmed.

Not sure how we should fix it:
 * fix the cursor
 * move the resizer back to where it should be (like in LTR case)

Comment 4

[minckster]

I have a similar issue. After moving the sidebar to the right, by adding the
following lines to userChrome.css

   window > hbox { direction:rtl; }
   window > hbox > * { direction:ltr; }

the Extension Manager, the Download Manager, and the Themes Manager all have the
resizer in the lower left-hand corner. The windows resize the way one would
expect, up-and-down from the lower right-side corner.

The Bookmarks Manager has the resizer in the lower right-hand corner.

Comment 5

[Tsahi Asher]

(In reply to comment #3)
> Confirmed.
> 
> Not sure how we should fix it:
>  * fix the cursor
>  * move the resizer back to where it should be (like in LTR case)
> 

i think the resizer is right where it should be right now, for an RTL app. we
should fix the cursor, make sure the graphics are correct, and that when we move
the resizer, we have the lower-left corner moving, and not the lower-right.

Comment 6

[(no longer active)]

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

Comment 7

[Kai Liu]

Attachment: part 1: implement dir=bottomcorner for resizer

I think we have two options here for fixing this bug.

Option 1: change the behavior of the existing resizer directions
Option 2: implement (a) new resizer direction(s)

I think that option 2 is the best.  The current directions refer to right, left, etc., and I don't think it would make sense for a bottomright resizer to suddenly act as a bottomleft resizer, just as it wouldn't make sense if padding-right turned into padding-left.  So I propose that we leave the current resizer direction behaviors as-is and mention in the xul:resizer documentation that these directions are not RTL-aware and that XUL consumers should avoid using them, unless they are willing to enforce their position (i.e., take steps to make sure that a bottomright resizer will always appear on the bottomright, regardless of RTL/LTR).

So I propose that we introduce a new resizer direction that is RTL-aware.  For lack of a better name, I'm calling it "bottomcorner".  This is the resizer direction that most XUL consumers should use.

If we go with this approach, then there are 2 steps that must be taken:
* Step 1: Implement the bottomcorner resizer direction and make sure that it looks right in RTL and LTR
* Step 2: Change the uses of the resizer in our tree from bottomright to bottomcorner.

This patch takes care of step 1.  I've tested it, and it seems to work just fine on Windows.  I don't have a Mac to test it on, and my Linux VPC is too out-of-date to run Firefox 3.  For Linux, it looks like the current drawing code for the resizer grip already respects LTR/RTL direction, but I can't confirm that.  As for the Mac, -moz-appearance: resizer is not even implemented, and I don't know where the resizer is being drawn on the Mac.  Is it being drawn as a part of the window itself and the resizer is serving only as a placeholder?

Step 2 would be tedious, and I'll get around to that later (or someone else can tackle it, if they want).  It would involve changing occurrences of the bottomright resizer to bottomcorner, adding a chromedir="&locale.dir;" to each one, and including global/locale/global.dtd (for the chromedir) if necessary.

Comment 8

[Kai Liu]

Ah, I forgot to mention that this patch is diffed against the patch in bug 403458 instead of trunk (since that patch introduces the ForceClassicFallback that this patch relies on)

Comment 9

[Kai Liu]

Alternatively, instead of adding chromedir to every use of the resizer (which seems a bit cumbersome, especially since that also involves including the global locale DTD for some XUL documents), changing the direction of the cursor could be the responsibility of whoever changed the direction to rtl (so intl.css)... I don't know which of these two would be a better approach.  Thoughts?

Comment 10

[Kai Liu]

Attachment: Part 1 (v2) - implement dir=bottomeend

This is a much better approach; it avoids having the consumers of <resizer> worry about setting chromedir or an appropriate default cursor.  Also, bottomend is probably a better name than bottomcorner.

Comment 11

[Kai Liu]

Attachment: Part 2 (v1) - update browser & toolkit to use the new RTL-aware resizer

Part 2 switches the resizers used in browser and toolkit to the RTL-aware dir=bottomend.  TB and SB should be handled in a separate patch.

The Part 1 patch above doesn't change the behavior of the existing resizers, so current consumers of <resizer>s can still keep using dir=bottomright and maintain the existing behavior (and I think that the hack in intl.css to hide the bottomright resizers should be kept in place to cover the the resizers that have not moved to the new RTL-aware dir=bottomend).

Um, not really sure who I should be asking for review here since these patches touch a disparate bunch of files...

Comment 12

[(no longer active)]

(In reply to comment #11)
> Um, not really sure who I should be asking for review here since these patches
> touch a disparate bunch of files...

The usual procedure is to ask a peer of each module you're touching for review.  Some modules, such as widget and layout also need super-review.  For the patches in this bug, you'd need a review from a browser/toolkit peer, as well as r/sr from a layout and widget peer.  It looks like Mats Palmgren would be a good reviewer for the latter...  <http://www.mozilla.org/hacking/reviewers.html>

Comment 13

[Kai Liu]

Comment on attachment 323024 [details] [diff] [review]
Part 1 (v2) - implement dir=bottomeend

This patch has bitrotted a lot.  I'll need to update it when I get some time next week.

Comment 14

[Kai Liu]

Attachment: Part 1 (v3) - implement dir=bottomeend

Finally got around to unbitrotting the part 1 patch.  Note that the only RTL-aware direction being implemented here is bottomend because, AFAIK, that's really the only direction that is ever used...

Comment 15

[Kai Liu]

Attachment: Part 1 (v3.1) - implement dir=bottomeend

Fix spacing nit.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 331729 [details] [diff] [review]
Part 1 (v3.1) - implement dir=bottomeend

+    return(direction);

return direction;

This doesn't handle dynamic direction changes. I guess handling them is rather difficult and I can't think of a good way to do it.

Comment 17

[Kai Liu]

(In reply to comment #16)
> This doesn't handle dynamic direction changes. I guess handling them is rather
> difficult and I can't think of a good way to do it.
> 

There are many elements in the UI that make use of chromedir, and unfortunately, none of them will be able to handle dynamic direction changes.  Fortunately, most of them are cosmetic problems; in the case of the resizer, it will just result in the wrong mouse cursor.  As for the actual function and appearance of the resizer, it does handle dynamic changes correctly (I've tested this).

Comment 18

[Kai Liu]

Attachment: Part 1 (v3.2) - implement dir=bottomeend

fixed nit

Comment 19

[(no longer active)]

(In reply to comment #17)
> There are many elements in the UI that make use of chromedir, and
> unfortunately, none of them will be able to handle dynamic direction changes. 
> Fortunately, most of them are cosmetic problems; in the case of the resizer, it
> will just result in the wrong mouse cursor.  As for the actual function and
> appearance of the resizer, it does handle dynamic changes correctly (I've
> tested this).

The mouse cursor is being set via CSS which should be able to handle the chromedir attribute change, right?

Comment 20

[Kai Liu]

(In reply to comment #19)
> The mouse cursor is being set via CSS which should be able to handle the
> chromedir attribute change, right?
> 

Yea, it would.  I should clarify; what I meant was that stuff that use chromedir can't handle dynamic direction changes unless the chromedir attr is manually changed as well (which is what ForceRTL does).  With the manual chromedir change, then dynamic direction changes work flawlessly for the bottomend resizer being implemented by this patch.

Comment 21

[Kai Liu]

Attachment: test

Just a reftest to check the rendering of the RTL resizer.  A more useful and important test would be one that checks for the correct behavior when the resizer is dragged.  I spoke with sdwilsh about how best to do such a test, and he says that this sort of test isn't possible at the moment...

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

nsIDOMWindowUtils::SendMouseEvent doesn't work for what you want?

Comment 23

[Kai Liu]

(In reply to comment #22)
> nsIDOMWindowUtils::SendMouseEvent doesn't work for what you want?
> 

I'm not sure.  I've never written a mochitest before, so I asked, and he said that doing drags in a mochitest doesn't really work.  I haven't actually tried yet, since I'm still pretty new to this sort of testing.  I can look into it more when I get a chance later in the week...

Comment 24

[Asaf Romano (gone)]

Comment on attachment 331838 [details] [diff] [review]
Part 1 (v3.2) - implement dir=bottomeend

You should take "chromedir" statically from gloabl.dtd (set it on the <content> element).

Comment 25

[Asaf Romano (gone)]

Comment on attachment 323028 [details] [diff] [review]
Part 2 (v1) - update browser & toolkit to use the new RTL-aware resizer

r=mano

Comment 26

[(no longer active)]

(In reply to comment #24)
> (From update of attachment 331838 [details] [diff] [review])
> You should take "chromedir" statically from gloabl.dtd (set it on the <content>
> element).

Kai: do you have free time to make this change, considering the holidays and all?  If not, I'd be happy to.

Thanks!

Comment 27

[(no longer active)]

Attachment: Unified patch

Updated, merged and addressed comment 24.

Requesting r from mano on toolkit/content/widgets/resizer.xml change, and r/sr from roc on the reftest.  The rest of the code has already been reviewed.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 356922 [details] [diff] [review]
Unified patch

   if(index < 0)
     return directions[0]; // default: topleft
+  else if (index >= 8 && GetStyleVisibility()->mDirection == NS_STYLE_DIRECTION_RTL) {
+    // Directions 8 and higher are RTL-aware directions and should reverse the
+    // horizontal component if RTL.
+    Direction direction = directions[index];
+    direction.mHorizontal *= -1;
+    return direction;
+  }
   else
     return directions[index];

Can you fix this so we're not doing else after return? Thanks!

Comment 29

[(no longer active)]

(In reply to comment #28)
> Can you fix this so we're not doing else after return? Thanks!

You mean something like this?

   if(index < 0)
     return directions[0]; // default: topleft
-  else
-    return directions[index];
+  else {
+    Direction direction = directions[index];
+    // Directions 8 and higher are RTL-aware directions and should reverse the
+    // horizontal component if RTL.
+    if (index >= 8 && GetStyleVisibility()->mDirection == NS_STYLE_DIRECTION_RTL)
+      direction.mHorizontal *= -1;
+    return direction;
+  }
 }

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No, like

  if (index < 0)
    return directions[0]; // default: topleft
  if (index >= 8 && && GetStyleVisibility()->mDirection == NS_STYLE_DIRECTION_RTL) {
    // Directions 8 and higher are RTL-aware directions and should reverse the
    // horizontal component if RTL.
    Direction direction = directions[index];
    direction.mHorizontal *= -1;
    return direction;
  }
  return directions[index];

Comment 31

[(no longer active)]

Ah, OK.  Silly me  :-)

Comment 32

[Asaf Romano (gone)]

Comment on attachment 356922 [details] [diff] [review]
Unified patch

r=mano

Comment 33

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/c45539c5f90e

Comment 34

[(no longer active)]

Comment on attachment 356922 [details] [diff] [review]
Unified patch

This would be a very nice fix for RTL locales to have in 1.9.1.  Requesting approval.

Comment 35

[Dão Gottwald [:dao]]

reftest.list syntax fix:

http://hg.mozilla.org/mozilla-central/rev/75ab48244c1e

Comment 36

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(In reply to comment #34)
> (From update of attachment 356922 [details] [diff] [review])
> This would be a very nice fix for RTL locales to have in 1.9.1.  Requesting
> approval.

I think bug 474693 should be fixed first (it should be relatively simple).

Comment 37

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(In reply to comment #14)
> Finally got around to unbitrotting the part 1 patch.  Note that the only
> RTL-aware direction being implemented here is bottomend because, AFAIK, that's
> really the only direction that is ever used...

Hmmm... I should have noticed this the first time I looked at it (when I filed bug 474693), but I'm a little unhappy about the "bottomend" name being used.  In particular, it's mixing physical dimensions with logical ones, which could get us into big trouble if we ever support vertical text (where bottom and end are the same side).  Then again, I'm not sure if anybody would understand what the "afterend" corner is.

(XSL, and other specs following it, have used the convention of the logical edges being before (which is top for English), start (which is left for English), end (which is right for English), and after (which is bottom for English).)

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How about just calling it "end"? It's at the end of the reading order.

Comment 39

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(In reply to comment #38)
> How about just calling it "end"? It's at the end of the reading order.

No, since left and right are also valid options already, we might want end to mean right-for-LTR and left-for-RTL.

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

endcorner?

Comment 41

[(no longer active)]

+1 for endcorner.

Comment 42

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

The other question here regarding the approval request is about the presence of the late-compat keyword:  what could this break, and what is it likely to break?

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't know. I don't think there should be any compat concerns.

Comment 44

[Dão Gottwald [:dao]]

It breaks themes that don't style dir="bottomend".

Comment 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That's true. That probably is too late for 1.9.1.

Ehsan, can we work around this for 1.9.1 by using script to flip the resizer direction for RTL chrome?

Comment 46

[(no longer active)]

(In reply to comment #45)
> Ehsan, can we work around this for 1.9.1 by using script to flip the resizer
> direction for RTL chrome?

Yes, I suppose we can.  Do you want me to prepare a separate patch which does this and ask for review, etc.?

Comment 47

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes please. We'll take that for 1.9.1 only, and we can take this patch on trunk.

Comment 48

[(no longer active)]

OK, sure.  In this case, this is no longer late-compat.

Comment 49

[(no longer active)]

I spoke too soon without testing it.  It won't work, because that way we would have to style resizer[dir="bottomleft"] in global.css, which would lead to the same problem.

As an alternative solution, would it be appropriate to move the stuff in resizer.css to xul.css for 1.9.1, so that themes won't have to do anything special (but still be able to override what we're doing there)?

Comment 50

[Eric Shepherd [:sheppy]]

Now documented here: https://developer.mozilla.org/En/XUL/Attribute/Resizer.dir (for Gecko 1.9.2)

Comment 51

[(no longer active)]

(In reply to comment #49)
> As an alternative solution, would it be appropriate to move the stuff in
> resizer.css to xul.css for 1.9.1, so that themes won't have to do anything
> special (but still be able to override what we're doing there)?

Roc, Neil, what do you think?

Comment 52

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't really know what themes currently do with resizer.css that would be affected.

Comment 53

[(no longer active)]

(In reply to comment #52)
> I don't really know what themes currently do with resizer.css that would be
> affected.

Themes that work with 1.9.1 builds do not provide resizer.css as of now, because that file does not yet exist on 1.9.1.  As I understand it, if we add the rules of resizer.css inside xul.css, then themes which do not provide resizer.css would be fine.  Dão, can you please confirm that this solution would fix your concerns?

Comment 54

[Dão Gottwald [:dao]]

Yes, that should work.

Comment 55

[(no longer active)]

Attachment: 1.9.1 branch patch

1.9.1 branch patch with resizer.css stuff moved to xul.css.  Requesting review from Enn on the xul.css changes.

Comment 56

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 369480 [details] [diff] [review]
1.9.1 branch patch

a1.9.1=dbaron (but don't forget to land the regression fixes too!)

Comment 57

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/af624384af85

(In reply to comment #56)
> (From update of attachment 369480 [details] [diff] [review])
> a1.9.1=dbaron (but don't forget to land the regression fixes too!)

Regression fix also landed as <http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0c9cb75bc9ba>.

Comment 58

[[e-mail]]

Comment on attachment 369480 [details] [diff] [review]
1.9.1 branch patch

>+  -moz-appearance: resizer;
-moz-appearance rules don't belong in xul.css :-(

Comment 59

[(no longer active)]

(In reply to comment #58)
> (From update of attachment 369480 [details] [diff] [review])
> >+  -moz-appearance: resizer;
> -moz-appearance rules don't belong in xul.css :-(

We did that because of the fact that we couldn't add a new css file for 1.9.1.  The -moz-appearance rules on trunk appear in the appropriate theme files.