Closed Bug 420763 Opened 12 years ago Closed 12 years ago

Alt + Scroll Down => can't scroll through all tabs

Categories

(Toolkit :: XUL Widgets, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9

People

(Reporter: gevazeichner, Assigned: dao)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3) Gecko/2008020514 Firefox/3.0b3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3) Gecko/2008020514 Firefox/3.0b3

While there were many tabs open, I've tried slow-scrolling (Alt+scroll) through them on the tab bar. I was successful when scrolling up (to the left), but unsuccessful (it scrolled only a few pixels and got stuck) when scrolling down (to the right).

Reproducible: Always

Steps to Reproduce:
1. Open at least 10 tabs on a 1024*768 display.
2. Resize the window to half of the screen width.
3. Slow scroll the tabs to the left (Alt + scroll up).
4. Scroll until you get to the most left tab.
5. Slow scroll the tabs to the right (Alt + scroll down).
6. Scroll until you get to the most right tab.
Actual Results:  
Scrolling to the left is perfect - reaches all the tabs, just by using slow-scrolling.
Scrolling to the right scrolls only a tab or two and then stops. No way to keep slow-scrolling without using regular speed scroll.

Expected Results:  
Scrolling to the left is perfect - reaches all the tabs, just by using slow-scrolling.
Scrolling to the right is perfect - reaches all the tabs, just by using slow-scrolling.
Version: unspecified → Trunk
Depends on: 347363
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch patch (obsolete) — Splinter Review
|scrollBox.screenX + scrollBox.width| is a point from within the scrollbox. The element that we want to find starts at |scrollBox.screenX + scrollBox.width + 1|, similar to the index < 0 case where we're already subtracting 1.
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #307106 - Flags: review?(enndeakin)
OS: Windows XP → All
Hardware: PC → All
Attachment #307106 - Flags: review?(enndeakin)
(In reply to comment #1)
> |scrollBox.screenX + scrollBox.width| is a point from within the scrollbox

Actually, it's not, but there seems to be a something like a rounding error in the boxObject.screenX getter. For instance, I see this for the tabs in tab bar:

index |  x  | screenX
  0   | 20  |  -57
  1   | 120 |   42
  2   | 220 |  142
  3   | 320 |  243
  3   | 420 |  342

... and so on.
Attachment #307106 - Attachment is obsolete: true
(In reply to comment #2)
> index |  x  | screenX
>   0   | 20  |  -57
>   1   | 120 |   42
>   2   | 220 |  142
>   3   | 320 |  243
>   3   | 420 |  342

Neil, do you understand why screenX is sometimes off by 1 pixel?
x=20 for the first tab doesn't seem to make sense either. scrollbox.boxObject.x is 18. I have no idea where the extra two pixels come from.
(In reply to comment #4)
> x=20 for the first tab doesn't seem to make sense either. scrollbox.boxObject.x
> is 18. I have no idea where the extra two pixels come from.
> 

From the border/padding/margin between the box and the tab.

> Neil, do you understand why screenX is sometimes off by 1 pixel?

Because the screen rectangle is in screen coordinates rounded to integers.
(In reply to comment #5)
> (In reply to comment #4)
> > x=20 for the first tab doesn't seem to make sense either. scrollbox.boxObject.x
> > is 18. I have no idea where the extra two pixels come from.
> 
> From the border/padding/margin between the box and the tab.

The scrollbox doesn't have padding or a border, the tabs don't have horizontal margin. Also, screenX doesn't contain these two pixels.

> > Neil, do you understand why screenX is sometimes off by 1 pixel?
> 
> Because the screen rectangle is in screen coordinates rounded to integers.

So we can't really use screenX for a non-buggy solution.
> The scrollbox doesn't have padding or a border, the tabs don't have horizontal
> margin. Also, screenX doesn't contain these two pixels.

I see a 2 pixel border on the tab.
Right, the tab has a border. I don't see why that affects x. Unfortunately, I didn't find information about what x actually means.
Assignee: dao → nobody
Status: ASSIGNED → NEW
Keywords: helpwanted
Component: Tabbed Browser → XUL Widgets
Product: Firefox → Toolkit
QA Contact: tabbed.browser → xul.widgets
Flags: blocking1.9?
Btw, this also means that we'll scroll two elements when we should scroll three.

I can't believe that screenX is working as expected here. It just doesn't report what's actually on the screen.
Blocks: 347363
No longer depends on: 347363
Don't believe this is a blocker - not marked as a regression, the str are fairly complicated.  Would take a patch and would fix in a dot release.
Flags: blocking1.9? → blocking1.9-
It's actually a regression (no matter if smooth scrolling is enabled or disabled).
Keywords: regression
Attached patch use getBoundingClientRect (obsolete) — Splinter Review
This also fixes an off-by-one-pixel bug in ensureElementIsVisible's amountToScroll calculation. Seems to be more reliable in general.
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #316676 - Flags: review?(enndeakin)
Keywords: helpwanted
Comment on attachment 316676 [details] [diff] [review]
use getBoundingClientRect

>+          var _rect = this._scrollbox.getBoundingClientRect();

Just 'rect', not '_rect'

Is there a test for scrollboxes?
Attachment #316676 - Flags: review?(enndeakin) → review+
Tests for scrollboxes are yet to be written (bug 401438). I'm going to request approval anyway since as I see it, this isn't broken in toolkit but on a lower level (boxObject). Put another way, a test using boxObject wouldn't detect this; neither would a test using getBoundingClientRect if getBoundingClientRect starts to malfunction ...
Attachment #316676 - Attachment is obsolete: true
Attachment #316679 - Flags: approval1.9?
Comment on attachment 316679 [details] [diff] [review]
use getBoundingClientRect

a1.9=beltzner
Attachment #316679 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
mozilla/toolkit/content/widgets/scrollbox.xml 	1.33 
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9
Duplicate of this bug: 431222
You need to log in before you can comment on or make changes to this bug.