Closed
Bug 350471
Opened 18 years ago
Closed 16 years ago
Reenable pixel scrolling (two-finger touchpad)
Categories
(Core :: DOM: UI Events & Focus Handling, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla1.9.1b1
People
(Reporter: mark, Assigned: mstange)
References
Details
Attachments
(4 files, 16 obsolete files)
(deleted),
text/plain
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
smaug
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
We had to turn off pixel scrolling in bug 347626 due to problems with XUL trees and listboxes, and some web apps. The exact problems are described in detail in that bug. We should come up with some kind of plan to intelligently support pixel-scrolling, at least for Gecko content areas.
Comment 1•17 years ago
|
||
Any chance the disabled code now behaves differently as we are using cocoa? I think the lack of one pixel scrolling on mac is something we really should fix--almost as much as any theme change.
Comment 2•17 years ago
|
||
I have no idea what this mac-style pixel-scrolling is about, but NS_MOUSE_SCROLL event handling has also changed in 1.9 so that it is processed like any other event. If there are problems handling xul trees/listboxes and web apps, perhaps nsEventStateManager::DoScrollText could check those and do pixel scrolling only on main content area (only top level scrolling view of a view manager).
Reporter | ||
Comment 3•17 years ago
|
||
Cocoa natively uses a different model to get scroll events, one which is probably slightly harder to work cleanly into Gecko.
Assignee | ||
Comment 4•17 years ago
|
||
This is going to rock! This patch makes pixel scrolling work, without any problems for trees or Google Maps. However, there are several drawbacks: 1. Blocking 0-line scroll events from reaching Google Maps is quite hacky. However, I don't know where it should be put instead. 2. Our chrome can't access the pixel deltas. We should rethink our DOMMouseScroll implementation. 3. Calling deviceDeltaX/Y when the device does not support it prints an assertion. I don't know how to get rid of it. Other than that, it works, and I can't live without it any more. :)
Attachment #302030 -
Flags: review?(joshmoz)
Comment 5•17 years ago
|
||
Comment on attachment 302030 [details] [diff] [review] Fix v0.8: First try >+ case NS_MOUSE_SCROLL_EVENT: >+ // Scroll events with .delta==0 should not be sent as DOMEvent. Doing >+ // so breaks Google Maps. >+ // XXX Is this the right place to block DOMEvents? >+ if (static_cast<nsMouseScrollEvent*>(aEvent)->delta==0) >+ return NS_ERROR_DOM_NOT_SUPPORTED_ERR; >+ return NS_NewDOMMouseEvent(aDOMEvent, aPresContext, >+ static_cast<nsInputEvent*>(aEvent)); >+ break; This is way too ugly hack. Why Google Maps needs special handling? Could you re-use .delta for deltaPixels and then depending on some preference interpret the value correctly?
Assignee | ||
Comment 6•17 years ago
|
||
Assignee | ||
Comment 7•17 years ago
|
||
Assignee | ||
Comment 8•17 years ago
|
||
(In reply to comment #5) > Why Google Maps needs special handling? When Google Maps receives a mouse scroll event, it checks event.detail. When event.detail is > 0, it zooms out, otherwise it zooms in. So it even zooms in when event.detail == 0. You could say that this is a bug in Google Maps. Making Google fix their behaviour would certainly be more desirable than introducing a hack into our code. Webkit blocks 0-delta scroll events as well, see WebCore/dom/EventTargetNode.cpp, line 499: http://www.koders.com/cpp/fidF29B9E177EF238EFCE90ACF9644357142305517D.aspx#L499 > Could you re-use .delta for deltaPixels and then depending on some > preference interpret the value correctly? This is exactly what I was going to avoid. I'll try to explain. Mozilla's scrolling architecture is used to dealing with line scroll events. Some widgets, like trees, can only be scrolled by line. When pixel scrolling was implemented last time (bug 319078), this led to problems: trees did a line scroll when they should only do a pixel scroll (which they can't). This was caused by the fact that nsMouseScrollEvent::delta, and likewise nsDOMMouseEvent::detail, was reused with a different meaning. Of course, the kIsPixels flag was meant to differentiate between pixel and line scrolls, but due to the fact that some parts of mozilla can't handle pixel scrolls, this did not work reliably. That's why I chose another approach (which happens to be the approach Cocoa uses). nsMouseScrollEvent::delta should keep its meaning, that is, it contains the amount of lines to scroll. This way, everything that works now (like trees) will keep working. And widgets that know how to scroll by pixel can be updated incrementally to check for kHasPixels and utilise deltaPixels when they want to. As you can see, this leads to a problem: We now need to create an event for every pixel that has been scrolled, even if there haven't been enough scrolls to amount for a line scroll. Thus, most of the pixel scroll events carry a delta that is zero. Ideally, this shouldn't be a problem - if delta==0, you just don't scroll. However, Google Maps gets it wrong and zooms in regardless. And there is one more problem. Currently, for every nsMouseScrollEvent, a nsDOMMouseEvent is created, with domevent.detail = nsEvent->delta. So on the way to the DOMEvent, deltaPixels is lost. This means that even things like the tabbar, which could support pixel scrolling easily, can't scroll by pixel because they have no access to the data. It's the same for trees, should they support pixel scrolling one day. So what can be done? I can imagine three alternatives. One alternative is the one I implemented (sadly, with an ugly hack): Simply block all events with delta==0 from being sent as DOMEvent. This means no problems with Google Maps and no chance for supporting pixel scrolling in things like the tabbar. The second alternative would be to add an attribute to the DOMEvent interface (like "deltaPixels"), and then mark events with delta==0 as chrome-only DOMEvents. (Is that possible? Would it be hard to implement?) The third alternative is to switch to an IE/Safari compatible implementation: - "DOMMouseScroll" --> "mousewheel" - event.detail --> event.wheelDelta Morover, we would add the wheelDeltaPixels attribute, document it, and expose it to scripts on the web. Then Google might be attracted to fixing Google Maps, because smooth zooming is quite nice. ;) Let me hear your thoughts.
Comment 9•17 years ago
|
||
What about a new event for pixel scrolling: NS_MOUSE_PIXEL_SCROLL/DOMMousePixelScroll? .detail / .delta would mean then pixels nsEventStateManager could check whether the current scrollable view supports pixel scrolling. Events could be dispatched perhaps so that if there is normal scrolling, dispatch DOMMouseScroll and if there is pixel scrolling, dispatch another event. So in case of line-scrolling, only DOMMouseScroll, and in case of pixel-scrolling, dispatch either only DOMMousePixelScroll, or if the line delta != 0, dispatch first DOMMouseScroll and then additional DOMMousePixelScroll. This all depends on how OSX generates those events. Note, because I don't have a mac, I can't test this at all.
Assignee | ||
Comment 10•17 years ago
|
||
(In reply to comment #9) > What about a new event for pixel scrolling: > NS_MOUSE_PIXEL_SCROLL/DOMMousePixelScroll? > .detail / .delta would mean then pixels > nsEventStateManager could check whether the current scrollable view supports > pixel scrolling. Right now I can't think of an easy way to do this. Remember, the nsEvent is created in nsChildView.mm. nsChildView.mm does not know whether the current scrollable view supports pixel scrolling. The decision what event to dispatch is up to nsEventStateManager. But in order to be able to send the appropriate event, nsEventStateManager needs to know both delta and deltaPixels. So we need to send both deltas anyway and can't simple reuse .delta. > So in case of line-scrolling, only DOMMouseScroll, and in case of > pixel-scrolling, dispatch either only DOMMousePixelScroll, or if the line > delta != 0, dispatch first DOMMouseScroll and then additional > DOMMousePixelScroll. I don't think I understand what you are trying to say here. If we dispatched both DOMMouseScroll and DOMMousePixelScroll, we'd end up scrolling twice. Take a look at the first 4 events in the example data in the attachments. Let's start at scroll position 0px, and let's assume one line equals 5 pixels. In the first three events, deltaPixels == 1px and delta == 0. --> New scroll position: (1 + 0) + (1 + 0) + (1 + 0) = 3. In the fourth event, enough pixel scrolls have been accumulated to make up for a line scroll, so delta == 1. --> New scroll position: 3 + (2 + 1*5) = 10. Intended scroll position: 5. This is certainly not what we want to have. Dispatching both events is not an option. In bug 347626, Mark tried to make this work by using the following approach: When a pixel scroll has occured, send a pixel scroll event. If dispatching this event fails, send a line scroll event. However, I think this makes things unnecessarily complex and we're much better off using the approach I outlined. > This all depends on how OSX generates those events. > Note, because I don't have a mac, I can't test this at all. The table of "example data" and the graph should make clear what OSX does.
Assignee | ||
Comment 11•17 years ago
|
||
Hah! Now I see the solution. You're right, introducing a new event is the key. We can simple mark all line scroll events that are pixel scroll replacement events as such. I'll try to come up with a patch soon.
Comment 12•17 years ago
|
||
Comment on attachment 302030 [details] [diff] [review] Fix v0.8: First try Assuming you don't want my review any more since you're going to create a new patch.
Attachment #302030 -
Flags: review?(joshmoz)
Assignee | ||
Comment 13•17 years ago
|
||
Well, this has turned out to be more challenging than I anticipated. The current strategy is like this: - Cocoa reports a scroll event - Is it a line scroll event? - Yes: - Send an NS_MOUSE_SCROLL event to gecko - No: - Send an NS_MOUSE_PIXEL_SCROLL event to gecko - If delta != 0: - Send an NS_MOUSE_SCROLL event to gecko, marked with kHasPixels so that it doesn't get dispatched by pixel scrollable targets (we don't want to scroll twice, see previous comments) This works quite well. Trees work, Google Maps work, I don't see any ugly hacks. However: 1. I still haven't been able to figure out how to get rid of the failing assertion. There doesn't seem to be a way I can predict it. I need help here. 2. I haven't created a DOMEvent for NS_MOUSE_PIXEL_SCROLL yet. I think this should be done in a follow-up bug. The thing is: I could implement a DOMMousePixelScroll event the same way the DOMMouseScroll event is implemented, but that wouldn't be of any use - DOMMouseScroll events haven't got a "hasPixels" attribute like nsMouseScroll, so there's again the problem of scrolling twice. So DOMMouseScroll needs to get a new attribute, "hasPixels". And, for that matter, an attribute "axis" as well, so we're able to fix wrong-axis-bugs like bug 378028. Why is this a problem? Currently, DOMMouseScroll inherits from nsIDOMMouseEvent and thus from nsIDOMUIEvent, so it gets the "detail" attribute which we use to store the delta in. However, now we need an attribute that hasn't been declared there yet. As far as I can see, in order to be able to extend this interface, we have to create a new IDL-file and do the XPConnect thing... is this correct? That sounds like a major undertaking. My current plan is to restrict this bug to the functionality that my patch already provides (ideally, without a failing assertion...). Then I'll file new bugs: - Extend DOMMouseScroll event by axis and hasPixels attribute - Add DOMMousePixelScroll event - Make the toolkit scrollbox widget understand pixel scrolling - the same for trees and listboxes Do you think that's ok?
Attachment #302030 -
Attachment is obsolete: true
Attachment #302686 -
Flags: review?(Olli.Pettay)
Comment 14•17 years ago
|
||
Don't add NS_MOUSE_PIXEL_SCROLL_EVENT. You're not adding new nsEvent structure type anyway. NS_MOUSE_PIXEL_SCROLL is enough, it uses nsMouseScrollEvent I don't think bug 378028 has anything to do with this. nsMouseScrollEvent may contain kIsVertical/kIsHorizontal flags. If the device supports pixel scrolling, in which case deltaX is 0 but deviceDeltaX != 0? I mean in practice - what is the difference in user interaction. (I really should get a mac for testing) Would it be terrible bad to always dispatch MouseScroll, so that if deltaX is 0, but deviceDeltaX > 0, deltaX = 1 (or -1 if deviceDeltaX < 0)? EventStateManager shouldn't handle those MouseScroll events, because they have the kHasPixels flag, but content (== web page/chrome) could still get a normal-like MouseScroll event. ... I'm just trying to figure out the least-regression-risky way to implement this. > 3. Calling deviceDeltaX/Y when the device does not support it prints an > assertion. I don't know how to get rid of it. Please figure out how to get rid of this. More comments and questions perhaps coming...
Assignee | ||
Comment 15•17 years ago
|
||
I've finally figured out how to prevent the failed assertions from being printed to the console. I had to define a custom NSAssertionHandler that ignores all errors and declare that as the thread's new default assertion handler. (In reply to comment #14) > Don't add NS_MOUSE_PIXEL_SCROLL_EVENT. You're not adding new nsEvent structure > type anyway. NS_MOUSE_PIXEL_SCROLL is enough, it uses nsMouseScrollEvent First I had added nsMousePixelScrollEvent but then I figured that I could simply reuse nsMouseScrollEvent. I think we do need NS_MOUSE_PIXEL_SCROLL_EVENT. Otherwise, the pixel scroll events would get dispatched as DOMMouseScroll events, which would be bad. > I don't think bug 378028 has anything to do with this. nsMouseScrollEvent > may contain kIsVertical/kIsHorizontal flags. nMouseScrollEvent does - nsDOMMouseEvent doesn't. However, scrolling trees works via DOM events. See http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/content/widgets/tree.xml&rev=1.54&root=/cvsroot#627 > If the device supports pixel scrolling, in which case deltaX is 0 but > deviceDeltaX != 0? I mean in practice - what is the difference in user > interaction. (I really should get a mac for testing) Hu? > Would it be terrible bad to always dispatch MouseScroll, so that > if deltaX is 0, but deviceDeltaX > 0, deltaX = 1 (or -1 if deviceDeltaX < 0)? I think it would be quite bad, and moreover, I don't think it is necessary. deltaX: number of lines. 1 line == 16 pixels (at least that's what I get) deviceDeltaX: number of pixels. If we scrolled for a line every time we received a pixel scroll, that would be way too fast. > EventStateManager shouldn't handle those MouseScroll events, because they have > the kHasPixels flag, but content (== web page/chrome) could still get a > normal-like MouseScroll event. Way too often. That's what caused bug 347626. And with my patch, they do receive "normal-like" MouseScroll events - again, have a look at the illustration I uploaded, that should make it clear. > ... I'm just trying to figure out the least-regression-risky way to implement > this. I'm pretty sure that what I'm doing now is quite regression-free. We can't use pixel scroll event in DOM-content yet, but that's not a regression. > > 3. Calling deviceDeltaX/Y when the device does not support it prints an > > assertion. I don't know how to get rid of it. > Please figure out how to get rid of this. Done. :) OK, please keep the questions coming, I really want to get this in. ;)
Attachment #302686 -
Attachment is obsolete: true
Attachment #302801 -
Flags: review?(Olli.Pettay)
Attachment #302686 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 16•17 years ago
|
||
Attachment #302801 -
Attachment is obsolete: true
Attachment #302802 -
Flags: review?(Olli.Pettay)
Attachment #302801 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 17•17 years ago
|
||
> have a look at the illustration I uploaded, that should make it clear.
Ah I see, that could be misleading... I used different scales for delta and deltaPixels there, so that's why you could think a delta of 1/-1 wouldn't be different from a pixel scroll with an equal deltaPixels...
However, mapping all positive/negative deltas to 1/-1 would certainly result in a loss of precision and in a perceivable difference to native Cocoa apps.
Comment 18•17 years ago
|
||
Comment on attachment 302802 [details] [diff] [review] Fix v0.11: remove garbage, update comments >+#define NS_MOUSE_PIXEL_SCROLL_EVENT 17 Still this? Remove it, since you don't have the new event struct type. And I don't think there is any need for a new struct type. >+ case NS_MOUSE_PIXEL_SCROLL: >+ { >+ if (mCurrentFocus) { >+ mCurrentTargetContent = mCurrentFocus; >+ } >+ } >+ break; How should pixel scroll work if Ctrl, Shift or some other modifier is pressed? What does OSX do in those cases? What does Safari do? About that assertion, please ask Josh or someone to make sure it is ok. > I think we do need > NS_MOUSE_PIXEL_SCROLL_EVENT. Otherwise, the pixel scroll events would get > dispatched as DOMMouseScroll events, which would be bad. No. DOM events aren't even created for NS_MOUSE_PIXEL_SCROLL, because there isn't just any need for that. Scripts may listen DOMMousePixelScroll, but because nsEvent->message <-> nsDOMEvent::GetType conversion is missing, scripts don't get access to NS_MOUSE_PIXEL_SCROLL events.
Attachment #302802 -
Flags: review?(Olli.Pettay) → review-
Assignee | ||
Comment 19•17 years ago
|
||
(In reply to comment #18) > (From update of attachment 302802 [details] [diff] [review]) > >+#define NS_MOUSE_PIXEL_SCROLL_EVENT 17 > Still this? Remove it, since you don't have the new event struct type. Oops! Now I finally understand. Sorry. > >+ case NS_MOUSE_PIXEL_SCROLL: > >+ { > >+ if (mCurrentFocus) { > >+ mCurrentTargetContent = mCurrentFocus; > >+ } > >+ } > >+ break; > How should pixel scroll work if Ctrl, Shift or some other modifier is pressed? > What does OSX do in those cases? What does Safari do? Usually, pressing a modifier key doesn't have any influence on scrolling. However, you can turn on screen zooming for a modifier key: http://docs.info.apple.com/article.html?artnum=304645 But when screen zooming is turned on for a modifier key, Cocoa doesn't send a scroll event while that key is pressed. So Firefox now behaves the same way as Safari and other OS X applications. Thanks for your patience!
Attachment #302802 -
Attachment is obsolete: true
Attachment #302811 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 20•17 years ago
|
||
> <josh> mstange: those lines are a little long
Fixed.
Attachment #302811 -
Attachment is obsolete: true
Attachment #302835 -
Flags: review?(mark)
Attachment #302811 -
Flags: review?(Olli.Pettay)
Assignee | ||
Updated•17 years ago
|
Attachment #302835 -
Flags: superreview?(roc)
Comment 21•17 years ago
|
||
Hmm, realized still one problem. DOMMouseScroll can be prevented (.preventDefault()) The new event can't be. If we just could dispatch that DOMMouseScroll and check if it was prevented and then not dispatch pixelscroll. Or only dispatch NS_MOUSE_SCROLL, (extended with pixel data), but .delta set always something else than 0. But I understand that doesn't work. What would be the best solution? And note, in gecko NS_MOUSE_SCROLL may be set up to do something else than scroll. That is the reason for GetBasePrefKeyForMouseWheel in ESM. What we really don't want is for example that ctrl+NS_MOUSE_SCROLL zooms-in but because there is also NS_MOUSE_PIXEL_SCROLL dispatched, page is scrolled too. Sorry for not thinking all these immediately, but there are quite many cases the mouse scrolling needs to handle.
Assignee | ||
Comment 22•17 years ago
|
||
Ah well, you're right again.
> What we really don't want is for example that ctrl+NS_MOUSE_SCROLL zooms-in
> but because there is also NS_MOUSE_PIXEL_SCROLL dispatched, page is scrolled
> too.
That's exactly what's happening.
I'll have a look at what Safari is doing.
Assignee | ||
Updated•17 years ago
|
Attachment #302835 -
Flags: superreview?(roc)
Attachment #302835 -
Flags: review?(mark)
Assignee | ||
Comment 23•17 years ago
|
||
Safari is doing the same as we are - pixel scroll events aren't cancelable. So how can we fix the zoom problem? Currently, I can only think of one solution: creating a DOMEvent for NS_MOUSE_PIXEL_SCROLL. Page zooming could simply .preventDefault() all of them as long as the zoom modifier key is pressed. But in order to make good use of DOMMousePixelScroll events, there needs to be a hasPixels attribute for DOMMouseScroll. So I can't really see a way around extending DOMMouseScroll. What do you think?
Comment 24•17 years ago
|
||
(In reply to comment #23) > Safari is doing the same as we are - pixel scroll events aren't cancelable. Unfortunately DOMMouseScroll has always been cancelable. So some content or extensions probably relies on that behavior. > So how can we fix the zoom problem? Currently, I can only think of one > solution: creating a DOMEvent for NS_MOUSE_PIXEL_SCROLL. Page zooming could > simply .preventDefault() all of them as long as the zoom modifier key is > pressed. Just for that reason the DOM Event isn't needed. You can always set NS_EVENT_FLAG_NO_DEFAULT and nsEventStatus_eConsumeNoDefault in EventStateManager::PreHandleEvent or ::PostHandleEvent, whatever effect is wanted. Still thinking how to get this working without breaking too many things.
Assignee | ||
Comment 25•17 years ago
|
||
> > So how can we fix the zoom problem? Currently, I can only think of one
> > solution: creating a DOMEvent for NS_MOUSE_PIXEL_SCROLL. Page zooming could
> > simply .preventDefault() all of them as long as the zoom modifier key is
> > pressed.
> Just for that reason the DOM Event isn't needed. You can always set
> NS_EVENT_FLAG_NO_DEFAULT and nsEventStatus_eConsumeNoDefault in
> EventStateManager::PreHandleEvent or ::PostHandleEvent, whatever effect is
> wanted.
>
Aha, ok. I think I've found out by now how to get zoom to work again without introducing the DOMMousePixelScroll event, a new patch is in the works.
The problem with the extensions is really tricky. I'm thinking about it too... the thing is, the first pixel scroll event is sent before the first line scroll event, so we can't just test if the last line scroll event has been canceled - which would also have been pretty unstable.
Assignee | ||
Comment 26•17 years ago
|
||
OK. What I'm doing here is to scroll pixels only when the standard scroll would have been a line scroll (and not a zoom, history scroll, or page scroll). This doesn't solve the preventDefault() problem.
Attachment #302835 -
Attachment is obsolete: true
Assigning this to Markus because he's generating patches :)
Assignee: events → mstange
Assignee | ||
Comment 28•17 years ago
|
||
(In reply to comment #24) > (In reply to comment #23) > > Safari is doing the same as we are - pixel scroll events aren't cancelable. > Unfortunately DOMMouseScroll has always been cancelable. So some content or > extensions probably relies on that behavior. Well, let's break them. After all, that's what extension compatibility checks are for. I can't think of a different way of implementing this that wouldn't break them. However, if we break extensions, we must give them a way to be fixed. So I introduced the DOMMousePixelScroll event, which can be .preventDefault()ed. And in order to make proper use of DOMMousePixelScroll events, there has to be a way to tell if the DOMMouseScroll events are replacement events (and thus should be ignored when interpreting pixel scroll events). So the DOM event needs to carry the scrollFlags. That's why I added nsIDOMMouseScrollEvent.idl (and the rest of the cruft that needs to be adjusted when adding a new DOM event...). Giving DOM content access to the scrollFlags is actually really great, as they carry the scroll axis as well. And slowly, but surely, this rocks.
Attachment #302900 -
Attachment is obsolete: true
Attachment #305250 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 29•17 years ago
|
||
Assignee | ||
Comment 30•17 years ago
|
||
Assignee | ||
Comment 31•17 years ago
|
||
Attachment #305250 -
Attachment is obsolete: true
Attachment #305390 -
Flags: review?(Olli.Pettay)
Attachment #305250 -
Flags: review?(Olli.Pettay)
Assignee | ||
Updated•17 years ago
|
Attachment #305390 -
Flags: superreview?(roc)
Attachment #305390 -
Flags: review?(mark)
Comment 32•17 years ago
|
||
Sorry about not reviewing this yet, but since this isn't blocking1.9+ and I've been quite busy with other things... :( (And I'm not yet absolutely sure the solution is the right one. I'm trying to avoid all possible regressions this late in 1.9)
Assignee | ||
Comment 33•17 years ago
|
||
No problem. However, the sooner we break compatibility, the better...
Status: NEW → ASSIGNED
Assignee | ||
Comment 35•17 years ago
|
||
Comment on attachment 305390 [details] [diff] [review] Fix v2.0, diff updated to trunk Marking as obsolete, since smaug and I decided to try another approach that hopefully does not break anything. In comment 25, I said: > the thing is, the first pixel scroll event is sent before the first line scroll > event, so we can't just test if the last line scroll event has been canceled However that's exactly what we're going to do. We're taking over the decision when a line scroll is sent, so we can just send it before the first pixel scroll. When the line scroll event has got through without being preventDefault()ed, we unlock a certain amount of pixels that may be scrolled by subsequent pixel scroll events. If the line scroll has been canceled, we consume those pixel scroll events without sending them into Gecko.
Attachment #305390 -
Attachment is obsolete: true
Attachment #305390 -
Flags: superreview?(roc)
Attachment #305390 -
Flags: review?(mark)
Attachment #305390 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 36•17 years ago
|
||
This is the master patch. 1. It honors .preventDefault(). 2. It introduces a pref (mousewheel.enabel_pixel_scrolling) to address bug 351105. 3. Otherwise, it feels equally awesome. That means the patch passes the gmaps-iframe test (in contrast to Safari): http://mozilla.pettay.fi/moztests/gmaps.html
Attachment #311060 -
Flags: review?(Olli.Pettay)
Markus, thanks for the patch and I hope we can eventually take it or something like it, but I need to let you know that it's too late to take a change like this for Firefox3.
Olli says that the DOM3 Events working group is going to specify a standard mousewheel event. If we make sure that that event supports pixel scrolling, that will be the way to to go for the long term. Markus, right now I'm stealing part of this patch for bug 378028 --- the nsDOMMouseScrollEvent class and related code, but not the actual pixel scrolling event. We can add the pixel scrolling event code post-1.9 (which is very very soon now!). I have made one change which is that instead of exposing scrollFlags directly to Javascript, I'm exposing an "axis" attribute.
Updated•17 years ago
|
Flags: wanted1.9.1?
Flags: blocking1.9.1?
Comment 42•17 years ago
|
||
IMO this might be possible for 1.9.1. Pixel scrolling isn't yet defined in DOM 3 Events, but I hope the draft will be updated within next few weeks. And if not, we could use DOMMousePixelScroll and add the standard event later.
Assignee | ||
Comment 43•17 years ago
|
||
Comment on attachment 311060 [details] [diff] [review] Fix v3.0: Don't break anything, add pref Cool! I'll create a new patch when bug 378028 lands.
Attachment #311060 -
Flags: review?(Olli.Pettay)
Not blocking, but really wanted
Flags: wanted1.9.1?
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
Priority: -- → P1
Assignee | ||
Comment 45•16 years ago
|
||
This patch is on top of that for bug 378028. Short summary of what this patch does: - Adds NS_MOUSE_PIXEL_SCROLL / DOMMousePixelScroll event type - Adds isFallback attribute for DOMMouseScroll events If this is true it means that you shouldn't process this line scroll event if you're processing pixel scroll events (to avoid double scrolling). - Makes .preventDefault() on line scroll events cancel subsequent pixel scroll events - Adds Mac OS X implementation
Attachment #311060 -
Attachment is obsolete: true
Attachment #332786 -
Flags: superreview?(roc)
Attachment #332786 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 46•16 years ago
|
||
Comment on attachment 332786 [details] [diff] [review] Fix v4.0: Updated patch Steven, could you review the widget/cocoa part?
Attachment #332786 -
Flags: review?(smichaud)
Assignee | ||
Comment 47•16 years ago
|
||
Attachment #305251 -
Attachment is obsolete: true
Attachment #305253 -
Attachment is obsolete: true
Comment 48•16 years ago
|
||
Comment on attachment 332786 [details] [diff] [review] Fix v4.0: Updated patch Please use -p when generating patches. >+NS_IMETHODIMP >+nsDOMMouseScrollEvent::GetIsFallback(PRBool* aResult) >+{ >+ NS_ENSURE_ARG_POINTER(aResult); >+ >+ if (mEvent->eventStructType == NS_MOUSE_SCROLL_EVENT) { >+ PRUint32 flags = static_cast<nsMouseScrollEvent*>(mEvent)->scrollFlags; >+ *aResult = (flags & nsMouseScrollEvent::kHasPixels); This may assign non-zero-or-one to *aResult. *aResult = !!(flags & nsMouseScrollEvent::kHasPixels) is better >+ } else { >+ *aResult = PR_FALSE; >+ } >+ return NS_OK; But I'm not sure about the isFallback, do we really need it in DOM? >- mTabbedThroughDocument(PR_FALSE) >+ mTabbedThroughDocument(PR_FALSE), >+ mLastLineScrollSucceededX(PR_TRUE), >+ mLastLineScrollSucceededY(PR_TRUE) So did you try to add some code which clears mLastLineScrollSucceededX/Y when focusing out from the ESM? >+ // When the last line scroll has been canceled, eat the pixel scroll event >+ nsMouseScrollEvent *msEvent = (nsMouseScrollEvent*) aEvent; >+ if (msEvent->scrollFlags & nsMouseScrollEvent::kIsHorizontal) { >+ if (!mLastLineScrollSucceededX) >+ *aStatus = nsEventStatus_eConsumeNoDefault; >+ } else if (msEvent->scrollFlags & nsMouseScrollEvent::kIsVertical) { >+ if (!mLastLineScrollSucceededY) >+ *aStatus = nsEventStatus_eConsumeNoDefault; >+ } >+ } I prefer {...} after |if| > interface nsIDOMMouseScrollEvent : nsISupports > { > const long HORIZONTAL_AXIS = 1; > const long VERTICAL_AXIS = 2; > > readonly attribute long axis; >+ readonly attribute boolean isFallback; If we have isFallback attribute, it really should be documented properly. And the name isn't perhaps the best :/ > class nsMouseScrollEvent : public nsMouseEvent_base > { > public: > enum nsMouseScrollFlags { > kIsFullPage = 1 << 0, > kIsVertical = 1 << 1, > kIsHorizontal = 1 << 2, >- kIsPixels = 1 << 3 >+ kHasPixels = 1 << 3 > }; Please document what kHasPixels means >+ if (scrollDelta != 0) { >+ // Send line scroll event >+ nsMouseScrollEvent geckoEvent(PR_TRUE, NS_MOUSE_SCROLL, nsnull); >+ [self convertCocoaMouseEvent:theEvent toGeckoEvent:&geckoEvent]; >+ geckoEvent.scrollFlags |= inAxis; >+ >+ if (hasPixels) { >+ // Mark as pixel scroll replacement >+ geckoEvent.scrollFlags |= nsMouseScrollEvent::kHasPixels; >+ } But what if scrollDeltaPixels is 0? Do we still want to add kHasPixels flag? The DOMMousePixelScroll event isn't dispatched in that case.
Assignee | ||
Comment 49•16 years ago
|
||
(In reply to comment #48) > (From update of attachment 332786 [details] [diff] [review]) > Please use -p when generating patches. Yeah, sorry, I didn't know how to do that with qdiff since it doesn't accept -p. But now I've found that you can generate mq queue patches with hg diff, too :-) > > >+NS_IMETHODIMP > >+nsDOMMouseScrollEvent::GetIsFallback(PRBool* aResult) > >+{ > >+ NS_ENSURE_ARG_POINTER(aResult); > >+ > >+ if (mEvent->eventStructType == NS_MOUSE_SCROLL_EVENT) { > >+ PRUint32 flags = static_cast<nsMouseScrollEvent*>(mEvent)->scrollFlags; > >+ *aResult = (flags & nsMouseScrollEvent::kHasPixels); > This may assign non-zero-or-one to *aResult. > *aResult = !!(flags & nsMouseScrollEvent::kHasPixels) is better OK. > >+ } else { > >+ *aResult = PR_FALSE; > >+ } > >+ return NS_OK; > But I'm not sure about the isFallback, do we really need it in DOM? Without isFallback the pixel scroll events are basically unusable (the double-scroll problem), so I think we do need it. > >- mTabbedThroughDocument(PR_FALSE) > >+ mTabbedThroughDocument(PR_FALSE), > >+ mLastLineScrollSucceededX(PR_TRUE), > >+ mLastLineScrollSucceededY(PR_TRUE) > So did you try to add some code which clears mLastLineScrollSucceededX/Y when > focusing out > from the ESM? I didn't because I forgot why I should do it. What was the use case again? And I made some experiments. If you leave the google maps zoom area after zooming and try to scroll, only the first 3 or so pixels of your scroll gesture are ignored which you don't really notice. > > >+ // When the last line scroll has been canceled, eat the pixel scroll event > >+ nsMouseScrollEvent *msEvent = (nsMouseScrollEvent*) aEvent; > >+ if (msEvent->scrollFlags & nsMouseScrollEvent::kIsHorizontal) { > >+ if (!mLastLineScrollSucceededX) > >+ *aStatus = nsEventStatus_eConsumeNoDefault; > >+ } else if (msEvent->scrollFlags & nsMouseScrollEvent::kIsVertical) { > >+ if (!mLastLineScrollSucceededY) > >+ *aStatus = nsEventStatus_eConsumeNoDefault; > >+ } > >+ } > I prefer {...} after |if| OK. > > > interface nsIDOMMouseScrollEvent : nsISupports > > { > > const long HORIZONTAL_AXIS = 1; > > const long VERTICAL_AXIS = 2; > > > > readonly attribute long axis; > >+ readonly attribute boolean isFallback; > If we have isFallback attribute, it really should be documented properly. OK. Where? On MDC? At the nsMouseScrollFlags enum? > And the name isn't perhaps the best :/ Yeah. Is hasPixels better? Or should I rename kHasPixels to kIsFallback? > > > class nsMouseScrollEvent : public nsMouseEvent_base > > { > > public: > > enum nsMouseScrollFlags { > > kIsFullPage = 1 << 0, > > kIsVertical = 1 << 1, > > kIsHorizontal = 1 << 2, > >- kIsPixels = 1 << 3 > >+ kHasPixels = 1 << 3 > > }; > Please document what kHasPixels means OK. > > >+ if (scrollDelta != 0) { > >+ // Send line scroll event > >+ nsMouseScrollEvent geckoEvent(PR_TRUE, NS_MOUSE_SCROLL, nsnull); > >+ [self convertCocoaMouseEvent:theEvent toGeckoEvent:&geckoEvent]; > >+ geckoEvent.scrollFlags |= inAxis; > >+ > >+ if (hasPixels) { > >+ // Mark as pixel scroll replacement > >+ geckoEvent.scrollFlags |= nsMouseScrollEvent::kHasPixels; > >+ } > But what if scrollDeltaPixels is 0? During my tests, deviceDeltaX/Y was always non-zero (if they exist; if they don't, hasPixels == false). But I can add another check so we're safe.
Comment 50•16 years ago
|
||
(In reply to comment #49) > Without isFallback the pixel scroll events are basically unusable (the > double-scroll problem), so I think we do need it. Hmm, right. But the name is a bit strange. > I didn't because I forgot why I should do it. What was the use case again? Scrolling and changing tab and then back to the original tab. > And I made some experiments. If you leave the google maps zoom area after > zooming and try to scroll, only the first 3 or so pixels of your scroll gesture > are ignored which you don't really notice. But if even those 3 pixels could be there with a small change to the patch.... > > > interface nsIDOMMouseScrollEvent : nsISupports > > > { > > > const long HORIZONTAL_AXIS = 1; > > > const long VERTICAL_AXIS = 2; > > > > > > readonly attribute long axis; > > >+ readonly attribute boolean isFallback; > > If we have isFallback attribute, it really should be documented properly. > > OK. Where? On MDC? At the nsMouseScrollFlags enum? In this interface. > Yeah. Is hasPixels better? Or should I rename kHasPixels to kIsFallback? Neither of those, isFallback or hasPixels, say much to the user of the interface. Hard to say what could be a good name.
Assignee | ||
Comment 51•16 years ago
|
||
Comment on attachment 332786 [details] [diff] [review] Fix v4.0: Updated patch On IRC smaug and I decided to drop the isFallback attribute and make the EventStateManager dispatch additional DOMMousePixelScroll events in that case.
Attachment #332786 -
Attachment is obsolete: true
Attachment #332786 -
Flags: superreview?(roc)
Attachment #332786 -
Flags: review?(smichaud)
Attachment #332786 -
Flags: review?(Olli.Pettay)
Comment 53•16 years ago
|
||
Comment on attachment 333929 [details] [diff] [review] updated patch >+ case NS_MOUSE_PIXEL_SCROLL: >+ { >+ if (mCurrentFocus) { >+ mCurrentTargetContent = mCurrentFocus; >+ } >+ >+ // When the last line scroll has been canceled, eat the pixel scroll event >+ nsMouseScrollEvent *msEvent = (nsMouseScrollEvent*) aEvent; Use C++ static_cast<> > nsresult >+nsEventStateManager::SendDOMPixelScrollEvent(nsIFrame* aTargetFrame, >+ nscoord aDelta, >+ nsInputEvent* aEvent, >+ nsPresContext* aPresContext) >+{ >+ nsCOMPtr<nsIContent> targetContent = aTargetFrame->GetContent(); >+ if (!targetContent) >+ GetFocusedContent(getter_AddRefs(targetContent)); >+ if (!targetContent) return NS_OK; >+ >+ nsMouseScrollEvent event(*(reinterpret_cast<nsMouseScrollEvent*>(aEvent))); Hmm, you rely on implicit copy-constructor, but does that know how to handle nsCOMPtr members? I think it does - at least it should. What I'm a bit worried that if someone adds a member variable which explicitly needs AddRef/Release, this would be broken. But if you add enough tests for pixel scrolling, testsuites should catch such error pretty easily. Anyway, shouldn't static_cast<> be enough? And make sure targetContent isn't #text node, but some element. >+ nsCOMPtr<nsIDOMEvent> domEvent; >+ NS_NewDOMMouseScrollEvent(getter_AddRefs(domEvent), aPresContext, &event); >+ nsCOMPtr<nsIDOMMouseScrollEvent> domScrollEvent(do_QueryInterface(domEvent)); >+ nsCOMPtr<nsIPrivateDOMEvent> privateEvent(do_QueryInterface(domScrollEvent)); >+ >+ if (privateEvent && NS_IS_TRUSTED_EVENT(aEvent)) { >+ privateEvent->SetTrusted(PR_TRUE); >+ } >+ >+ nsCOMPtr<nsIDOMEventTarget> target(do_QueryInterface(targetContent)); >+ if (target) { >+ PRBool defaultActionEnabled; >+ target->DispatchEvent(domEvent, &defaultActionEnabled); If we want that pixelscroll actually causes the scrolling, the event should be dispatched via presshell. See nsIPresShell::HandleEventWithTarget That also means that there isn't need to explicitly create nsIDOMEvent. >@@ -2199,16 +2251,21 @@ nsEventStateManager::DoScrollText(nsPres ... > if (lineHeight != 0) { >+ if (aSendPixelScrollEvent && aScrollQuantity == eScrollByLine) { >+ SendDOMPixelScrollEvent(aTargetFrame, aNumLines * lineHeight, aEvent, >+ aPresContext); >+ } >+ I think DoScrollText should really scroll the text, not fire pixelscrollevent. To get the lineHeight info from scrollview, you could perhaps make a simple helper method which returns the current scrollable view. >@@ -2459,21 +2516,32 @@ nsEventStateManager::PostHandleEvent(nsP ... > case NS_MOUSE_SCROLL: >+ case NS_MOUSE_PIXEL_SCROLL: >+ nsMouseScrollEvent *msEvent = reinterpret_cast<nsMouseScrollEvent*>(aEvent); Is reinterpret_cast really needed? Why not static_cast<>? >@@ -830,17 +831,18 @@ public: > > class nsMouseScrollEvent : public nsMouseEvent_base > { > public: > enum nsMouseScrollFlags { > kIsFullPage = 1 << 0, > kIsVertical = 1 << 1, > kIsHorizontal = 1 << 2, >- kIsPixels = 1 << 3 >+ kHasPixels = 1 << 3 // marks line scroll events that are provided as >+ // a fallback for pixel scroll events If you still explain what "fallback" means in this context...
Comment 54•16 years ago
|
||
Hello Firefox 3.1a2pre (checkout from today) fail to build with this patch and gcc 4.2. It works with gcc 4.0. This is the error: g++-4.2 -o nsEventStateManager.o -c -I../../../dist/include/system_wrappers -include /Users/simon/Desktop/mozilla/firefox3.1/src/config/gcc_hidden.h -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_COM_OBSOLETE -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES -DZLIB_INTERNAL -DOSTYPE=\"Darwin9.4.0\" -DOSARCH=Darwin -D_IMPL_NS_LAYOUT -I/Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/../../base/src -I/Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/../../html/base/src -I/Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/../../xul/content/src -I/Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/../../xml/content/src -I/Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/../../../dom/src/base -I/Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/../../../layout/generic -I/Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/../../../layout/xul/base/src -I/Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src -I. -I../../../dist/include/xpcom -I../../../dist/include/string -I../../../dist/include/dom -I../../../dist/include/js -I../../../dist/include/locale -I../../../dist/include/gfx -I../../../dist/include/thebes -I../../../dist/include/layout -I../../../dist/include/widget -I../../../dist/include/caps -I../../../dist/include/xpconnect -I../../../dist/include/webshell -I../../../dist/include/docshell -I../../../dist/include/pref -I../../../dist/include/view -I../../../dist/include/necko -I../../../dist/include/unicharutil -I../../../dist/include/imglib2 -I../../../dist/include/editor -I../../../dist/include -I../../../dist/include/content -I../../../dist/include/nspr -I../../../dist/sdk/include -I/usr/X11/include -fPIC -I/usr/X11/include -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-long-long -fno-strict-aliasing -fpascal-strings -fno-common -fshort-wchar -pthread -I/Developer/Headers/FlatCarbon -pipe -DNDEBUG -DTRIMMED -O2 -I/usr/X11/include -DMOZILLA_CLIENT -include ../../../mozilla-config.h -Wp,-MD,.deps/nsEventStateManager.pp /Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/nsEventStateManager.cpp /Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/nsEventStateManager.cpp: In member function βvirtual nsresult nsEventStateManager::PreHandleEvent(nsPresContext*, nsEvent*, nsIFrame*, nsEventStatus*, nsIView*)β: /Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/nsEventStateManager.cpp:1398: warning: enumeral mismatch in conditional expression: βnsIDOMNSUIEvent::<anonymous enum>β vs βnsIDOMNSUIEvent::<anonymous enum>β /Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/nsEventStateManager.cpp: In member function βvirtual nsresult nsEventStateManager::PostHandleEvent(nsPresContext*, nsEvent*, nsIFrame*, nsEventStatus*, nsIView*)β: /Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/nsEventStateManager.cpp:2624: error: jump to case label /Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/nsEventStateManager.cpp:2525: error: crosses initialization of βnsMouseScrollEvent* msEventβ /Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/nsEventStateManager.cpp:2625: error: jump to case label /Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/nsEventStateManager.cpp:2525: error: crosses initialization of βnsMouseScrollEvent* msEventβ /Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/nsEventStateManager.cpp:2631: error: jump to case label /Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/nsEventStateManager.cpp:2525: error: crosses initialization of βnsMouseScrollEvent* msEventβ /Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/nsEventStateManager.cpp:2634: error: jump to case label /Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/nsEventStateManager.cpp:2525: error: crosses initialization of βnsMouseScrollEvent* msEventβ /Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/nsEventStateManager.cpp:2736: error: jump to case label /Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/nsEventStateManager.cpp:2525: error: crosses initialization of βnsMouseScrollEvent* msEventβ make[6]: *** [nsEventStateManager.o] Error 1 make[5]: *** [libs] Error 2 make[4]: *** [libs] Error 2 make[3]: *** [libs_tier_gecko] Error 2 make[2]: *** [tier_gecko] Error 2 make[1]: *** [default] Error 2 make: *** [build] Error 2 My mozconfig only has ".$topsrcdir/browser/config/mozconfig" and the compilers are from Xcode 3.1.
Assignee | ||
Comment 56•16 years ago
|
||
Simons: Thanks, looks like there needs to be another set of curly braces around the stuff between "case NS_MOUSE_PIXEL_SCROLL:" and the corresponding "break;".
Comment 57•16 years ago
|
||
Comment on attachment 333929 [details] [diff] [review] updated patch >+ nsMouseScrollEvent event(*(reinterpret_cast<nsMouseScrollEvent*>(aEvent))); Actually, because this copies .target, .originalTarget and .currentTarget, those may point to wrong values when dispatching event. So at least set all those values to nsnull before dispatching
Assignee | ||
Comment 58•16 years ago
|
||
Attachment #333929 -
Attachment is obsolete: true
Attachment #334493 -
Flags: review?(Olli.Pettay)
Attachment #333929 -
Flags: review?(Olli.Pettay)
Comment 59•16 years ago
|
||
Comment on attachment 334493 [details] [diff] [review] updated patch Josh or someone should review the cocoa part, though I think it looks ok.
Attachment #334493 -
Flags: review?(Olli.Pettay) → review+
Assignee | ||
Updated•16 years ago
|
Attachment #334493 -
Flags: review?(smichaud)
Comment 60•16 years ago
|
||
I'll try to review the cocoa part of attachment 334493 [details] [diff] [review] by the end of the week.
Comment 63•16 years ago
|
||
(Reviewing Cocoa part of current patch, attachment 334493 [details] [diff] [review]) I've read through the patch several times, and am preparing to test with it. It mostly looks fine to me, but there's one thing that makes me very uncomfortable -- permanently loading your own, empty, NSAssertionHandler on the main thread. As best I can tell, this stops all Cocoa assertions (of any kind) from being logged (to the system console) -- which may make certain kinds of problems/crashes harder to debug. In my tests I'm going to try to come up with alternative ways to stop errors from being displayed when you send deviceDeltaX or deviceDeltaY to an NSEvent generated from a device that doesn't support them. You'll help me a lot, Markus, by telling me how to make these assertions happen (after removing the code that loads your empty NSAssertionHandler).
Assignee | ||
Comment 64•16 years ago
|
||
(In reply to comment #63) > It mostly looks fine to me, but there's one thing that makes me very > uncomfortable -- permanently loading your own, empty, > NSAssertionHandler on the main thread. As best I can tell, this stops > all Cocoa assertions (of any kind) from being logged (to the system > console) -- which may make certain kinds of problems/crashes harder to > debug. Could be. Have there been crashes / problems in the past where an assertion was helpful? > You'll help me a lot, Markus, by telling me how to make these > assertions happen (after removing the code that loads your empty > NSAssertionHandler). I saw them with a Logitech MX Revolution on a Macbook with the default Logitech driver (Logitech Control Center) when using the mouse wheel. Sorry that I don't have easier STR... :(
Comment 65•16 years ago
|
||
(In reply to comment #64) > Have there been crashes / problems in the past where an assertion > was helpful? I don't remember any, off the top of my head. But, on general principles, I still think your empty NSAssertionHandler is a bad idea. > I saw them with a Logitech MX Revolution on a Macbook with the > default Logitech driver (Logitech Control Center) when using the > mouse wheel. Sorry that I don't have easier STR... :( I've got an idea how to avoid using the empty NSAssertionHandler ... but it looks like I'm going to have to ask you to test it. If I can get it to work on my side, I'll post my code here for you to test (hopefully in a few hours).
Comment 66•16 years ago
|
||
I happen to have one of those mice here, although I wasn't planning on installing that horrid excuse for a driver they call "Logitech Control Center", so I may be able to help test things too.
Hardware: Macintosh → All
Comment 67•16 years ago
|
||
My idea worked! Here's a version of Markus' latest patch (updated to current trunk code) that gets rid of the replacement NSAssertionHandler and the try/catch block around calls to [NSEvent deviceDeltaX] and [NSEvent deviceDeltaY]. Markus, Chris and others, please try it out. Turns out I _was_ able to test it (and to see that it gets rid of the assertion and NSInternalInconsistency exception). I have both a Microsoft mouse (with a scroll wheel) and a Mighty Mouse. Without the NSAssertionHandler and try/catch block (and without my workaround), using the Microsoft mouse's scroll wheel triggers the assertion and exception (and crash). My workaround gets rid of all this. I'll continue my testing (and review) tomorrow.
Attachment #334493 -
Attachment is obsolete: true
Attachment #334493 -
Flags: review?(smichaud)
Assignee | ||
Comment 68•16 years ago
|
||
Nice! Maybe add a comment along the lines of "For these events, the event kind will be kEventMouseWheelMoved instead of kEventMouseScroll."? I haven't tested your patch yet, but it sounds pretty promising.
Comment 69•16 years ago
|
||
> Maybe add a comment along the lines of "For these events, the event
> kind will be kEventMouseWheelMoved instead of kEventMouseScroll."?
Makes sense. Feel free to do this yourself, in the next update of
your patch.
Comment 70•16 years ago
|
||
Markus, please tell me how I should go about testing pixel scrolling (as opposed to line scrolling). What pages to visit (or parts of the browser chrome to hover over), and what to do there. I've got a Mighty Mouse and a MacBook Pro with a trackpad.
Assignee | ||
Comment 71•16 years ago
|
||
0. Make sure that "smooth scrolling" is disabled: Preferences Β» Advanced Β» General Β» uncheck "Use smooth scrolling" 1. Go to any page that needs a scrollbar, e.g. this bug. 2. Hover your mouse over any scrollable part of the page, e.g. this comment. 3. Scroll very slowly using your trackpad. 4. Closely observe how the scroll position responds to your fingers' movement. Results without pixel scrolling: "Falling down the stairs" The first pixels of your scroll gesture don't result in any scrolling. When a certain threshold is crossed, the page's scroll position jumps at least 10px. Results with pixel scrolling: "Taking the elevator" Every slight movement of your fingers is accurately reflected in the scroll position. No jumping, but smooth, pixel-wise motions. When testing with the Mighty Mouse, the difference is much more subtle: With pixel scrolling, the first "tick" of a wheel scroll gesture moves the scroll position by only one pixel, subsequent "ticks" move it by at least 10px. Without pixel scrolling, there's no difference between the first tick of a scroll gesture and the rest of the ticks.
Comment 72•16 years ago
|
||
Comment on attachment 335578 [details] [diff] [review] Get rid of NSAssertionHandler Thanks, Markus, for your detailed description of how to test pixel scrolling. I found it very useful, and so will others who want to do manual tests (like QA). It should also provide clues how to write an automated test. My tests went as you predicted, and I had no problems. The improved responsiveness of two-fingered scrolling (on my MacBook Pro's trackpad) was really quite dramatic. I also went over the Cocoa code (in your patch) another time, and found no problems. I do think it makes sense, though, to follow your own suggestion from comment #68, and expand the comment, in [ChildView scrollWheel:...], above the following: if (carbonEventKind != kEventMouseScroll) checkPixels = PR_FALSE;
Attachment #335578 -
Flags: review+
Comment 73•16 years ago
|
||
(Following up comment #72) Something I forgot to mention: I tested both on OS X 10.5.4 (with my Mighty Mouse) and on OS X 10.4.11 (with my MacBook Pro's trackpad).
Comment 74•16 years ago
|
||
(Following up comment #72) And another thing: As best I can tell, bug 347626 hasn't been re-regressed by this patch.
Assignee | ||
Comment 75•16 years ago
|
||
Comment on attachment 335578 [details] [diff] [review] Get rid of NSAssertionHandler I'll work on tests soon.
Attachment #335578 -
Flags: superreview?(roc)
I think we should have some documentation somewhere describing what the strategy is here, and how events flow in the common cases ... it's a bit tricky. + kHasPixels = 1 << 3 // Marks line scroll events that are provided as + // a fallback for pixel scroll events. + // These scroll events are used by things that can't + // be scrolled pixel-wise, like trees. You should + // ignore them when processing pixel scroll events + // to avoid double-processing the same scroll gesture. I think this comment should say that when kHasPixels is set, we promise that there will be a follow-up event which contains pixel scrolling information. Instead of mLastLineScrollSucceededX/Y, I'd call it mLastLineScrollConsumedX/Y. It's ambiguous whether "succeeded" means the event was consumed or it wasn't. + // Reset scroll event consumption. + mLastLineScrollSucceededX = mLastLineScrollSucceededY = PR_TRUE; Why is this under NS_LOSTFOCUS? Why is it needed at all? + // We shouldn't scroll lines when the event is a replacement event + // for pixel scroll events. I'd say, "we shouldn't scroll lines when a pixel scroll event will follow". + // There are no pixel scroll events for this line scroll event. + // We must create and send them now. "No generated pixel scroll event will follow. Create and send a pixel-scroll DOM event now." I'm not sure why we're doing that, though. Is this to make it easier to write client code, so that you can listen for pixel-scroll events and get them even if the platform doesn't support it? Also, I know we haven't done this before, but should we call the event MozDOMPixelScroll? This is really something that's always going to be only for us...
Comment 77•16 years ago
|
||
(In reply to comment #76) > Also, I know we haven't done this before, but should we call the event > MozDOMPixelScroll? This is really something that's always going to be only for > us... Maybe, though, we have been adding new events using DOM prefix. DOMContentLoaded, DOMMouseScroll, etc.
True, but those were added long ago and maybe were a mistake.
Assignee | ||
Comment 79•16 years ago
|
||
This patch contains lots of changes: - tests! - documented the event flow in nsGUIEvent.h - renamed DOMMousePixelScroll to MozMousePixelScroll - renamed mLastLineScrollSucceededX/Y to mLastLineScrollConsumedX/Y - updated comments (Thanks for rewriting them, roc!) - removed the NS_LOSTFOCUS reset: I think it's unnecessary, and smaug agreed to remove it when I told him that NS_LOSTFOCUS fires each time the map in Google Maps is clicked. - SendDOMPixelScrollEvent changes: - no longer fails if the mouse is over a text node (looks for the next non-text parent element) - no longer fails if the mouse isn't in a scrollable view - no longer uses the implicit copy constructor trick - MOZ_COUNT_CTOR(nsEvent) ended up not being called, so the leak stats were messed up - initializes the MozMousePixelScroll event's status with the DOMMouseScroll event's status - preventDefault on a DOMMouseScroll event without hasPixels no longer prevents the pixel scroll DOM event from being sent - now it only cancels its default handling - nsChildView changes: - removed nsIPrefBranch memory leak: Turns out that using a nsCOMPtr in an Objective C field doesn't go down too well because its destructor is never called. - defined mozkEventMouseScroll instead of kEventMouseScroll because of symbol conflicts when building on 10.5 without using the 10.4 SDK. According to Stuart Morgan, there's no simple future-proof way to #ifdef around it. (It would break on some combination of build and runtime SDK) (In reply to comment #76) > + // Reset scroll event consumption. > + mLastLineScrollSucceededX = mLastLineScrollSucceededY = PR_TRUE; > > Why is this under NS_LOSTFOCUS? Why is it needed at all? The idea was that the mouse might be outside of a event-eating box after switching to another tab and back, so we shouldn't drop the pixels that come before the first line scroll. But it's not worth the trouble. > "No generated pixel scroll event will follow. Create and send a pixel-scroll > DOM event now." > > I'm not sure why we're doing that, though. Is this to make it easier to write > client code, so that you can listen for pixel-scroll events and get them even > if the platform doesn't support it? Right. And, unlike the nsMouseScrollEvent, DOMMouseScroll events don't have any kHasPixels flag, so you'd have no way of telling which events to ignore and which not. So the strategy is to ignore all DOMMouseScroll events and only process MozMousePixelScroll events. > Also, I know we haven't done this before, but should we call the event > MozDOMPixelScroll? This is really something that's always going to be only for > us... On IRC, smaug and I agreed on MozMousePixelScroll. Are you OK with that?
Attachment #335578 -
Attachment is obsolete: true
Attachment #338307 -
Flags: superreview?(roc)
Attachment #338307 -
Flags: review?(Olli.Pettay)
Attachment #335578 -
Flags: superreview?(roc)
Comment 80•16 years ago
|
||
Comment on attachment 338307 [details] [diff] [review] rev6, with tests >+ PRBool isTrusted = !!(aEvent->flags & NS_EVENT_FLAG_TRUSTED); >+ nsMouseScrollEvent event(isTrusted, NS_MOUSE_PIXEL_SCROLL, nsnull); >+ event.refPoint = aEvent->refPoint; >+ event.widget = aEvent->widget; >+ event.time = aEvent->time; >+ event.nativeMsg = aEvent->nativeMsg; Hmm, I don't think we want to copy nativeMsg, although in practice in shouldn't make any difference. >+ void SendDOMPixelScrollEvent(nsIFrame* aTargetFrame, >+ nsMouseScrollEvent* aEvent, >+ nsPresContext* aPresContext, >+ nsEventStatus* aStatus); Nit, maybe this could be called SendPixelScrollEvent >+ * This event flow model satisfies several requirements: >+ * - DOMMouseScroll handlers don't need to know about the existence of pixel >+ * scrolling. >+ * - preventDefault on a DOMMouseScroll event results in no scrolling. You could mention also MozMousePixelScroll here.
Attachment #338307 -
Flags: review?(Olli.Pettay) → review+
Comment on attachment 338307 [details] [diff] [review] rev6, with tests + PRBool isTrusted = !!(aEvent->flags & NS_EVENT_FLAG_TRUSTED); In most places I think we use != 0. Either way's fine. +/* Mouse Scroll Events: Line Scrolling, Pixel Scrolling and Common Event Flows Nice! I think you should add something here to mention that if a DOMMouseScroll event is preventDefaulted, then we suppress the associated following MozMousePixelScroll events.
Attachment #338307 -
Flags: superreview?(roc) → superreview+
Assignee | ||
Comment 82•16 years ago
|
||
pushed: http://hg.mozilla.org/mozilla-central/rev/93f23e3efbb4 (In reply to comment #81) > I think you should add something here to mention that if a DOMMouseScroll > event is preventDefaulted, then we suppress the associated following > MozMousePixelScroll events. I added a comment that clarifies that they're sent but don't cause any scrolling.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b1
Comment 83•16 years ago
|
||
Is there any way this could be dropped into 1.9.0? That way Camino 2.0 could pick it up, which would be a big win IMO.
Updated•6 years ago
|
Component: Event Handling → User events and focus handling
You need to log in
before you can comment on or make changes to this bug.
Description
•