587248 - Add RTL support to Panorama

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect, P3)

Description

[juan becerra [:juanb]]

It might be the least of problems with RTL, but I can't seem to be able to select the tab candy button on an ar build (from Aug 13). Clean profile.

This is probably more related to theme work/toolbars than tab candy.

Earlier I had installed this build on an existing profile, and when doing anything inside Tab Candy I could not select any groups or tabs, or get out of tab candy mode.

Comment 1

[Axel Hecht [:Pike]]

Ehsan, I guess you have your own blockers to work on? I'll CC you on RTL bugs nevertheless, or is there a better alias?

Comment 2

[(no longer active)]

(In reply to comment #1)
> Ehsan, I guess you have your own blockers to work on? I'll CC you on RTL bugs
> nevertheless, or is there a better alias?

Well, there's no alias, but since I'm probably the only developer who cares about RTL on a personal level, I'll take this!

Oh, and now that practically everything in the browser works in RTL locales, this does need to block the final release of Firefox 4.

Comment 3

[Simon Montagu :smontagu]

(In reply to comment #2)

> Well, there's no alias, but since I'm probably the only developer who cares
> about RTL on a personal level, I'll take this!

This is not true, but I'm not going to fight over taking this bug ;-)

> 
> Oh, and now that practically everything in the browser works in RTL locales,
> this does need to block the final release of Firefox 4.

Agreed.

Comment 4

[Elad Alfassa]

(In reply to comment #0)
> It might be the least of problems with RTL, but I can't seem to be able to
> select the tab candy button on an ar build (from Aug 13). Clean profile.
> 
> This is probably more related to theme work/toolbars than tab candy.
> 
> Earlier I had installed this build on an existing profile, and when doing
> anything inside Tab Candy I could not select any groups or tabs, or get out of
> tab candy mode.

I could not reproduce the problem you are describing, but Firefox Panorama has other problem related to RTL:
the rightmost tab will appear leftmost.
in other words, the whole Firefox Panorama UI is not aligned to the right.

it may look like a minor problem, but it causes weird behavior when moving tabs around.

Comment 5

[Aza Raskin [:aza]]

What is the status of this bug?

Comment 6

[(no longer active)]

I'm going to write a small piece of JS which parses out the locale.dir value from global.dtd, so that we can use it in tabcandy's HTML file (which is not an XHTML file and therefore cannot load global.dtd as an entity).  Axel, Simon, do you guys think that's a good idea?

Comment 7

[Axel Hecht [:Pike]]

How does that relate to just making the html an xhtml file, and make it include global.dtd?

Comment 8

[(no longer active)]

(In reply to comment #7)
> How does that relate to just making the html an xhtml file, and make it include
> global.dtd?

Well, that may be very tricky because of the differences in HTML and XHTML documents, which could mean that random things may break right now, or in the future.  We tried that once with file listing pages, and the result was horrible (we still have open bugs on them) so I'd rather not make the same mistake twice.

Comment 9

[Axel Hecht [:Pike]]

Well, one could argue that the mistake was to use html instead of xhtml in the beginning.

I'm personally not subscribing to not breaking whatever "small piece of js" you come up with. It's a one-off solution, and it'll likely be subject to fuzz testing in the wild of our 80+ in tree localizations, plus the unnumbered out there in the real wild.

It's the usual rock and hard place, I guess.

Comment 10

[(no longer active)]

(In reply to comment #9)
> Well, one could argue that the mistake was to use html instead of xhtml in the
> beginning.

Hmm, not really.  XHTML is tricky to get right, and that's why almost nobody in the world uses it.

> I'm personally not subscribing to not breaking whatever "small piece of js" you
> come up with. It's a one-off solution, and it'll likely be subject to fuzz
> testing in the wild of our 80+ in tree localizations, plus the unnumbered out
> there in the real wild.

Hmm, I'm not sure how to parse this.  I was not planning on writing my own parser, I was planning to use our existing DTD parser, to guarantee that things work if global.dtd works in other places as well...

Comment 11

[(no longer active)]

Actually a better option would be to query the XUL chrome registry for this information.  Axel agreed on IRC, so that's what I'll be doing.

Comment 12

[(no longer active)]

Attachment: Part 1: Add support for retrieving the default UI direction to toolkit

Comment 13

[Axel Hecht [:Pike]]

Comment on attachment 480777 [details] [diff] [review]
Part 1: Add support for retrieving the default UI direction to toolkit

I don't think that "LocaleDir" is a convincing API, fwiw. Not that I can come up with a convincing one ad-hoc, or even in an fx4 time frame.

Comment 14

[Dão Gottwald [:dao]]

(In reply to comment #8)
> (In reply to comment #7)
> > How does that relate to just making the html an xhtml file, and make it include
> > global.dtd?
> 
> Well, that may be very tricky because of the differences in HTML and XHTML
> documents, which could mean that random things may break right now, or in the
> future.  We tried that once with file listing pages, and the result was
> horrible (we still have open bugs on them) so I'd rather not make the same
> mistake twice.

It wasn't that horrible. As far as I remember there was a problem with the encoding of some FTP dir listings, but that wouldn't apply here.

(In reply to comment #10)
> > Well, one could argue that the mistake was to use html instead of xhtml in the
> > beginning.
> 
> Hmm, not really.  XHTML is tricky to get right, and that's why almost nobody in
> the world uses it.

Compared to HTML the tricky part of XHTML is XML, which we already use for most parts of the browser chrome, so...

Comment 15

[(no longer active)]

(In reply to comment #13)
> I don't think that "LocaleDir" is a convincing API, fwiw. Not that I can come
> up with a convincing one ad-hoc, or even in an fx4 time frame.

What do you mean exactly by a convincing API?

Comment 16

[(no longer active)]

(In reply to comment #14)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > How does that relate to just making the html an xhtml file, and make it include
> > > global.dtd?
> > 
> > Well, that may be very tricky because of the differences in HTML and XHTML
> > documents, which could mean that random things may break right now, or in the
> > future.  We tried that once with file listing pages, and the result was
> > horrible (we still have open bugs on them) so I'd rather not make the same
> > mistake twice.
> 
> It wasn't that horrible. As far as I remember there was a problem with the
> encoding of some FTP dir listings, but that wouldn't apply here.

Well, by horrible I mean that there is no way to fix this problem in XHTML, and we have to switch back to using HTML somehow.

> (In reply to comment #10)
> > > Well, one could argue that the mistake was to use html instead of xhtml in the
> > > beginning.
> > 
> > Hmm, not really.  XHTML is tricky to get right, and that's why almost nobody in
> > the world uses it.
> 
> Compared to HTML the tricky part of XHTML is XML, which we already use for most
> parts of the browser chrome, so...

Here's the most complete list that I know of <http://wiki.whatwg.org/wiki/HTML_vs._XHTML>.  Anyway, unless someone suggests a real problem with just retrieving the locale direction directly, I don't think that this is a useful discussion to have.

Comment 17

[Axel Hecht [:Pike]]

(In reply to comment #15)
> (In reply to comment #13)
> > I don't think that "LocaleDir" is a convincing API, fwiw. Not that I can come
> > up with a convincing one ad-hoc, or even in an fx4 time frame.
> 
> What do you mean exactly by a convincing API?

I see jsm's as APIs that people should use. LocaleDir as a single-entry-point API doesn't convince me.

There's stuff like bcp47 locale code matching and other locale/chrome-reg related things that could go in there.

I'd probably just go for xpcom directly and file a bug for cleanup.

Comment 18

[(no longer active)]

Hmm, I don't think that I agree with you about this.  Gavin, can you please weigh in?  (As an aside, I'm waiting for Gavin's review on the first patch in this bug before I move ahead with the rest of the code...)

Comment 19

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

Comment on attachment 480777 [details] [diff] [review]
Part 1: Add support for retrieving the default UI direction to toolkit

I'm not sure I see much benefit to adding a JS module for what boils down to a couple of lines of code, with AFAICT only one planned user at the moment. I'd rather just add nsIXULChromeRegistry to Services.jsm to make the boilerplate smaller, and just use it directly for now (can always refactor later if a greater need arises).

Comment 20

[(no longer active)]

Fair enough.  Thanks a lot for weighing in Gavin!  :-)

Comment 21

[(no longer active)]

Attachment: Part 1: set the correct direction on the tabview document

Comment 22

[(no longer active)]

Attachment: Part 2: Add RTL support to content CSS for TabView

Comment 23

[(no longer active)]

Attachment: Part 3: Add RTL support to theme CSS for TabView on Mac

Comment 24

[Dão Gottwald [:dao]]

Comment on attachment 487717 [details] [diff] [review]
Part 3: Add RTL support to theme CSS for TabView on Mac

>@@ -18,41 +18,65 @@ body {
>   background-color: #D7D7D7;
>   border-radius: 0.4em;
>   box-shadow: 0 1px 1.5px rgba(0, 0, 0, 0.4);
>   border: 1px solid rgba(255, 255, 255, 0.5);
>   cursor: pointer;
>   margin: 8px;
> }
> 
>+html[dir=rtl] .tab {
>+  padding-right: 4px;
>+  padding-left: 6px;
>+  border-radius: 0.4em;
>+}

-moz-padding-start/end should be used in the .tab section

>+html[dir=rtl] .favicon {
>+  border-bottom-left-radius: 0.4em;
>+  box-shadow:
>+    0 -1px 0 rgba(225, 225, 225, 0.8) inset,
>+    1px 0 0 rgba(225, 225, 225, 0.8) inset;
>+  padding-right: 4px;
>+  padding-left: 6px;

ditto

> .overlay {
>   background-color: rgba(0,0,0,.7) !important;
>   box-shadow: 3px 3px 5.5px rgba(0,0,0,.5);
>   border-radius: 0.4em;
>   /*
>   border: 1px solid rgba(230,230,230,1);
>   background-color: rgba(248,248,248,1);
>   box-shadow:
>     rgba(0,0,0, .3) 2px 2px 5.5px,
>     inset rgba(255, 255, 255, 0.6) 0 0 0 2px; */
> }
> 
>+html[dir=rtl] .overlay {
>+  box-shadow: -3px 3px 5.5px rgba(0,0,0,.5);
>+}

This appears to be a different shadow for rtl, not just a different orientation.

> .guideTrench {
>   opacity: 0.9;
>   border: 1px dashed  rgba(0,0,0,.12);
>   border-bottom: none;
>+  box-shadow: 1px 1px 0 rgba(255,255,255,.15);
>+}
>+
>+html[dir=ltr] .guideTrench {
>   border-right: none;
>-  box-shadow: 1px 1px 0 rgba(255,255,255,.15);
>+}
>+
>+html[dir=rtl] .guideTrench {
>+  border-left: none;
>+  box-shadow: -1px 1px 0 rgba(255,255,255,.15);
> }

-moz-border-end/start

> input.name {
>   background: transparent;
>   border: 1px solid transparent;
>   color: #999;
>   margin: 3px 0px 0px 3px;
>   padding: 1px;
>   background-image: url(chrome://browser/skin/tabview/edit-light.png);
>-  padding-left: 20px;
>+  -moz-padding-start: 20px;
>+}
>+
>+html[dir=rtl] input.name {
>+  margin-left: 0;
>+  margin-right: 3px;
>+  background-position: right top;
> }

-moz-margin-start/end

>   margin: 3px 0px 0px 3px;
>   padding: 1px;
>   left: 0;
>   top: 0;
>   width: 100%;
>   height: 100%;
> }
> 
>+html[dir=rtl] .title-shield {
>+  margin-left: 0;
>+  margin-right: 3px;
>+  left: auto;
>+  right: 0;
>+}

ditto

Comment 25

[(no longer active)]

(In reply to comment #24)
> > .overlay {
> >   background-color: rgba(0,0,0,.7) !important;
> >   box-shadow: 3px 3px 5.5px rgba(0,0,0,.5);
> >   border-radius: 0.4em;
> >   /*
> >   border: 1px solid rgba(230,230,230,1);
> >   background-color: rgba(248,248,248,1);
> >   box-shadow:
> >     rgba(0,0,0, .3) 2px 2px 5.5px,
> >     inset rgba(255, 255, 255, 0.6) 0 0 0 2px; */
> > }
> > 
> >+html[dir=rtl] .overlay {
> >+  box-shadow: -3px 3px 5.5px rgba(0,0,0,.5);
> >+}
> 
> This appears to be a different shadow for rtl, not just a different
> orientation.


No, I only switched the X coordinate's sign.  I think you're confused by the box-shadow rule in comments.

I'll address the rest of the comments in the next iteration.

Comment 26

[Dão Gottwald [:dao]]

(In reply to comment #25)
> I think you're confused by the box-shadow rule in comments.

Ah, yes, please remove that :(

Comment 27

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

Comment on attachment 487683 [details] [diff] [review]
Part 1: set the correct direction on the tabview document

Seems like you could refactor some of that test code to avoid unnecessary duplication (e.g. combining onTabViewLoadedAndShownLTR/onTabViewLoadedAndShownRTL, helper for the button.doCommand code, etc.).

Comment 28

[(no longer active)]

Attachment: Part 3: Add RTL support to theme CSS for TabView on Mac

With comments addressed.

Comment 29

[(no longer active)]

Attachment: Part 1: set the correct direction on the tabview document

With comments addressed.

Comment 30

[(no longer active)]

Attachment: Part 4: Add RTL support to theme CSS for TabView on Windows

Comment 31

[(no longer active)]

Attachment: Part 5: Add RTL support to theme CSS for TabView on Linux

Comment 32

[(no longer active)]

Attachment: Part 6: Add RTL support to the TabView functionality implemented in ui.js

Comment 33

[(no longer active)]

Attachment: Part 6: Add RTL support to the TabView functionality implemented in ui.js

Comment 34

[(no longer active)]

Attachment: Part 7: Correct some direction dependent CSS code in tabitems.js

Comment 35

[Dão Gottwald [:dao]]

Comment on attachment 487806 [details] [diff] [review]
Part 3: Add RTL support to theme CSS for TabView on Mac

> .tab {
>-  padding: 4px 6px 6px 4px;
>+  padding-top: 4px;
>+  -moz-padding-end: 6px;
>+  padding-bottom: 6px;
>+  -moz-padding-start: 4px;
>   background-color: #D7D7D7;
>   border-radius: 0.4em;
>   box-shadow: 0 1px 1.5px rgba(0, 0, 0, 0.4);
>   border: 1px solid rgba(255, 255, 255, 0.5);
>   cursor: pointer;
>   margin: 8px;
> }
> 
>+html[dir=rtl] .tab {
>+  border-radius: 0.4em;
>+}

Remove the html[dir=rtl] .tab rule.

> input.name {
>   background: transparent;
>   border: 1px solid transparent;
>   color: #999;
>-  margin: 3px 0px 0px 3px;
>+  margin-top: 3px;
>+  -moz-margin-end: 0;
>+  margin-bottom: 0;
>+  -moz-margin-begin: 3px;
>   padding: 1px;
>   background-image: url(chrome://browser/skin/tabview/edit-light.png);
>-  padding-left: 20px;
>+  -moz-padding-start: 20px;
>+}
>+
>+html[dir=rtl] input.name {
>+  background-position: right top;
> }

What prevents the image from being repeated here?

> #exit-button {
>   cursor: default;
>   top: 0;
>   right: 0;
>   width: 28px;
>   height: 27px;
>   background: -moz-image-rect(url(chrome://browser/skin/tabview/tabview.png), 0, 20, 20, 0) no-repeat scroll 4px 4px #b7b7b7;
>   border-bottom: 1px solid #909090;
>-  border-left: 1px solid #B7B7B7;
>+  -moz-border-start: 1px solid #B7B7B7;
>   border-top: 1px solid #CFCFCF;
>-  border-radius: 3px 0 0 3px;
>+}
>+
>+html[dir=rtl] #exit-button {
>+  right: auto;
>+  left: 0;
>+  border-radius: 0 3px 3px 0;
> }

Removing border-radius: 3px 0 0 3px; seems wrong.

> #otherresults .label {
>   color: #999;
>   line-height:30px;
>-  margin-left:5px;
>-  margin-right: 5px;
>+  -moz-margin-start:5px;

Add a space after the colon.

r=me with those things addressed

Comment 36

[(no longer active)]

Attachment: Part 8: Fix resizing and drag and drop in RTL mode

Comment 37

[(no longer active)]

Attachment: Part 9: Correct group title style calculations in RTL mode (well, and some LTR corrections as well!)

Comment 38

[(no longer active)]

(In reply to comment #35)
> > input.name {
> >   background: transparent;
> >   border: 1px solid transparent;
> >   color: #999;
> >-  margin: 3px 0px 0px 3px;
> >+  margin-top: 3px;
> >+  -moz-margin-end: 0;
> >+  margin-bottom: 0;
> >+  -moz-margin-begin: 3px;
> >   padding: 1px;
> >   background-image: url(chrome://browser/skin/tabview/edit-light.png);
> >-  padding-left: 20px;
> >+  -moz-padding-start: 20px;
> >+}
> >+
> >+html[dir=rtl] input.name {
> >+  background-position: right top;
> > }
> 
> What prevents the image from being repeated here?

Why would we repeat it here?  It's the same image for both cases.

Will address the rest of the comments in the next iteration.

Comment 39

[Dão Gottwald [:dao]]

repeat as in background-repeat.

Comment 40

[(no longer active)]

Attachment: Part 10: Arrange tabs from right to left in RTL mode

Comment 41

[(no longer active)]

(In reply to comment #39)
> repeat as in background-repeat.

Oh, I'm already handled that in attachment 488016 [details] [diff] [review].  :-)

Comment 42

[(no longer active)]

Attachment: Part 11: Rotate the collapsed tab items towards top right in RTL mode

Comment 43

[(no longer active)]

Attachment: Part 3: Add RTL support to theme CSS for TabView on Mac

With Dao's comments addressed.

Comment 44

[(no longer active)]

I think that I've covered all of the cases where we have to act differently in RTL mode.  I've also asked Ian to test the patches on this bug to make sure that I haven't missed anything.  These are ready to land as far as I'm concerned, sans any r-'es.

Comment 45

[Dão Gottwald [:dao]]

Comment on attachment 487953 [details] [diff] [review]
Part 4: Add RTL support to theme CSS for TabView on Windows

> .guideTrench {
>   opacity: 0.9;
>   border: 1px dashed  rgba(0,0,0,.12);
>   border-bottom: none;
>+  box-shadow: 1px 1px 0 rgba(255,255,255,.15);
>+}
>+
>+html[dir=ltr] .guideTrench {
>   border-right: none;
>-  box-shadow: 1px 1px 0 rgba(255,255,255,.15);
>+}
>+
>+html[dir=rtl] .guideTrench {
>+  border-left: none;
>+  box-shadow: -1px 1px 0 rgba(255,255,255,.15);
> }

> #searchbox{
>   width: 270px;
>   height: 30px;
>   box-shadow: 0px 1px 0px rgba(255,255,255,.5), 0px -1px 0px rgba(0,0,0,1), 0px 0px 9px rgba(0,0,0,.8);
>   color: white;
>   border: none;
>   background-color: #272727;
>   border-radius: 0.4em;
>-  padding-left:5px; padding-right: 5px;
>+  -moz-padding-start:5px;

Please fix according to the comments on the other patch.

>+html[dir=ltr] #actions{
>+  border-bottom-left-radius: 0.4em;
>+  border-top-left-radius: 0.4em;    
>+}
>+
>+html[dir=rtl] #actions{
>+  border-bottom-right-radius: 0.4em;
>+  border-top-right-radius: 0.4em;    
>+  box-shadow: 0px 1px 0px rgba(255,255,255,.5), 0px -1px 0px rgba(0,0,0,.8), inset -6px 6px 9px rgba(0,0,0,.56);
>+}

Insert a space before {

Comment 46

[Dão Gottwald [:dao]]

Comment on attachment 487965 [details] [diff] [review]
Part 5: Add RTL support to theme CSS for TabView on Linux

> #searchbox{
>   width: 270px;
>   height: 30px;
>   box-shadow: 0px 1px 0px rgba(255,255,255,.5), 0px -1px 0px rgba(0,0,0,1), 0px 0px 9px rgba(0,0,0,.8);
>   color: white;
>   border: none;
>   background-color: #272727;
>   border-radius: 0.4em;
>-  padding-left:5px; padding-right: 5px;
>+  -moz-padding-start:5px;

ditto (add space)

Comment 47

[(no longer active)]

Attachment: Part 4: Add RTL support to theme CSS for TabView on Windows

Comment 48

[(no longer active)]

Attachment: Part 5: Add RTL support to theme CSS for TabView on Linux

Comment 49

[Dão Gottwald [:dao]]

Comment on attachment 488190 [details] [diff] [review]
Part 4: Add RTL support to theme CSS for TabView on Windows

>+html[dir=ltr] #actions {
>+  border-bottom-left-radius: 0.4em;
>+  border-top-left-radius: 0.4em;    

trailing spaces

>+}
>+
>+html[dir=rtl] #actions {
>+  border-bottom-right-radius: 0.4em;
>+  border-top-right-radius: 0.4em;    

ditto

Comment 50

[(no longer active)]

Attachment: Part 4: Add RTL support to theme CSS for TabView on Windows

Comment 51

[Ian Gilman [:iangilman]]

Comment on attachment 487982 [details] [diff] [review]
Part 6: Add RTL support to the TabView functionality implemented in ui.js

>+  get rtl() {
>+    return document.documentElement.getAttribute("dir") == "rtl";
>+  },

Can we cache this value instead of checking every time? I imagine we may be accessing this property from some high performance code.

Otherwise looking good. I haven't run the code yet... I figure I'll review first.

Btw, I don't know that I'm authorized to give an official r+ for anything, though maybe we should look into changing that... if I'm not the "module owner" for Panorama, I don't know who is.

Comment 52

[(no longer active)]

(In reply to comment #51)
> Comment on attachment 487982 [details] [diff] [review]
> Part 6: Add RTL support to the TabView functionality implemented in ui.js
> 
> >+  get rtl() {
> >+    return document.documentElement.getAttribute("dir") == "rtl";
> >+  },
> 
> Can we cache this value instead of checking every time? I imagine we may be
> accessing this property from some high performance code.

We could, but I'd rather not, because the direction can change (by Force RTL fore example), and a couple of calls through DOM shouldn't really make us noticeably slow.

> Btw, I don't know that I'm authorized to give an official r+ for anything,
> though maybe we should look into changing that... if I'm not the "module owner"
> for Panorama, I don't know who is.

Gavin said that he wants you to review these, so I think we should be fine in that regard.

Comment 53

[Ian Gilman [:iangilman]]

Comment on attachment 487988 [details] [diff] [review]
Part 7: Correct some direction dependent CSS code in tabitems.js

Looks good.

Comment 54

[Ian Gilman [:iangilman]]

Comment on attachment 488004 [details] [diff] [review]
Part 8: Fix resizing and drag and drop in RTL mode

Looks good.

Comment 55

[Ian Gilman [:iangilman]]

Comment on attachment 488016 [details] [diff] [review]
Part 9: Correct group title style calculations in RTL mode (well, and some LTR corrections as well!)

Looks good

Comment 56

[Ian Gilman [:iangilman]]

Comment on attachment 488060 [details] [diff] [review]
Part 10: Arrange tabs from right to left in RTL mode

>+    var initialOffset = 0;

I know there's lots of var around this code, but for new code we're trying to use let.

>+      box.left += (UI.rtl ? -1 : 1) * (box.width + padding);

This is one of our hot loops, so I'd prefer not to check UI.rtl every time through it; just calculate the x offset once before the loop.

r+ with that

Comment 57

[Ian Gilman [:iangilman]]

Comment on attachment 488071 [details] [diff] [review]
Part 11: Rotate the collapsed tab items towards top right in RTL mode

>+        if (Utils.rtl) {
>+          rightMostRight = rects[0].right;
>+        } else {
>+          for each (var rect in rects) {
>+            if (rect.right > rightMostRight)
>+              rightMostRight = rect.right;
>+            else
>+              break;
>+          }

Shouldn't that be UI.rtl?

By the way, this chunk will go away entirely in bug 609446.

Comment 58

[(no longer active)]

Comment on attachment 488004 [details] [diff] [review]
Part 8: Fix resizing and drag and drop in RTL mode

>diff --git a/browser/base/content/tabview/groupitems.js b/browser/base/content/tabview/groupitems.js
>--- a/browser/base/content/tabview/groupitems.js
>+++ b/browser/base/content/tabview/groupitems.js
>@@ -448,18 +448,18 @@ GroupItem.prototype = Utils.extend(new I
>     if (!Utils.isRect(rect)) {
>       Utils.trace('GroupItem.setBounds: rect is not a real rectangle!', rect);
>       return;
>     }
> 
>     if (!options)
>       options = {};
> 
>-    rect.width = Math.max(110, rect.width);
>-    rect.height = Math.max(125, rect.height);
>+    rect.width = Math.max(90, rect.width);
>+    rect.height = Math.max(90, rect.height);
> 
>     var titleHeight = this.$titlebar.height();
> 
>     // ___ Determine what has changed
>     var css = {};
>     var titlebarCSS = {};
>     var contentCSS = {};
> 

So, apparently this hunk causes a leak, like this:

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1289274631.1289277417.25253.gz&fulltext=1#err0

I'm still trying to debug and understand the leak...

Comment 59

[(no longer active)]

The leak doesn't seem to be fixed with the patch to bug 611089...

Comment 60

[(no longer active)]

Attachment: Part 12: Replace UI.rtl with a static property to make sure that we don't incur any DOM performance penalty when checking it

Comment 61

[Ian Gilman [:iangilman]]

Comment on attachment 489622 [details] [diff] [review]
Part 12: Replace UI.rtl with a static property to make sure that we don't incur any DOM performance penalty when checking it

Lovely... thanks for doing this.

Comment 62

[Ian Gilman [:iangilman]]

I've now had a chance to run it; by and large it's looking great! Issues:

* If you drag a tab or group left so it pushes against the left wall, the item gets squished rather than pinning itself. It should just stop but not change size, as it does against the other three walls. 

* The tabs hang too far to the right in their group (when they're not stacked, and only when no app tabs are present). At first I thought it may be related to bug 597931, but now I'm not so sure.

That's all I could see... nicely done.

Comment 63

[(no longer active)]

Attachment: Part 13: Correct the welcome box positioning on first run in RTL mode

Comment 64

[(no longer active)]

Attachment: Part 14: Observe the group box right margin when arranging tab items in RTL mode

Comment 65

[(no longer active)]

(In reply to comment #62)
> I've now had a chance to run it; by and large it's looking great! Issues:
> 
> * If you drag a tab or group left so it pushes against the left wall, the item
> gets squished rather than pinning itself. It should just stop but not change
> size, as it does against the other three walls. 

I'm not sure at all what this means...

> * The tabs hang too far to the right in their group (when they're not stacked,
> and only when no app tabs are present). At first I thought it may be related to
> bug 597931, but now I'm not so sure.

Part 14 should have fixed this (don't know what changed in tabcandy code, but this wasn't an issue when I first wrote the patches).

> That's all I could see... nicely done.

Great!  Please note that once these patches land, all the UI/UX changes should be tested in RTL mode as well (and yes, I can help with that).

Comment 66

[Ian Gilman [:iangilman]]

Comment on attachment 489664 [details] [diff] [review]
Part 14: Observe the group box right margin when arranging tab items in RTL mode

Looks good.

Comment 67

[Ian Gilman [:iangilman]]

Comment on attachment 489660 [details] [diff] [review]
Part 13: Correct the welcome box positioning on first run in RTL mode

>-      let welcomeBounds = new Rect(box.right + padding, box.top,
>+      let welcomeBounds = new Rect(UI.rtl ? $actions.width() : (box.right + padding), box.top,
>                                    welcomeWidth, welcomeWidth * aspect);

I believe this will make the welcome tab smashed up against the actions. It should be $actions.width() + padding. This probably means you need more padding on the change above as well. 

Actually, probably the cleanest thing is to add $actions.width() to pageBounds.left at the same time you adjust page bounds.width, and then use it as appropriate.

Comment 68

[(no longer active)]

Attachment: Part 13: Correct the welcome box positioning on first run in RTL mode

(In reply to comment #67)
> Comment on attachment 489660 [details] [diff] [review]
> Part 13: Correct the welcome box positioning on first run in RTL mode
> 
> >-      let welcomeBounds = new Rect(box.right + padding, box.top,
> >+      let welcomeBounds = new Rect(UI.rtl ? $actions.width() : (box.right + padding), box.top,
> >                                    welcomeWidth, welcomeWidth * aspect);
> 
> I believe this will make the welcome tab smashed up against the actions. It
> should be $actions.width() + padding. This probably means you need more padding
> on the change above as well. 

This is actually a problem with LTR mode as well.  I fixed both of them in this revision of the patch.

Comment 69

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #68)
> Created attachment 489686 [details] [diff] [review]
> Part 13: Correct the welcome box positioning on first run in RTL mode
> 
> (In reply to comment #67)
> > Comment on attachment 489660 [details] [diff] [review] [details]
> > Part 13: Correct the welcome box positioning on first run in RTL mode
> > 
> > >-      let welcomeBounds = new Rect(box.right + padding, box.top,
> > >+      let welcomeBounds = new Rect(UI.rtl ? $actions.width() : (box.right + padding), box.top,
> > >                                    welcomeWidth, welcomeWidth * aspect);
> > 
> > I believe this will make the welcome tab smashed up against the actions. It
> > should be $actions.width() + padding. This probably means you need more padding
> > on the change above as well. 
> 
> This is actually a problem with LTR mode as well.  I fixed both of them in this
> revision of the patch.

Ehsan, I'm not sure what exactly was a problem in LTR. In LTR, we already adjusted the pageBounds.width to not include the actions area width, so it shouldn't have been pushed up agains the actions. Looking at your patch, it's not clear to me what changes were made in the LTR behavior. Could you elaborate?

Comment 70

[(no longer active)]

(In reply to comment #69)
> (In reply to comment #68)
> > Created attachment 489686 [details] [diff] [review] [details]
> > Part 13: Correct the welcome box positioning on first run in RTL mode
> > 
> > (In reply to comment #67)
> > > Comment on attachment 489660 [details] [diff] [review] [details] [details]
> > > Part 13: Correct the welcome box positioning on first run in RTL mode
> > > 
> > > >-      let welcomeBounds = new Rect(box.right + padding, box.top,
> > > >+      let welcomeBounds = new Rect(UI.rtl ? $actions.width() : (box.right + padding), box.top,
> > > >                                    welcomeWidth, welcomeWidth * aspect);
> > > 
> > > I believe this will make the welcome tab smashed up against the actions. It
> > > should be $actions.width() + padding. This probably means you need more padding
> > > on the change above as well. 
> > 
> > This is actually a problem with LTR mode as well.  I fixed both of them in this
> > revision of the patch.
> 
> Ehsan, I'm not sure what exactly was a problem in LTR. In LTR, we already
> adjusted the pageBounds.width to not include the actions area width, so it
> shouldn't have been pushed up agains the actions. Looking at your patch, it's
> not clear to me what changes were made in the LTR behavior. Could you
> elaborate?

In LTR mode, we only adjust the box width to not include the actions width, which means that the welcome box will kind of touch the left side of the action buttons.

This change will make the welcome box have a padding between it and the action buttons:

-      let welcomeBounds = new Rect(box.right + padding, box.top,
+      let welcomeBounds = new Rect(UI.rtl ? pageBounds.left : box.right, box.top,
                                    welcomeWidth, welcomeWidth * aspect);

Comment 71

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #70)
> In LTR mode, we only adjust the box width to not include the actions width,
> which means that the welcome box will kind of touch the left side of the action
> buttons.

Ah, I see. Thanks!

Comment 72

[Ian Gilman [:iangilman]]

(In reply to comment #65)
> (In reply to comment #62)
> > I've now had a chance to run it; by and large it's looking great! Issues:
> > 
> > * If you drag a tab or group left so it pushes against the left wall, the item
> > gets squished rather than pinning itself. It should just stop but not change
> > size, as it does against the other three walls. 
> 
> I'm not sure at all what this means...

Somehow our "keep items within the window while dragging" code is causing a resize when pushing against the left side of the window. Presumably this is somewhere in Drag.snap (in drag.js), though I'm not familiar with that code. Mitcho, perhaps you can point Ehsan in the right direction?

Comment 73

[Ian Gilman [:iangilman]]

(In reply to comment #58)
> >-    rect.width = Math.max(110, rect.width);
> >-    rect.height = Math.max(125, rect.height);
> >+    rect.width = Math.max(90, rect.width);
> >+    rect.height = Math.max(90, rect.height);
> 
> So, apparently this hunk causes a leak, like this:

Doesn't seem possible... you're just changing some coordinates!

Even so, if this really is it, we'd rather change the resizeOptions.minWidth/minHeight anyway, like so:

   setResizable: function GroupItem_setResizable(value, immediately) {
-    this.resizeOptions.minWidth = 90;
-    this.resizeOptions.minHeight = 90;
+    this.resizeOptions.minWidth = 110;
+    this.resizeOptions.minHeight = 125;

Comment 74

[(no longer active)]

(In reply to comment #73)
> (In reply to comment #58)
> > >-    rect.width = Math.max(110, rect.width);
> > >-    rect.height = Math.max(125, rect.height);
> > >+    rect.width = Math.max(90, rect.width);
> > >+    rect.height = Math.max(90, rect.height);
> > 
> > So, apparently this hunk causes a leak, like this:
> 
> Doesn't seem possible... you're just changing some coordinates!

Trust me, it does cause leaks!  I've been spending the past few days trying to debug this.  I'll have more to day shortly...

> Even so, if this really is it, we'd rather change the
> resizeOptions.minWidth/minHeight anyway, like so:
> 
>    setResizable: function GroupItem_setResizable(value, immediately) {
> -    this.resizeOptions.minWidth = 90;
> -    this.resizeOptions.minHeight = 90;
> +    this.resizeOptions.minWidth = 110;
> +    this.resizeOptions.minHeight = 125;

I'll use this as a last resort, but I'd really like to figure out what's causing the leak.

Comment 75

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #72)
> (In reply to comment #65)
> > (In reply to comment #62)
> > > I've now had a chance to run it; by and large it's looking great! Issues:
> > > 
> > > * If you drag a tab or group left so it pushes against the left wall, the item
> > > gets squished rather than pinning itself. It should just stop but not change
> > > size, as it does against the other three walls. 
> > 
> > I'm not sure at all what this means...
> 
> Somehow our "keep items within the window while dragging" code is causing a
> resize when pushing against the left side of the window. Presumably this is
> somewhere in Drag.snap (in drag.js), though I'm not familiar with that code.
> Mitcho, perhaps you can point Ehsan in the right direction?

Perhaps this is a bug in Drag_snapToEdge. Make sure snapToEdge is called with assumeConstantSize true... I guess in RTL-land, it should also be given stationaryCorner = 'topright' (in LTR, it's 'topright'). If assumeConstantSize is set right, though, then that must mean it's a bug in its implementation.

Comment 76

[(no longer active)]

So, here is some very interesting info about the leak!  The leak goes away if I remove this test: <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/tabview/browser_tabview_startup_transitions.js>, but that test is wrong, really.  I will file a separate bug on that, I guess.

Comment 77

[(no longer active)]

Attachment: Part 14 - fix a test

Comment 78

[(no longer active)]

Attachment: Part 15 - Use the correct dimensions for groups

Comment 79

[(no longer active)]

Attachment: Part 16: Don't resize the groups if we explicitly request their size to remain constant while dragging

This solves the problem in comment 75.

Comment 80

[Ian Gilman [:iangilman]]

Comment on attachment 489686 [details] [diff] [review]
Part 13: Correct the welcome box positioning on first run in RTL mode

So does #actions have some built-in padding? If so, then this is good, except: 

>+      box.left = pageBounds.left + welcomeWidth + 2 * padding;

... doesn't seem like it would need 2 * padding.

If #actions doesn't have built-in padding (which is what I originally assumed), this patch needs some fixing.

Comment 81

[Ian Gilman [:iangilman]]

Comment on attachment 490003 [details] [diff] [review]
Part 14 - fix a test

Thanks for catching this!

Comment 82

[Ian Gilman [:iangilman]]

Comment on attachment 490004 [details] [diff] [review]
Part 15 - Use the correct dimensions for groups

Cool.

Comment 83

[Ian Gilman [:iangilman]]

Comment on attachment 490006 [details] [diff] [review]
Part 16: Don't resize the groups if we explicitly request their size to remain constant while dragging

Looks good to me, if Mitcho's happy with it.

Comment 84

[(no longer active)]

(In reply to comment #80)
> Comment on attachment 489686 [details] [diff] [review]
> Part 13: Correct the welcome box positioning on first run in RTL mode
> 
> So does #actions have some built-in padding? If so, then this is good, except: 

Well, kind of.  Its width is 30px, and the width of its buttons are 20px, so it does have kind of an implicit padding...

http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/browser/tabview/tabview.css#437

> >+      box.left = pageBounds.left + welcomeWidth + 2 * padding;
> 
> ... doesn't seem like it would need 2 * padding.

Why?  That will cause the welcome box to stick to the primary group.  Is that what we want?

Comment 85

[Ian Gilman [:iangilman]]

(In reply to comment #84)
> > >+      box.left = pageBounds.left + welcomeWidth + 2 * padding;
> > 
> > ... doesn't seem like it would need 2 * padding.
> 
> Why?  That will cause the welcome box to stick to the primary group.  Is that
> what we want?

I was expecting the padding built in to the actions to take care of that padding need, but we may be running afoul of bug 597931. Let's leave this patch as is, and I'll add a note to that bug.

Comment 86

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/9a7e34883f86
http://hg.mozilla.org/mozilla-central/rev/dbb425ab070e
http://hg.mozilla.org/mozilla-central/rev/52a7905f4cc4
http://hg.mozilla.org/mozilla-central/rev/a1803da45561
http://hg.mozilla.org/mozilla-central/rev/51bcf78f8c59
http://hg.mozilla.org/mozilla-central/rev/e902872c33b0
http://hg.mozilla.org/mozilla-central/rev/238b879aec66
http://hg.mozilla.org/mozilla-central/rev/d67887a20f71
http://hg.mozilla.org/mozilla-central/rev/bca076d3a6a7
http://hg.mozilla.org/mozilla-central/rev/059ffd92444b
http://hg.mozilla.org/mozilla-central/rev/e716f4ca4f7a
http://hg.mozilla.org/mozilla-central/rev/3c140d56fd3e
http://hg.mozilla.org/mozilla-central/rev/7310d5f75b7e
http://hg.mozilla.org/mozilla-central/rev/cfcd17257d00
http://hg.mozilla.org/mozilla-central/rev/3e275c148e5d
http://hg.mozilla.org/mozilla-central/rev/674f2ed15cea

Comment 87

[Tracy Walker [:tracy]]

we don't typically write rtl test cases for litmus.