Closed Bug 984156 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect, major)

29 Branch
x86
Windows 8.1
defect
Not set
major

Tracking

()

VERIFIED FIXED
Firefox 31
Tracking Status
firefox28 --- unaffected
firefox29 + verified
firefox30 + verified
firefox31 --- verified

People

(Reporter: alice0775, Assigned: mconley)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [Australis:P3])

Attachments

(3 files, 8 obsolete files)

Attached image 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
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
Version: 30 Branch → 29 Branch
Whiteboard: [Australis:P3]
Whiteboard: [Australis:P3] → [Australis:P3+]
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?
Flags: needinfo?(alice0775)
(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
Flags: needinfo?(alice0775)
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 ?
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
(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?
Flags: needinfo?(alice0775)
(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
Flags: needinfo?(alice0775)
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
(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 / ...?
Marco, do you have ideas here?
Flags: needinfo?(mak77)
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?
Flags: needinfo?(mak77)
(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...
(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.
I'm going to do my damndest to reproduce this today.
Assignee: nobody → mconley
Status: NEW → ASSIGNED
Alice,

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

-Mike
Flags: needinfo?(alice0775)
Attached file screen capture
"New Folder38" and "New Folder39" are affected
Flags: needinfo?(alice0775)
(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!
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?
Flags: needinfo?(alice0775)
Considering how much difficulty we're having reproducing this, I think we can take the priority down a notch.
Whiteboard: [Australis:P3+] → [Australis:P3]
(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.
Flags: needinfo?(alice0775)
On 1024*768 monitor, the problem occurs if number of bookmarks is 34....
\o/ Just reproduced it on a Win XP laptop using 1024*768 with 34 bookmarks.

Thanks Alice!
Mike, see comment 11 for some of the rules we added that might be involved (totally blind guess though)
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.
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.
Flags: needinfo?(enndeakin)
Whoops, didn't mean to needinfo Neil there.
Flags: needinfo?(enndeakin)
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.
Attached patch WIP Patch 1 (obsolete) — Splinter Review
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.
Related Bug 859126?
(In reply to Alice0775 White from comment #29)
> Related Bug 859126?

Definitely related.
See Also: → 859126
Mike, was this ready for review? I know it's marked WIP, but the screenshots look fine to me, so... :-)
Flags: needinfo?(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.
Flags: needinfo?(mconley)
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #8395053 - Attachment is obsolete: true
Attachment #8395054 - Attachment is obsolete: true
Attachment #8395056 - Attachment is obsolete: true
Attachment #8395058 - Attachment is obsolete: true
Attachment #8395064 - Attachment is obsolete: true
Attachment #8395065 - Attachment is obsolete: true
Attachment #8395308 - Flags: review?(mak77)
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.
Attachment #8395308 - Flags: review?(mak77) → review+
Ok, done. Thanks mak!
Attachment #8395308 - Attachment is obsolete: true
Attachment #8395468 - Flags: review+
remote:   https://hg.mozilla.org/integration/fx-team/rev/64995acca4a8
Whiteboard: [Australis:P3] → [Australis:P3][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/64995acca4a8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
Target Milestone: --- → Firefox 31
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.
Attachment #8395468 - Flags: approval-mozilla-beta?
Attachment #8395468 - Flags: approval-mozilla-aurora?
Attachment #8395468 - Flags: approval-mozilla-beta?
Attachment #8395468 - Flags: approval-mozilla-beta+
Attachment #8395468 - Flags: approval-mozilla-aurora?
Attachment #8395468 - Flags: approval-mozilla-aurora+
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.
Depends on: 859126
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...
(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.
(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.
Backed out since the underlying issue should be fixed by bug 859126:

https://hg.mozilla.org/integration/fx-team/rev/d072eebc5383
(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
You need to log in before you can comment on or make changes to this bug.