594002 - Make tabbar scrollbox handle padding properly

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Markus Stange [:mstange]]

Attachment: v1

In bug 547787 I want to give tabs a negative margin so that their border images can overlap each other. However, negative margins confuse the tabbar scrollbox. So I want to give that scrollbox horizontal padding at the start and end so that the margins of the first and last tabs don't extend the scrollable area. (Another reason is that it looks better - if you see a little space before or after the tab you know that you're at the end of the scrollable area.)

This patch does three things to improve the padding handling:
 - Scrolling to the first/last element will scroll all the way to the left/right, moving the padding into view.
 - App tabs get the same amount of padding in front of them. (So in the app tab case the padding is present twice: In front of the app tabs, and behind the app tabs at the beginning of the scrollable area.)
 - The tabbar scrollbox test is tweaked.

The patch also gives the scrollbox a class so that it can be styled from browser.css.

Comment 1

[Dietrich Ayala (:dietrich)]

Why should we hold the release for this bug?

and Shorlander, you should know by now how to make a high-quality blocking request ;)

Comment 2

[Stephen Horlander [:shorlander]]

(In reply to comment #1)
> Why should we hold the release for this bug?
> 
> and Shorlander, you should know by now how to make a high-quality blocking
> request ;)

I nominated this because it is required for bug 547787 which is a blocker. I am actually a little fuzzy on how that works. Should bugs blocking a blocker be blocking as well?

Comment 3

[Neil Deakin]

Comment on attachment 472614 [details] [diff] [review]
v1

I don't fully understand what this is doing. Maybe Dao should review it as well.

Some comments:


>-      <xul:scrollbox xbl:inherits="orient,align,pack,dir" flex="1" anonid="scrollbox">
>+      <xul:scrollbox class="scrollbox"
>+                     anonid="scrollbox"
>+                     flex="1"
>+                     xbl:inherits="orient,align,pack,dir">

You add this class, but don't use it for anything.


