Closed Bug 492152 Opened 11 years ago Closed 11 years ago

Scrollable menus should support panning

Categories

(Core :: XUL, defect, P3)

x86
Windows 7
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: jimm, Assigned: Felipe)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Menus that overflow (such as bookmarks) should support panning on win7 touch displays rather than relying on the top and bottom menu scroll buttons.
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.
Flags: blocking1.9.1?
removing blocking? as this will not make it to trunk and branch. It's also pretty minor.
Flags: blocking1.9.1?
Priority: -- → P3
Assignee: jmathies → felipc
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.
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.
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
Attached patch Disable feedback on menus - rev1 (obsolete) — Splinter Review
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.
Attachment #389573 - Flags: review?(jmathies)
(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.
> 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.
(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?
> 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.
Attachment #389573 - Flags: review?(jmathies) → review+
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.
Carrying forward Jim's review

Vlad, would you take a look at these changes?
Attachment #389573 - Attachment is obsolete: true
Attachment #390079 - Flags: superreview?(vladimir)
Attachment #390079 - Flags: review+
Attachment #390079 - Flags: superreview?(vladimir) → superreview+
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
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Felipe, could you verify this applies / works on Firefox 3.5.x? Thanks
Blocks: 548100
Component: XP Toolkit/Widgets: Menus → XUL
You need to log in before you can comment on or make changes to this bug.