461500 - 3 finger swipe up/down doesn't scroll unless the area under the mouse is focused

Status: ASSIGNED

(Firefox :: General, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

I think focusing shouldn't be needed, because it is not needed for 2 finger
scrolling either.
I haven't yet checked whether this is a firefox or core bug.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Verified, this is a firefox bug, not core. The event is dispatched to the right
document, but firefox code tries to use the focused document.

Comment 2

[Ed Lee :Mardak]

Are you only talking about scroll to top/bottom? It seems like back/forward work correctly when the page isn't focused.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Yes I'm talking about scroll to top/bottom (which is the current behavior for 3 finger swipe up/down).

Comment 4

[Ed Lee :Mardak]

So cmd_scrollTop/Bottom will get dispatched to the currently focused element. This could be undesired in cases where I'm typing into this textarea to comment on the bug, but how about pages that contain iframes (maybe gmail?)..

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

There are few options.
(1) Always scroll whatever would be scrolled with 2 finger scroll. So basically
    the scrollable area under pointer.
(2) Always scroll the *document* which is under the pointer.
(3) Always scroll the focused element (current behavior).
(4) ...?

For me 2 would be good (scrolling up/down long bugs, mxr pages etc.), but
in many cases that would be a bit strange. Depending on the web page, some
things could be scrolled or not; <textarea> wouldn't work, but possibly same
looking <iframe> would.

So perhaps (1) would be the best option for consistency. That may need
some changes to core.
Currently if one tries to swipe up/down over an empty focused <textarea>,
nothing happens.

Comment 7

[Asa Dotzler [:asa]]

this is a pretty frustrating inconsistency and I can't see why the behavior should be any different than two finger swipe scrolling. Carrying over my nomination from dupe.

Comment 8

[Ed Lee :Mardak]

Right now we send a cmd_scrollTop which will get dispatched to the focused element. This is why it sometimes scrolls the page and othertimes moves the cursor in a text box.

I've tried doing this as a special command in the browser..

gBrowser.docShell.
QueryInterface(Ci.nsIInterfaceRequestor).
getInterface(Ci.nsISelectionDisplay).
QueryInterface(Ci.nsISelectionController).
completeScroll(aBottom);

That works great for many pages, but those that use frames like gmail... it doesn't scroll. Clicking around then trying the command doesn't help.

Is there some way to get what the mouse pointer is over? Or something like nsLayoutUtils::GetNearestScrollingView? I found completeScroll by snooping around that method, but GetViewToScroll is called first and doesn't always call NearestScrollingView?

Tom: Curious, why again do we send a fake event that isn't actually an event but a duck-type-almost-event (as long as people only check for alt/ctrl/etc).. but doesn't provide a target, etc? We prevent content from getting the event. But I suppose just to be safe, make sure commands can't accidentally pass the actual event to a content node. (The command could accidentally leak the simplegestureevent to some content node.)

Comment 9

[Ed Lee :Mardak]

Oh and I suppose the target of the simplegestureevent isn't totally useful either because it's just the document always, yeah?

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You can use document.elementFromPoint to detect which element the mouse is over. If it's an iframe, you can dig into the iframe's contentDocument and call elementFromPoint again with adjusted coordinates. That won't work with CSS tranforms or a few other situations though since the transformation into iframe coordinates could get complex. Maybe we need a new super-cross-iframe version of elementFromPoint...

Comment 11

[Tom Dyas]

Is there an API to access the vertical scroll bars for the entire window?  If there was, we could just adjust the position of that scrollbar programatically.

Comment 12

[Ed Lee :Mardak]

That's basically what I'm doing in comment 8. The problem is that you can have iframes, frames or blocks with scrollable overflow under the pointer and wheel/two-finger scroll respects that.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Wheel scrolling does complex stuff like if a scrollable element is scrolled to the bottom, scrolling down will skip that element and scroll the nearest ancestor that actually has room to scroll. But there are timers involved, and the idea of "mousewheel transactions" so that if the user is scrolling in a box and it reaches the buttom we don't *suddenly* start scrolling an ancestor.

So I guess our choices are to add APIs to make that algorithm implementable in the browser, or create new APIs so you can get involved in mousewheel transactions, or just move some of this swipe code into the event state manager where the mousewheel transactions are.

Comment 14

[Ed Lee :Mardak]

(In reply to comment #13)
> the idea of "mousewheel transactions"
Curious, does that not work on OS X? I just did a quick data uri test with a div/iframe and overflow and two finger scrolling didn't seem to stop even if I went slowly. But I do remember running into the behavior you described.

But anyway, at least for the current 3-finger behavior of scroll top/bottom, there shouldn't really be much need for the transaction because it's going to scroll the page/frame all the way to to the beginning/end. It's as if the user hit cmd+up after scrolling.

> just move some of this swipe code into the event state manager
I would say this is the least desirable approach as the functionality of 3-finger swipe up/down is customizable to the user and doesn't have to relate to scrolling at all.

Tom: Do you remember why we only pick and choose to make a fake event with the key modifiers? Either way, the {layer,page}{X,Y} values don't seem accurate so it's not useful for document.elementFromPoint.

I also noticed that for Gmail's chat, clicking on the chat scrollback has some javascript to automatically focus the textbox, so cmd_scrollTop will always trigger on the textbox doing nothing. Additionally with the code in comment 8, this causes gmail to clip off at the top when scrolling to the bottom... I guess the outer document frame was incorrectly sized when I resized the window.. ?

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I guess I misunderstood the problem. So what exactly is the desired behaviour? Comment #5 was never explicitly answered.

Comment 16

[Ed Lee :Mardak]

(In reply to comment #15)
> So what exactly is the desired behaviour?
Beltzner? As per comment 5, we currently only scroll the focused element, which potentially causes no scrolling at all.

So there's actually 2 separate issues here:
A) What to scroll "from"
B) When to scroll

As per this bug, (B) should be "always" as in, if we can't scroll "what" from (A), we should always try scrolling an ancestor.

For (A), there's a 2x2 classification for {from focus, from pointer} vs {inside out, outside in}
Currently mouse scroll is "from pointer + inside out" whereas 3-finger scroll is "from focus + inside out" except that we don't go to ancestors for (B).

The "outside in" could be useful if we just want to directly scroll the containing page/frame/iframe first before scrolling whatever is pointed/focused. So per gmail case, it would try scrolling the frameset, fail then scroll the descendant frame that contains focus, etc.

Additionally per comment 5, there can be some caveats in what to scroll: only "pages"? include overflowed scrollbar (e.g., div) content? include inputs like textarea and selects?

Comment 17

[Tom Dyas]

(In reply to comment #14)
> Tom: Do you remember why we only pick and choose to make a fake event with the
> key modifiers? Either way, the {layer,page}{X,Y} values don't seem accurate so
> it's not useful for document.elementFromPoint.

The fake event adds the "button" property with a value of 0 to simulate a left click.  The default setting for the left swipe activates BrowserBack() in browser/base/content/browser.js.  That function calls whereToOpenLink() in browser/base/content/utilityOverlay.js, which examines the button property to determine if the user clicked using the middle button.

whereToOpenLink()'s second argument is a boolean that determines whether it ignores the "button" property.  BrowserBack() has this argument set to false.

Same logic applies to BrowserForward(), which is activated by the right swipe.

Comment 18

[Ed Lee :Mardak]

Sure, but why can't we just take the existing event and add a button property with value 0?

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

First, I think browser.js should not handle them as scrolling request. If the gestures should be scrolling request always, they should be handled ESM like mouse wheel events. I believe that all XUL applications should be same behavior.


We should think which scrollable view is wanted to scroll by the user. I think that a focused content is not good solution in some cases. E.g., focused content may be in outside by 2 finger gesture. Then, the user cannot get any feedback by the eyes. And I think that mouse cursor position is not good in some cases too. E.g., immediately after key typing. During key typing, mouse cursor may be outside of the focused text editor for readability of the editing text.

Note that current our logic to decide a scrollable view is based on mouse and its wheel. But it seems that we should not use same logic for scrolling events from simple gestures. Because simple gesture events may not be related to mouse cursor. E.g., text inputting case (above). So, wheel transaction manager should check the previous user action when the scrolling request comes from simple gesture event.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I have a question, what is behavior on safari at 3 finger swipe up/down? Many users might prefer same behavior as the OS vendor's application, if it is good behavior.

Comment 21

[Tom Dyas]

(In reply to comment #20)
> I have a question, what is behavior on safari at 3 finger swipe up/down? Many
> users might prefer same behavior as the OS vendor's application, if it is good
> behavior.

Safari does not assign any behavior to up or down swipes, only right and left swipes.  We go beyond Safari's gesture handling by supporting scrolling with the up and down swipes.  (We also go beyond Safari by having "rotate" gestures go through open tabs.)

Comment 22

[Ed Lee :Mardak]

Attachment: v1

Thanks for the fix to bug 471883.

This patch takes the target of the event and tries to scroll it to the top/bottom. If it doesn't move, keep trying each of its ancestors.

This works well for divs and iframes.

Additionally, I figured out why gmail was clipping off with a chat screen open. completeScroll was scrolling the HTML element, which by default has overflow-y: hidden. So this patch also explicitly checks for overflow-y hidden. (I accidentally ran into this on other sites as well that used overflow hidden on other content and stuff magically showed up.)

Comment 23

[Ed Lee :Mardak]

Here's a data uri to test scrolling.

Oh and I do see the scrolling transaction working on OS X for two finger scroll. I just didn't realize it was ~2 seconds long. :)

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 355227 [details] [diff] [review]
v1

I still don't think this bug should be fixed in browser.js. ESM is a best owner for this handling.

Comment 25

[Ed Lee :Mardak]

3-finger swipe-up is a customizable action. It could be open new tab, undo close tab, reload page, jump to top.

But even then, sure ESM could provide some API to scroll completely, (page? line? pixel?) that browser.js calls. Perhaps exposed with something like document.scrollFromPointer().

That would be the better solution as it would colocate with the existing implementation for wheel/2-finger scrolling. But would that be something we want in 1.9.1?

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

My idea is ideally way. If you want to fix this in 1.9.1, I don't like to implement in ESM, because my current work for wheel transaction implementations  in ESM will be finished only on trunk. So, I don't want to break my patch now.

Is this an important issue which should be fixed on 1.9.1? I don't think so...

Comment 27

[Ed Lee :Mardak]

Asa: Here's a build if you want to try.

https://build.mozilla.org/tryserver-builds/2009-01-03_17:[e-mail]/[e-mail]

Comment 28

[Asa Dotzler [:asa]]

Edward, this build passes my preliminary test/complaint for all situations except when another app has focus. 

With two finger scrolling, as long as the pointer is over the Firefox window, it doesn't matter that other apps have focus. With three finger scrolling, I get no action at all if another app has the focus.

This is a huge step forward and I'd be pretty happy with it even with that one "failure"

Comment 29

[Tom Dyas]

Is nsIPresShell::ScrollContentIntoView a viable API for top/bottom scrolling?  It doesn't seem callable from Javascript code, however.

Comment 30

[Tom Dyas]

I haven't had a chance to apply the patch yet and test it, but here are some preliminary comments:

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -804,8 +804,7 @@
>    */
>   _doAction: function GS__doAction(aEvent, aGesture) {
>     // Create a fake event that pretends the gesture is a button click
>-    let fakeEvent = { shiftKey: aEvent.shiftKey, ctrlKey: aEvent.ctrlKey,
>-      metaKey: aEvent.metaKey, altKey: aEvent.altKey, button: 0 };
>+    aEvent.button = 0;

Should update the comment to reflect reasoning for adding a "button" property to the event (i.e., tricking BrowserBack() and BrowserForward into thinking the event is a left mouse click).
 
>+/**
>+ * Recursively scroll once to the top/bottom of a target or its ancestor
>+ *
>+ * @param aNode
>+ *        Node from which to start scrolling
>+ * @param aBottom
>+ *        True to scroll forwards to the bottom
>+ */
>+function BrowserScroll(aNode, aBottom) {
>+  // Only scroll things if we're showing overflowed content
>+  let style = aNode.ownerDocument.defaultView.getComputedStyle(aNode, "");
>+  if (style.getPropertyValue("overflow-y") != "hidden") {
>+    // Keep track of the original scroll position to see if we change
>+    let oldScroll = aNode.scrollTop;
>+    aNode.scrollTop = aBottom ? aNode.scrollHeight : 0;
>+
>+    // We're done if we changed the scroll position
>+    if (oldScroll != aNode.scrollTop)
>+      return;
>+  }
>+
>+  // Use the immediate parent unless it's the Document element
>+  let parent = aNode.parentNode;
>+  if (parent && parent.nodeType == 9)
>+    parent = parent.defaultView && parent.defaultView.frameElement;
>+
>+  // Recursively try scrolling the parent if we have one
>+  if (parent)
>+    BrowserScroll(parent, aBottom);
>+}

No need for recursion since it is just a tail call.  You can just use a while loop instead.

We should probably decide first where the appropriate place to solve this issue is.

Comment 31

[Tom Dyas]

r+ subject to my prior comment

The patch works for me and does not require modification of the event state manager so IMHO it can do to fix the problem for 3.1.

Note: I tried to set r+ on the patch but because Bugzilla returned the message: "You are not authorized to edit attachment 355227 [details] [diff] [review]."

Comment 32

[Mike Beltzner [:beltzner, not reading bugmail]]

This doesn't block, but we'd take a tested, reviewed patch that doesn't touch other areas of the code.

Comment 33

[Ed Lee :Mardak]

Comment on attachment 355227 [details] [diff] [review]
v1

r=tdyas

Comment 34

[Ed Lee :Mardak]

Attachment: v1.1

Converted to iterative BrowserScroll and added a testcase automating the behavior expected with url in this bug.

Comment 37

[Nick Lowe]

If a page contains nested scrollable areas and that scrollable area is
-already- at its bottom, a three-finger gesture down in that area should
propagate to the parent scrollable area. (And so on...)

(Obviously the same should be true if the scrollable area is at its top and a three-finger gesture up occurs.)

Comment 38

[Ed Lee :Mardak]

Comment on attachment 357479 [details] [diff] [review]
v1.1

There's an issue with iframes because the body element might have something scrollable with a scrollable style, but it's not allowed to scroll because its containing iframe says not to scroll.

I'm looking to implement a BrowserScroll that's basically what's in bug 477863 comment 0.

Comment 39

[Ed Lee :Mardak]

Masayuki: With now that I can send a DOMMouseScroll event with a given delta with bug 477863, I've been running into some strange overflow issues with a really large "aNumLines".

After doing some binary reduction, I found that -2236968 was the most negative number I could use to scroll to the top. But then it turns out this is dependent on the element being scrolled as when I tried this with other elements, it just didn't scroll. While I was doing the binary reduction, I noticed something a bit *more* negative caused it to scroll to the very bottom of the element instead of the top.

An unsigned 21 bit value goes from 0-2097151, and 22 bit value goes from 0-4194303. So that cutoff is fairly strange.. And FYI, the element that does scroll has a scrollHeight of 204. Elements with a smaller scrollHeight don't scroll.

But as a good note, 3-finger swipe can make use of the mousewheel transaction now because it looks just like another mouse scroll event. And this overflow isn't really a huge issue.

Comment 40

[Ed Lee :Mardak]

Attachment: v2

Uses bug 477863 for gesture events as mouse events. I haven't updated the testcase yet though.

Comment 41

[Ed Lee :Mardak]

Attachment: v2.1

Updated testcase which now checks for scrolling in divs, frames, header, nested divs.

Comment 42

[Mike Beltzner [:beltzner, not reading bugmail]]

/me notes that the dependencies have landed, hoppity-hop!

Comment 43

[Alex Faaborg [:faaborg] (Firefox UX)]

This bug's priority relative to the set of other polish bugs is:
P4 - Polish issue that is rarely encountered, and is easily identifiable.

Obvious when you encounter it, but effected users are limited to people who have hardware that supports gesture, and of that group users who actually use the three finger gesture (sounds kind of disrespectful :).

Comment 44

[Bryan Clark (DevTools PM) [:clarkbw]]

yo yo!  the student who is working on bug 465257 (multi-touch for TB) will be stealing this nice code you have presented us.  We appreciate your efforts as we have more likely unfocused areas than you do (oh snap!). :)  If there is anything we could do to help please let us know.

Comment 45

[u358732]

Hello, it seems clarkbw introduced me a little in the comment above.  I recently pulled the latest code for Firefox 3.7 and noticed that the code in file "browser.js" has changed within a place where your solution proposed above now seems compromised.  It's within the gGestureSupport['_doAction'] area.  I thought I would share.  Your solution has been providing direction for Thunderbird though, so thanks!

Comment 46

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 362450 [details] [diff] [review]
v2.1

This has bitrotten a bit, but nothing too bad.

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+function BrowserScroll(aEvent, aBottom) {

This name is too generic given what it does. BrowserMouseScrollToEnd(), perhaps?

>+  // Pick a big number of lines to scroll up or down
>+  let lines = (aBottom ? 1 : -1) * 1000000;

Use 0x7FFFFFFF?

>diff --git a/browser/base/content/test/browser_scroll.js b/browser/base/content/test/browser_scroll.js

>+function test()

>+  // Enable privileges so we can use nsIDOMWindowUtils interface
>+  netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");

Isn't needed for browser chrome tests - they run in the context of the chrome window...

>+    // Wait a short bit to let things scroll before we check if they did
>+    let delayedCheck = function() setTimeout(function() {

:(

This will almost certainly cause random orange - is there no way to make this test not rely on a random timer?

Comment 47

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 3 duplicates.
:Mardak, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 48

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.