>+      <property name="scrollPaddingRect" readonly="true">
>+        <getter><![CDATA[
>+          // This assumes that this._scrollbox doesn't have any border.
>+          var outerRect = this.scrollClientRect;
>+          var innerRect = {
>+            left: outerRect.left - this._scrollbox.scrollLeft,
>+            top: outerRect.top - this._scrollbox.scrollTop,
>+            width: this._scrollbox.scrollWidth,
>+            height: this._scrollbox.scrollHeight

The width and height are never used directly.


>+      <property name="scrollContentRect" readonly="true">
>+        <getter><![CDATA[
>+          let style = window.getComputedStyle(this._scrollbox, null);
>+          let padding = {};
>+          ["left", "top", "right", "bottom"].forEach(function (edge) {
>+            padding[edge] = parseFloat(style.getPropertyValue("padding-" + edge));
>+          });
> 
>+          var paddingRect = this.scrollPaddingRect;
>+          var contentRect = {
>+            left: paddingRect.left + padding.left,
>+            top: paddingRect.top + padding.top,
>+            right: paddingRect.right - padding.right,
>+            bottom: paddingRect.bottom - padding.bottom

Seems like paddingRect.left + parseFloat(style.paddingLeft) would be simpler.


>+          };
>+          contentRect.height = contentRect.bottom - contentRect.top;
>+          contentRect.width = contentRect.right - contentRect.left;

These are never used.

In fact, judging from the way scrollContentRect is used, it doesn't seem that this property should be needed, as all you're doing it adding the padding value to each edge.

>+
>+          var scrollPaddingRect = this.scrollPaddingRect;
>+          var scrollContentRect = this.scrollContentRect;

I'd also get rid of the scrollPaddingRect property and just determine the values inline. That way, scrollPaddingRect isn't computed twice here.


>-      <xul:scrollbox xbl:inherits="orient,align,pack,dir" flex="1" anonid="scrollbox">
>+      <xul:scrollbox class="scrollbox"
>+                     anonid="scrollbox"
>+                     flex="1"
>+                     xbl:inherits="orient,align,pack,dir">

The class isn't used here either.

Comment 4

[Markus Stange [:mstange]]

(In reply to comment #3)
> >-      <xul:scrollbox xbl:inherits="orient,align,pack,dir" flex="1" anonid="scrollbox">
> >+      <xul:scrollbox class="scrollbox"
> >+                     anonid="scrollbox"
> >+                     flex="1"
> >+                     xbl:inherits="orient,align,pack,dir">
> 
> You add this class, but don't use it for anything.

I'm using it in bug 547787. Maybe not for this scrollbox binding, but for the one used in the tab bar (whichever that is).

I'll change the rest of the patch to be simpler. I wanted to add the rect APIs so that they can be used just like the result from getBoundingClientRect (because that's the API they mirror), but it's probably not worth it for just one consumer.

Comment 5

[Dão Gottwald [:dao]]

(In reply to comment #4)
> (In reply to comment #3)
> > >-      <xul:scrollbox xbl:inherits="orient,align,pack,dir" flex="1" anonid="scrollbox">
> > >+      <xul:scrollbox class="scrollbox"
> > >+                     anonid="scrollbox"
> > >+                     flex="1"
> > >+                     xbl:inherits="orient,align,pack,dir">
> > 
> > You add this class, but don't use it for anything.
> 
> I'm using it in bug 547787. Maybe not for this scrollbox binding, but for the
> one used in the tab bar (whichever that is).

"scrollbox" doesn't sound distinctive enough. Maybe use arrowscrollbox-scrollbox instead.

Comment 6

[Markus Stange [:mstange]]

Attachment: v2

Used class="arrowscrollbox-scrollbox", removed superfluous variable computations, removed scrollContentRect property and calculated it inline.

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 477285 [details] [diff] [review]
v2

>+          if (elementStart <= (vertical ? scrollContentRect.top : scrollContentRect.left)) {
>+            elementStart = vertical ? scrollPaddingRect.top : scrollPaddingRect.left;
>+          }
>+          if (elementEnd >= (vertical ? scrollContentRect.bottom : scrollContentRect.right)) {
>+            elementEnd = vertical ? scrollPaddingRect.bottom : scrollPaddingRect.right;
>+          }

Math.min/max?

Comment 8

[Markus Stange [:mstange]]

How exactly? I'm using different values (content vs padding).

Comment 9

[Markus Stange [:mstange]]

Attachment: v2.1

Fixed a typo so that it actually works.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

(Asked and answered on the blocking request issue, so blocking+)

Comment 11

[Markus Stange [:mstange]]

Attachment: fix another test: make the window wide enough

This test tries to make the window wide enough to accommodate 5.5 tabs but it doesn't take into consideration that tabs can't use the full window width. The other buttons in the tab bar take up space, too, so the tab bar only ends up being about 5.05 tabs wide. And with the additional padding that I'm adding in bug 547787, it's only 4.95 tabs wide, so the restoration order only takes 5 visible tabs into consideration instead of 6 and the test fails.
Correcting the window width based on the real tab width fixes the failure.

Comment 12

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 485526 [details] [diff] [review]
fix another test: make the window wide enough

I'm fine with the test change, but want to make sure nothing in sessionstore would need to change as well. Specifically http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#2399

Comment 13

[Markus Stange [:mstange]]

I don't think it needs to change. scrollClientSize is still the right number to base the calculation on. It's maximally off by 5 pixels, which means that we might prioritize one tab too many in rare edge cases, but I don't think that's worth worrying about.

Comment 14

[Markus Stange [:mstange]]

Landed:
http://hg.mozilla.org/mozilla-central/rev/7ef7de3f34b1
http://hg.mozilla.org/mozilla-central/rev/0acf72149fc9

but backed out again:
http://hg.mozilla.org/mozilla-central/rev/6a5e901211e0
http://hg.mozilla.org/mozilla-central/rev/1c2824959740
http://hg.mozilla.org/mozilla-central/rev/fc789e115fe1
http://hg.mozilla.org/mozilla-central/rev/662ef98d2e27

because browser_480148.js failed on Linux and Windows.

Comment 15

[Markus Stange [:mstange]]

Attachment: use the tab's outer width in session restore code

It turns out that the test change (attachment 485526 [details] [diff] [review]) exposed a real bug: session restore code calculates the number of visible tabs incorrectly.

Tabs have a minimum outer width of 100px. On Windows and Linux, they also have 11px of horizontal border (6px on the left, 5px on the right). So when the tab bar overflows, the tabs' outer width is 100px, and their inner width is 89px.
tab.clientWidth returns the *inner* width, but we need the outer width here, so we should use tab.getBoundingClientRect().width instead.

Comment 16

[Markus Stange [:mstange]]

http://hg.mozilla.org/mozilla-central/rev/a6b991eee683
http://hg.mozilla.org/mozilla-central/rev/c32ed2272509
http://hg.mozilla.org/mozilla-central/rev/fad69d390b23