Closed Bug 315727 Opened 19 years ago Closed 15 years ago

Request Firefox respond to Windows scroll messages so pages can be scroll captured

Categories

(Core :: Widget: Win32, enhancement)

Product:
Core
Component:
Widget: Win32
Platform:
x86
Windows XP
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.3a3

People

(Reporter: dave.bugzilla, Assigned: masayuki)

References

Details

(Whiteboard: parity-IE)

Attachments

(1 file, 6 obsolete files)

Patch v2.1.2
29.12 KB, patch
roc
: superreview+
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6 Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6 On behalf of millions of people who use screen capture tools such as SnagIt and CaptureWiz, I’d like to request that Firefox follow Windows page scrolling conventions so its display area can be “scroll captured”. Scroll capture is a new and very popular feature in screen capture programs that enables users to capture areas larger than their screen. The tools capture an entire page or list, automatically scrolling vertically and/or horizontally as required. The feature works great in most programs, including Internet Explorer, but not Firefox because it doesn’t respond to the conventional Windows messages WM_VSCROLL or WM_HSCROLL. This makes it very difficult for capture programs to scroll pages in Firefox vertically, and impossible to scroll pages horizontally. I believe the fix would be a fairly simple addition to the message loop, something like this: switch Message . . . case WM_VSCROLL: if (wParam == SB_LINEDOWN) then ScrollDownOneLine; else if (wParam == SB_LINEUP) then ScrollUpOneLine; case WM_HSCROLL: if (wParam == SB_LINERIGHT) then ScrollRight; else if (wParam == SB_LINELEFT) then ScrollLeft; . . . As it is now, I don’t see any way for external programs to scroll Firefox horizontally. So, the right side of web pages can't be automatically scroll captured. To get an idea what I’m talking about, here’s a Flash demo that explains scroll capture: http://www.PixelMetrics.com/CapWizPro/DemoCwp.htm And, here’s where you can try the feature in a 30-day free trial: http://pixelmetrics.com/CapWizPro/Download.htm I’m the author of CaptureWizPro and I can be contacted at dave@pixlemtrics.com. Thanks! Dave Eisler http://www.PixelMetrics.com Reproducible: Always Steps to Reproduce: Download a free-trial of a screen capture program, such as SnagIt or CaptureWizPro, then try to use the scroll capture tool to automatically capture an entire web page. CaptureWizPro free-trial download link: http://pixelmetrics.com/CapWizPro/Download.htm If you'll email me back, I can give you a free password! Actual Results: The right side of web pages, the portion off-screen, can not be captured because Firefox does not scroll right in response to Windows WM_HSCROLL messages.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → win32
Component: OS Integration → Widget: Win32
Product: Firefox → Core
QA Contact: os.integration → ian
Version: unspecified → Trunk
Firefox needs this feature badly. Internet Explorer allows scroll capture, but Firefox does not.
I'll try to fix this in 1.9 a2 or b1 stage. But if you can do, take this free :)
Assignee: win32 → masayuki
If there's any way I can help, please let me know. Sorry, I don't know anything about the system for fixing bugs in Firefox or "1.9 a2 or b1 stage". Dave (In reply to comment #3) > I'll try to fix this in 1.9 a2 or b1 stage. > But if you can do, take this free :) >
I agree - FF scrolling is sub-par compared to IE, esp when using CaptureWizPro and SnagIt Mozilla, please help!
I have an HP mouse driver which sends WM_HSCROLL and WM_VSCROLL events when the mouse wheel is scrolled. This prevents me (and presumably many other users) from scrolling Firefox using the mouse wheel.
Sorry for the delay. I'll work on this. I have a question, on IE (or some other applications), can you scroll subframes (iframes or overflow: scroll;) by the mouse wheel when you move the mouse cursor over them? Or does the wheel always scroll body element?
It scrolls the iframe when the mouse is over it.
Ugh... This bug is difficult... There are two problems by this bug. 1. Cannot use capture software We should implement nsNativeScrollEvent in nsGUIEvent.h and we should dispatch it. Then nsEventStateManager(ESM) can handle the event. 2. Cannot scroll by mouse wheel I think that we can implement this easily. We can generate wheel events in nsWindow. However, if we do so, we meet very difficult issue. We don't create native windows to each scrollable view. Therefore, we need to honor the mouse cursor position. So, it can break the behavior of 1st (and also this may generate wheel scroll DOM events, is it correct??). Clearly, the 2nd issue is a problem of such mouse drivers, because they should use WM_MOUSEWHEEL and WM_MOUSEHWHEEL instead of WM_VSCROLL and WM_HSCROLL. So, we should prefer the 1st behavior. I think that the best way for this conflict is that we create a pref. # the better default value of the new pref might be 2nd. Because the users who have the 2nd trouble can be beginner of computers. But I hate this idea... Ere and Kimura-san, do you have idea?
QA Contact: ian → win32
Whiteboard: parity-IE
(In reply to comment #8) > It scrolls the iframe when the mouse is over it. Thank you for the information. That means IE checks the mouse cursor position, because IE doesn't create native windows for each iframes. So, even if we don't create a new pref of comment 9, we should be able to fix this bug. However, it may be difficult, because we cannot look the source code :-p
Angus: Can you zoom-in/zoom-out on IE by Ctrl + mouse wheel?
Alps pointing device for VAIO sends WM_HSCROLL for horizontal scrolling. However, it sends the message only when the target window has a native horizontal scrollbar or several window class's windows (e.g., IE always doesn't have native scrollbars, but the driver always sends the WM_HSCROLL messages). I guess that HP mouse driver may not send WM_VSCROLL and WM_HSCROLL to Gecko too. Angus, can you check it? If that doesn't send the messages to us and users cannot customize it, we don't need to care the mouse wheel issue. Because driver sent WM_MOUSEWHEEL and WM_MOUSEHWHEEL should send to us in new driver ;-)
> Because driver sent WM_MOUSEWHEEL and WM_MOUSEHWHEEL should send to us in new driver ;-) I meant: WM_MOUSEWHEEL and WM_MOUSEHWHEEL should be sent to us in new driver ;-)
The way I originally discovered that it sends WM_VSCROLL is by writing a test program which creates a window and prints the messages it receives. So I guess it doesn't matter what the window is or whether it has a scrollbar.
How about selecting the scroll target by focus? So when WM_VSCROLL or WM_HSCROLL is received, scroll the frame that has focus. Optionally, if a scrollable frame is not focused, scroll the frame under mouse cursor if it's feasible to do that. But starting with the focused frame might be enough.
Blocks: 507222
There are test builds of bug 507222: https://build.mozilla.org/tryserver-builds/masayuki@d-toybox.com-bug507222/ I want the feedback from the hp mouse users who have this problem.
I'd be happy to test if the latest version of Firefox can be scroll captured, if that would help. But how, are there instructions? When I click to download the new version, I see 4 different files with unfamiliar extensions plus 3 directories containing more strange things. Sorry, I only know Windows. Dave Eisler, author of CaptureWizPro
Dave: The test build's implementation is only for the mouse drivers. The WM_*SCROLL should not use wheel events in gecko. E.g., even if the Ctrl key is pressed, WM_*SCROLL should not change the zoom level. I want to know whether the test build can fix the hp mouse driver issue of comment 6. We're looking for the way for distinguishing the WM_*SCROLL messages coming from tools or mouse drivers. If your tool doesn't work with the test build, we are happy because we find the way. The Windows test builds are: *-win32.zip ... zipped file of the installed image. install/sea/*-win32.installer.exe ... installer version, you need to install.
Thank you for explaining, Masayuki. I see in comment 6 that an HP mouse driver sends WM_HSCROLL and WM_VSCROLL events when the mouse wheel is scrolled. It sounds to me like the HP mouse driver was written incorrectly; it should be sending WM_MOUSEWHEEL messages instead. Wouldn’t it make more sense to contact HP and have them fix their driver? Sorry, but it seems a mistake to permanently and unnecessarily complicate Firefox just so it works with one defective mouse driver. I suggest keeping things simple and treat messages from mouse drivers exactly the SAME as messages from “scroll programs”. Whether the message comes from a scroll program or mouse driver, the meaning of the message is the same. The intended response is the same. But, I admit I’m no Firefox expert.
Here's a wild possibility as to why the HP mouse driver is sending WM_*SCROLL messages; perhaps the mouse driver DETECTS that Firefox isn't responding to WM_MOUSEWHEEL messages, so it switches to "PLAN B" and tries sending WM_*SCROLL messages instead. Just a thought.
(In reply to comment #19) > it should be sending WM_MOUSEWHEEL messages instead. Wouldn’t it make more > sense to contact HP and have them fix their driver? Of course, you're right. > I suggest keeping things simple and treat messages from mouse drivers exactly > the SAME as messages from “scroll programs”. Whether the message comes from a > scroll program or mouse driver, the meaning of the message is the same. The > intended response is the same. But, I admit I’m no Firefox expert. As you know, we're not using the native windows for all widgets, so, they are fake. Such way was selected by some reasons, and I believe that the choice is correct. Therefore, we should emulate the native behaviors as far as possible. At this issue, the main problem is WM_*SCROLL should (or shouldn't) honor the mouse cursor position. If gecko never honors it, the mouse driver users are not happy. But if gecko always honors it, the mouse driver users are happy but your tool users are not so.
I agree with you Dave, we shouldn't have to work around mouse drivers that do bizarre things, but unfortunately if we ship 3.6 in a way that doesn't scroll on all of the Thinkpads out there those users won't care that it's not really our fault. All they will notice is that Firefox doesn't scroll anymore. I suppose it's the "Raymond Chen" philosophy to some extent. http://blogs.msdn.com/oldnewthing/archive/2003/12/23/45481.aspx Amusingly, the second situation is somewhat similar to ours.
Well, thinking more about this problem, I think it makes no difference that the HP mouse driver sends WM_*SCROLL instead of WM_MOUSEWHEEL. Either way, Firefox should simply scroll the appropriate window. Everyone will be happy. WM_MOUSEWHEEL -> Firefox window scrolls -> everyone is happy WM_*SCROLL -> Firefox window scrolls -> everyone is happy Masayuki said "if gecko (I assume gecko means Firefox) honors it (I assume this means gecko scrolls the window in response to WM_*SCROLL), the mouse driver users are happy but your tool users are not so." Actually, everyone will be happy. Scroll tools DO want Firefox to scroll when sent WM_*SCROLL. I suggest Firefox scroll the scrolling area that has the focus. Scrolling the area under the mouse pointer is hazardous if the user is moving the mouse and it's incompatible with scroll programs.
If no scrolling area has the focus, scroll the entire page.
Status: NEW → ASSIGNED
Attached patch Patch v1.0 (obsolete) — — Splinter Review
I didn't test this patch. I'll create automated tests for this.
It seems that the scrollable view which has the focused node is scrolled, that doesn't make sense with any tools because the view might be outside of the parent view (e.g., hidden by the scroll of the parent (or ancestor) view). And my patch v1.0 scrolls the scrollable view which is ancestor of the focused element and which is nearest of the root view. I thought that this is good behavior for some web pages which have non-scrollable root views but some contents are scrollable. However, such contents cannot be captured because the external applications cannot know the rect of the scrollable view. Therefore, I suggest that only the root scrollable view of the root document is scrollable by WM_*SCROLL. How about this idea? Note that we should add a mode which generates mouse wheel events by WM_*SCROLL for the some wrong mouse drivers. the new scrolling mechanism cannot generate the DOM mouse wheel events, it might make some users unhappy. Such users should use the new mode.
Attachment #400439 - Attachment is obsolete: true
Hi Masayuki, I will try to comment on your comments as the author of CaptureWiz, one of the programs that tries to "scroll capture" Firefox. First, I think it is a great, simple, idea to scroll the "scrollable view" that has the focus. Here are the special cases I see: Case 1 - Both the page and an object on that page are scrollable. By “page”, I mean the web page. The page is scrollable because it is larger than Firefox’s current “client area”. By “object”, I mean an object with a scrolling area, such as a text box, list, or drop down combo box. In this case, the page or the object has the focus. In either case, I see no problem scrolling the item with the focus. Generally, the focus is on the page when the page first opens. The focus switches to an object when a users clicks on that object. Case 2 - The object with the focus is outside the client area. I see no harm in scrolling the object with the focus, even if it’s hidden. Since the object is outside Windows’s clipping area, the drawing actions will be ignored by Windows and nothing will happen. This should be rare. The only way I see this happening is if the user presses TAB to shift the focus to an out-of-view object. Note that if a user clicks on a scrolling object, then clicks or drags the page’s scroll bar, the focus will shift to the page. Case 3 – An object is obscured by another object. This should never happen. It would be poor page design. Of course, some objects, like combo boxes, open up over other objects, but the focus will always be on the top object, so I don’t see any problem. I suppose Firefox could have “containers” that contain other objects and one container might obscure another, but still, the focus should never go to an object obscured by another. Generally, an object must be visible to get the focus. And also, at least in Windows, there’s no harm in scrolling an obscured object – the drawing functions know when an object is obscured and ignore the calls. Note that scroll programs can sometimes find the edges of scrolling areas optically, though it’s slow and less reliable. They do NOT require that scrolling areas be a Windows window, I mean one created by calling CreateWindow. It would be a great loss of functionality if only the full page could be scroll captured. It is very common that people want to scroll capture objects on pages, such as text boxes, lists, and drop down combo boxs. It would be a big disadvantage if users must select a “mode”, if you mean a Firefox setting, in order to scroll capture. Nobody will guess that such a setting exists. Few people will read the Firefox help file or look for a FAQ when the scroll capture function fails.
More comments from CaptureWiz author. I would discourage you from having Firefox try to guess, based on what ojects are in view, which one to scroll capture. I think that would be unreliable and the user looses control - he'd have no way to select a specific object to capture. We tell our users to click on the object they want to scroll capture. Users expect to do this.
I am making switch from explorer to fire fox. I have used the capture wiz for some time. It is a very good program. Works in explorer but not fire fox. That is the only thing I have found wrong with fire fox.
Attached patch Patch v1.0 (obsolete) — — Splinter Review
Sorry for the delay. This patch implements the handling of WM_*SCROLL. If mousewheel.emulate_at_wm_scroll is false (default), WM_*SCROLL always scroll the root scrollable frame which is body of HTML document. Otherwise, WM_*SCROLL are processed as WM_MOUSEWHEEL or WM_MOUSEHWHEEL. So, if some users want to scroll a sub-frame from another application, they can do it by this setting and moving the cursor onto the target. This patch doesn't support SB_ENDSCROLL, SB_THUMBPOSITION and SB_THUMBTRACK. I'll build a test build on tryserver.
I tried but same Using Logitech wire less mouse and keyboard Neither snagit nor capture wiz pro works(scrolling)
Michael: Sorry, I'm not sure what you mean. All of Logitech wireless mouse, keyboard, snagit and capture wiz pro didn't work?? The patch tries to support WM_*SCROLL messages for some capture software in default setting. If you hope that the patch fixes to support some mouse driver which sends the messages, you should change "mousewheel.emulate_at_wm_scroll" to true in about:config (needs reboot Fx). If the capture software uses SB_ENDSCROLL, SB_THUMBPOSITION and SB_THUMBTRACK, we cannot fix this request easily.
(In reply to comment #28) > We tell our users to click on the object they want to scroll capture. Users > expect to do this. Is the function really needed? My last patch doesn't support such feature. It scrolls only root scrollable view of root of the content area.
Thank you Michael for testing that patch! I'm Dave, the author of CaptureWiz and I'll do anything I can to help you guys fix this problem. CaptureWiz does not use SB_THUMBPOSITION or SB_THUMBTRACK. It does, however, send SB_ENDSCROLL after most other mouse commands, such as SB_TOP.
Hi Masayuki, In response to comment 36, I created this simple page to show why this function is valuable: http://pixelmetrics.com/Temp/TwoScrollingareas.html On the page you can see two scrolling areas. Using Internet Explorer, users can scroll capture the entire text of either area. If that doesn't answer your question, please email me back.
https://build.mozilla.org/tryserver-builds/masayuki@d-toybox.com-try-ac9319eea431/ O.K. This build uses last clicked scrollable view. Would you test this?
Sounds great! I'd love to test the latest build, but sorry, I'm a Windows guy and don't know anything about compiling Firefox. When I click to download the new version, I see a bunch of files with unfamiliar extensions plus some directories containing more strange things. Are there instructions? Is there an EXE file in there somewhere?
That is trunk build (currently 3.7a2pre). The easiest and simplest testing way is: 1. download the zip version (https://build.mozilla.org/tryserver-builds/masayuki@d-toybox.com-try-ac9319eea431/try-ac9319eea431-win32.zip) 2. unzip it to somewhere. 3. create a shortcut file to firefox.exe. 4. change the target of the shortcut file as ...\firefox.exe" -p (i.e., append " -p") 5. execute the shortcut, then, you can see Profile Manager. (If you have launched another firefox instance, you must quit from it.) 6. create a new profile for protecting the your main profile. (at only first time). 7. select the new profile and launch. 8. if you want to install some add-ons, you should install "Nightly Tester Tools" or something, first. Then, you can ignore the version checking of the add-ons (but of course, some add-ons may not work fine).
Hooray, it works! Even scrolling to the right works, good job! I tested it on XP, later I'll test Windows 7. Thank you Masayuki, on behalf of thousands of screen capture software users everywhere! Dave Eisler
Thank you. It does work. The browser I have now is called minefield version3.7a2pre. Program folder shows fire fox and does not list mine field. Tried Capture wiz pro. It is working. Sorry not a programmer. This build is really a lot better in appearance and speed. I expanded the download in a different folder and the run. Can I get back to one fire fox. Regardless of name. If I can I would prefer this version. No Hurry. This working. Thanks. Capture wiz is working and that was the main thing. Oh, It took so long as I was not expecting the minefield thing and took some time thinking before proceeding. Glad I did.
(In reply to comment #42) > This build is > really a lot better in appearance and speed. I expanded the download in a > different folder and the run. Can I get back to one fire fox. Regardless of > name. > If I can I would prefer this version. No Hurry. This working. Thanks. Capture > wiz is working and that was the main thing. I don't recommend that you use the trunk build. It is pre alpha version, so, it's not tested enough.
Attached patch Patch v2.0 (obsolete) — — Splinter Review
Adding an event whose name is NS_CONTENT_COMMAND_SCROLL to content command events. nsWindow of Windows fires the event when WM_VSCROLL or WM_HSCROLL comes. If |mousewheel.emulate_at_wm_scroll| is true, nsWindow dispatches mouse wheel events instead of new events. That improves the mouse wheel handling with some mouse drivers which send the WM_*SCROLL. ESM handling the events at DoContentCommandScrollEvent(). That uses a nearest scrollable frame from current DOM selection. It uses same method for the scrolling by keyboard except that it chooses the scrollable view to either direction. The reason is that the capturing software may try to scroll both directions, then, they shouldn't scroll different views for vertical and horizontal. I think that this could help to improve cocoa's NSResponder implementation.
Attachment #424216 - Attachment is obsolete: true
Attachment #429787 - Flags: review?(emaijala)
Attachment #429787 - Flags: review?(Olli.Pettay)
Comment on attachment 429787 [details] [diff] [review] Patch v2.0 (I'm not sure if Ere is available for the review, :jimm should be) >+ /* >+ * Get scrollable frame from current DOM selection, this returns a nearest >+ * frame that is scrollable with overflow:scroll or overflow:auto in >+ * some direction when aDirection is eEither. Otherwise, this returns a >+ * nearest frame that is scrollable in the specified direction. >+ */ >+ enum ScrollDirection { eHorizontal, eVertical, eEither }; >+ nsIScrollableFrame* GetFrameToScrollAsScrollable(ScrollDirection aDirection); The comment should mention that if there is a focused content, the nearest scrollable frame related to that is returned. >+// If your mouse drive sends WM_*SCROLL messages when you turn your mouse wheel, >+// set this to true. Then, gecko processes them as mouse wheel messages. >+pref("mousewheel.emulate_at_wm_scroll", false); >+ Very few people will know about this pref, but I guess we just can't support broken drivers by default. Though, I wonder if we should support broken drivers at all. Or is there any good reason for a driver to use WM_(H)SCROLL? Do we know which drivers or how many drivers use wrong messages? Jimm, comments? > class nsContentCommandEvent : public nsGUIEvent > { > public: > nsContentCommandEvent(PRBool aIsTrusted, PRUint32 aMsg, nsIWidget *aWidget, > PRBool aOnlyEnabledCheck = PR_FALSE) : > nsGUIEvent(aIsTrusted, aMsg, aWidget, NS_CONTENT_COMMAND_EVENT), >+ mFlags(0), mValue(0), > mOnlyEnabledCheck(PRPackedBool(aOnlyEnabledCheck)), > mSucceeded(PR_FALSE), mIsEnabled(PR_FALSE) > { > } > > nsCOMPtr<nsITransferable> mTransferable; // [in] >+ // mFlags is used by: >+ // NS_CONTENT_COMMAND_SCROLL >+ enum { >+ eCmdScroll_IsHorizontal = 0x1, >+ eCmdScroll_IsVertical = 0x2, >+ eCmdScroll_DirectionMask = 0xF, >+ eCmdScroll_LineScroll = 0x10, >+ eCmdScroll_PageScroll = 0x20, >+ eCmdScroll_WholeScroll = 0x40, >+ eCmdScroll_UnitMask = 0xF0, >+ eCmdScroll_NoDefer = 0x100 >+ }; >+ PRInt32 mFlags; // [in] Please split variable. nsEvent has already flags. You could have separate enum for direction and for scrolling type, and then also a boolean flag for NoDefer. Though, seems like the NoDefer flag is always set. Do we need it at all?
Comment on attachment 429787 [details] [diff] [review] Patch v2.0 Perhaps better to add support for scroll messages here, and for strange mouse drivers in a different bug? Anyway, because of few nits in the patch, r-
Attachment #429787 - Flags: review?(Olli.Pettay) → review-
(In reply to comment #45) > >+// If your mouse drive sends WM_*SCROLL messages when you turn your mouse wheel, > >+// set this to true. Then, gecko processes them as mouse wheel messages. > >+pref("mousewheel.emulate_at_wm_scroll", false); > >+ > Very few people will know about this pref, but I guess we just can't > support broken drivers by default. > Though, I wonder if we should support broken drivers at all. > Or is there any good reason for a driver to use WM_(H)SCROLL? > Do we know which drivers or how many drivers use wrong messages? Especially, WM_HSCROLL should be used by some old drivers because there were no WM_MOUSEHWHEEL message on XP and earlier. And also there are many duplicated bugs in bug 20618, so, I guess that there sill be some mouse drivers which are using WM_*SCROLL.
(In reply to comment #47) > Especially, WM_HSCROLL should be used by some old drivers because there were no > WM_MOUSEHWHEEL message on XP and earlier. Oh, interesting. But still, if gecko browsers don't support those drivers by default, I doubt many users will change the pref. Is there any reasonable way to detect when WM_(H)SCROLL is coming from a mouse driver? I guess no.
(In reply to comment #48) > Is there any reasonable way to detect when WM_(H)SCROLL is coming from a > mouse driver? I guess no. Yeah, I have no idea too.
Comment on attachment 429787 [details] [diff] [review] Patch v2.0 What smaug said and you'd be better off asking someone else as I'm pretty much absent...
Attachment #429787 - Flags: review?(emaijala)
I have a simple idea. When someone rolls the mousewheel on an old mouse, and its driver sends a WM_Scroll message to Firefox, how about scrolling the window? Scroll the Firefox window no mater who sends the WM_Scroll message? Isn't that the simplest solution?
(In reply to comment #51) > I have a simple idea. When someone rolls the mousewheel on an old mouse, and > its driver sends a WM_Scroll message to Firefox, how about scrolling the > window? Scroll the Firefox window no mater who sends the WM_Scroll message? > Isn't that the simplest solution? We need to know who sent the message... I think that if the message was posted, the sender might be non-captured utilities. However, some other utilities which are not related to the mouse cursor position could post the messages too. I guess that we can handle WM_INPUT message. http://msdn.microsoft.com/en-us/library/ms645536%28VS.85%29.aspx When the mouse device was operated but comes WM_*SCROLL, we can assume that the WM_*SCROLL comes from the mouse driver. But I need more works (experiments) because I've never done such raw programing. And of course, such complex code should be in another bug. The issue is whether we should include the pref in the patch or not. I think that that is an accessibility problem, so, even if the hidden pref isn't useful, it should be there because the code isn't complex.
Attached patch Patch v2.1 (obsolete) — — Splinter Review
Attachment #429787 - Attachment is obsolete: true
Attachment #430246 - Flags: review?(jmathies)
Attachment #430246 - Flags: review?(Olli.Pettay)
Doesn't EVERY program that sends WM_Scroll to Firefox want Firefox to scroll? I guess you have your reasons, but sorry, to me it feels dangerous and complicated to change Firefox's behavior based on the source of a message.
(In reply to comment #54) > Doesn't EVERY program that sends WM_Scroll to Firefox want Firefox to scroll? Yes. We're discussing about whether the scrolling target should be decided by focus or mouse cursor. And also should fire whether DOMMouseScroll event or noting. WM_*SCROLL messages don't fire DOMMouseScroll event, so, just scrolls the focused content. However, if the messages are sent by mouse driver, firing DOMMouseScroll and scrolling under the cursor is better for the users.
(In reply to comment #55) > (In reply to comment #54) > > Doesn't EVERY program that sends WM_Scroll to Firefox want Firefox to scroll? > > Yes. We're discussing about whether the scrolling target should be decided by > focus or mouse cursor. And also should fire whether DOMMouseScroll event or > noting. WM_*SCROLL messages don't fire DOMMouseScroll event, so, just scrolls > the focused content. However, if the messages are sent by mouse driver, firing > DOMMouseScroll and scrolling under the cursor is better for the users. Am I following the comments correctly - Fx receives WM_*SCROLL, with the patch and the pref set to false, we scroll the content with focus. This address the screen capture program issues. With the pref set to true, we assume WM_*SCROLL is being sent by a driver, and scroll the content under the cursor, as if the event were a wheel scroll event. Is that right?
Yes. Exactly.
(In reply to comment #57) > Yes. Exactly. Any sense of how wide spread the "old driver" problem is? If it's limited this seems like a reasonable trade-off. I just don't think we should be setting our defaults to address capture software issues over mouse driver functionality if the mouse driver problem is wide spread.
I think that the problem isn't major. Seems the latest models of Microsoft and Logitech send WM_MOUSE*WHEEL messages correctly. And Alps touchpad sends them too. Thinkpad's track pad has same problem. But we already have hacking code for them. And it sets a window handle to lParam. Therefore, we're handling the WM_*SCROLL messages with non-zero lParam as WM_MOUSE*WHEEL. So, major devices are not related the new pref. Even if that isn't so, I don't think we should set the default setting to true because the messages shouldn't be used for mouse wheel scrolling. So, I think the capture software's request is more right. The bad mouse drivers could not work on some other applications. So, they should correct their behavior themselves.
Comment on attachment 430246 [details] [diff] [review] Patch v2.1 Sounds good. Widget side of things looks fine, tests also pass and I've confirmed the CapWizPro works with this applied.
Attachment #430246 - Flags: review?(jmathies) → review+
Comment on attachment 430246 [details] [diff] [review] Patch v2.1 >+ nsIScrollableFrame* sf = >+ ps->GetFrameToScrollAsScrollable(nsIPresShell::eEither); >+ if (!sf) { >+ aEvent->mIsEnabled = PR_FALSE; >+ return NS_OK; >+ } >+ >+ aEvent->mIsEnabled = CanScrollOn(sf, aEvent->mScroll.mAmount, >+ aEvent->mScroll.mIsHorizontal); >+ >+ if (!aEvent->mIsEnabled || aEvent->mOnlyEnabledCheck) { >+ return NS_OK; >+ } You could write something like: nsIScrollableFrame* sf = ps->GetFrameToScrollAsScrollable(nsIPresShell::eEither); aEvent->mIsEnabled = sf ? CanScrollOn(sf, aEvent->mScroll.mAmount, aEvent->mScroll.mIsHorizontal) : PR_FALSE; if (!aEvent->mIsEnabled || aEvent->mOnlyEnabledCheck) { return NS_OK; } >@@ -1232,17 +1233,38 @@ public: > nsContentCommandEvent(PRBool aIsTrusted, PRUint32 aMsg, nsIWidget *aWidget, > PRBool aOnlyEnabledCheck = PR_FALSE) : > nsGUIEvent(aIsTrusted, aMsg, aWidget, NS_CONTENT_COMMAND_EVENT), > mOnlyEnabledCheck(PRPackedBool(aOnlyEnabledCheck)), > mSucceeded(PR_FALSE), mIsEnabled(PR_FALSE) > { > } > >+ // NS_CONTENT_COMMAND_PASTE_TRANSFERABLE >+ // XXX Cannot move this to the union and change to a raw pointer? >+ // Seems that the destructor can release the instance automatically when >+ // the event type is NS_CONTENT_COMMAND_PASTE_TRANSFERABLE. > nsCOMPtr<nsITransferable> mTransferable; // [in] Yeah, using nsCOMPtr ins a union is hard. But no need to optimize memory usage here. The class is used in stack. You can remove the XXX comment, IMO. >+ >+ // for mScroll.mUnit >+ enum { >+ eCmdScrollUnit_Line, >+ eCmdScrollUnit_Page, >+ eCmdScrollUnit_Whole >+ }; >+ >+ union { >+ // NS_CONTENT_COMMAND_SCROLL >+ struct { >+ PRInt32 mAmount; // [in] >+ PRUint8 mUnit; // [in] >+ PRPackedBool mIsHorizontal; // [in] >+ } mScroll; >+ }; Why you need union and struct? Perhaps just struct ScrollInfo and then member variable ScrollInfo mScroll; And make sure you initialize the struct properly in the constructor! With those, r=me
Attachment #430246 - Flags: review?(Olli.Pettay) → review+
Attached patch Patch v2.1 (obsolete) — — Splinter Review
Thank you, smaug and jimm. roc, would you sr the interface change?
Attachment #430246 - Attachment is obsolete: true
Attachment #430793 - Flags: superreview?(roc)
+ // NS_CONTENT_COMMAND_SCROLL + // for mScroll.mUnit Can you document what gets scrolled by this command? It seems to scroll the focused content, is that correct?
Attached patch Patch v2.1.1 (obsolete) — — Splinter Review
yes.
Attachment #430793 - Attachment is obsolete: true
Attachment #431748 - Flags: superreview?(roc)
Attachment #430793 - Flags: superreview?(roc)
+// NS_CONTENT_COMMAND_SCROLL scrolls nearest scrollable view from focused +// content or latest DOM selection, so, it should be same target as keyboard +// scroll except that this event handler scrolls a scrollable view which is +// scrollable to either direction. I.e., even if a scrollable view can be +// scrolled to vertical only, this event doesn't try to scroll its ancestor +// by horizontal scroll event. Don't talk about scrollable views, there's no such thing anymore. Say "scrollable content" instead. The part about direction needs to be clarified. How about // NS_CONTENT_COMMAND_SCROLL scrolls the nearest scrollable element to the // currently focused content or latest DOM selection. This would normally be // the same element scrolled by keyboard scroll commands, except that this event // will scroll an element scrollable in either direction. I.e., if the nearest // scrollable ancestor element can only be scrolled vertically, and horizontal // scrolling is requested using this event, no scrolling will occur. Is that comment accurate?
(In reply to comment #65) > The part about direction needs to be clarified. How about > > // NS_CONTENT_COMMAND_SCROLL scrolls the nearest scrollable element to the > // currently focused content or latest DOM selection. This would normally be > // the same element scrolled by keyboard scroll commands, except that this > event > // will scroll an element scrollable in either direction. I.e., if the nearest > // scrollable ancestor element can only be scrolled vertically, and horizontal > // scrolling is requested using this event, no scrolling will occur. > > Is that comment accurate? Absolutely. Thank you!
Attached patch Patch v2.1.2 — — Splinter Review
Attachment #431748 - Attachment is obsolete: true
Attachment #431791 - Flags: superreview?(roc)
Attachment #431748 - Flags: superreview?(roc)
Attachment #431791 - Flags: superreview?(roc) → superreview+
http://hg.mozilla.org/mozilla-central/rev/9da3f465942d Thank you, everybody!
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: