Closed Bug 474964 Opened 15 years ago Closed 15 years ago

Can't scroll tabs/tabbar with mouse scrollwheel/mousewheel

Categories

(Firefox :: Tabbed Browser, defect, P2)

3.5 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 3.6a1

People

(Reporter: ojab, Assigned: Gavin)

References

Details

(Keywords: regression, verified1.9.1)

Attachments

(2 files, 5 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090123 Minefield/3.2a1pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090123 Minefield/3.2a1pre

When tabbar becomes scrollable (with many tabs) and I'm trying to scroll it with mouse - message in error console instead of scrolling:

Error: element.getBoundingClientRect is not a function
Source File: chrome://global/content/bindings/scrollbox.xml
Line: 266

Reproducible: Always
Are you seeing this in Safe Mode as well? (I'm also seeing this on OS X, but I haven't yet tried Safe Mode, myself.)

OS -> All
OS: Linux → All
Hardware: x86 → All
Version: unspecified → Trunk
No dupes found. Confirming and marking NEW with the following build in Safe Mode:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090123 Minefield/3.2a1pre

(Also, updated summary to be more query-friendly.)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Can't scroll tabbar with mouse scrollwhell → Can't scroll tabs/tabbar with mouse scrollwheel/mousewheel
Flags: blocking-firefox3.1?
Version: Trunk → 3.1 Branch
Severity: normal → major
Keywords: regression
Sure that this is a regression from bug 471499?
Ok, so I'm getting the following in the console:

Error: element.getBoundingClientRect is not a function
Source File: chrome://global/content/bindings/scrollbox.xml
Line: 266

Dao, am I off the hook for this? Do you think bug 457651 had anything to do with this? I know with other patches it didn't happen, but that second children tag I hadn't tested so much, although I doubt it was that.
Most likely it's bug 457651.
That seems most likely, yes. Should try backing it out locally to know for sure.
Target Milestone: --- → Firefox 3.1b3
Blocks: 457651
No longer blocks: 471499
crowder debugged that error in comment 4, and the problem is that |element| is a Comment.

I thought not putting comments in XBL binding <content> was standard operating procedure, for precisely that reason.
Assignee: nobody → highmind63
Thank you much for that, I had no idea about the xbl binding problem, I'll upload a patch for this later today, this time after appropriate testing :)

On a side note, should I move the comment out of the content, #ifdef it or just totally remove it?
Status: NEW → ASSIGNED
(In reply to comment #8)
> On a side note, should I move the comment out of the content, #ifdef it or just
> totally remove it?

Use...

# this style
The comment doesn't fix this all, scrolling still won't work and hitting the arrow scroll buttons is a bit wonky when there's a lot of tabs. I'll upload the comment fix, but there's still more to do here.
Attachment #358478 - Flags: review?(gavin.sharp)
Attachment #358478 - Flags: review?(gavin.sharp) → review+
Ok so the problem is here: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/scrollbox.xml?mark=253-255#250 . Specifically the scrollbox checks if there are any children, otherwise it goes to the binding parent, with the toolbarbutton on the tabbar, it now has children and is therefore returning the toolbarbutton, which obviously can't work. I'll have to look further into this tomorrow night.
Attached patch Scrolling fix (obsolete) — Splinter Review
Ok, here's a quick fix for the scrolling problem, I'm not 100% this is correct, but it seems like the right thing to do.
Attachment #358488 - Flags: review?(gavin.sharp)
Now that I think of it this may need other fixing similar to this in other functions, I gotta go though, so I'll upload a patch tomorrow night for the rest (unless you can beat me to it).
Comment on attachment 358488 [details] [diff] [review]
Scrolling fix

this.childNodes > 1 is always false - did you mean .length? I don't see how the new tab button would be changing anything here, since there's always at least one tab, right?
Attachment #358488 - Flags: review?(gavin.sharp) → review-
>this.childNodes > 1 is always false - did you mean .length? I don't see how the

Yes I did mean length, I'm a bit in a rush ;) .

>new tab button would be changing anything here, since there's always at least
>one tab, right?

The new tab button is a direct descendant of the scrollbox, the tabs come from a binding, hence the issue.
Attached patch real patch (obsolete) — Splinter Review
I'm a bit wary of just changing this one and not the other by _distanceScroll, but i don't think tabbrowser hits that particular code.
Attachment #358488 - Attachment is obsolete: true
Attachment #358491 - Flags: review?(gavin.sharp)
So why is changing the behavior for all scrollboxes the right thing to do here? How is the 1 in "childNodes.length > 1" significant apart from being the number of elements hardcoded in the tabbrowser's scrollbox?
I figured that with only one element there shouldn't be any scrolling anyhow, I may be wrong. The only place to fix this is in scrollbox.xml, unless we override the function in tabbrowser.xml. Another option would be to reverse the check:

if (document.getClientBindingRect(this).childNodes) ...

else if (this.hasChildNodes()) ...

If that's better I can do that.
Any other suggestions?
Keywords: checkin-needed
Blocks: 475138
(In reply to comment #21)
> Is that the right fix, or a work-around until attachment in comment #17 is
> approved ?

It's necessary, but apparently not sufficient. I think it fixed most of the regressions caused by bug 457651's landing, though.
Testing with the latest hourly, the mouse-wheel still not scroll tabs for me.
The errors in console2 are no longer reported. 

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090123 Minefield/3.2a1pre Firefox/3.0.4 ID:20090123232759
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090124 Minefield/3.2a1pre ID:20090124024054

tabscrolling is still broken
Assignee: highmind63 → dao
Attachment #358491 - Attachment is obsolete: true
Attachment #358612 - Flags: review?(gavin.sharp)
Attachment #358491 - Flags: review?(gavin.sharp)
Blocks: 475030
Blocks: 475031
> +            var container = (document.getBindingParent(this) || this);
Is the outer parenthesis needed?
No, it's not needed.
Thanks Dao, this is indeed what I meant in comment 19, however ludicrous it sounds now. I'm sorry I wasn't able to update here, I was afk on Saturday. The only caveat of this patch is that now if you have children and a binding it checks the binding first, and will discard the children. Before it just was the other way around. This will fix the rest of the regressions from that bug.
Blocks: 475203
Attached patch More completeness (obsolete) — Splinter Review
Dao, this patch settles all matters between direct childNodes and bound childNodes, can you review it and give some comments, this guarantees backwards compatibility, and now also correctly checks both the childNodes (wherever they may be) and the bound childNodes (wherever they may be). I'll also ask review from gavin so that this can land ASAP.
Attachment #358666 - Flags: superreview?(gavin.sharp)
Attachment #358666 - Flags: review?(dao)
Keywords: late-compat
This is kind of a mess... That the scrollbox binding supported checking the binding parent at all is a tabbrowser specific hack as far as I can tell, since there's no way to know that the children are actually there (e.g. they could be children of another ancestor binding passed down via <children/>).

Both current patches mean we'll always take into account the binding parent's children, which may not even be in the scrollbox at all (e.g. for a binding with <content><children/><xul:scrollbox><children includes="tab"></xul:scrollbox></content>). I don't think we can get away with that.

I don't see a way to get the actual inherited childNodes of a given anonymous element (i.e. the expanded value of "<children/>" as a nodelist), so it sounds like we should get rid of the bindingparent checks in scrollbox.xml and just override the relevant methods in tabbrowser's scrollbox bindings, or otherwise make the scrollbox behavior customizable (by letting you pass in a parent of the children to take into account, or something - tabbrowser could pass in mTabContainer). Something tabbrowser-specific is probably safest at this point.
+          var bindingElements = document.getBindingParent(this) ?
+                                document.getBindingParent(this).childNodes :
+                                null;
+          var childElements = this.hasChildNodes() ?
+                              this.childNodes :
+                              null;
+          var elements;
+          if (bindingElements)
+            if (childElements)
+              elements = Array.slice(bindingElements).concat(Array.slice(childElements));
+            else
+              elements = Array.slice(bindingElements);
+          else
+            elements = Array.slice(childElements);

Can't this be simplified as:

var elements = Array.slice(document.getBindingParent(this).childNodes).concat(Array.slice(this.childNodes));
(In reply to comment #30)
> (e.g. for a binding with <content><children/><xul:scrollbox>
> <children includes="tab"></xul:scrollbox></content>). I don't think we can
> get away with that.

Although I suppose that specific example would already be broken with the current code (since the scrollbox doesn't have any children of its own). Still, I'd prefer we avoid making the problem worse by adding more code to deal with the bindingParent's children.
Attachment #358666 - Attachment is obsolete: true
Attachment #358666 - Flags: superreview?(gavin.sharp)
Attachment #358666 - Flags: review?(dao)
Attachment #358612 - Flags: review?(gavin.sharp) → review-
(In reply to comment #31)
> Can't this be simplified as:
> 
> var elements =
> Array.slice(document.getBindingParent(this).childNodes).concat(Array.slice(this.childNodes));

If there's no binding parent that would be wrong, and i wasn't sure what slice/concat do on null/empty array

gavin: can I find you on irc sometime, I'd like to work out the best way to go about this, putting the change in tabbrowser is bound to be difficult and will have to change scrollbox.xml anyhow.
And in the specific case you mentioned, attachment 358666 [details] [diff] [review] will do the exact
same thing as current code, in fact both patches here will because
elementFromPoint will return null, AFAICT.
Attached patch patch (obsolete) — Splinter Review
This is what I had in mind. It doesn't fix the scrollbox bustage, but we can do that separately.
Attachment #358612 - Attachment is obsolete: true
Attachment #358677 - Flags: review?(highmind63)
Attachment #358677 - Flags: review?(dao)
Attached patch rework ifdefsSplinter Review
That previous patch probably didn't work on non-Mac.
Attachment #358677 - Attachment is obsolete: true
Attachment #358681 - Flags: review?(highmind63)
Attachment #358681 - Flags: review?(dao)
Attachment #358677 - Flags: review?(highmind63)
Attachment #358677 - Flags: review?(dao)
Comment on attachment 358681 [details] [diff] [review]
rework ifdefs

r=me, nice
Attachment #358681 - Flags: review?(highmind63) → review+
Attachment #358681 - Flags: review?(dao) → review+
Comment on attachment 358681 [details] [diff] [review]
rework ifdefs

r=mano
Assignee: dao → gavin.sharp
Keywords: late-compat
I pushed this to both mozilla-central and mozilla-1.9.1, assuming it's a blocker:
https://hg.mozilla.org/mozilla-central/rev/0ecb7f4e5724
https://hg.mozilla.org/releases/mozilla-1.9.1/rev/ac38de3f3b9e
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Keywords: fixed1.9.1
Resolution: --- → FIXED
Target Milestone: Firefox 3.1b3 → Firefox 3.2a1
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090125 Minefield/3.2a1pre ID:20090125034316

VERIFIED
Verified on trunk and 1.9.1 with:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090125
Shiretoko/3.1b3pre (.NET CLR 3.5.30729) ID:20090125052901

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre)
Gecko/20090126 Shiretoko/3.1b3pre Ubiquity/0.1.5 ID:20090126020313

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090125
Minefield/3.2a1pre ID:20090125034316

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre)
Gecko/20090126 Minefield/3.2a1pre ID:20090126020316
Status: RESOLVED → VERIFIED
Flags: blocking-firefox3.1? → in-litmus?
Flags: blocking-firefox3.1?
Flags: blocking-firefox3.1? → blocking-firefox3.1+
Priority: -- → P2
I've written up a little synopsis of these changes on mdc's arrowscrollbox talk page https://developer.mozilla.org/Talk:En/XUL/Arrowscrollbox . The actual page should probably get updated...
Since the method is called _getScrollableElements, not getScrollableElements, and we might change it at will, we shouldn't document it.
Flags: in-testsuite+
Dao, could you please point to the checked in test so we can consider if we need a litmus test or not? Thanks.
(In reply to comment #44)
> Dao, could you please point to the checked in test so we can consider if we
> need a litmus test or not? Thanks.

Dao? Do you have an answer for me?
browser/base/content/test/browser_overflowScroll.js
Existing Litmus test should be sufficient:
https://litmus.mozilla.org/show_test.cgi?id=10235
Flags: in-litmus? → in-litmus+
You need to log in before you can comment on or make changes to this bug.