888749 - cant scroll in bookmarks list with mouse wheel ?

Status: VERIFIED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Mayank Bansal]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20130630 Firefox/25.0 (Nightly/Aurora)
Build ID: 20130630031138

Steps to reproduce:

1. Create fresh profile
2. Add enough bookmarks that they fill the vertical height of the screen, and you need to scroll to see all the bookmarks.
3. Use the mouse-wheel to scroll the bookmarks


Actual results:

Cant scroll them


Expected results:

Should have been able to.

Comment 1

[Mayank Bansal]

i am using the bookmark button, not the bookmark menu, neither the bookmark bar.

Comment 2

[Alice0775 White]

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/c5ce065936fa
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20130628 Firefox/25.0 ID:20130628182742
Bad:
http://hg.mozilla.org/mozilla-central/rev/cbb24a4a96af
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20130629 Firefox/25.0 ID:20130629065543
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c5ce065936fa&tochange=cbb24a4a96af

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/4df4f2767a69
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20130628 Firefox/25.0 ID:20130628184843
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/1b8207252e9c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20130628 Firefox/25.0 ID:20130628185242
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4df4f2767a69&tochange=1b8207252e9c

Regressed by : Bug 653881


I wonder, Is the automatic test absent?

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed patch v1

This patch maintains the current behavior against the code that changed for bug 653881 since we no longer remove <xbl:children> elements from the tree. Unfortunately, this fix is pretty ugly. I don't see a way of making any nicer, though.

Comment 4

[Dão Gottwald [:dao]]

I'm not sure I understand this patch. I thought xbl:children would no longer be removed from the shadow DOM tree, but then it should still be invisible to non-shadow DOM methods called on the bound element, shouldn't it? Why would this.childNodes contain the xbl:children node?

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Dão Gottwald [:dao] from comment #4)
> I'm not sure I understand this patch. I thought xbl:children would no longer
> be removed from the shadow DOM tree, but then it should still be invisible
> to non-shadow DOM methods called on the bound element, shouldn't it? Why
> would this.childNodes contain the xbl:children node?

The 'this' element in question here is found at: http://hg.mozilla.org/mozilla-central/file/23ce4eab8fb1/toolkit/content/widgets/scrollbox.xml#l33 so there's a binding applied to it, but at the same time, it itself is part of a binding's <content>. Before, we'd remove the <children> from the tree, leading this particular <scrollbox> to have no children. Now, we leave the <children> elements in the tree, causing the confusion.

Does that make sense?

Comment 7

[Dão Gottwald [:dao]]

(In reply to Blake Kaplan (:mrbkap) from comment #6)
> (In reply to Dão Gottwald [:dao] from comment #4)
> > I'm not sure I understand this patch. I thought xbl:children would no longer
> > be removed from the shadow DOM tree, but then it should still be invisible
> > to non-shadow DOM methods called on the bound element, shouldn't it? Why
> > would this.childNodes contain the xbl:children node?
> 
> The 'this' element in question here is found at:
> http://hg.mozilla.org/mozilla-central/file/23ce4eab8fb1/toolkit/content/widgets/scrollbox.xml#l33

I don't think that's true. _getScrollableElements is implemented on <arrowscrollbox>, not <scrollbox>.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Dão Gottwald [:dao] from comment #7)
> I don't think that's true. _getScrollableElements is implemented on
> <arrowscrollbox>, not <scrollbox>.

Sorry, the correct link is http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/menu.xml?rev=f4157e8c4107#20 -- it's the same situation, though.

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 769965 [details] [diff] [review]
Proposed patch v1

>       <method name="_getScrollableElements">
>         <body><![CDATA[
>-          var nodes = this.hasChildNodes() ?
>-                        this.childNodes : document.getBindingParent(this).childNodes;
>+          var nodes = this.childNodes;
>+          if (nodes.length === 0 ||
>+              (nodes.length === 1 &&
>+               nodes[0].localName === "children" &&
>+               nodes[0].namespaceURI === "http://www.mozilla.org/xbl")) {
>+            nodes = document.getBindingParent(this).childNodes;
>+          }

If nodes.length is 0, we know that the binding parent's child nodes (if there's a binding parent at all) won't be child nodes of the scrollbox, so why would we consider them scrollable?

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed patch v1.1

That was me being too conservative in maintaining the old behavior.

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 770517 [details] [diff] [review]
Proposed patch v1.1

The strict type checking of === isn't useful here, please use the way more common ==.

Comment 13

[Landis Reed]

On a Long folder list, if I use the 'down arrow' of folder list to navigate to the end of the long list, using the mouse wheel, list will scroll up only, approximately 1/4 line or 10px per 'up' movement of the mouse wheel, But ONLY in a Bookmarks Toolbar folder, sub-folder and Not in the top Bookmarks folder (parent folder in Bookmarks Toolbar) and 'scroll up' only (not down)

(openSuSE 12.3, KDE 4.10.4/5, system set to 1 'line' per scroll of mouse)

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e8f88b5e649

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7e8f88b5e649

Comment 16

[Landis Reed]

Thank You!
Landis.

Comment 17

[Ioana (away)]

Verified as fixed on Firefox 25 beta 1: Windows 7 64bit, Mac OSX 10.8.4 64bit, Ubuntu 13.04 64bit.