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
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.)
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.
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.
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?
(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.
Created attachment 358478 [details] [diff] [review] Comment Fix. (checked in)
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.
Created attachment 358488 [details] [diff] [review] Scrolling fix 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.
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?
>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.
Created attachment 358491 [details] [diff] [review] real patch 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.
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?
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
(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 ?
(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
Created attachment 358612 [details] [diff] [review] use the binding parent if there's one
> + 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.
Created attachment 358666 [details] [diff] [review] More completeness 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.
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.
(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.
Created attachment 358677 [details] [diff] [review] patch This is what I had in mind. It doesn't fix the scrollbox bustage, but we can do that separately.
Created attachment 358681 [details] [diff] [review] rework ifdefs That previous patch probably didn't work on non-Mac.
Comment on attachment 358681 [details] [diff] [review] rework ifdefs r=me, nice
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
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
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.
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?
Existing Litmus test should be sufficient: https://litmus.mozilla.org/show_test.cgi?id=10235