734653 - Bookmarks menupopup does not scroll up(down) by mouse wheel

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Alice0775 White]

Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/3fdc1c14a8ce
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120310 Firefox/13.0a1 ID:20120310031046

Bookmarks menupopup does not scroll up(down) by mouse wheel

Reproducible: Always

Steps to Reproduce 1:
1. Start Firefox
2. Add a folder in Bookmarks Menu
3. Add some bookmark(more than 50) to the folder
4. Appbutton > Bookmarks > Folder
5. Try scroll the bookmarks popup by mouse wheel

Steps to Reproduce 2:
1. Start Firefox
2. Add a folder in Bookmarks Toolbar
3. Add some bookmark(more than 50) to the folder
4. Unhde Bookmarks Toolbar
5. Click the Folder
6. Try scroll the bookmarks popup by mouse wheel

Actual Results:
 Bookmarks submenu popup does not scroll up(down) by mouse wheel
 Scroll stops
 I cannot scroll to the top/end of popup.

Expected Results:  
 Should scroll,

Regression window(m-c),
Works:
http://hg.mozilla.org/mozilla-central/rev/fca77ac7cdf9
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120306 Firefox/13.0a1 ID:20120306195308
Fails:
http://hg.mozilla.org/mozilla-central/rev/8ef88a69f861
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120307 Firefox/13.0a1 ID:20120307013408
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fca77ac7cdf9&tochange=8ef88a69f861

Regression window(m-i),
Works:
http://hg.mozilla.org/integration/mozilla-inbound/rev/a9e38eff9ea6
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120306 Firefox/13.0a1 ID:20120306110708
Fails:
http://hg.mozilla.org/integration/mozilla-inbound/rev/5aa498cf8658
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120306 Firefox/13.0a1 ID:20120306113008
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a9e38eff9ea6&tochange=5aa498cf8658
Triggered by:
5aa498cf8658	Marco Bonardo — Bug 731563 - Make Places view markers proper elements in the popups. r=mano

Comment 1

[Alice0775 White]

Attachment: Sample Bookmarks JSON file

Ctrl+Shift+B
Import and Backup > Restore > Choose File…

This includes "Mozilla Firefox" folders in Bookmarks Menu and Bookmarks Toolbar.
The folder includes 92 items each.

Comment 2

[Alice0775 White]

Attachment: Sample Bookmarks JSON file

Comment 3

[Marco Bonardo [:mak]]

If I close and reopen the popup it seems to work fine, doesn't work only if I try to scroll just after adding the elements. Is it still not working for you if you do that?

Comment 4

[Marco Bonardo [:mak]]

fwiw, looks like the scrollbox is confused by hidden elements (I actually noticed similar issues on Mac where it was instead confused by collapsed elements)

Comment 5

[Marco Bonardo [:mak]]

and I think it's due to the fact _canScrollToElement in scrollbox.xml always returns true, but that implementation can't clearly scroll to hidden elements (this seems to have been workarounded in tabbrowser.xml implementation by explicitly checking hidden).
So either we fix the toolkit scrollbox or we inherit our own.

Comment 6

[Marco Bonardo [:mak]]

Attachment: patch v1.0

do you know a valid reason to not check at least hidden?

Comment 7

[Alice0775 White]

(In reply to Marco Bonardo [:mak] from comment #3)
> If I close and reopen the popup it seems to work fine, doesn't work only if
> I try to scroll just after adding the elements. Is it still not working for
> you if you do that?
Scrolling up does not work any time.

Comment 8

[Asaf Romano (gone)]

Comment on attachment 604808 [details] [diff] [review]
patch v1.0

Given that we're in xul-context, you should check collapsed (also update the test). r=mano otherwise.

Comment 9

[Marco Bonardo [:mak]]

(In reply to Mano from comment #8)
> Given that we're in xul-context, you should check collapsed (also update the
> test). r=mano otherwise.

but collapsed is not an issue afaict, scrollbox uses getBoundingClientRect top/left that are defined for collapsed elements, not for hidden elements.

Comment 10

[[e-mail]]

Comment on attachment 604808 [details] [diff] [review]
patch v1.0

Is it me or is there a flaw in the smooth scroll code whereby it will fail to scroll if there are N unscrollable elements in a row?

Comment 11

[Marco Bonardo [:mak]]

(In reply to [e-mail] from comment #10)
> Is it me or is there a flaw in the smooth scroll code whereby it will fail
> to scroll if there are N unscrollable elements in a row?

the list of elements (_getScrollableElements) is filtered through _canScrollToElement before being used, as it is now (no filter) it is justbroken when trying to scroll to hidden elements. Though I'm not sure if it's broken in case of consecutive unscrollable elements, do you have a code to point at or an example to try?

Comment 12

[Asaf Romano (gone)]

Attachment: test case for hidden-elements in a row (enable dom.allow_XUL_XBL_for_file and, save it, and load locally...)

This testcase is broken on current trunk (and release). It works with Marco's patch, but as for smooth scrolling, I did notice a bit of delay somewhere. My machine is a little too fast to tell where...

This should be fixed in a follow up, IMO.

Comment 13

[Marco Bonardo [:mak]]

Attachment: patch v1.1

so, my assumption is that:
- changing collapsed too is too scary now, I will file a follow-up to investigate that. While hidden elements can't work at all (due to being removed from the frame) collapsed elements do work, though make not much sense so scroll to those. I can't tell if anyone relies on that though, so as I said, scarier.
- smooth scrolling code afaict is not affected negatively, it has the same defect of using getBoundingClientRect, so my patch can just bring improvements there
- jagginess, may have many reasons, from gc to any other stuff running in background, hard to tell offhand

So I think to push this that imo is a clear improvement and file the followup for collapsed, but I'm open to further suggestions of things to check/try/test/file.

Comment 14

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c16f72c62374

Comment 15

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/c16f72c62374

Comment 16

[[e-mail]]

It seems that the original issue is caused by _elementFromPoint which checks whether aX > elements[high].getBoundingClientRect()[end] which gives bogus results when that element is hidden.

As for scrolling when many elements are hidden, _elementFromPoint will (if it works at all) always return a visible element, so scrolling will then happen. However the code should really count the number of scrollable elements (i.e. it should only adjust index when it finds a scrollable element).

Collapsed doesn't make sense in menus although it might in other scrollboxes.

Comment 17

[Marco Bonardo [:mak]]

(In reply to [e-mail] from comment #16)
> It seems that the original issue is caused by _elementFromPoint which checks
> whether aX > elements[high].getBoundingClientRect()[end] which gives bogus
> results when that element is hidden.

makes much sense.

> As for scrolling when many elements are hidden, _elementFromPoint will (if
> it works at all) always return a visible element, so scrolling will then
> happen. However the code should really count the number of scrollable
> elements (i.e. it should only adjust index when it finds a scrollable
> element).

I will file a follow-up for this

> Collapsed doesn't make sense in menus although it might in other scrollboxes.

I filed bug 735192, willing to comment there?

Comment 18

[Marco Bonardo [:mak]]

and filed bug 735235 for _elementFromPoint