Tony Chung [:tchung] Reporter
	Description • 14 years ago

With a certain group box size, your app tabs will bleed out of the region

See screenshot

Repro:
1) install Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b6pre) Gecko/20100913 Firefox/4.0b6pre
2) open a few app tabs (eg. 6)
3) In panorama, open a few group boxes.  verify app tabs bleed out of the regions

Expected:
- app tabs fit within the size of a smaller box, or at least handles it better.

	Johnathan Nightingale [:johnath]
	Comment 1 • 14 years ago

Looks unpolished, I agree.

	Michael Yoshitaka Erlewine [:mitcho]
	Comment 2 • 14 years ago

Assigning to Aza for design.

	Aza Raskin [:aza]
	Comment 3 • 14 years ago

Specification here: http://www.flickr.com/photos/azaraskin/4946904112/lightbox/

A couple other notes:

* The app icons should be centered on the close box icon. They are too far right-justified.
* The app icons should be around 0.8 opacity so that on hover they can go to 1.0 opacity

	Kevin Hanes
	Comment 5 • 14 years ago

bugspam (moving b9 to b10)

	Kevin Hanes
	Comment 6 • 14 years ago

bugspam (removing b9)

	Ian Gilman [:iangilman]
	Comment 7 • 14 years ago

At the rate things are going, I may not get to this; reassigning to "nobody" so Mitcho or Tim can take it.

	Mihai Sucan [:msucan] Assignee
	Comment 8 • 14 years ago

I was looking for blockers to take on. Given this is unassigned, I have investigated the code and I believe I can fix this with a patch.

Can I take it?

	Tim Taubert [:ttaubert] (inactive)
	Comment 9 • 14 years ago

Please, feel free to take it.

	Mihai Sucan [:msucan] Assignee
	Comment 10 • 14 years ago

Thanks! I'll submit a patch ASAP.

	Mihai Sucan [:msucan] Assignee
	Comment 11 • 14 years ago

This is the proposed fix.

The appTabTray now aligns with the close button. The icons have opacity 0.8 as requested, and the tray has a border-left as in the screenshot from flickr (comment #3).

The appTab icons wrap using CSS 3 columns, and no longer overflow the tab group.

Looking forward for feedback. Not sure who to ask for feedback, so please change as needed if I picked wrongly. Thanks! :)

	Mihai Sucan [:msucan] Assignee
	Comment 12 • 14 years ago

Comment on attachment 503933 [details] [diff] [review]
proposed fix

Asking for review directly from Ian, as suggested on IRC.

	Michael Yoshitaka Erlewine [:mitcho]
	Comment 13 • 14 years ago

softblocker = critical

	Ian Gilman [:iangilman]
	Comment 14 • 14 years ago

Comment on attachment 503933 [details] [diff] [review]
proposed fix

Looks great! Can you write the test now?

Other comments:

>+    var iconHeight = parseInt(iQ(icons[0]).height());
>+    var contentHeight = parseInt(this.getContentBounds().height);

Both of those are numbers; no need to parseInt them.

