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)
Tracking
()
VERIFIED
FIXED
Firefox 3.6a1
People
(Reporter: ojab, Assigned: Gavin)
References
Details
(Keywords: regression, verified1.9.1)
Attachments
(2 files, 5 obsolete files)
1.08 KB,
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
5.91 KB,
patch
|
asaf
:
review+
Natch
:
review+
|
Details | Diff | Splinter Review |
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
Comment 1•15 years ago
|
||
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
Updated•15 years ago
|
Hardware: x86 → All
Version: unspecified → Trunk
Comment 2•15 years ago
|
||
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
Updated•15 years ago
|
Flags: blocking-firefox3.1?
Version: Trunk → 3.1 Branch
Updated•15 years ago
|
Severity: normal → major
Keywords: regression
Comment 3•15 years ago
|
||
Sure that this is a regression from bug 471499?
Comment 4•15 years ago
|
||
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.
Comment 5•15 years ago
|
||
Most likely it's bug 457651.
Comment 6•15 years ago
|
||
That seems most likely, yes. Should try backing it out locally to know for sure.
Keywords: regressionwindow-wanted
Updated•15 years ago
|
Target Milestone: --- → Firefox 3.1b3
Updated•15 years ago
|
Comment 7•15 years ago
|
||
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
Comment 8•15 years ago
|
||
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
Comment 9•15 years ago
|
||
(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
Keywords: regressionwindow-wanted
Comment 10•15 years ago
|
||
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.
Comment 11•15 years ago
|
||
Attachment #358478 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•15 years ago
|
Attachment #358478 -
Flags: review?(gavin.sharp) → review+
Updated•15 years ago
|
Keywords: checkin-needed
Comment 12•15 years ago
|
||
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.
Comment 13•15 years ago
|
||
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)
Comment 14•15 years ago
|
||
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).
Assignee | ||
Comment 15•15 years ago
|
||
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-
Comment 16•15 years ago
|
||
>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.
Comment 17•15 years ago
|
||
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)
Assignee | ||
Comment 18•15 years ago
|
||
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?
Comment 19•15 years ago
|
||
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?
Comment 20•15 years ago
|
||
Comment on attachment 358478 [details] [diff] [review] Comment Fix. (checked in) http://hg.mozilla.org/mozilla-central/rev/096ab3b6abe0 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ca801f82facf
Attachment #358478 -
Attachment description: Comment Fix. → Comment Fix. (checked in)
Updated•15 years ago
|
Keywords: checkin-needed
Comment 21•15 years ago
|
||
(In reply to comment #20) > (From update of attachment 358478 [details] [diff] [review]) > http://hg.mozilla.org/mozilla-central/rev/096ab3b6abe0 > http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ca801f82facf Is that the right fix, or a work-around until attachment in comment #17 is approved ?
Assignee | ||
Comment 22•15 years ago
|
||
(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.
Comment 23•15 years ago
|
||
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
Comment 24•15 years ago
|
||
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
Comment 25•15 years ago
|
||
Assignee: highmind63 → dao
Attachment #358491 -
Attachment is obsolete: true
Attachment #358612 -
Flags: review?(gavin.sharp)
Attachment #358491 -
Flags: review?(gavin.sharp)
Comment 26•15 years ago
|
||
> + var container = (document.getBindingParent(this) || this);
Is the outer parenthesis needed?
Comment 27•15 years ago
|
||
No, it's not needed.
Comment 28•15 years ago
|
||
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.
Comment 29•15 years ago
|
||
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)
Updated•15 years ago
|
Keywords: late-compat
Assignee | ||
Comment 30•15 years ago
|
||
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.
Comment 31•15 years ago
|
||
+ 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));
Assignee | ||
Comment 32•15 years ago
|
||
(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.
Assignee | ||
Updated•15 years ago
|
Attachment #358666 -
Attachment is obsolete: true
Attachment #358666 -
Flags: superreview?(gavin.sharp)
Attachment #358666 -
Flags: review?(dao)
Assignee | ||
Updated•15 years ago
|
Attachment #358612 -
Flags: review?(gavin.sharp) → review-
Assignee | ||
Updated•15 years ago
|
Attachment #358666 -
Flags: review-
Comment 33•15 years ago
|
||
(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.
Comment 34•15 years ago
|
||
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.
Assignee | ||
Comment 35•15 years ago
|
||
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)
Assignee | ||
Comment 36•15 years ago
|
||
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 37•15 years ago
|
||
Comment on attachment 358681 [details] [diff] [review] rework ifdefs r=me, nice
Attachment #358681 -
Flags: review?(highmind63) → review+
Updated•15 years ago
|
Attachment #358681 -
Flags: review?(dao) → review+
Comment 38•15 years ago
|
||
Comment on attachment 358681 [details] [diff] [review] rework ifdefs r=mano
Assignee | ||
Updated•15 years ago
|
Assignee: dao → gavin.sharp
Keywords: late-compat
Assignee | ||
Comment 39•15 years ago
|
||
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
Comment 40•15 years ago
|
||
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090125 Minefield/3.2a1pre ID:20090125034316 VERIFIED
Comment 41•15 years ago
|
||
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?
Keywords: fixed1.9.1 → verified1.9.1
Updated•15 years ago
|
Flags: blocking-firefox3.1?
Updated•15 years ago
|
Flags: blocking-firefox3.1? → blocking-firefox3.1+
Priority: -- → P2
Comment 42•15 years ago
|
||
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...
Comment 43•15 years ago
|
||
Since the method is called _getScrollableElements, not getScrollableElements, and we might change it at will, we shouldn't document it.
Updated•15 years ago
|
Flags: in-testsuite+
Comment 44•15 years ago
|
||
Dao, could you please point to the checked in test so we can consider if we need a litmus test or not? Thanks.
Comment 45•14 years ago
|
||
(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?
Comment 46•14 years ago
|
||
browser/base/content/test/browser_overflowScroll.js
Comment 47•14 years ago
|
||
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.
Description
•