Closed Bug 594002 Opened 9 years ago Closed 9 years ago

Make tabbar scrollbox handle padding properly

Categories

(Toolkit :: XUL Widgets, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b8
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: mstange, Assigned: mstange)

References

Details

Attachments

(3 files, 2 obsolete files)

Attached patch v1 (obsolete) — Splinter Review
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.
Attachment #472614 - Flags: review?(dao)
Why should we hold the release for this bug?

and Shorlander, you should know by now how to make a high-quality blocking request ;)
(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?
Attachment #472614 - Flags: review?(dao) → review?(enndeakin)
Component: Tabbed Browser → XUL Widgets
Product: Firefox → Toolkit
QA Contact: tabbed.browser → xul.widgets
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.
(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.
(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.
Attached patch v2 (obsolete) — Splinter Review
Used class="arrowscrollbox-scrollbox", removed superfluous variable computations, removed scrollContentRect property and calculated it inline.
Attachment #472614 - Attachment is obsolete: true
Attachment #477285 - Flags: review?(enndeakin)
Attachment #472614 - Flags: review?(enndeakin)
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?
How exactly? I'm using different values (content vs padding).
Attached patch v2.1Splinter Review
Fixed a typo so that it actually works.
Attachment #477285 - Attachment is obsolete: true
Attachment #477294 - Flags: review?(enndeakin)
Attachment #477285 - Flags: review?(enndeakin)
(Asked and answered on the blocking request issue, so blocking+)
blocking2.0: ? → betaN+
Attachment #477294 - Flags: review?(enndeakin) → review+
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.
Attachment #485526 - Flags: review?(paul)
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
Attachment #485526 - Flags: review?(paul) → review+
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.
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.
Attachment #492776 - Flags: review?(paul)
You need to log in before you can comment on or make changes to this bug.