>+    // Make sure the AppTab icons fit the new groupItem size.
>+    this.adjustAppTabTray();
>+
>     // ___ Deal with children
>     if (css.width || css.height) {
>       this.arrange({animate: !immediately}); //(immediately ? 'sometimes' : true)});

We should only adjust the app tab tray here if we have resized (otherwise we're needlessly slowing down drag operations); testing (css.width || css.height) will do it.

	Ian Gilman [:iangilman]
	Comment 15 • 14 years ago

This patch is definitely an improvement, but it's still possible to get into an unfortunate state. See the attached screenshot with a minimum size group and 8 app tabs. Note that: 

* The app tabs have all but squished the tab stack out of existence (so that it's now hanging over the app tabs)

* The stack expander lies in the app tab area, not under the stack.

If we add another 4 app tabs, the stack is pushed entirely out of the group, and some of the app tabs extend out of the group as well.

Faaborg, any thoughts?

Seems like perhaps at some point we have to cut off the app tabs, or stack them too, or squish them down. It wouldn't really work to change the minimum size of a group in response to how many app tabs there are... you'd quickly run into space issues between the groups.

	Mihai Sucan [:msucan] Assignee
	Comment 16 • 14 years ago

(In reply to comment #15)
> Created attachment 504564 [details]
> Too many app tabs
> 
> This patch is definitely an improvement, but it's still possible to get into an
> unfortunate state. See the attached screenshot with a minimum size group and 8
> app tabs. Note that: 

At that point I'd suggest hiding the app tabs, if setting a minimum group size such that the tab stack is visible with all those app tabs is not ideal.

I'll update the patch based on Faaborg's suggestions (with an included TC as well).

	Michael Yoshitaka Erlewine [:mitcho]
	Comment 17 • 14 years ago

In the future requiring a minimum group size won't be a problem (after bug 601534) but that won't happen for fx4. There's still a ux question as well: do we really want to use that much real estate for a redundant display of many app tabs in each group?

Please also update the patch on current m-c which includes bug 597776, and may afford some additional space for the app tab tray.

	Mihai Sucan [:msucan] Assignee
	Comment 18 • 14 years ago

Updated the patch to address Ian's comments. Added a mochitest.

Once a decision is reached on the problem of too many app tabs when the group item is too small, I will update the patch accordingly. If that doesn't happen early enough I think checking-in this patch should be enough for the most common cases and we can file a follow up bug.

Thanks Ian for your review!

	Ian Gilman [:iangilman]
	Comment 19 • 14 years ago

Comment on attachment 504782 [details] [diff] [review]
patch update 2

How about you force the app tab tray to never be wider than 1/3 the group width? Extra app tabs just get cut off (until the user resizes the group, of course). That's not the ideal solution, I suppose, but I'd call it good enough for this patch. I'm not comfortable landing it without some sort of cut off like that.

The test has some overlap with browser_tabview_apptabs.js; did you take a look at that? I suppose there might be something to be said for combining this test into that file, or thinning out the overlap. Or not. Whichever you think is fine. 

>+  // remove all tabs
>+  for (let i = 1; i < xulTabs.length; i++) {
>+    gBrowser.removeTab(xulTabs[i]);
>+  }

Seems like that comment should be something like "remove all but one app tabs". 

Otherwise looking good.

	Mihai Sucan [:msucan] Assignee
	Comment 20 • 14 years ago

(In reply to comment #19)
> Comment on attachment 504782 [details] [diff] [review]
> patch update 2
> 
> How about you force the app tab tray to never be wider than 1/3 the group
> width? Extra app tabs just get cut off (until the user resizes the group, of
> course). That's not the ideal solution, I suppose, but I'd call it good enough
> for this patch. I'm not comfortable landing it without some sort of cut off
> like that.

Agreed. Still, I'll look into the code and see how the tab stack is shown, and if I can determine a nice way to make sure that the apptabs don't overlap the stack, I think it's better than just saying "never be wider than 1/3 of the group width". The extra app tab columns just won't show.


> The test has some overlap with browser_tabview_apptabs.js; did you take a look
> at that? I suppose there might be something to be said for combining this test
> into that file, or thinning out the overlap. Or not. Whichever you think is
> fine. 

You're correct. I thought of changing the original apptabs.js tests file, but I decided against that because I think it will over-complicate it. One of the (hard) lessons we learned working on the new Web Console was that we do not need/want big test files - keep them simple. :) I hope you agree.


> >+  // remove all tabs
> >+  for (let i = 1; i < xulTabs.length; i++) {
> >+    gBrowser.removeTab(xulTabs[i]);
> >+  }
> 
> Seems like that comment should be something like "remove all but one app tabs". 

Ah, yeah. I forgot to update my comment. :)


> Otherwise looking good.

Thanks for the review! Will update the patch ASAP.

	Mihai Sucan [:msucan] Assignee
	Comment 21 • 14 years ago

Fixed the problem of showing too many app tabs when the group box is too small. Now the number of columns is limited such that the width of the app tab tray doesn't take more space than 0.35 * group box width.

This approach works surprisingly well. Having looked at the tab stack implementation, I see there's no bounding box that I can use, so 0.35 * group box width is really fine.

Please let me know if the patch needs further changes. Thanks!

	Michael Yoshitaka Erlewine [:mitcho]
	Comment 22 • 14 years ago

Comment on attachment 505772 [details] [diff] [review]
patch update 3

(I'm doing some feedback as a first pass to help with Ian's review load.)

>+  adjustAppTabTray: function GroupItem_adjustAppTabTray(arrangeGroup) {

>+    var contentHeight = this.getContentBounds().height;

This is a little odd, personally, as GroupItem_getContentBounds is dependent upon the appTabTray's width. Now, granted, it's not a real code loop, and we're using the height here, not the width, but it still sits poorly with me. Maybe just use this.getBounds().height - this.$titlebar.height() - (some extra value as a margin) ?

In particular, it often feels like there should be extra space below that we're wasting, and this is a direct consequence of using getContentBounds()... getContentBounds removes 33px for the new tab button below, but actually on the side of the group that the app tab tray is, the only thing we have is the resizer, which is 16px x 16px in all default themes. I just changed this line to be:

>+    var contentHeight = this.getBounds().height - this.$titlebar.height() - 26;

and it feels good to me.

Also, nit: please use let instead of var in new code.

>+    var rows = Math.floor(contentHeight / iconBounds.height);
>+    var columns = Math.ceil(icons.length / rows);
>+    var columnsGap = parseInt(this.$appTabTray.css("-moz-column-gap"));
>+    var maxColumns = Math.floor((boxWidth * 0.35) / (iconBounds.width + columnsGap));
>+    columns = Math.min(columns, maxColumns);
>+    var oldColumns = parseInt(this.$appTabTray.css("-moz-column-count"));
>+
>+    this.$appTabTray.css("-moz-column-count", columns);
>+    this.$appTabTray.css("height", contentHeight + "px");

Love the use of CSS3 columns. When this lands, please make sure to file a follow up bug to change this to "column-count" and "column-gap" if/when we drop the -moz- prefix on these. (Hopefully there's a CSS bug for this someplace that you can make it depend on.)

>+    // Rearrange the groupItem if the width changed in some way.
>+    if (arrangeGroup && (widthUpdated || oldColumns != columns)) {
>+      this.arrange();
>+    }

Nit: you can drop the superfluous {} on if statements with just one line consequents.

>    // Make sure the AppTab icons fit the new groupItem size.
>    if (css.width || css.height) {
>      this.adjustAppTabTray({dontArrange: true});
>    }

Nit: {}

>   getContentBounds: function GroupItem_getContentBounds() {

>     var appTabTrayWidth = this.$appTabTray.width();
>+    if (appTabTrayWidth > 0) {
>+      appTabTrayWidth += parseInt(this.$appTabTray.css(UI.rtl ? "left" : "right"));
>+    }

if (appTabTrayWidth) works, no? Also, drop the {}.

>+    // Make sure the AppTab icons fit the new groupItem size.
>+    if (css.width || css.height) {
>+      this.adjustAppTabTray();
>+    }

nit: {}

>+++ b/browser/base/content/test/tabview/browser_tabview_bug595965.js

>+  // add enough tabs to have two columns
>+  for (let i = 1; i < icons; i++) {
>+    xulTabs.push(gBrowser.loadOneTab("about:blank"));
>+    gBrowser.pinTab(xulTabs[i]);
>+  }

Why not 

> let newTab = gBrowser.loadOneTab("about:blank");
> xulTabs.push(newTab);
> gBrowser.pinTab(newTab);

? I guess the index should work, but... meh.

Test looks good. Since you create a new group which can move things around, could you use newWindowWithTabView? It's supplied in head.js and is used by a number of other tests. Let me know if you have any questions about it.

	Michael Yoshitaka Erlewine [:mitcho]
	Comment 23 • 14 years ago

(In reply to comment #22)
> I just changed this line
> to be:
> 
> >+    var contentHeight = this.getBounds().height - this.$titlebar.height() - 26;
> 
> and it feels good to me.

Make that - 30.

	Michael Yoshitaka Erlewine [:mitcho]
	Comment 24 • 14 years ago

One last comment on patch 3: under this patch, as per your discussions with Ian, if the group is too small, it'll simply hide some app tabs. I don't think it's a good idea to do this without indicating this to the user... it may feel like some of their tabs were closed on them, even though it wasn't.

I propose adding some ellipsis-like dots underneath in this situation.

	Mihai Sucan [:msucan] Assignee
	Comment 25 • 14 years ago

(In reply to comment #22)
> Comment on attachment 505772 [details] [diff] [review]
> patch update 3
> 
> (I'm doing some feedback as a first pass to help with Ian's review load.)
> 
> >+  adjustAppTabTray: function GroupItem_adjustAppTabTray(arrangeGroup) {
> 
> >+    var contentHeight = this.getContentBounds().height;
> 
> This is a little odd, personally, as GroupItem_getContentBounds is dependent
> upon the appTabTray's width. Now, granted, it's not a real code loop, and we're
> using the height here, not the width, but it still sits poorly with me. Maybe
> just use this.getBounds().height - this.$titlebar.height() - (some extra value
> as a margin) ?
> 
> In particular, it often feels like there should be extra space below that we're
> wasting, and this is a direct consequence of using getContentBounds()...
> getContentBounds removes 33px for the new tab button below, but actually on the
> side of the group that the app tab tray is, the only thing we have is the
> resizer, which is 16px x 16px in all default themes. I just changed this line
> to be:
> 
> >+    var contentHeight = this.getBounds().height - this.$titlebar.height() - 26;
> 
> and it feels good to me.

My rationale is: if we are going to hard-code in each method how we calculate the content bounds, we are going to have problems later in the development cycle when we change the meaning of "content bounds". We'll have more places to change, not just one method (getContentBounds()).

So, just to make sure: shall I make this change?

Thanks!

	Michael Yoshitaka Erlewine [:mitcho]
	Comment 26 • 14 years ago

(In reply to comment #25)
> My rationale is: if we are going to hard-code in each method how we calculate
> the content bounds, we are going to have problems later in the development
> cycle when we change the meaning of "content bounds". We'll have more places to
> change, not just one method (getContentBounds()).
> 
> So, just to make sure: shall I make this change?
> 
> Thanks!

I believe "content bounds" is well defined: it's the space available to lay out children (TabItem's). Perhaps the mysterious -30 would be better if it's based on the size of the resizer, but I do think it's definitely preferable, both aesthetically and code-sanity wise.

Of course, Ian has the last word, though. Ian? :)

	Ian Gilman [:iangilman]
	Comment 27 • 14 years ago

(In reply to comment #26)
> (In reply to comment #25)
> > My rationale is: if we are going to hard-code in each method how we calculate
> > the content bounds, we are going to have problems later in the development
> > cycle when we change the meaning of "content bounds". We'll have more places to
> > change, not just one method (getContentBounds()).
> > 
> > So, just to make sure: shall I make this change?
> > 
> > Thanks!
> 
> I believe "content bounds" is well defined: it's the space available to lay out
> children (TabItem's). Perhaps the mysterious -30 would be better if it's based
> on the size of the resizer, but I do think it's definitely preferable, both
> aesthetically and code-sanity wise.
> 
> Of course, Ian has the last word, though. Ian? :)

I too felt the app tab tray didn't extend down far enough (sorry I didn't say anything). I hadn't noticed the app tray/content bounds interdepedency, but it also makes me a little uncomfortable (even though it's width versus height at the moment, that could change in the future). 

So yes, let's make the change mitcho proposes. If you want to base it off of the resizer height, that's cool.

	Mihai Sucan [:msucan] Assignee
	Comment 28 • 14 years ago

(In reply to comment #27)
> (In reply to comment #26)
> > (In reply to comment #25)
> > > My rationale is: if we are going to hard-code in each method how we calculate
> > > the content bounds, we are going to have problems later in the development
> > > cycle when we change the meaning of "content bounds". We'll have more places to
> > > change, not just one method (getContentBounds()).
> > > 
> > > So, just to make sure: shall I make this change?
> > > 
> > > Thanks!
> > 
> > I believe "content bounds" is well defined: it's the space available to lay out
> > children (TabItem's). Perhaps the mysterious -30 would be better if it's based
> > on the size of the resizer, but I do think it's definitely preferable, both
> > aesthetically and code-sanity wise.
> > 
> > Of course, Ian has the last word, though. Ian? :)
> 
> I too felt the app tab tray didn't extend down far enough (sorry I didn't say
> anything). I hadn't noticed the app tray/content bounds interdepedency, but it
> also makes me a little uncomfortable (even though it's width versus height at
> the moment, that could change in the future). 
> 
> So yes, let's make the change mitcho proposes. If you want to base it off of
> the resizer height, that's cool.

Oky, no problem. I'll make all the changes requested by mitcho then. Thanks for your quick reply!

	Kevin Hanes
	Comment 29 • 14 years ago

bugspam. Moving b10 to b11

	Kevin Hanes
	Comment 30 • 14 years ago

bugspam. Removing b10

	Alex Faaborg [:faaborg] (Firefox UX)
	Comment 31 • 14 years ago

Sorry about the delay, I'm going to try to get to this review tomorrow.

	Alex Faaborg [:faaborg] (Firefox UX)
	Comment 32 • 14 years ago

Comment on attachment 505831 [details]
Adding dots to app tab trays which are hiding some app tabs

Sorry about the delay in getting to this ui-review

>Faaborg, any thoughts?

>Seems like perhaps at some point we have to cut off the app tabs, or stack them
>too, or squish them down. It wouldn't really work to change the minimum size of
>a group in response to how many app tabs there are... you'd quickly run into
>space issues between the groups.

Generally speaking, I don't like the redundancy of having app tabs in every group.  Also I don't really think that being able to switch to a particular app tab in the context of a certain group is compelling enough to warrant displaying them in each group.  But that's of course outside the scope of this bug.

I would be fine with a single column of app tabs, that simply gets truncated (with a displayed ellipsis) when the group isn't large enough to display all of them.  Keeping enough rows to fill 1/3 of the width of the group seems to start to emphasis app tabs too much compared to the actual contents of the group.  We could also consider scaling this value down, instead of just forcing a single row.

In terms of the ellipsis, I think it might work better to just use the UI font.  This will automatically pick up contrast for accessibility.  If we use an image, we should try to get the right look on each platform, and generally speaking the dots should be smaller.

	Michael Yoshitaka Erlewine [:mitcho]
	Comment 34 • 14 years ago

(In reply to comment #32)
> We
> could also consider scaling this value down, instead of just forcing a single
> row.

Do you mean that we could always force them into one column, scaling down the size of each icon, if necessary?

I personally think cutting it off after one column's worth with an ellipsis sounds (looks) good.

	Mihai Sucan [:msucan] Assignee
	Comment 35 • 14 years ago

The initial mockups showed us multiple columns of app tabs. I liked the idea.

Faaborg: shall I limit them, then, to only one column and cut them off? With an ellipsis.

Personally I'd prefer us to keep the multiple cols, and only maybe reduce the percent of width they can take (less than 30%, maybe 20%?).

I'd like a decision on this, before I update the patch.

Thanks!

	Ian Gilman [:iangilman]
	Comment 36 • 14 years ago

(In reply to comment #34)
> (In reply to comment #32)
> > We
> > could also consider scaling this value down, instead of just forcing a single
> > row.
> 
> Do you mean that we could always force them into one column, scaling down the
> size of each icon, if necessary?

No, I think he means scaling the 33% allotment down to, say, 20%. At any rate, the icons are small enough as is; we can't really make them smaller and have them clickable.

So, my reading of comment 32 is that Alex would be fine with a single column or with multiple columns as long as they took appreciably less than one third of the space. He'd also like an ellipsis (if tabs are getting cut off), rendered in the UI font. Presumably clicking on the ellipsis doesn't do anything... it's just an indicator. 

Mihai, please try 20% and add the ellipsis. Sorry for all the back and forth!

	Mihai Sucan [:msucan] Assignee
	Comment 37 • 14 years ago

(In reply to comment #36)
> Mihai, please try 20% and add the ellipsis. Sorry for all the back and forth!

Thanks Ian for the clarification.

	Mihai Sucan [:msucan] Assignee
	Comment 38 • 14 years ago

Patch updated based on the latest review/feedback comments. Thanks to Faaborg, Michael and Ian! :)

Changes:

- switched away from var to let.
- addressed the nits about using { } when not needed.
- changed the way the contentHeight is calculated in adjustAppTabTray(). Instead of using titlebar height, i used appTabTray top position, because that's more accurate, and as suggested by Ian, I also use the resizer height.
- lowered the allowed appTabTray width to 20% from 35%.
- added an ellipsis at the bottom of the tray when the columns are cut off. This uses the ellipsis character, not an image, and it's smaller than the provided mockup. It can be made even smaller if needed by just changing the font-size from CSS.
- updated the test to use newWindowWithTabView() and hideTabView() from head.js. I didn't know of these functions when I wrote the test. Thanks for the tip Michael! :)
- reported follow-up bug 629452 as requested by Michael, but I couldn't find a bug to put it as a dependency to.
- rebased the code on top of the latest changes from mozilla-central.

Looking forward to more comments. Thanks!

	Ian Gilman [:iangilman]
	Comment 39 • 14 years ago

Comment on attachment 507592 [details] [diff] [review]
patch update 4

Looking great! Just a couple comments:

* I guess I was wrong about the test header... we do want the public domain header after all; it seems the policy has changed and I just didn't get the memo. 

* You've got a couple RTL issues: the ellipsis doesn't show up at all, and the dotted line seems to be just white? Or maybe it's missing and it's just my imaginiation. At any rate, please fix (use the forceRTL extension to check it). 

>+.appTabTrayTruncated:after {
>+  content: "â¦";
>+  position: absolute;
>+  bottom: 2px;
>+  left: -5px;

It seems this left -5 is to get the ellipsis to display? How volatile is this? Also, it doesn't seem well centered under a single stack of icons on my Mac.

	Mihai Sucan [:msucan] Assignee
	Comment 40 • 14 years ago

(In reply to comment #39)
> Comment on attachment 507592 [details] [diff] [review]
> patch update 4
> 
> Looking great! Just a couple comments:
> 
> * I guess I was wrong about the test header... we do want the public domain
> header after all; it seems the policy has changed and I just didn't get the
> memo. 

Will fix this.

> * You've got a couple RTL issues: the ellipsis doesn't show up at all, and the
> dotted line seems to be just white? Or maybe it's missing and it's just my
> imaginiation. At any rate, please fix (use the forceRTL extension to check it). 

Ah, sorry. I forgot to test with RTL enabled.

> >+.appTabTrayTruncated:after {
> >+  content: "â¦";
> >+  position: absolute;
> >+  bottom: 2px;
> >+  left: -5px;
> 
> It seems this left -5 is to get the ellipsis to display? How volatile is this?
> Also, it doesn't seem well centered under a single stack of icons on my Mac.

No, not to make it visible. It's visible if I put left 0, but it's not centered, due to the padding-left 5px of .appTabTray. left -5px is used to center the ellipsis in .appTabTray.

The ellipsis should always be centered. Please post a screenshot. Thanks!

Will update the patch to fix the RTL problems.

	Mihai Sucan [:msucan] Assignee
	Comment 41 • 14 years ago

Updated the patch with RTL fixes, and test license change. Also fixed the ellipsis centering, I believe. Please retest and let me know if further changes are needed.

Thank you for your reviews!

	Ian Gilman [:iangilman]
	Comment 42 • 14 years ago

Comment on attachment 507913 [details] [diff] [review]
patch update 5

>   this.$appTabTray = iQ("<div/>")
>     .addClass("appTabTray")
>+    .attr("dir", "ltr")
>     .appendTo($container);

What is this dir = ltr for?

Perhaps related: the app tabs in RTL mode seem to stack left to right rather than right to left; seems like that's wrong.

Everything else looks beautiful, though! If getting the app tabs to stack correctly is a big deal, let's just land this and file a follow-up. Otherwise, please fix. Either way, r+.

(In reply to comment #41)
> Thank you for your reviews!

Thank you for your perseverance!

	Ian Gilman [:iangilman]
	Comment 43 • 14 years ago

A snap of the app tabs stacking left to right in RTL mode (you can see the extra space is on the right column rather than on the left column).

	Mihai Sucan [:msucan] Assignee
	Comment 44 • 14 years ago

(In reply to comment #43)
> Created attachment 508013 [details]
> App tabs stack LTR in RTL mode
> 
> A snap of the app tabs stacking left to right in RTL mode (you can see the
> extra space is on the right column rather than on the left column).

Correct. That's why I added the dir:ltr to .appTabsTray.

The stacking worked correctly in rtl mode (with dir:rtl): the new icons were added to the left, and pushed to the left as the height got smaller. However, due to the fact we want to cut off columns when the width of the group box is too small.... i have to reduce the width. Width is always reduced only from the right side, which is wrong in rtl mode, because you'd have stacking in rtl and cutting in ltr mode. :) Hence, I was forced to set dir=ltr, to always have stacking in ltr mode, so the cutting of columns looks right and behaves correctly.

A fix for the above would require a bit of "rework": we'd need to wrap the whole apptabtray into a wrapper div, such that we can have a "view" of the columns, of the icons. We could then reduce the width and move the icons to the left, in rtl mode, as needed.

Shall I file a follow up bug report for this?

Thanks for the review+!

	Ian Gilman [:iangilman]
	Comment 45 • 14 years ago

(In reply to comment #44)
> (In reply to comment #43)
> > Created attachment 508013 [details]
> > App tabs stack LTR in RTL mode
> > 
> > A snap of the app tabs stacking left to right in RTL mode (you can see the
> > extra space is on the right column rather than on the left column).
> 
> Correct. That's why I added the dir:ltr to .appTabsTray.
> 
> The stacking worked correctly in rtl mode (with dir:rtl): the new icons were
> added to the left, and pushed to the left as the height got smaller. However,
> due to the fact we want to cut off columns when the width of the group box is
> too small.... i have to reduce the width. Width is always reduced only from the
> right side, which is wrong in rtl mode, because you'd have stacking in rtl and
> cutting in ltr mode. :) Hence, I was forced to set dir=ltr, to always have
> stacking in ltr mode, so the cutting of columns looks right and behaves
> correctly.
> 
> A fix for the above would require a bit of "rework": we'd need to wrap the
> whole apptabtray into a wrapper div, such that we can have a "view" of the
> columns, of the icons. We could then reduce the width and move the icons to the
> left, in rtl mode, as needed.
> 
> Shall I file a follow up bug report for this?
> 
> Thanks for the review+!

Yes, please file a follow-up for that issue along with the above explanation, and let's land this patch! :-)

	Mihai Sucan [:msucan] Assignee
	Comment 46 • 14 years ago

(In reply to comment #45)
> Yes, please file a follow-up for that issue along with the above explanation,
> and let's land this patch! :-)

Reported bug 630236. Thanks!

	(no longer active)
	Comment 47 • 14 years ago

This doesn't apply cleanly, and I don't think that we should take it in its current form because of the RTL regression, as well as some review comments that I have here (will post them shortly).

	(no longer active)
	Comment 48 • 14 years ago

Comment on attachment 507913 [details] [diff] [review]
patch update 5

>diff --git a/browser/base/content/tabview/groupitems.js b/browser/base/content/tabview/groupitems.js
>   this.$appTabTray = iQ("<div/>")
>     .addClass("appTabTray")
>+    .attr("dir", "ltr")
>     .appendTo($container);

Why is this needed?  This looks very wrong to me.  I suspect that the stacking problem will be solved if you remove this.

>diff --git a/browser/themes/gnomestripe/browser/tabview/tabview.css b/browser/themes/gnomestripe/browser/tabview/tabview.css
> .appTabTray {
>   top: 34px;
>-  right: 1px;
>+  right: 6px;
>+  -moz-column-width: 16px;
>+  -moz-column-count: 0;
>+  -moz-column-gap: 5px;
>+  border-left: 1px solid #E1E1E1;
>+  padding-left: 5px;

Please use -moz-border-start and -moz-padding-start instead of these, and remove the corresponding border and padding properties in the [dir=rtl] case.

>+.appTabTrayTruncated:after {
>+  content: "â¦";
>+  position: absolute;
>+  bottom: 2px;
>+  left: -5px;

You need a corresponding [dir=rtl] case here which sets left to auto and right to -5px.

>diff --git a/browser/themes/pinstripe/browser/tabview/tabview.css b/browser/themes/pinstripe/browser/tabview/tabview.css
> .appTabTray {
>   top: 34px;
>-  right: 1px;
>+  right: 6px;
>+  -moz-column-width: 16px;
>+  -moz-column-count: 0;
>+  -moz-column-gap: 5px;
>+  border-left: 1px solid #E1E1E1;
>+  padding-left: 5px;

Same as above.

>+.appTabTrayTruncated:after {
>+  content: "â¦";
>+  position: absolute;
>+  bottom: 2px;
>+  left: -5px;

Same as above.

>diff --git a/browser/themes/winstripe/browser/tabview/tabview.css b/browser/themes/winstripe/browser/tabview/tabview.css
> .appTabTray {
>   top: 34px;
>-  right: 1px;
>+  right: 6px;
>+  -moz-column-width: 16px;
>+  -moz-column-count: 0;
>+  -moz-column-gap: 5px;
>+  border-left: 1px solid #E1E1E1;
>+  padding-left: 5px;

Same as above.

>+.appTabTrayTruncated:after {
>+  content: "â¦";
>+  position: absolute;
>+  bottom: 2px;
>+  left: -5px;

Same as above.


Please ask me for review again when you address these comments.  And please submit a screenshot which shows the RTL UI in contrast to the LTR UI.

Thanks!

	Mihai Sucan [:msucan] Assignee
	Comment 50 • 14 years ago

(In reply to comment #48)
> Comment on attachment 507913 [details] [diff] [review]
> patch update 5
> 
> >diff --git a/browser/base/content/tabview/groupitems.js b/browser/base/content/tabview/groupitems.js
> >   this.$appTabTray = iQ("<div/>")
> >     .addClass("appTabTray")
> >+    .attr("dir", "ltr")
> >     .appendTo($container);
> 
> Why is this needed?  This looks very wrong to me.  I suspect that the stacking
> problem will be solved if you remove this.

Yes and no. If I remove this attribute, the icons are indeed correctly stacked to the left (from the right side). However, this creates complications ... read below ...

> >diff --git a/browser/themes/gnomestripe/browser/tabview/tabview.css b/browser/themes/gnomestripe/browser/tabview/tabview.css
> > .appTabTray {
> >   top: 34px;
> >-  right: 1px;
> >+  right: 6px;
> >+  -moz-column-width: 16px;
> >+  -moz-column-count: 0;
> >+  -moz-column-gap: 5px;
> >+  border-left: 1px solid #E1E1E1;
> >+  padding-left: 5px;
> 
> Please use -moz-border-start and -moz-padding-start instead of these, and
> remove the corresponding border and padding properties in the [dir=rtl] case.
> 
> >+.appTabTrayTruncated:after {
> >+  content: "â¦";
> >+  position: absolute;
> >+  bottom: 2px;
> >+  left: -5px;
> 
> You need a corresponding [dir=rtl] case here which sets left to auto and right
> to -5px.

... tried this. The complication is that the right: -5px part of the .appTabTray element is considered outside of the view. You never really get to see the ellipsis, because of a bug(?) in Gecko. The entire width of the all columns is used when positioning to right: -5px. It doesn't put the element at position x = appTabTray width -5px as expected. It is put outside, which means that the :after pseudo element is not visible (due to overflow-x: hidden).

The solution? It is surprising: left:-5px renders fine, no need to do right: -5px. :)


> Please ask me for review again when you address these comments.  And please
> submit a screenshot which shows the RTL UI in contrast to the LTR UI.

Will do. Due to the way we need to cut the number of columns based on group width ... we cannot do it with RTL in the current code "setup" (see comment 44).

Making the suggested changes do not entirely fix the issue (I tried them, hehe), they make it worse: you get RTL stacking with LTR removal of columns. Hence, I decided to have LTR stacking for the icons.

I was not aware that RTL in this case is very important - in the sense that's only a visual aspect (not really related to reading?). Nonetheless, no worries - I'll make the necessary changes to the code to accommodate to the RTL requirements.

Thanks for your review!

	(no longer active)
	Comment 51 • 14 years ago

(In reply to comment #50)
> ... tried this. The complication is that the right: -5px part of the
> .appTabTray element is considered outside of the view. You never really get to
> see the ellipsis, because of a bug(?) in Gecko. The entire width of the all
> columns is used when positioning to right: -5px. It doesn't put the element at
> position x = appTabTray width -5px as expected. It is put outside, which means
> that the :after pseudo element is not visible (due to overflow-x: hidden).

Can you create a simple HTML testcase which reproduces this problem?

> The solution? It is surprising: left:-5px renders fine, no need to do right:
> -5px. :)

This is really surprising.  I think you should write a minimized test case for this problem...

	Mihai Sucan [:msucan] Assignee
	Comment 52 • 14 years ago

Patch that fixes the stacking of app tab icons in RTL mode. Things are a bit more complicated now, because of the UI RTL requirements.

Please let me know if the code is fine now. Thanks!

Ehsan: I'll submit a link to a TC that shows weirdness of -moz-columns a bit later.

Ian: the patch has r+ already from you, but you should take a quick glance at the changes - to let me know if there's anything I should change. Thanks!

	Mihai Sucan [:msucan] Assignee
	Comment 53 • 14 years ago

(In reply to comment #51)
> (In reply to comment #50)
> > ... tried this. The complication is that the right: -5px part of the
> > .appTabTray element is considered outside of the view. You never really get to
> > see the ellipsis, because of a bug(?) in Gecko. The entire width of the all
> > columns is used when positioning to right: -5px. It doesn't put the element at
> > position x = appTabTray width -5px as expected. It is put outside, which means
> > that the :after pseudo element is not visible (due to overflow-x: hidden).
> 
> Can you create a simple HTML testcase which reproduces this problem?

Here's the TC:

http://www.robodesign.ro/coding/0075/2.html

Another TC that shows why I have to calculate in JS the number of columns:

http://www.robodesign.ro/coding/0075/1.html

	Ian Gilman [:iangilman]
	Comment 54 • 14 years ago

Comment on attachment 509088 [details] [diff] [review]
patch update 6

The changes look good, and I've run them and verified that the RTL stacking is correct. Just one comment:

>+        if (container.hasClass("appTabTrayTruncated"))
>+          container.removeClass("appTabTrayTruncated");

This CSS class should be changed to "appTabTrayContainerTruncated". I realize that's awfully long, but otherwise it's misleading.

R+ with that fix.

	(no longer active)
	Comment 55 • 14 years ago

(In reply to comment #53)

Cool layout bugs you've found there!

> Here's the TC:
> 
> http://www.robodesign.ro/coding/0075/2.html

Bug 631112.

> Another TC that shows why I have to calculate in JS the number of columns:
> 
> http://www.robodesign.ro/coding/0075/1.html

Bug 631114.

	(no longer active)
	Comment 56 • 14 years ago

Comment on attachment 509088 [details] [diff] [review]
patch update 6

Looks good to me, but can you please submit a screenshot of how things look like in RTL and LTR mode with this patch?

	Mihai Sucan [:msucan] Assignee
	Comment 57 • 14 years ago

(In reply to comment #55)
> (In reply to comment #53)
> 
> Cool layout bugs you've found there!
> 
> > Here's the TC:
> > 
> > http://www.robodesign.ro/coding/0075/2.html
> 
> Bug 631112.
> 
> > Another TC that shows why I have to calculate in JS the number of columns:
> > 
> > http://www.robodesign.ro/coding/0075/1.html
> 
> Bug 631114.

Yay, thanks for reporting the bugs! I wasn't sure if they point out real bugs in the way css3 columns are supposed to work, or not. I did not read the css3 columns specification, yet.


(In reply to comment #56)
> Comment on attachment 509088 [details] [diff] [review]
> patch update 6
> 
> Looks good to me, but can you please submit a screenshot of how things look
> like in RTL and LTR mode with this patch?

Sure.

LTR: http://img.i7m.de/show/yg31x.png
RTL: http://img.i7m.de/show/0pyqz.png

Thanks for looking into the patch!

	(no longer active)
	Comment 58 • 14 years ago

Comment on attachment 509088 [details] [diff] [review]
patch update 6

Beautiful!  Thanks!

	Mihai Sucan [:msucan] Assignee
	Comment 59 • 14 years ago

Updated the patch, changed the class name .appTabTrayTruncated to .appTabTrayContainerTruncated as requested.

Thanks Ian and Ehsan for your reviews! The patch is now ready to land. :)

	Ian Gilman [:iangilman]
	Comment 60 • 14 years ago

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

Thank you, Mihai, for writing this!

	Tracy Walker [:tracy]
	Comment 61 • 14 years ago

This is not fixed in Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre  app tabs still bleed out when the group is made small enough.

	Mihai Sucan [:msucan] Assignee
	Comment 62 • 14 years ago

(In reply to comment #61)
> This is not fixed in Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre)
> Gecko/20110204 Firefox/4.0b12pre  app tabs still bleed out when the group is
> made small enough.

I cannot confirm that. Groups have a minimum size. On Linux I cannot make the groups small enough to have app tabs bleed out.

Please post a screen shot. Thanks!

	Tracy Walker [:tracy]
	Comment 63 • 14 years ago

yeah, you just have to add a few random tabs as app tabs.

	Mihai Sucan [:msucan] Assignee
	Comment 64 • 14 years ago

(In reply to comment #63)
> Created attachment 509850 [details]
> appTabOverflow
> 
> yeah, you just have to add a few random tabs as app tabs.

did you make the build yourself? or is it a nightly?

	Tracy Walker [:tracy]
	Comment 65 • 14 years ago

nightly, noted in comment #61

	Marcia Knous [:marcia]
	Comment 66 • 14 years ago

I can reproduce this as well using Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre. In my case it started to happen after I surpassed 4 app tabs.

	Mihai Sucan [:msucan] Assignee
	Comment 67 • 14 years ago

(In reply to comment #65)
> nightly, noted in comment #61

I believe this patch is not in the nightly yet.

	Tracy Walker [:tracy]
	Comment 68 • 14 years ago

sorry, this won't show up 'til tomorrow's nightly build.

	Daniel Desira
	Comment 69 • 14 years ago

verified: 4.0b12pre(2011-02-05)

	Tracy Walker [:tracy]
	Comment 70 • 14 years ago

yes, verified with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110205 Firefox/4.0b12pre