878020 - Add inner shadows to the tab bar's scrollbox when overflowing

Status: VERIFIED FIXED

(Firefox :: Theme, enhancement)

Description

[Matthew N. [:MattN]]

I don't know that anything needs to be done but there are some cases that weren't in mockups that I'd like to be considered:
1) Between the last tab in the scrollbox and the scroll arrow during overflow
2) Between the last pinned tab and the scrollbox during overflow

A general ui-review of how tab separators are handled in other cases would be helpful.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Removing the items from M7 that do not block landing on m-c.

Comment 2

[Justin Dolske [:Dolske]]

Seems like we're generally good-to-go here, but perhaps steven has thoughts.

Comment 3

[Stephen Horlander [:shorlander]]

Attachment: Tab Overflow Indicators

Things look good to me here WRT the tab separators. 

The one thing I think we do need to do is to make the overflow separators more obvious and add the shadow effect. In addition to looking nice and serving as an indicator that tabs are flowing *under* the chrome, the tab separators and the overflow indicators are too similar.

Comment 4

[Justin Dolske [:Dolske]]

P4+ for the moment, but seems likely that this is too big a change at this point and should be M-. Opinions on difficulty of implementation?

Comment 5

[Matthew N. [:MattN]]

I talked about it with Stephen and this shouldn't be too hard to do I think.

I believe we need to make sure the shadow doesn't appear when the scrollbox arrow is disabled.

Stephen, can you provide images?

Comment 6

[Stephen Horlander [:shorlander]]

Attachment: tabOverflow-shadow.png

Comment 7

[Stephen Horlander [:shorlander]]

Attachment: [e-mail]

Comment 8

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: Patch v1

This fixes the issue on OS X. It should also work on Windows and Linux, but I couldn't test there yet in lieu of having a dev environment on those platforms.
So if someone could try it on those platforms, that would be A+ :)

Comment 9

[:Gijs (he/him)]

Comment on attachment 8388508 [details] [diff] [review]
Patch v1

Review of attachment 8388508 [details] [diff] [review]:
-----------------------------------------------------------------

