492152 - Scrollable menus should support panning

Status: RESOLVED FIXED

(Core :: XUL, defect, P3)

Description

[Jim Mathies [:jimm]]

Menus that overflow (such as bookmarks) should support panning on win7 touch displays rather than relying on the top and bottom menu scroll buttons.

Comment 1

[Jim Mathies [:jimm]]

This should "just work". For some reason the scrollbox mouse scroll event handler isn't getting the synthetic line scroll events from the patch in bug 479901. I'll dig into this more this week. This should land on 1.9.1 if possible.

Comment 2

[Jim Mathies [:jimm]]

removing blocking? as this will not make it to trunk and branch. It's also pretty minor.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

This seems to be already working! Anyway the pan feedback interferes with it (just like the trees on bug 503888), but I've got a patch going on that can just disable the feedback on the menus and so the panning *just work* :) Hopefully I'll get to something similar for the <tree>s too.

I don't know if this will be a standalone patch or if it will come together in bug 503888 or bug 503541, so I'll leave this open for now.

Comment 4

[Robert Strong (they/them - no direct email)]

With the recent changes this seems to now be working though it doesn't seem to be working optimally in that it is not sensitive enough.

Comment 5

[Robert Strong (they/them - no direct email)]

Felipe has identified the issue with is not being sensitive enough being caused by the feedback stopping the panning. This remaining issue will likely be fixed by either bug 503888 or bug 503541 as he noted

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Attachment: Disable feedback on menus - rev1

Hey Jim, this patch simply disables the feedback calls for a menu, so the panning works.

To avoid two |if| checking the same thing, I did a small change in the function parameters for UpdatePanFeedbackX|Y, which previously received the same event object and acessed the scrollOverflow property, and now I store the scrollOverflow property and just pass the PRInt32.

Could you take a look at these changes?

Note: We will probably want a better overall fix for the menus and trees for 3.6 or later, but this is a small patch that could fix the menu problem in 3.5.

Comment 7

[Jim Mathies [:jimm]]

(In reply to comment #6)
> Created an attachment (id=389573) [details]
> Disable feedback on menus - rev1

If we are going to disable for menus, we should also filter on the popuptype - 

http://mxr.mozilla.org/mozilla-central/source/widget/public/nsWidgetInitData.h#57

which is also handed down when the widget is created. (aInitData->mPopupHint might need to be set during menu creation since popuphint was added fairly recently.)

If we disable for menus, how do people deal with a large scrollable menu when they are in tablet mode? If I have a large, full height plus test favorites menu how do I navigate it in touch mode?

> 
> Note: We will probably want a better overall fix for the menus and trees for
> 3.6 or later, but this is a small patch that could fix the menu problem in 3.5.

Well, a quick fix for 3.5 considering how close 3.6 is probably isn't warranted at this point. However, the bigger changes we need to make for gesture event support will not be in 3.6, so some sort of fix for menus would be great for for 3.6.

Comment 8

[:Felipe Gomes (needinfo for replies!)]

> If we disable for menus, how do people deal with a large scrollable menu when
> they are in tablet mode? If I have a large, full height plus test favorites
> menu how do I navigate it in touch mode?

Hey Jim, this disabling that I mentioned is just disabling the overflow feedback when the scroll reaches an end. The panning itself continues to work, so the user can just open the menu and scroll it normally with the touch panning.

Comment 9

[Jim Mathies [:jimm]]

(In reply to comment #8)
> 
> Hey Jim, this disabling that I mentioned is just disabling the overflow
> feedback when the scroll reaches an end. The panning itself continues to work,
> so the user can just open the menu and scroll it normally with the touch
> panning.

Ok, so I'm curious, how does IE handle something like this? I guess disabling it is ok. How complex a problem would it be to make feedback "just work" without the one off for popups in the gesture code?

Comment 10

[:Felipe Gomes (needinfo for replies!)]

> Ok, so I'm curious, how does IE handle something like this? I guess disabling
> it is ok. How complex a problem would it be to make feedback "just work"
> without the one off for popups in the gesture code?

Just checked, and IE doesn't support panning in scrollable menus. To scroll one you have to use the click+drag technique, i.e. you tap and hold your finger and have to reach one of the ends of the menu where the scroll arrows are.

Comment 11

[Jim Mathies [:jimm]]

Comment on attachment 389573 [details] [diff] [review]
Disable feedback on menus - rev1

Replace all the tab indentation in the new code with spaces. With that r+ from me.

Comment 12

[:Felipe Gomes (needinfo for replies!)]

Attachment: Disable feedback on menus - rev2

Carrying forward Jim's review

Vlad, would you take a look at these changes?

Comment 13

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/658b2c3bc535

Not in-testsuite since we don't have a way to send external events for Windows... yet

Comment 14

[Robert Strong (they/them - no direct email)]

Felipe, could you verify this applies / works on Firefox 3.5.x? Thanks