342841 - Scrolling arrow should look disabled when you can't scroll in its direction

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect, P2)

Description

[Adam Kowalczyk]

Scrolling arrows introduced by patch in bug 318168 fail to indicate that you have reached the end of the tab bar and you can't scroll any further in that direction. As a result, user can't tell (i.e. has no visual indication) where he is in the tab bar.

Comment 1

[Mike Connor [:mconnor]]

Disabled appearance > hidden, since otherwise there's annoying jumps.

Not a blocker, will take patch.

Comment 2

[hhh]

Can a :hover state be added to the arrows as well under this bug? Maybe a popup list of the tabs that are not yet visible?

Comment 3

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

*** Bug 342907 has been marked as a duplicate of this bug. ***

Comment 4

[(not reading, please use [e-mail] instead)]

adam (and ben) point out this is needed because otherwise, it can be hard to know if you have more tabs (to the left or right)

Comment 5

[Ben Goodger (use ben at mozilla dot org for email)]

This is a blocker I think, probably not for b1 though. Since this is new functionality, and we're going to crow about it, it should work right. Right now it's a bit misleading and has been confusing the people I've seen trying to use it. 

Comment 6

[(not reading, please use [e-mail] instead)]

in order to prevent the ui from moving out from underneath the user while scrolling (and they hit the edge), mconnor and I agree that disabling is the right fix.

see related bug #222274

Comment 7

[Asaf Romano (gone)]

Taking. I'm attaching the binding changes. Could someone provide disabled arrows images (at least temporary images)?

Comment 8

[Asaf Romano (gone)]

Attachment: patch (binding changes only)

This is just the binding changes.

(side note: this will probably also fix bug 222274 if vertical disabled arrows images are provided as well ;) )

Comment 9

[Asaf Romano (gone)]

Attachment: patch (binding changes only)

typos fixes

Comment 10

[(not reading, please use [e-mail] instead)]

Comment on attachment 227986 [details] [diff] [review]
patch (binding changes only)

r=sspitzer

as mentioned on irc, please remove these two lines from the _updateScrollButtonsDisabledState() method:

+            var isLTR = window.getComputedStyle(this.parentNode, "")
+                          .direction == "ltr";

Comment 11

[Asaf Romano (gone)]

Actually, it looks like we do have disabled arrows images.

Comment 12

[Asaf Romano (gone)]

Attachment: patch as checked in (binding changes only)

mozilla/toolkit/content/widgets/scrollbox.xml 1.11

Comment 13

[Asaf Romano (gone)]

Comment on attachment 228022 [details] [diff] [review]
patch as checked in (binding changes only)

This can't land on the 1.8 branch without the fix for bug 314350.

Comment 14

[Asaf Romano (gone)]

Attachment: Winstripe changes

Comment 15

[Mike Connor [:mconnor]]

Comment on attachment 228027 [details] [diff] [review]
Winstripe changes

r=me, with the missing piece (dis) in the right places

Comment 16

[Asaf Romano (gone)]

Attachment: Winstripe changes as checked in

mozilla/toolkit/themes/winstripe/global/scrollbox.css 1.7

Comment 17

[Asaf Romano (gone)]

Attachment: make this work for legacy autorepeat-based scrollboxes (checked in)

This makes the above magic work for menus (e.g. bookmark folders).

Comment 18

[Asaf Romano (gone)]

Comment on attachment 228029 [details] [diff] [review]
make this work for legacy autorepeat-based scrollboxes (checked in)

mozilla/toolkit/content/widgets/scrollbox.xml 1.12

Comment 19

[Asaf Romano (gone)]

Attachment: Pinstripe changes (checked in)

Comment 20

[Asaf Romano (gone)]

Comment on attachment 228032 [details] [diff] [review]
Pinstripe changes (checked in)

mozilla/toolkit/themes/pinstripe/global/scrollbox.css 1.5

Comment 21

[Asaf Romano (gone)]

Comment on attachment 228022 [details] [diff] [review]
patch as checked in (binding changes only)

Asking for 181approval now that the fix for bug 314350 is on the branch (my request here is on the entire set of patches for this bug).

Comment 22

[Mike Schroepfer]

Comment on attachment 228022 [details] [diff] [review]
patch as checked in (binding changes only)

All patches approved for 1.8.1 drivers.

Comment 23

[Asaf Romano (gone)]

Fixed on 1.8 branch for beta 1 (branch patch is on bug 318168).