f+ on the images and the hard work! Yes, the images are in the right place and in the jar, at least on Windows, too (but the jar.mn looks fine for Linux, so I'm confident it'll work there.

However, I have quite some notes/feedback:

- can't you just set the images as border-images ? That's what the existing border images use (you can look at this using DOM Inspector). You could probably just replace those images if they're no longer used, or combine them if both should be there (tab-overflow-border.png)
- the common style CSS should go in browser/themes/shared/tabs.inc.css
- your CSS will likely make things look wonky/misdirected on RTL (whee!)

Oh, and this is misaligned on Windows. Screenshot coming up.

::: browser/themes/osx/browser.css
@@ +2925,5 @@
>  }
>  
> +.scrollbutton-up::after {
> +  /* Translating Y by -13.6px makes sure that the element is aligned correctly on normal and retina screens */
> +  transform: translate(6px, -13.6px);

13.6px is an interestingly random-looking number... it makes me think we're using an em value somewhere, which is scary because it means it'll be wrong when the font size isn't what you expect it to be.

Comment 10

[:Gijs (he/him)]

Attachment: shadow.png

Comment 11

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to :Gijs Kruitbosch from comment #9)
> Comment on attachment 8388508 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 8388508 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> f+ on the images and the hard work! Yes, the images are in the right place
> and in the jar, at least on Windows, too (but the jar.mn looks fine for
> Linux, so I'm confident it'll work there.
> 
> However, I have quite some notes/feedback:
> 
> - can't you just set the images as border-images ? That's what the existing
> border images use (you can look at this using DOM Inspector). You could
> probably just replace those images if they're no longer used, or combine
> them if both should be there (tab-overflow-border.png)
I tried that, but the problem was that it changed the click-area of the arrow and messed with the hover state. Not sure if there's a way around this, but I couldn't find one.

> - the common style CSS should go in browser/themes/shared/tabs.inc.css
Shall do!

> - your CSS will likely make things look wonky/misdirected on RTL (whee!)
Any way I can test this?

> Oh, and this is misaligned on Windows. Screenshot coming up.
That is weird because I tested the CSS on Windows 8 and XP (with the image resource loading from the network). I'd say it is unlikely that this is a Windows 7 issue, so I'll set up a dev environment in my Win 8 VM and try to fix this.

> ::: browser/themes/osx/browser.css
> @@ +2925,5 @@
> >  }
> >  
> > +.scrollbutton-up::after {
> > +  /* Translating Y by -13.6px makes sure that the element is aligned correctly on normal and retina screens */
> > +  transform: translate(6px, -13.6px);
> 
> 13.6px is an interestingly random-looking number... it makes me think we're
> using an em value somewhere, which is scary because it means it'll be wrong
> when the font size isn't what you expect it to be.
Well, it's not *that* random ;)
Retina screens allow for half pixel values while normal screens use full pixels. -13.6 makes retina screens use -13.5 and normal screens -14. I think OS X doesn't allow font scaling, so that shouldn't be an issue (?)

Comment 12

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: Patch v1.1

So here's a second version of the patch.
It moves the code to the right position and also deals with the rtl issues (tested on mac).

I tried to fix the positioning on Windows, but I couldn't get a build environment running, so this has been done mostly blind.

Gijs, could you run this on Windows once again? And if you have a binary, could you upload that for me so that I can confirm it also works on XP and 8 (and use the browser toolbox to fix any issues that come up)?

Comment 13

[:Gijs (he/him)]

Comment on attachment 8388853 [details] [diff] [review]
Patch v1.1

(In reply to Philipp Sackl [:phlsa] from comment #11)
> (In reply to :Gijs Kruitbosch from comment #9)
> > Comment on attachment 8388508 [details] [diff] [review]
> > Patch v1
> > 
> > Review of attachment 8388508 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > f+ on the images and the hard work! Yes, the images are in the right place
> > and in the jar, at least on Windows, too (but the jar.mn looks fine for
> > Linux, so I'm confident it'll work there.
> > 
> > However, I have quite some notes/feedback:
> > 
> > - can't you just set the images as border-images ? That's what the existing
> > border images use (you can look at this using DOM Inspector). You could
> > probably just replace those images if they're no longer used, or combine
> > them if both should be there (tab-overflow-border.png)
> I tried that, but the problem was that it changed the click-area of the
> arrow and messed with the hover state. Not sure if there's a way around
> this, but I couldn't find one.

Hrmpf. Yeah, that sounds plausible. Did you try messing with the bounding box and/or the various clip-box bits (e.g. background-clip)?

> > - your CSS will likely make things look wonky/misdirected on RTL (whee!)
> Any way I can test this?

I guess you figured this out, but the Force RTL add-on can help here.

> > Oh, and this is misaligned on Windows. Screenshot coming up.
> That is weird because I tested the CSS on Windows 8 and XP (with the image
> resource loading from the network). I'd say it is unlikely that this is a
> Windows 7 issue, so I'll set up a dev environment in my Win 8 VM and try to
> fix this.

It's properly aligned on the left with this patch, but it seems completely gone on the right? Not sure why. :-)

> > ::: browser/themes/osx/browser.css
> > @@ +2925,5 @@
> > >  }
> > >  
> > > +.scrollbutton-up::after {
> > > +  /* Translating Y by -13.6px makes sure that the element is aligned correctly on normal and retina screens */
> > > +  transform: translate(6px, -13.6px);
> > 
> > 13.6px is an interestingly random-looking number... it makes me think we're
> > using an em value somewhere, which is scary because it means it'll be wrong
> > when the font size isn't what you expect it to be.
> Well, it's not *that* random ;)
> Retina screens allow for half pixel values while normal screens use full
> pixels. -13.6 makes retina screens use -13.5 and normal screens -14. I think
> OS X doesn't allow font scaling, so that shouldn't be an issue (?)

This explanation should go in the file. :P

(still doesn't explain where all the magical values come from, though... I guess part of it is the width of the box, plus any borders, plus the width of the image if moving it left? Not sure about the rest)

Comment 14

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: Patch v1.2

So I tried a couple of things that Gijs and I discussed on IRC.

- Attaching the shadow to the scrollarea: caused problems with the shadow fading out/disappearing
- Using border image and clipping: didn't find a combination that nearly worked (but that might be due to my lack of knowledge). FWIW this would definitely mean we'd lose the transition.

What I ended up doing was setting the bottom property of the pseudoelements to 0. This *kinda* consolidates the vertical alignment differences between OS X, Windows 8/XP and Linux – we are now down to 1px difference instead of more than 10 as before. This makes me hopeful that it might work on Windows 7/Vista as well.

I also tested RTL on all platforms available to me. Changing the font size on Windows 8 also didn't cause any problems.

Comment 15

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Oh, and the magic -13.6px are gone too ;)

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 8389485 [details] [diff] [review]
Patch v1.2

>--- a/browser/themes/linux/browser.css
>+++ b/browser/themes/linux/browser.css

>+.scrollbutton-up::after {
>+  transform: translate(5px, -1px);
>+}
>+
>+.scrollbutton-up:-moz-locale-dir(rtl)::after {
>+  transform: translate(-5px, -1px) scale(-1, 1);
>+}
>+
>+.scrollbutton-down::before {
>+  transform: translate(-17px, -1px) scale(-1, 1);
>+}

You need to use .tabbrowser-arrowscrollbox > .scrollbutton-up and .tabbrowser-arrowscrollbox > .scrollbutton-down in these selectors.

Comment 17

[:Gijs (he/him)]

Comment on attachment 8389485 [details] [diff] [review]
Patch v1.2

Review of attachment 8389485 [details] [diff] [review]:
-----------------------------------------------------------------

This has a missing shadow on the right (left in rtl, maybe?) and I don't know why, but Philipp tells me he's working on another patch!

::: browser/themes/osx/jar.mn
@@ +272,5 @@
>    skin/classic/browser/tabbrowser/tabDragIndicator.png                   (tabbrowser/tabDragIndicator.png)
>    skin/classic/browser/tabbrowser/[e-mail]                (tabbrowser/[e-mail])
>    skin/classic/browser/tabbrowser/tab-separator.png                      (tabbrowser/tab-separator.png)
>    skin/classic/browser/tabbrowser/[e-mail]                   (tabbrowser/[e-mail])
> +  skin/classic/browser/tabbrowser/tab-overflow-shadow.png                (../shared/tabbrowser/tab-overflow-shadow.png)

No @2x version?

::: browser/themes/shared/tabs.inc.css
@@ +301,5 @@
> +  position: absolute;
> +  background-image: url('chrome://browser/skin/tabbrowser/tab-overflow-shadow.png');
> +  width: 12px;
> +  height: 28px;
> +  content: " ";

nit: empty string instead of space, please.

Comment 18

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: WIP Patch 2.0

Since the previous approach descended into more platform and theme specific hacks than even I was comfortable with, here's a new approach.
The good news:
- It works on every platform (Windows XP,7,8, OS X, Linux, all also tested with RTL text).
- It is MUCH shorter
- It addresses all Gijs's comments on the last patch

The bad news: it is incomplete

What happens here is that I use two pseudoelements on the .tabstrip-arrowscrollbox. This means that I lose the possibility to tie the visibility of the shadows to the disabled state of the scrollbutton.
What needs to happen is that the arrowscrollbox needs to get a class (I called it .scrollbutton-up-disabled and .scrollbutton-down-disabled) whenever one of the scrollbuttons gets disabled. I took a brief look at the JS that does the disabling, but I don't know the best approach here.

That's why I'm hoping that someone else can pick this up from here and add the JS bits.

Comment 19

[Matthew N. [:MattN]]

Comment on attachment 8391309 [details] [diff] [review]
WIP Patch 2.0

Review of attachment 8391309 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/shared/tabs.inc.css
@@ +170,5 @@
> +  -moz-margin-start: 22px;
> +}
> +
> +@media (min-resolution: 2dppx) {
> +  .tabbrowser-arrowscrollbox::before,

Note that this needs to be an ifdef otherwise we'll try use it on Windows where that image doesn't exist

@@ +172,5 @@
> +
> +@media (min-resolution: 2dppx) {
> +  .tabbrowser-arrowscrollbox::before,
> +  .tabbrowser-arrowscrollbox::after {
> +    background-image:url('chrome://browser/skin/tabbrowser/[e-mail]');

Nit: Use a space after the colon for CSS properties.

Comment 20

[:Gijs (he/him)]

Attachment: Add inner shadows to the tab overflow container,

This is what I made of it... I switched the CSS to using an attribute instead of a class, and I made it conditional on the container overflowing, because otherwise buttons are sometimes not disabled but collapsed instead. I could probably use the tabstrip's own overflow attribute for that, but I thought it was odd the scrollbox didn't already keep track of that in an attribute, so I added code for that instead. Hope that makes sense.

Comment 21

[:Gijs (he/him)]

Attachment: Add inner shadows to the tab overflow container,

Argh, forgot Matt's comments.

Comment 22

[Dão Gottwald [:dao]]

Comment on attachment 8392153 [details] [diff] [review]
Add inner shadows to the tab overflow container,

>--- a/browser/base/content/tabbrowser.xml
>+++ b/browser/base/content/tabbrowser.xml

>-            if (isEndTab || !this.mTabstrip._scrollButtonDown.disabled)
>+            if (isEndTab || !this.mTabstrip._scrollButtonDownDisabled)
>               return;

Why this change? Shouldn't be necessary. It doesn't matter much on which private property we depend on, but _scrollButtonDown.disabled seems more straightforward whereas the reason why _scrollButtonDownDisabled exists at all is a bit obscure.

>+.tabbrowser-arrowscrollbox[overflow=true] {
>+    position:relative;

space after color (throughout this patch)

>+.tabbrowser-arrowscrollbox[overflow=true]::before,
>+.tabbrowser-arrowscrollbox[overflow=true]::after {

These intercept clicks on the tabs that they overlay. I think you can avoid this with pointer-events:none.

>+  background-image:url('chrome://browser/skin/tabbrowser/tab-overflow-shadow.png');

Use double quotes or drop the quotes entirely.

>+  transition: opacity 150ms ease;

Does this work? I looked closely but couldn't really make out any transition.

>+.tabbrowser-arrowscrollbox[overflow=true]::before {
>+  -moz-margin-start: 22px;
>+}
>+
>+.tabbrowser-arrowscrollbox[overflow=true]:-moz-locale-dir(rtl)::before {
>+  left: 0;
>+  -moz-margin-end: 22px;
>+}

Seems like you want to use 'margin-left' regardless of the locale direction. Or maybe just 'left' -- not sure why you used margin instead.

>   <binding id="arrowscrollbox" extends="chrome://global/content/bindings/scrollbox.xml#scrollbox-base">
>     <content>
>       <xul:autorepeatbutton class="autorepeatbutton-up"
>                             anonid="scrollbutton-up"
>                             collapsed="true"
>-                            xbl:inherits="orient"
>+                            xbl:inherits="orient,disabled=scrollbutton-up-disabled"
>                             oncommand="_autorepeatbuttonScroll(event);"/>
>       <xul:scrollbox class="arrowscrollbox-scrollbox"
>                      anonid="scrollbox"
>                      flex="1"
>                      xbl:inherits="orient,align,pack,dir">
>         <children/>
>       </xul:scrollbox>
>       <xul:autorepeatbutton class="autorepeatbutton-down"
>                             anonid="scrollbutton-down"
>                             collapsed="true"
>-                            xbl:inherits="orient"
>+                            xbl:inherits="orient,disabled=scrollbutton-down-disabled"
>                             oncommand="_autorepeatbuttonScroll(event);"/>

You've used scrolling-up-disabled / scrolling-down-disabled elsewhere. I actually prefer scrollbutton-up-disabled / scrollbutton-down-disabled.

With this patch applied, the scroll buttons would never get into the disabled state. This may be due to the above mismatch, although quickly correcting it myself didn't seem to help. I may have done something wrong.

>+        this.removeAttribute("overflow");
>         this._scrollButtonUp.collapsed = true;
>         this._scrollButtonDown.collapsed = true;

nit: insert a blank line after the added one

Comment 23

[Dão Gottwald [:dao]]

Also, can the existing tab-overflow-border.png be merged into the shadow image? It seems dumb to have separate images and styles for these. Maybe something for a followup.

Comment 24

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Reassigning to reflect reality.

Comment 25

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Dão Gottwald [:dao] from comment #22)
> >+  transition: opacity 150ms ease;
> 
> Does this work? I looked closely but couldn't really make out any transition.

It does work (just tested it with a higher duration). For some reason, the disappearing transition feels shorter than the other way around. We should probably bump up the duration to 300ms here.

Another think I just encountered is that the buttons don't get disabled anymore…

Comment 26

[:Gijs (he/him)]

(In reply to Philipp Sackl [:phlsa] from comment #25)
> (In reply to Dão Gottwald [:dao] from comment #22)
> > >+  transition: opacity 150ms ease;
> > 
> > Does this work? I looked closely but couldn't really make out any transition.
> 
> It does work (just tested it with a higher duration). For some reason, the
> disappearing transition feels shorter than the other way around. We should
> probably bump up the duration to 300ms here.

Doublecheck this on the different platforms if these elements are actually hidden/removed, it might be that there are differences. :-\

> Another think I just encountered is that the buttons don't get disabled
> anymore…

Yes, Dão pointed that out here:

(In reply to Dão Gottwald [:dao] from comment #22)
> >   <binding id="arrowscrollbox" extends="chrome://global/content/bindings/scrollbox.xml#scrollbox-base">
> >     <content>
> >       <xul:autorepeatbutton class="autorepeatbutton-up"
> >                             anonid="scrollbutton-up"
> >                             collapsed="true"
> >-                            xbl:inherits="orient"
> >+                            xbl:inherits="orient,disabled=scrollbutton-up-disabled"
> >                             oncommand="_autorepeatbuttonScroll(event);"/>
> >       <xul:scrollbox class="arrowscrollbox-scrollbox"
> >                      anonid="scrollbox"
> >                      flex="1"
> >                      xbl:inherits="orient,align,pack,dir">
> >         <children/>
> >       </xul:scrollbox>
> >       <xul:autorepeatbutton class="autorepeatbutton-down"
> >                             anonid="scrollbutton-down"
> >                             collapsed="true"
> >-                            xbl:inherits="orient"
> >+                            xbl:inherits="orient,disabled=scrollbutton-down-disabled"
> >                             oncommand="_autorepeatbuttonScroll(event);"/>
> 
> You've used scrolling-up-disabled / scrolling-down-disabled elsewhere. I
> actually prefer scrollbutton-up-disabled / scrollbutton-down-disabled.
> 
> With this patch applied, the scroll buttons would never get into the
> disabled state. This may be due to the above mismatch, although quickly
> correcting it myself didn't seem to help. I may have done something wrong.


Per discussion on IRC, punting back to Philipp. :-)

Comment 27

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: Patch v3

This patch does some further simplification of the CSS and addresses the issues that Dão raised.

One thing I am a bit unsure about: I had to include both attributes _scrollButtonDown.disabled and _scrollButtonDownDisabled because I only could access the latter from the CSS while the former is responsible for actually disabling the scroll buttons.

Tested on OS X, XP, Win 7, Win 8 and Ubuntu. Gijs, I didn't encounter any issues with the visibility, but I'm only using opacity here.

Comment 28

[:Gijs (he/him)]

Comment on attachment 8394811 [details] [diff] [review]
Patch v3

Review of attachment 8394811 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great from a code perspective. I need to test it out properly (leaving feedback request for that), but generally I like it a lot. See below about both attributes (strictly speaking, they're XBL properties - attributes are only ever DOM things). If you have a moment, please check if my suggestion works...

Note also that Dão is on PTO the coming week, and I'm on PTO the week after that. You may or may not want to find an alternative reviewer (maybe Matt?).

::: toolkit/content/widgets/scrollbox.xml
@@ +501,5 @@
>  
>            this._scrollButtonUp.disabled = disableUpButton;
>            this._scrollButtonDown.disabled = disableDownButton;
> +          this._scrollButtonUpDisabled = disableUpButton;
> +          this._scrollButtonDownDisabled = disableDownButton;

This is confusing (I'm guessing this is what you meant when you said "I need both attributes"?). As I understand XBL, you should be able to just assign to _scrollButton(Up|Down)Disabled here. That'll alter the scrollbox attribute, which XBL should inherit down into the button's disabled attribute, which should make the button be disabled! If that doesn't work the way I think it should, then maybe my understanding is wrong, or maybe the xbl:inherits notation at the top (disabled=...) is wrong, or something.

Comment 29

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: Patch v3.1

Got rid of the attribute duplication mentioned in the last comment (thanks Gijs!)

Matt, could you review this since Dão is on PTO?

Comment 30

[:Gijs (he/him)]

(this looked good to me on Win8 and OS X, so retroactive f+! :-) )

Comment 31

[Matthew N. [:MattN]]

Comment on attachment 8395644 [details] [diff] [review]
Patch v3.1

Review of attachment 8395644 [details] [diff] [review]:
-----------------------------------------------------------------

Talos push:
Baseline (140ac04d7454): https://tbpl.mozilla.org/?tree=Try&rev=176db2cd0fa1
With patch: https://tbpl.mozilla.org/?tree=Try&rev=027937d38189

I'm reviewing the rest still.

::: browser/themes/shared/tabs.inc.css
@@ +128,5 @@
>    color: inherit;
>  }
>  
> +/* Tab Overflow */
> +.tabbrowser-arrowscrollbox[overflow=true] {

Please move this whole addition below the existing rule ".tabbrowser-arrowscrollbox > .arrowscrollbox-scrollbox".

@@ +160,5 @@
> +
> +.tabbrowser-arrowscrollbox[overflow=true]::after {
> +  right: 0;
> +  margin-right: 22px;
> +  transform: scale(-1, 1);

Nit: Use "scaleX(-1)". In the past we had performance problems with scaleX(-1) (e.g. on the tab ends) so we switched to flipped images. We should measure the effect on TART and the ts_paint/tpaint.

Comment 32

[Matthew N. [:MattN]]

Comment on attachment 8395644 [details] [diff] [review]
Patch v3.1

Review of attachment 8395644 [details] [diff] [review]:
-----------------------------------------------------------------

This looks really nice. I'm looking forward to this landing.

A (non-blocking) polish question: Is this the effect we want in this case? http://grab.by/vtik (hard line at the bottom of the shadow on the selected tab)

I highly suspect this will affect TART and I'm concerned the pseudo-elements will do weird things with the scrollbox but I need to play with it more. I'm still trying to catch up on the previous reviews and decisions.

I want to sleep on this and take another look tomorrow. Feel free to address the other comments in the meantime to get them out of the way. I think this is pretty close and likely the overall approach is fine (assuming no regressions).

::: browser/themes/shared/tabs.inc.css
@@ +135,5 @@
> +
> +.tabbrowser-arrowscrollbox[overflow=true]::before,
> +.tabbrowser-arrowscrollbox[overflow=true]::after {
> +  display: block;
> +  background-image: url("chrome://browser/skin/tabbrowser/tab-overflow-shadow.png");

We rarely use single quotes in our CSS but anyways the convention in this file is to not include any quotes for url(…).

@@ +137,5 @@
> +.tabbrowser-arrowscrollbox[overflow=true]::after {
> +  display: block;
> +  background-image: url("chrome://browser/skin/tabbrowser/tab-overflow-shadow.png");
> +  width: 12px;
> +  height: 30px;

I think you should use @tabHeight@ instead of 30px and specify background-position and background-repeat properties. Even better if height: 100% works here but I have a feeling it doesn't.

@@ +139,5 @@
> +  background-image: url("chrome://browser/skin/tabbrowser/tab-overflow-shadow.png");
> +  width: 12px;
> +  height: 30px;
> +  position: absolute;
> +  z-index: 3;

Add a comment about what the z-index is for.

@@ +141,5 @@
> +  height: 30px;
> +  position: absolute;
> +  z-index: 3;
> +  content: "";
> +  transition: opacity 150ms ease;

We should probably do the same transition on the arrow images in the future. It actually might be easy to add to this patch if you want (for consistency).

@@ +142,5 @@
> +  position: absolute;
> +  z-index: 3;
> +  content: "";
> +  transition: opacity 150ms ease;
> +  pointer-events: none;

Nit: By default, try to sort properties in new blocks alphabetically (especially when there are more than a handful) as it makes it easier to visually scan to find properties.

@@ +154,5 @@
> +}
> +
> +.tabbrowser-arrowscrollbox[overflow=true]::before {
> +  left: 0;
> +  margin-left: 22px;

Can you add a comment explaining where 22px comes from?

@@ +164,5 @@
> +  transform: scale(-1, 1);
> +}
> +
> +%ifdef XP_MACOSX
> +@media (min-resolution: 2dppx) {

I personally would prefer this ruleset right below the low DPI version.

@@ +167,5 @@
> +%ifdef XP_MACOSX
> +@media (min-resolution: 2dppx) {
> +  .tabbrowser-arrowscrollbox[overflow=true]::before,
> +  .tabbrowser-arrowscrollbox[overflow=true]::after {
> +    background-image: url('chrome://browser/skin/tabbrowser/[e-mail]');

Remove quotes here too

::: toolkit/content/widgets/scrollbox.xml
@@ +191,5 @@
> +        <setter><![CDATA[
> +          if (val) {
> +            this.setAttribute("scrollbutton-down-disabled", "true");
> +          } else {
> +            this.removeAttribute("scrollbutton-down-disabled");

Isn't this a breaking behaviour change for the existing attribute compared to setting |this._scrollButtonDown.disabled = false|?  IIUC, before the attribute would exist with the false value but now you are removing it. This could break selectors looking for [disabled="false"]. I don't see any extensions on add-ons MXR so maybe you're fine.

Comment 33

[Matthew N. [:MattN]]

Compare-talos: http://compare-talos.mattn.ca/?oldRevs=176db2cd0fa1&newRev=027937d38189&submit=true

Comment 34

[Matthew N. [:MattN]]

(In reply to Matthew N. [:MattN] from comment #33)
> Compare-talos:
> http://compare-talos.mattn.ca/
> ?oldRevs=176db2cd0fa1&newRev=027937d38189&submit=true

Looks like perf will be fine, what a surprise! (only 10.8 retriggers still need to come in)

Comment 35

[Matthew N. [:MattN]]

Comment on attachment 8395644 [details] [diff] [review]
Patch v3.1

I tested this on Windows 7, OS X 10.9, and Ubuntu 13.10 and didn't see behaviour regressions related to scrolling or the buttons.

Can you please address comment 31 and comment 32 and re-request review?

Comment 36

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: Patch v3.2

This addresses the points you've raised with two exceptions:

> @@ +137,5 @@
> > +.tabbrowser-arrowscrollbox[overflow=true]::after {
> > +  display: block;
> > +  background-image: url("chrome://browser/skin/tabbrowser/tab-overflow-shadow.png");
> > +  width: 12px;
> > +  height: 30px;
> 
> I think you should use @tabHeight@ instead of 30px and specify
> background-position and background-repeat properties. Even better if height:
> 100% works here but I have a feeling it doesn't.

I tried both and they have the same effect. However, they both mess with the positioning of the element. I can counter that, but it shifts kinda unpredictably when using larger text sizes on Windows. Using 30px I didn't encounter that issue so I'm leaving it there for now.


> ::: toolkit/content/widgets/scrollbox.xml
> @@ +191,5 @@
> > +        <setter><![CDATA[
> > +          if (val) {
> > +            this.setAttribute("scrollbutton-down-disabled", "true");
> > +          } else {
> > +            this.removeAttribute("scrollbutton-down-disabled");
> 
> Isn't this a breaking behaviour change for the existing attribute compared
> to setting |this._scrollButtonDown.disabled = false|?  IIUC, before the
> attribute would exist with the false value but now you are removing it. This
> could break selectors looking for [disabled="false"]. I don't see any
> extensions on add-ons MXR so maybe you're fine.

I think the scrollbutton-down-disabled has been introduced only by this patch, so it shouldn't break anything. I tried to do this in a couple of different ways, but every attempt broke something.

Comment 37

[:Gijs (he/him)]

(In reply to Philipp Sackl [:phlsa] from comment #36)
> > ::: toolkit/content/widgets/scrollbox.xml
> > @@ +191,5 @@
> > > +        <setter><![CDATA[
> > > +          if (val) {
> > > +            this.setAttribute("scrollbutton-down-disabled", "true");
> > > +          } else {
> > > +            this.removeAttribute("scrollbutton-down-disabled");
> > 
> > Isn't this a breaking behaviour change for the existing attribute compared
> > to setting |this._scrollButtonDown.disabled = false|?  IIUC, before the
> > attribute would exist with the false value but now you are removing it. This
> > could break selectors looking for [disabled="false"]. I don't see any
> > extensions on add-ons MXR so maybe you're fine.
> 
> I think the scrollbutton-down-disabled has been introduced only by this
> patch, so it shouldn't break anything. I tried to do this in a couple of
> different ways, but every attempt broke something.

Yes, but it's inherited into the buttons' disabled attributes. However, that seems to also be how the disabled attribute works:

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/general.xml#15

so I still think the current code is correct.

As far as add-on compat goes, this system will go wonky if add-ons set the scrollbutton's disabled values themselves (because it'll go out of sync with the attribute we're setting on the scrollbox), but there's not really anything we can do about that and I would have thought it unlikely. :-)

Comment 38

[Matthew N. [:MattN]]

Comment on attachment 8398430 [details] [diff] [review]
Patch v3.2

Review of attachment 8398430 [details] [diff] [review]:
-----------------------------------------------------------------

Reminder to look into polish question at the top of comment 32.

(In reply to Philipp Sackl [:phlsa] from comment #36)
> > @@ +137,5 @@
> > > +.tabbrowser-arrowscrollbox[overflow=true]::after {
> > > +  display: block;
> > > +  background-image: url("chrome://browser/skin/tabbrowser/tab-overflow-shadow.png");
> > > +  width: 12px;
> > > +  height: 30px;
> > 
> > I think you should use @tabHeight@ instead of 30px and specify
> > background-position and background-repeat properties. Even better if height:
> > 100% works here but I have a feeling it doesn't.
> 
> I tried both and they have the same effect. However, they both mess with the
> positioning of the element. I can counter that, but it shifts kinda
> unpredictably when using larger text sizes on Windows. Using 30px I didn't
> encounter that issue so I'm leaving it there for now.

Well, tabHeight is currently 31px so you would have to offset by 1px in order to get the identical size. If this is caused by the nav-bar overlap, you can use the new define in bug 990384: e.g. calc(@tabHeight@ - @tabToolbarNavbarOverlap@) (with that define in your tree).

Can you try this? I'll try it myself tomorrow otherwise. I'm trying to avoid another magic number of 30 since it's fragile and likely to need a change for bug 946987.

(In reply to :Gijs Kruitbosch from comment #37)
> However, that seems to also be how the disabled attribute works:
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/
> general.xml#15

Ah, I should have looked at that.

::: browser/themes/shared/tabs.inc.css
@@ +156,5 @@
> +
> +.tabbrowser-arrowscrollbox[overflow=true]::before {
> +  left: 0;
> +  /* Move the shadow so that it doesn't overlap with the scrollbutton */
> +  margin-left: 22px;

I meant for the comment to explain how you came up with the number 22px. The width of the button is 23px on 10.9 regular DPI so is there an intentional 1px difference, if so, that should be explained so somebody doesn't come along and "fix" it to match 23px.

::: toolkit/content/widgets/scrollbox.xml
@@ +499,5 @@
>                disableDownButton = true;
>            }
>  
> +          //this._scrollButtonUp.disabled = disableUpButton;
> +          //this._scrollButtonDown.disabled = disableDownButton;

This was supposed to be removed, right?

Comment 39

[Dão Gottwald [:dao]]

(In reply to Matthew N. [:MattN] from comment #38)
> Comment on attachment 8398430 [details] [diff] [review]
> Patch v3.2
> 
> Review of attachment 8398430 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Reminder to look into polish question at the top of comment 32.
> 
> (In reply to Philipp Sackl [:phlsa] from comment #36)
> > > @@ +137,5 @@
> > > > +.tabbrowser-arrowscrollbox[overflow=true]::after {
> > > > +  display: block;
> > > > +  background-image: url("chrome://browser/skin/tabbrowser/tab-overflow-shadow.png");
> > > > +  width: 12px;
> > > > +  height: 30px;
> > > 
> > > I think you should use @tabHeight@ instead of 30px and specify
> > > background-position and background-repeat properties. Even better if height:
> > > 100% works here but I have a feeling it doesn't.
> > 
> > I tried both and they have the same effect. However, they both mess with the
> > positioning of the element. I can counter that, but it shifts kinda
> > unpredictably when using larger text sizes on Windows. Using 30px I didn't
> > encounter that issue so I'm leaving it there for now.
> 
> Well, tabHeight is currently 31px so you would have to offset by 1px in
> order to get the identical size. If this is caused by the nav-bar overlap,
> you can use the new define in bug 990384: e.g. calc(@tabHeight@ -
> @tabToolbarNavbarOverlap@) (with that define in your tree).

30px is just the height of used image (tab-overflow-shadow.png), right? Nothing really magic about that, nor is it directly tied to the tab height. If you want to decouple this element's height from the image size, then you'll need to make sure the background image is either stretched or positioned correctly and not repeated.

Comment 40

[Matthew N. [:MattN]]

(In reply to Dão Gottwald [:dao] from comment #39)
> (In reply to Matthew N. [:MattN] from comment #38)
> > Comment on attachment 8398430 [details] [diff] [review]
> > Patch v3.2
> > 
> > Review of attachment 8398430 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Reminder to look into polish question at the top of comment 32.
> > 
> > (In reply to Philipp Sackl [:phlsa] from comment #36)
> > > > @@ +137,5 @@
> > > > > +.tabbrowser-arrowscrollbox[overflow=true]::after {
> > > > > +  display: block;
> > > > > +  background-image: url("chrome://browser/skin/tabbrowser/tab-overflow-shadow.png");
> > > > > +  width: 12px;
> > > > > +  height: 30px;
> > > > 
> > > > I think you should use @tabHeight@ instead of 30px and specify
> > > > background-position and background-repeat properties. Even better if height:
> > > > 100% works here but I have a feeling it doesn't.
> > > 
> > > I tried both and they have the same effect. However, they both mess with the
> > > positioning of the element. I can counter that, but it shifts kinda
> > > unpredictably when using larger text sizes on Windows. Using 30px I didn't
> > > encounter that issue so I'm leaving it there for now.
> > 
> > Well, tabHeight is currently 31px so you would have to offset by 1px in
> > order to get the identical size. If this is caused by the nav-bar overlap,
> > you can use the new define in bug 990384: e.g. calc(@tabHeight@ -
> > @tabToolbarNavbarOverlap@) (with that define in your tree).
> 
> 30px is just the height of used image (tab-overflow-shadow.png), right?
> Nothing really magic about that, nor is it directly tied to the tab height.
> If you want to decouple this element's height from the image size, then
> you'll need to make sure the background image is either stretched or
> positioned correctly and not repeated.

Right, I proposed we make the height 100% and set background-repeat a few replies up (quoted above).

Comment 41

[Matthew N. [:MattN]]

Comment on attachment 8398430 [details] [diff] [review]
Patch v3.2

See comment 38

Comment 42

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: Patch v3.3

> A (non-blocking) polish question: Is this the effect we want in this case?
> http://grab.by/vtik (hard line at the bottom of the shadow on the selected
> tab)
Yes, that's intentional. It might be something to investigate again in the future, but I think it's fine for now.

> Well, tabHeight is currently 31px so you would have to offset by 1px in
> order to get the identical size. If this is caused by the nav-bar overlap,
> you can use the new define in bug 990384: e.g. calc(@tabHeight@ -
> @tabToolbarNavbarOverlap@) (with that define in your tree).
> 
> Can you try this? I'll try it myself tomorrow otherwise. I'm trying to avoid
> another magic number of 30 since it's fragile and likely to need a change
> for bug 946987.
Yes, that does work :)

> ::: browser/themes/shared/tabs.inc.css
> @@ +156,5 @@
> > +
> > +.tabbrowser-arrowscrollbox[overflow=true]::before {
> > +  left: 0;
> > +  /* Move the shadow so that it doesn't overlap with the scrollbutton */
> > +  margin-left: 22px;
> 
> I meant for the comment to explain how you came up with the number 22px. The
> width of the button is 23px on 10.9 regular DPI so is there an intentional
> 1px difference, if so, that should be explained so somebody doesn't come
> along and "fix" it to match 23px.
Fixed!

> ::: toolkit/content/widgets/scrollbox.xml
> @@ +499,5 @@
> >                disableDownButton = true;
> >            }
> >  
> > +          //this._scrollButtonUp.disabled = disableUpButton;
> > +          //this._scrollButtonDown.disabled = disableDownButton;
> 
> This was supposed to be removed, right?
Yes!

Comment 43

[Matthew N. [:MattN]]

Comment on attachment 8401333 [details] [diff] [review]
Patch v3.3

Review of attachment 8401333 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/shared/tabs.inc.css
@@ +114,5 @@
>  }
>  
> +/* Tab Overflow */
> +.tabbrowser-arrowscrollbox[overflow=true] {
> +    position:relative;

Nit: Should only be indented 2 spaces and the space after the colon is missing.

@@ +123,5 @@
> +  background-image: url(chrome://browser/skin/tabbrowser/tab-overflow-shadow.png);
> +  background-repeat: no-repeat;
> +  content: "";
> +  display: block;
> +  height: calc(@tabHeight@ - @tabToolbarNavbarOverlap@);

This should be calc(100% - @tabToolbarNavbarOverlap@) with the proper background-size so it stretches vertically.

@@ +157,5 @@
> +
> +.tabbrowser-arrowscrollbox[overflow=true]::before {
> +  left: 0;
> +  /* 22px to avoid gap between the button and the shadow on Windows */
> +  margin-left: 22px;

I'm confused because the comment is saying this is for Windows but it's applying on all OSs. From what I can tell this should be 22px on Windows and 23px elsewhere.

Comment 44

[Matthew N. [:MattN]]

Attachment: Patch v.3.4 - Address comment 43 - Height 100% stretching and define correct sizes of the scrollbuttons

This addresses comment 43.

Can you test that this doesn't "mess with the positioning of the element" and review the interdiff? If it's fine with you, put checkin-needed in the whiteboard and we're good to go.

Thanks for your patience on this. If we were on IRC for the same hours this process could have been faster.

Comment 45

[Matthew N. [:MattN]]

I also adjusted the comment. See the interdiff at https://bugzilla.mozilla.org/attachment.cgi?oldid=8401333&action=interdiff&newid=8401730&headers=1

Comment 46

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Looks great anywhere I tested (which was OS X, Linux, Windows XP, 7, 8)!
Thanks for your help!

Comment 47

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/5e4eaa3891be

Comment 48

[Matthew N. [:MattN]]

Comment on attachment 8401730 [details] [diff] [review]
Patch v.3.4 - Address comment 43 - Height 100% stretching and define correct sizes of the scrollbuttons

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New visual design
User impact if declined: tab overflow separators will look too much like background tab separators and doesn't make it as clear that you can scroll.
Testing completed (on m-c, etc.): locally on many platforms and m-c soon
Risk to taking this patch (and alternatives if risky): Low-Medium risk since there is a potential for bugs related to the scrolling of tabs or the state of the scroll buttons. We have seen unusual bugs come from changes of similar scope that were unexpected. Sometimes the bugs are subtle and require things like switching between windows. I will request QA take a pass as well.
String or IDL/UUID changes made by this patch: None

We will probably wait a few days before actually uplifting to get bake time since this is not the lowest risk.

Comment 49

[Matthew N. [:MattN]]

QA: could we get some manual testing of this patch within the next few days when it makes it to m-c? We would like to uplift this to aurora and beta so the additional testing will help reduce the risk.

The change was to add shadows for tab overflow (when you have scroll arrows in the tabstrip) on all platforms. The shadows should not change where you can click (i.e. clicks should go through them).

We should look for issues like the following (as a starting point) and check if they exist before this patch:
* Tabs scroll unexpectedly with overflow. e.g. switching to another Firefox window or selecting a pinned or fully visible tab shouldn't lead to scrolling.
* The overflow scroll buttons should work the same as before (including when they're enabled and disabled)
* See how resizing the window affects the overflow scroll

Existing tab overflow bugs: 
* bug 927173 - I'm not sure if this is still a problem. I guess we should check that and see if this makes it worse.

Comment 50

[Matthew N. [:MattN]]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/ed58d156be61 due to mochitest b-c failures:
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_removeTabsToTheEnd.js | Length is 2 - Got 3, expected 2
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_removeTabsToTheEnd.js | Found an unexpected tab at the end of test run: about:blank

Log: https://tbpl.mozilla.org/php/getParsedLog.php?id=37342604&tree=Fx-Team

Comment 51

[:Gijs (he/him)]

Removing QA keywords now that this was backed out...

Comment 52

[u279076]

Adding back verifyme so this stays on our radar once fixed.

Comment 53

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Matthew N. [:MattN] from comment #50)
> Backed out in https://hg.mozilla.org/integration/fx-team/rev/ed58d156be61
> due to mochitest b-c failures:
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/base/content/test/general/
> browser_removeTabsToTheEnd.js | Length is 2 - Got 3, expected 2
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/base/content/test/general/
> browser_removeTabsToTheEnd.js | Found an unexpected tab at the end of test
> run: about:blank
> 
> Log: https://tbpl.mozilla.org/php/getParsedLog.php?id=37342604&tree=Fx-Team

FYI, this only reproduces when running a larger set of tests, but not when the test is run standalone. Still investigating...

Comment 54

[Jared Wein [:jaws] (please needinfo? me)]

Narrowed it down to:
mach mochitest-browser browser/base/content/test/general/ --start-at browser/base/content/test/general/browser_overflowScroll.js --end-at browser/base/content/test/general/browser_removeTabsToTheEnd.js

Comment 55

[Jared Wein [:jaws] (please needinfo? me)]

I think it has something to do with the "overflow" attribute not being removed from the tabstrip after browser_overflowScroll.js when this patch is applied.

Comment 56

[Jared Wein [:jaws] (please needinfo? me)]

So I think removing the "overflow" attribute in scrollbox.xml is causing the "underflow" event to never be fired.

Comment 57

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8401730 [details] [diff] [review]
Patch v.3.4 - Address comment 43 - Height 100% stretching and define correct sizes of the scrollbuttons

Waited two days. Approving to make sure it is in beta 7

Comment 58

[Dão Gottwald [:dao]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #57)
> Comment on attachment 8401730 [details] [diff] [review]
> Patch v.3.4 - Address comment 43 - Height 100% stretching and define correct
> sizes of the scrollbuttons
> 
> Waited two days. Approving to make sure it is in beta 7

This isn't even on mozilla-central. It got backed out for test failures.

Comment 59

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8401730 [details] [diff] [review]
Patch v.3.4 - Address comment 43 - Height 100% stretching and define correct sizes of the scrollbuttons

Oups. Sorry.

Comment 60

[Jared Wein [:jaws] (please needinfo? me)]

I need to stop investigating this as it's consumed a couple days now and I still don't have the fix for it.

A hack to get the tests passing is to set browser.tabs.animate=false within browser_removeTabsToTheEnd.js, but that doesn't fix the root cause of the issue.

browser_overflowScroll.js adds a bunch of tabs and then removes them. At the end of this test, the "overflow" attribute isn't removed (the "underflow" event is never fired). I think this may be caused by the presence of the generated content. My hypothesis is that having the generated content showing is preventing underflow on the tab strip.

Matt, what do you think about this?

Comment 61

[Matthew N. [:MattN]]

That seems very reasonable. As discussed in our meeting, we are going to treat this just like other P4+'s (instead of increased priority due to its visibility) so it may miss 29.

Philipp or whoever finishes this: We may need to go back to something like a previous approach with the pseudo elements on a different parent. You can run the test command in comment 54 to see if it fixes the issue.

Comment 62

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Since I probably won't get to this in the next two weeks, I'll unassign myself for now and instead nominate it for the backlog.

Comment 63

[Dão Gottwald [:dao]]

Attachment: patch v4

Comment 64

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Just tried the patch and it looks great overall.
The only thing is that when I start the browser with just one tab, the shadows are visible until I open a second tab.

Comment 65

[Dão Gottwald [:dao]]

Attachment: patch v5

(In reply to Philipp Sackl [:phlsa] from comment #64)
> Just tried the patch and it looks great overall.
> The only thing is that when I start the browser with just one tab, the
> shadows are visible until I open a second tab.

Should be fixed now. Thanks for testing.

Comment 66

[Dão Gottwald [:dao]]

Attachment: patch v6

Added back the rule that would invert the scroll arrows on dark backgrounds. I had accidentally removed this in the previous versions.

Comment 67

[Dão Gottwald [:dao]]

Comment on attachment 8433417 [details] [diff] [review]
patch v6

Switching reviewers since I'm already keeping Matt occupied in another bug. Gijs for browser, Neil for toolkit.

Comment 68

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #67)
> Comment on attachment 8433417 [details] [diff] [review]
> patch v6
> 
> Switching reviewers since I'm already keeping Matt occupied in another bug.
> Gijs for browser, Neil for toolkit.

I'll look at this in more detail in the next couple of hours, but can you do try pushes to verify that (a) this doesn't break the tests it broke before, and (b) it doesn't regress TART?

Comment 69

[Dão Gottwald [:dao]]

Tests are passing (https://tbpl.mozilla.org/?tree=Try&rev=4a1907a9c775).

Comment 70

[:Gijs (he/him)]

Comment on attachment 8433417 [details] [diff] [review]
patch v6

Review of attachment 8433417 [details] [diff] [review]:
-----------------------------------------------------------------

The browser/ side looks exceedingly fine to me, with one caveat...

Comment 71

[:Gijs (he/him)]

Attachment: Updated indicator png

... this file is a 20% smaller version of the PNG we're using, and should look the same (done using ImageOptim.app)

Comment 72

[Dão Gottwald [:dao]]

Attachment: patch v7

with the optimized PNG

Comment 73

[Dão Gottwald [:dao]]

http://compare-talos.mattn.ca/?oldRevs=e1b280df9ff6&newRev=dfea7f9af835&server=graphs.mozilla.org&submit=true

Comment 74

[Neil Deakin]

Could you explain what the change in scrollbox.xml is fixing or changing?

Comment 75

[Dão Gottwald [:dao]]

(In reply to Neil Deakin from comment #74)
> Could you explain what the change in scrollbox.xml is fixing or changing?

Basically, I'm adding the arrowscrollbox-overflow-start-indicator and arrowscrollbox-overflow-end-indicator nodes and collapse them when the tab strip doesn't overflow or is scrolled to the start or to the end, respectively. These two elements aren't styled in stock scrollboxes and require the individual consumer to do so if desired; the the browser/ part of my patch does that.

Comment 76

[Dão Gottwald [:dao]]

Note that I could implement this entirely in tabbrowser in a binding that extends the toolkit one. However, I would then need to duplicate / fork toolkit's the anonymous content definition or inject the two nodes into the anonymous content on the fly, both of which isn't ideal. I figured that the two nodes might come handy to other arrowscrollbox consumers as well and are otherwise harmless.

Comment 77

[[e-mail]]

Comment on attachment 8434095 [details] [diff] [review]
patch v7

(Obviously I've only looked at the toolkit changes.)

The use of the (inverted) overflow attribute concerned me, and in fact I just noticed that calendar has some autorepeatbuttons which would be affected by this change. I don't have permission to MXR addons, so I was wondering whether it would be a better idea to use an attribute which you can then inherit to collapse the buttons (in a similar way as for the scrolled-to-start attribute).

Nit: the overflow indicators could probably be spacers.

Comment 78

[Dão Gottwald [:dao]]

Attachment: patch v8

(In reply to [e-mail] from comment #77)
> The use of the (inverted) overflow attribute concerned me, and in fact I
> just noticed that calendar has some autorepeatbuttons which would be
> affected by this change. I don't have permission to MXR addons, so I was
> wondering whether it would be a better idea to use an attribute which you
> can then inherit to collapse the buttons (in a similar way as for the
> scrolled-to-start attribute).

I replaced the overflow attribute with one that I called not-overflowing. Suggestions for a better name are welcome.

> Nit: the overflow indicators could probably be spacers.

Yes, this works.

Comment 79

[Neil Deakin]

Comment on attachment 8437377 [details] [diff] [review]
patch v8

+.tabbrowser-arrowscrollbox > .arrowscrollbox-overflow-start-indicator:not([collapsed]),
+.tabbrowser-arrowscrollbox > .arrowscrollbox-overflow-end-indicator:not([collapsed]) {
 ...

You have some rules here that only apply if the indicator is collapsed. If the indicator is collapsed though it doesn't matter what the properties are. I didn't review the browser changes so I'm guessing I'm missing something here.



>-          this._scrollButtonUp.disabled = disableUpButton;
>-          this._scrollButtonDown.disabled = disableDownButton;
>+          if (scrolledToEnd)
>+            this.setAttribute("scrolled-to-end", "true");
>+          else
>+            this.removeAttribute("scrolled-to-end");
>+
>+          if (scrolledToStart)
>+            this.setAttribute("scrolled-to-start", "true");
>+          else
>+            this.removeAttribute("scrolled-to-start");

Please no hyphens in xul attribute names.


>-        this._scrollButtonUp.collapsed = true;
>-        this._scrollButtonDown.collapsed = true;
>+        this.setAttribute("not-overflowing", "true");
>+

Same here.

I thought of maybe using 'underflowing' but 'notoverflowing' seems clearer.

>+      <xul:spacer class="arrowscrollbox-overflow-end-indicator"
>+                  xbl:inherits="collapsed=scrolled-to-end"/>

Yes, spacers should be used when you don't expect the element to have any children. It looks like you're using it like an image though.

Comment 80

[Dão Gottwald [:dao]]

(In reply to Neil Deakin from comment #79)
> +.tabbrowser-arrowscrollbox >
> .arrowscrollbox-overflow-start-indicator:not([collapsed]),
> +.tabbrowser-arrowscrollbox >
> .arrowscrollbox-overflow-end-indicator:not([collapsed]) {
>  ...
> 
> You have some rules here that only apply if the indicator is collapsed. If
> the indicator is collapsed though it doesn't matter what the properties are.
> I didn't review the browser changes so I'm guessing I'm missing something
> here.

AFAIK, the element being collapsed wouldn't prevent the image from being loaded. Since most users won't have enough tabs to let the tab bar overflow, this seems wasteful.

> Please no hyphens in xul attribute names.

ok...

> Yes, spacers should be used when you don't expect the element to have any
> children. It looks like you're using it like an image though.

I started with images, but they inherited list-style-image in a menupopup's anonymous arrowscrollbox from the parent menuitem. I would need to add extra CSS to prevent that.

Comment 81

[Neil Deakin]

Comment on attachment 8437377 [details] [diff] [review]
patch v8

(In reply to Dão Gottwald [:dao] from comment #80)
> AFAIK, the element being collapsed wouldn't prevent the image from being
> loaded. Since most users won't have enough tabs to let the tab bar overflow,
> this seems wasteful.

Hmm. That's very likely the case.

Comment 82

[Dão Gottwald [:dao]]

(In reply to Neil Deakin from comment #81)
> Comment on attachment 8437377 [details] [diff] [review]
> patch v8
> 
> (In reply to Dão Gottwald [:dao] from comment #80)
> > AFAIK, the element being collapsed wouldn't prevent the image from being
> > loaded. Since most users won't have enough tabs to let the tab bar overflow,
> > this seems wasteful.
> 
> Hmm. That's very likely the case.

And using "hidden" instead of "collapsed" would break the opacity transition...

https://hg.mozilla.org/integration/fx-team/rev/b37d661c63df

Comment 83

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b37d661c63df

Comment 84

[[e-mail]]

(In reply to Dão Gottwald from comment #80)
> I started with images, but they inherited list-style-image in a menupopup's
> anonymous arrowscrollbox from the parent menuitem. I would need to add extra
> CSS to prevent that.

Not sure what you mean there; normally you either want to inherit list-style-image (e.g. for a toolbarbutton's icon) or to have explicit list-style-image (and therefore not inherit).

Comment 85

[Dão Gottwald [:dao]]

(In reply to [e-mail] from comment #84)
> (In reply to Dão Gottwald from comment #80)
> > I started with images, but they inherited list-style-image in a menupopup's
> > anonymous arrowscrollbox from the parent menuitem. I would need to add extra
> > CSS to prevent that.
> 
> Not sure what you mean there; normally you either want to inherit
> list-style-image (e.g. for a toolbarbutton's icon) or to have explicit
> list-style-image (and therefore not inherit).

For arrowscrollbox-overflow-start-indicator and arrowscrollbox-overflow-end-indicator in a menupopup's arrowscrollbox, we want to neither inherit nor set an image. I.e. the indicators should just be invisible there. We would have had to set list-style-image:none to that effect.

Comment 86

[Ada [:adalucinet]]

Verified with latest Nightly 33.0a1 (Build ID: 20140612030349) on Windows 7 x64 [1], Ubuntu 14.04 x32 [2] and Mac OS X 10.9.2 [3] based on steps from comment 49.
Following some additional exploratory testing, there was one new issue found - Bug 1024496 - which will be verified as soon as today's Nightly builds are available.

Since the above issue is tracked separately, marking this as verified.

[1] Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:33.0) Gecko/20100101 Firefox/33.0
[2] Mozilla/5.0 (X11; Linux i686; rv:33.0) Gecko/20100101 Firefox/33.0
[3] Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:33.0) Gecko/20100101 Firefox/33.0

Comment 87

[Dão Gottwald [:dao]]

Attachment: patch for aurora uplift, including the fix for bug 1024496

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: see comment 3
Testing completed (on m-c, etc.): verified on m-c
Risk to taking this patch (and alternatives if risky): medium, easy to back out if necessary
String or IDL/UUID changes made by this patch: none

Comment 88

[Lawrence Mandel [:lmandel] (use needinfo)]

Given that this has medium risk, I'd like to give it a few more days of bake time on m-c before granting uplift to Aurora. If everything is well on m-c, let's target the middle of next week for uplift.

Comment 89

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8439924 [details] [diff] [review]
patch for aurora uplift, including the fix for bug 1024496

I haven't heard of any fallout. Aurora approval granted.

Comment 90

[Dão Gottwald [:dao]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bbfe5c6ea7c9

Comment 91

[Catalin Varga [QA][:VarCat]]

Verified as fixed using the following environment:

FF 32
Build Id:20140619004025
OS: Win 7 x64, Mac Os X 10.8.5, Ubuntu 13.04 x64