984156 - Australis: Mouse wheel scroll no longer works in bookmarks sub folder

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Alice0775 White]

Attachment: screenshot

Build Identifier:
https://hg.mozilla.org/mozilla-central/rev/e3b76b155ca4
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 ID:20140316030202

Steps To Reproduce:
1. Create a bookmark folder in Bookmarks Menu
2. Create 40 bookmarks in the sub folder.
   Number of bookmarks may be depend on vertical monitor size and default system font
   (40 : 900px in vertical monitor size and "" default system font of Japanese edition of Windows8.1)

3. Click Bookmarks Widget to open Panel
4. Click the sub folder to expand lists
5. Attemt to scroll the lists by Mouse wheel

Actual Results:
Mouse wheel scroll does not work

Expected Results:
Mouse wheel scroll should work

Comment 1

[Alice0775 White]

Aurora29.0a2 also affected.
 https://hg.mozilla.org/releases/mozilla-aurora/rev/cc0fd2701505
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140316004001

However, number of bookmarks is 31

Comment 2

[:Gijs (he/him)]

I can't reproduce this on my 29 beta build on windows 8.1; my mouse scrolling works fine in both the main bookmarks menu button's menu and in subfolders. Alice, did this get fixed recently?

Comment 3

[Alice0775 White]

(In reply to :Gijs Kruitbosch from comment #2)
> I can't reproduce this on my 29 beta build on windows 8.1; my mouse
> scrolling works fine in both the main bookmarks menu button's menu and in
> subfolders. Alice, did this get fixed recently?

I can still reproduce the problem on 29b1candidate, Aurora30.0a2 and Nightly31.0a1..

https://hg.mozilla.org/releases/mozilla-beta/rev/904eb7fb178c
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140318013849

https://hg.mozilla.org/releases/mozilla-aurora/rev/b07ecc057abf
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 ID:20140319004002

https://hg.mozilla.org/mozilla-central/rev/3bc3b9e2cd99
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0 ID:20140319030201

Comment 4

[Jim Jeffery not reading bug-mail 1/2/11]

There was a recent thread here:
http://forums.mozillazine.org/viewtopic.php?p=13413713#p13413713 

stating something about screen size causing scrolling not to work, but was later fixed by bug https://bugzilla.mozilla.org/show_bug.cgi?id=969584 which consolidated the Bookmarks/History views. 

Did 969584 make it into 29 beta ?  or is this bug something totally different ?

Comment 5

[Alice0775 White]

It depend on screen size and number of bookmarks in sub folder and font.
Bug 969584 only changed the number of bookmark(screen size and font) which triggered it.

Now it happens in case,  Screen vertical 900px and 40 bookmarks and "Meiryo UI" which is japanese edition of Win8.1 default.

https://hg.mozilla.org/releases/mozilla-beta/rev/904eb7fb178c
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140318013849

https://hg.mozilla.org/releases/mozilla-aurora/rev/b07ecc057abf
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 ID:20140319004002

https://hg.mozilla.org/mozilla-central/rev/3bc3b9e2cd99
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0 ID:20140319030201

Comment 6

[:Gijs (he/him)]

(In reply to Alice0775 White from comment #5)
> It depend on screen size and number of bookmarks in sub folder and font.
> Bug 969584 only changed the number of bookmark(screen size and font) which
> triggered it.
> 
> Now it happens in case,  Screen vertical 900px and 40 bookmarks and "Meiryo
> UI" which is japanese edition of Win8.1 default.

Does this happen for you with Segoe UI? And is 40 bookmarks just enough to get it to scroll at that screensize, or is it an important number for some other reason?

It's just weird because I'm trying to reproduce on win8.1 and failing, and I don't know of anything that we've done that'd influence the scrollwheel working or not. Can you try to find a regression window?

Comment 7

[Alice0775 White]

(In reply to :Gijs Kruitbosch from comment #6)
> (In reply to Alice0775 White from comment #5)
> > It depend on screen size and number of bookmarks in sub folder and font.
> > Bug 969584 only changed the number of bookmark(screen size and font) which
> > triggered it.
> > 
> > Now it happens in case,  Screen vertical 900px and 40 bookmarks and "Meiryo
> > UI" which is japanese edition of Win8.1 default.
> 

I do not know how to change system font.
So, I tried other way. i.e., created following userChrome.css in profile/chrome.

@namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"); /* set default namespace to XUL */
* {
font-family: 'Segoe UI' !important;
font-size: 12px !important;
}

And I can reproduce the problem with 40 bookmarks with the css on latest Nightly.

> Does this happen for you with Segoe UI? And is 40 bookmarks just enough to
> get it to scroll at that screensize, or is it an important number for some
> other reason?
> 

It depended on number of bookmarks in a sub folder. It is important to reproduce.

> It's just weird because I'm trying to reproduce on win8.1 and failing, and I
> don't know of anything that we've done that'd influence the scrollwheel
> working or not. Can you try to find a regression window?

It happens 31 bookmarks with default font "Meiryo UI".
It happens 32 bookmarks with with 'Segoe UI'.
pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fdbf79a891de&tochange=5a9bd0aa940b

Comment 8

[Alice0775 White]

Pushlog(fx)
http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=ab6e51e115bc&tochange=c8a1458bfe7d

Triggered by:
1a9fa8fde7e0	Darrin Henein — Bug 969592 - Restyle Australis' Bookmarks Menu Popups, r=mak

Comment 9

[:Gijs (he/him)]

(In reply to Alice0775 White from comment #8)
> Pushlog(fx)
> http://hg.mozilla.org/integration/fx-team/
> pushloghtml?fromchange=ab6e51e115bc&tochange=c8a1458bfe7d
> 
> Triggered by:
> 1a9fa8fde7e0	Darrin Henein — Bug 969592 - Restyle Australis' Bookmarks Menu
> Popups, r=mak

Alright, this is a good first step. I still can't reproduce though. Really frustrating. :-(

Does scrolling with the scroll buttons which are shown in the screenshot work normally? And is this just on Windows 8.1 or can you reproduce on Windows 7 / XP / ...?

Comment 10

[:Gijs (he/him)]

Marco, do you have ideas here?

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I cannot reproduce on Win8.1, I tried adding a lots of bookmarks, then I retried starting with 40 bookmarks and adding 1 by one between tests.
I wonder if there's a specific resolution of the japanese version is needed...
Just by looking at the patch the only rules that makes me think are padding changes in #BMB_bookmarksPopup menupopup and #BMB_bookmarksPopup arrowscrollbox, as well as the margin-top in #BMB_bookmarksPopup menupopup > hbox (see https://hg.mozilla.org/releases/mozilla-aurora/rev/79c318aee3a1). Some of these may confuse the arrowscrollbox.
I would try to disable these rules one by one and see if the problem goes away, but unfortunately I can't really reproduce so I couldn't tell. Maybe Alice may try doing this?

Comment 12

[:Gijs (he/him)]

(In reply to Marco Bonardo [:mak] from comment #11)
> Just by looking at the patch the only rules that makes me think are padding
> changes in #BMB_bookmarksPopup menupopup and #BMB_bookmarksPopup
> arrowscrollbox, as well as the margin-top in #BMB_bookmarksPopup menupopup >
> hbox (see https://hg.mozilla.org/releases/mozilla-aurora/rev/79c318aee3a1).
> Some of these may confuse the arrowscrollbox.

This is what I thought as well, but then it still wouldn't make sense to me why the scroll wheel would not work while the normal buttons would continue to work...

Comment 13

[Alice0775 White]

(In reply to :Gijs Kruitbosch from comment #9)

> 
> Does scrolling with the scroll buttons which are shown in the screenshot
> work normally? 

autorepeatbutton works as expected.
Press down arrow key works as expected.

> And is this just on Windows 8.1 or can you reproduce on
> Windows 7 / XP / ...?

I can reproduce on WinXP
Monitor 1152x866px , 38 bookmarks, default font "MS UI Gothic"
https://hg.mozilla.org/mozilla-central/rev/3bc3b9e2cd99
Mozilla/5.0 (Windows NT 5.1; rv:31.0) Gecko/20100101 Firefox/31.0 ID:20140319030201

I did not try on Win7.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

I'm going to do my damndest to reproduce this today.

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Alice,

Are you able to record a short screencast of yourself reproducing this bug from a blank profile on Windows XP?

-Mike

Comment 16

[Alice0775 White]

Attachment: screen capture

"New Folder38" and "New Folder39" are affected

Comment 17

[Mike Conley (:mconley) (:⚙️)]

(In reply to Alice0775 White from comment #16)
> Created attachment 8394877 [details]
> screen capture
> 
> "New Folder38" and "New Folder39" are affected

Thank you for the screencast, Alice!

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Try as I might, I cannot reproduce this. I set up my bookmarks menu to look identical to Alice's, and populated my folders with the exact same contents, and duplicated her behaviour. No dice, I'm afraid. :/

Alice - a few random stabs:

1) Do you have an en-US version of Windows XP that you can test on? If so, can you reproduce it there?
2) What about if you're not viewing about:home, but have some scrollable page in the background? I only ask because I wonder if the scrollevents are getting to the browser content instead somehow. Just a guess, and I doubt it, but I thought I'd ask.
3) Have you heard of anyone else hitting this bug?

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Considering how much difficulty we're having reproducing this, I think we can take the priority down a notch.

Comment 20

[Alice0775 White]

(In reply to Mike Conley (:mconley) from comment #18)
> Try as I might, I cannot reproduce this. I set up my bookmarks menu to look
> identical to Alice's, and populated my folders with the exact same contents,
> and duplicated her behaviour. No dice, I'm afraid. :/
> 
> Alice - a few random stabs:
> 
> 1) Do you have an en-US version of Windows XP that you can test on? If so,
> can you reproduce it there?

No.

> 2) What about if you're not viewing about:home, but have some scrollable
> page in the background? I only ask because I wonder if the scrollevents are
> getting to the browser content instead somehow. Just a guess, and I doubt
> it, but I thought I'd ask.

The problem occurs on not only about:home but also any page

> 3) Have you heard of anyone else hitting this bug?

I saw http://forums.mozillazine.org/viewtopic.php?p=13413713#p13413713
And I also reproduced the problem on my windows.

Comment 21

[Alice0775 White]

On 1024*768 monitor, the problem occurs if number of bookmarks is 34....

Comment 22

[Mike Conley (:mconley) (:⚙️)]

\o/ Just reproduced it on a Win XP laptop using 1024*768 with 34 bookmarks.

Thanks Alice!

Comment 23

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Mike, see comment 11 for some of the rules we added that might be involved (totally blind guess though)

Comment 24

[Mike Conley (:mconley) (:⚙️)]

I'll try disabling those and see where I get.

I've also determined that we're not scrolling because we're returning early from the ensureElementIsVisible function from scrollbox.xml. In particular, here:

http://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/scrollbox.xml?from=scrollbox.xml&case=true#254

because elementStart = 0, and containerStart = -99, and elementEnd = 0.

Not exactly sure what that means, but Cc'ing Enn because he reviewed that code.

Comment 25

[Mike Conley (:mconley) (:⚙️)]

Hm - it seems that the reason elementStart and elementEnd are 0 is because the rect that we're getting back from element.getBoundingClientRect() is reporting that the selected element has no dimensions.

Looking at the debugger, it appears to be a menuseparator.

Comment 26

[Mike Conley (:mconley) (:⚙️)]

Whoops, didn't mean to needinfo Neil there.

Comment 27

[Mike Conley (:mconley) (:⚙️)]

Ok, it seems related to the fact that we're display: none'ing the separator at the end of the menupopup:

https://hg.mozilla.org/releases/mozilla-aurora/rev/79c318aee3a1#l6.41

Removing that display: none fixes this bug.

Comment 28

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch 1

So the solution mak and I came up with is to set visibility: hidden on the .bookmarks-actions-menuseparator, and negative-margin it away. It's pretty hacky, but this fixes scrollbox's behaviour without having to change it - and also makes this menu just as theme-able as before.

Going to try this on OS X and Linux to make sure we look OK here, and then provide some before / after screenshots.

Comment 29

[Alice0775 White]

Related Bug 859126?

Comment 30

[Mike Conley (:mconley) (:⚙️)]

(In reply to Alice0775 White from comment #29)
> Related Bug 859126?

Definitely related.

Comment 31

[Mike Conley (:mconley) (:⚙️)]

Attachment: Windows 7 - before patch

Comment 32

[Mike Conley (:mconley) (:⚙️)]

Attachment: Windows 7 - after patch

Comment 33

[Mike Conley (:mconley) (:⚙️)]

Attachment: OSX - before patch

Comment 34

[Mike Conley (:mconley) (:⚙️)]

Attachment: OSX - after patch

Comment 35

[Mike Conley (:mconley) (:⚙️)]

Attachment: Ubuntu - before patch

Comment 36

[Mike Conley (:mconley) (:⚙️)]

Attachment: Ubuntu - after patch

Comment 37

[:Gijs (he/him)]

Mike, was this ready for review? I know it's marked WIP, but the screenshots look fine to me, so... :-)

Comment 38

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Gijs Kruitbosch from comment #37)
> Mike, was this ready for review? I know it's marked WIP, but the screenshots
> look fine to me, so... :-)

Not just yet - I'm not 100% convinced that this fixes all of our problems, and I had to leave my XP machine before I could confirm. Gonna run the patch through some more paces this weekend, or on Monday.

Comment 39

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

So I think this is the safer route. With the last patch, removing the border and margin resulted in a box with 0 height and that seemed to still cause scrollbox stickyness under some (hard, but not impossible to reproduce) circumstances.

I think this is the safer route - we force a 1px height, and then negative margin it away. This seems, from my testing anyway, to make the scrollbox happy.

This puts all of my screenshots out of date, I'm afraid. Not sure if it's worth it for just one pixel. Marco, if you think it is, let me know and I'll toss some up.

Comment 40

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8395308 [details] [diff] [review]
Patch v1

Review of attachment 8395308 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +748,5 @@
> +   */
> +  visibility: hidden;
> +  min-height: 1px;
> +  margin-top: -1px;
> +  margin-bottom: 0px;

margin: -1px 0 0; ?

just to be on the safe side, I'd set -moz-appearance: none (for the osx theme)
the only other rule that makes me think is http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/menu.css#232, though, if you tested thoroughly on Mac and there's no added space, that's ok.

Comment 41

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.1 (r+'d by mak)

Ok, done. Thanks mak!

Comment 42

[Mike Conley (:mconley) (:⚙️)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/64995acca4a8

Comment 43

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/64995acca4a8

Comment 44

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8395468 [details] [diff] [review]
Patch v1.1 (r+'d by mak)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

Bug 969592


User impact if declined: 

Users who use the bookmarks menu button may find that they cannot scroll through their subfolders with their scrollwheel / trackpad. This only occurs when the bottom menuseparator element is the next element to be scrolled into view, so it only affects folders that have a certain number of bookmarks on certain resolutions.


Testing completed (on m-c, etc.): 

Local testing, and some time to bake on m-c.


Risk to taking this patch (and alternatives if risky): 

Low risk - it's style only, and modifies something that was display: none to begin with, and is not visibility: hidden.


String or IDL/UUID changes made by this patch:

None.

Comment 45

[Mike Conley (:mconley) (:⚙️)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/46f788fb37c4

https://hg.mozilla.org/releases/mozilla-beta/rev/ad7b3066aa84

Comment 46

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Windows NT 6.3; rv:31.0) Gecko/20100101 Firefox/31.0

Confirming the fix using 3 sub-folders each containing 50+ bookmarks on:
- latest Nightly (build ID: 20140327030203)
- latest Aurora (build ID: 20140327004002)
- Firefox 29 beta 3 (build ID: 20140327113732)

Marking issue verified.

Comment 47

[Dão Gottwald [:dao]]

Comment on attachment 8395468 [details] [diff] [review]
Patch v1.1 (r+'d by mak)

> #BMB_bookmarksPopup menupopup > .bookmarks-actions-menuseparator {
>-  /* Hide bottom separator as the styled footer includes a top border serving the same purpose */
>-  display: none;
>+  /* Hide bottom separator as the styled footer includes a top border serving the same purpose.
>+   * We can't just use display: none here, otherwise scrollbox.xml will flip out and sometimes
>+   * refuse to scroll for us (see bug 984156). Instead, we set it to visibility hidden, force
>+   * a minimum height, and then negative-margin that single pixel into oblivion. That seems
>+   * to be enough to make scrollbox happy.
>+   */
>+  -moz-appearance: none;
>+  visibility: hidden;
>+  min-height: 1px;
>+  margin: -1px 0 0;
>+  border: none;

Looks like you're basically emulating visibility:collapse here...

Comment 48

[Mike Conley (:mconley) (:⚙️)]

(In reply to Dão Gottwald [:dao] from comment #47)
> Comment on attachment 8395468 [details] [diff] [review]
> Patch v1.1 (r+'d by mak)
> 
> > #BMB_bookmarksPopup menupopup > .bookmarks-actions-menuseparator {
> >-  /* Hide bottom separator as the styled footer includes a top border serving the same purpose */
> >-  display: none;
> >+  /* Hide bottom separator as the styled footer includes a top border serving the same purpose.
> >+   * We can't just use display: none here, otherwise scrollbox.xml will flip out and sometimes
> >+   * refuse to scroll for us (see bug 984156). Instead, we set it to visibility hidden, force
> >+   * a minimum height, and then negative-margin that single pixel into oblivion. That seems
> >+   * to be enough to make scrollbox happy.
> >+   */
> >+  -moz-appearance: none;
> >+  visibility: hidden;
> >+  min-height: 1px;
> >+  margin: -1px 0 0;
> >+  border: none;
> 
> Looks like you're basically emulating visibility:collapse here...

Almost, but not quite - according to https://developer.mozilla.org/en-US/docs/Web/CSS/visibility on "collapse":

"For XUL elements, the computed size of the element is always zero, regardless of other styles that would normally affect the size, although margins still take effect."

And the root of this bug was that scrollbox.xml didn't like the fact that the menuseparator had no dimensions.

It's possible I could have gotten away with visibility: collapse; margin-top: 1px;, but I don't think there's much value in going back and correcting it at this point.

Comment 49

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) from comment #48)
> (In reply to Dão Gottwald [:dao] from comment #47)
> > Comment on attachment 8395468 [details] [diff] [review]
> > Patch v1.1 (r+'d by mak)
> > 
> > > #BMB_bookmarksPopup menupopup > .bookmarks-actions-menuseparator {
> > >-  /* Hide bottom separator as the styled footer includes a top border serving the same purpose */
> > >-  display: none;
> > >+  /* Hide bottom separator as the styled footer includes a top border serving the same purpose.
> > >+   * We can't just use display: none here, otherwise scrollbox.xml will flip out and sometimes
> > >+   * refuse to scroll for us (see bug 984156). Instead, we set it to visibility hidden, force
> > >+   * a minimum height, and then negative-margin that single pixel into oblivion. That seems
> > >+   * to be enough to make scrollbox happy.
> > >+   */
> > >+  -moz-appearance: none;
> > >+  visibility: hidden;
> > >+  min-height: 1px;
> > >+  margin: -1px 0 0;
> > >+  border: none;
> > 
> > Looks like you're basically emulating visibility:collapse here...
> 
> Almost, but not quite - according to
> https://developer.mozilla.org/en-US/docs/Web/CSS/visibility on "collapse":
> 
> "For XUL elements, the computed size of the element is always zero,
> regardless of other styles that would normally affect the size, although
> margins still take effect."
> 
> And the root of this bug was that scrollbox.xml didn't like the fact that
> the menuseparator had no dimensions.

I don't see where this code cares about dimensions. It does care about the position.

I also just landed bug 859126 on fx-team.

Comment 50

[Dão Gottwald [:dao]]

Backed out since the underlying issue should be fixed by bug 859126:

https://hg.mozilla.org/integration/fx-team/rev/d072eebc5383

Comment 51

[Ed Morley [:emorley]]

(In reply to Dão Gottwald [:dao] from comment #50)
> Backed out since the underlying issue should be fixed by bug 859126:
> 
> https://hg.mozilla.org/integration/fx-team/rev/d072eebc5383

Merged:
https://hg.mozilla.org/mozilla-central/rev/d072eebc5383