User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040717 Firefox/0.9.1+ (bangbang023) Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040717 Firefox/0.9.1+ (bangbang023) If you go to http://wtfpeople.com/ (for example) and middle-click on the white iframe section in the middle of the page, the autoscroll icon appears (as expected), however niether moving the mouse up or down scrolls the page. To scroll using auto-scroll, you've got to middle-click elsewhere on the page. I believe this is because you are middle-clicking in an iframe, so the scrolling only occurs in the iframe, and as all the content is already visible, no scrolling occurs in the iframe (because none is required to see any more content). Although this is (i guess) the proper behaviour for autoscroll, its not quite what the user expects. The user expects to be able to middle-click anywhere on a page, move the mouse up/down and have the page scroll. If nessecary, im sure i can make a simplified testcase of the problem. (I'm not quite sure whether this should be an RFE or not, so ive filed it as trivial for now. sorry if thats wrong) Reproducible: Always Steps to Reproduce: 1. Go to http://wtfpeople.com/ 2. Middle-click somewhere in the white iframe in the centre of the page. 3. Move the mouse down. Actual Results: The page did not scroll downwards. Expected Results: The page should have scrolled down.

Here's another example (not a testcase, just an example) http://liquideagle.co.uk/whalley/251903-example.htm

*** This bug has been marked as a duplicate of 223542 ***

whoops. bad dup.

Mouse is ok, but the problem appears with PgUp/PgDn (Firefox 1.0RC1) Non-Scroll-IFRAMES should scroll the frame above - after scrolling an IFRAME down it should continue scrolling the frame above. !!! Comparable problem with TEXTAREA fields !!! 1. Fill a textarea like this "Additional Comments" with some lines to have a scrollbar 2. Put the mouse above the TEXTAREA (for example on the text "Additional Comments" here 3. Try to scroll down with mouse wheel Actual Results: Page-Scroll stopps and TEXTAREA begins to scroll Expected Results: Page should continue to scroll after TEXTAREA scroll...

(In reply to comment #4) > Mouse is ok, but the problem appears with PgUp/PgDn (Firefox 1.0RC1) > Non-Scroll-IFRAMES should scroll the frame above - after scrolling an IFRAME > down it should continue scrolling the frame above. > > !!! Comparable problem with TEXTAREA fields !!! > 1. Fill a textarea like this "Additional Comments" with some lines to have a > scrollbar > 2. Put the mouse above the TEXTAREA (for example on the text "Additional > Comments" here > 3. Try to scroll down with mouse wheel > Actual Results: > Page-Scroll stopps and TEXTAREA begins to scroll > > Expected Results: > Page should continue to scroll after TEXTAREA scroll... How about when you're on a page with an iframe (eg. https://bugzilla.mozilla.org/enter_bug.cgi?product=Thunderbird) and you middle click and move the mouse down to let it autoscroll, it will mess up when you pass an iframe. There are also some more issues with autoscrolling I have noticed, as I use it often. One in particular, when Firefox loses focus, I think the autoscroll should stop. Also, like IE, it would be nice to use the up/down arrow keys to move the mouse, controling autoscroll speed.

Hmm, I can't seem to duplicate that iframe problem again..go figure Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0 PS - Sorry for the quoting on the last reply. I'm new to this Mozilla bugreport thing....

Ok, this patch solves the issue, by ignoring the iframe when it the content of the iframe is smaller than the iframe itself. It then takes the parent window and checks if that window has scrollbars, if that window doesn't have, it then takes again the parent window, etc... It stops when the main window has reached, or when a scrollable iframe/window has been found. This patch incorporates the patch from bug 223542, since it is needed to get this patch working well. This patch would override the patch from bug 212002. This patch also has the patches from bug 280584 and bug 212002 inside it, but these are not really necessary to solve this bug.

This patch fixes an error in the previous patch. Every autoscroll image has to be inserted in the top window document, which the previous patch didn't do.

*** Bug 142355 has been marked as a duplicate of this bug. ***

*** Bug 355641 has been marked as a duplicate of this bug. ***

Neil, this affects us too.

(In reply to comment #12) >Neil, this affects us too. So we need to look for a window with non-zero scrolling in a similar way. Maybe we could implement this as part of a search for an overflowing div.

:roc, I get the feeling that this is a valid improvement, that never moved forward, because the original reporter never requested review from anyone. Although the chances are low that a 6-year old patch applies, - do you think this is a valid issue (in the sense that we would like to see this fixed) and, - what do you think of the patch, i.e. is it taking the right approach, and if an up-to-date patch were to be submitted, would you accept it? Feel free to CC some more people if you're not the right person to ask, I just feel like this is layout-ish...

I don't know. Is it still a bug? This is not layout, autoscroll is a Firefox UI feature.

I think this is still a bug: the behaviour is the same as six years ago, from what I can tell. Why should it no longer be a bug then? I think :protz nails the important questions pretty well.

Old test pages are either porn sites or gone. A test case would be useful here!

This bug is still in effect. My gaming community uses iframe to embed SMF forum inside Wordpress page. You can visit the page here: http://aseveljet.net/?page_id=14 . I can't give you login information so we have to reproduce the situation that needs autoscrolling. Inspect the iframe element (ID = advanced_iframe) and change its height from 1080px to e.g. 3000px. Notice that you can only autoscroll by placing the mouse outside the iframe e.g. on the left sidebar. This is a browser bug (Chrome has this also) because IE allows autoscrolling inside non-zero scrolling iframe. Please fix this soon because my community needs this feature badly.

testcase

For reference, bug 295977 is where this was fixed for overflow: scroll divs.

It's probably better not to name the method findNearestScrollingElement, but findNearestScrollableElement instead. But I can change that in a follow-up patch, there might be more issues with the patch anyway.

Comment on attachment 8460267 [details] [diff] [review] 251903.diff Review of attachment 8460267 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, looks good with the comments addressed. Not sure about the review policy however, I’m not a peer. ::: toolkit/content/browser-content.js @@ +113,5 @@ > + }, > + > + startScroll: function(event) { > + > + this.findNearestScrollingElement(event.originalTarget); this._scrollable = this.findNearestScrollingElement(event.originalTarget); Might be clearer instead of having that method set the var as a side-effect. Feel free to do that as a followup. ::: toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js @@ +40,5 @@ > <body id="j"><div style="height: 2000px"></div>\ > <iframe id="iframe" style="display: none;"></iframe>\ > </body></html>'}, > + {elem: 'j', expected: expectScrollVert}, // bug 914251 > + {dataUri: 'data:text/html,<html><head><style> iframe { width: 100px; height: 80px;}</style></head><body>\ The only iframe without an inline style="" has display:none, so this <style> tag is unused? @@ +45,5 @@ > +<div id="k" style="height: 150px; width: 200px; overflow: scroll; border: 1px solid black;">\ > +<iframe style="height: 200px; width: 300px;"></iframe>\ > +</div>\ > +<div id="l" style="height: 150px; width: 300px; overflow: scroll; border: 1px dashed black;">\ > +<iframe style="height: 200px; width: 200px;" src="data:text/html;charset=utf-8;base64,PGRpdiBzdHlsZT0iYm9yZGVyOiA1cHggc29saWQgYmx1ZTsgaGVpZ2h0OiAyMDAlOyB3aWR0aDogMjAwJTsiPjwvZGl2Pg%3D%3D"></iframe>\ You should be able to nest data URIs, right? In that case please avoid the base64 here, you should be able to actually read the contents of the iframe. @@ +48,5 @@ > +<div id="l" style="height: 150px; width: 300px; overflow: scroll; border: 1px dashed black;">\ > +<iframe style="height: 200px; width: 200px;" src="data:text/html;charset=utf-8;base64,PGRpdiBzdHlsZT0iYm9yZGVyOiA1cHggc29saWQgYmx1ZTsgaGVpZ2h0OiAyMDAlOyB3aWR0aDogMjAwJTsiPjwvZGl2Pg%3D%3D"></iframe>\ > +</div>\ > +<div style="height: 200%; border: 5px dashed black;">filler to make document overflow: scroll;</div>\ > + <iframe id="iframe" style="display: none;"></iframe>\ Do you really need this? Since its display:none? Otherwise please remove it. And take care of the indentation please.

(In reply to Arpad Borsos (Swatinem) from comment #24) > Comment on attachment 8460267 [details] [diff] [review] > 251903.diff > > Review of attachment 8460267 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks, looks good with the comments addressed. > Not sure about the review policy however, I’m not a peer. I addressed most of your comments. I'll ask review for a peer after you've given this patch r+. > ::: toolkit/content/browser-content.js > @@ +113,5 @@ > > + }, > > + > > + startScroll: function(event) { > > + > > + this.findNearestScrollingElement(event.originalTarget); > > this._scrollable = this.findNearestScrollingElement(event.originalTarget); > > Might be clearer instead of having that method set the var as a side-effect. > Feel free to do that as a followup. Ok, I did that in this patch. Also, it might be better to not set this._scrolldir in findNearestScrollableElement and do that somewhere else? > ::: toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js > The only iframe without an inline style="" has display:none, so this <style> > tag is unused? Yes, so I removed it now. > You should be able to nest data URIs, right? In that case please avoid the > base64 here, you should be able to actually read the contents of the iframe. Did that. > > +<div style="height: 200%; border: 5px dashed black;">filler to make document overflow: scroll;</div>\ > > + <iframe id="iframe" style="display: none;"></iframe>\ > > Do you really need this? Since its display:none? > Otherwise please remove it. > And take care of the indentation please. It was needed because of: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js#109 I've added an if (iframe) block around that code now, so I was able to remove display: none iframe. I also added some extra test checking code, whether the window should/should not have scrollled.

Comment on attachment 8460267 [details] [diff] [review] 251903.diff > if (this._scrollable.scrollMaxX > 0) { > this._scrolldir = this._scrollable.scrollMaxY > 0 ? "NSEW" : "EW"; > } else if (this._scrollable.scrollMaxY > 0) { > this._scrolldir = "NS"; > } else { >+ if (this._scrollable.frameElement) { >+ this.findNearestScrollingElement(this._scrollable.frameElement); >+ return; >+ } > this._scrollable = null; // abort scrolling > return; > } > } >+ }, Two style nits: 1. You don't need to return any more, you'll just fall off the end of the function. 2. The else { if } looks odd, I would have gone for something like if (this._scrollable.scrollMaxX > 0) { this._scrolldir = this._scrollable.scrollMaxY > 0 ? "NSEW" : "EW"; } else if (this._scrollable.scrollMaxY > 0) { this._scrolldir = "NS"; } else if (this._scrollable.frameElement) { this.findNearestScrollingElement(this._scrollable.frameElement); } else { this._scrollable = null; // abort scrolling }

(In reply to Arpad Borsos from comment #24) > this._scrollable = this.findNearestScrollingElement(event.originalTarget); > > Might be clearer instead of having that method set the var as a side-effect. It also sets _scrolldir as a side-effect, so having it set one and not the other might be even less clear.

Comment on attachment 8460267 [details] [diff] [review] 251903.diff > this._scrollable = null; // abort scrolling > return; > } > } >+ }, >+ >+ startScroll: function(event) { >+ >+ this.findNearestScrollingElement(event.originalTarget); For // abort scrolling to work, you should check whether findNearestScrollingElement actually found a scrolling element, no?

Comment on attachment 8460617 [details] [diff] [review] 251903_v2.diff Review of attachment 8460617 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the long coding style discussions. Hope they are all for the better :-) ::: toolkit/content/browser-content.js @@ +96,3 @@ > } > } > Instead of changing the `break` to `return node`, you can leave those in and rather do an `if (node) { return [node, scrolldir]; }` at the end of the for. Then you also don’t need to wrap the block below in `if (!node)` @@ +114,5 @@ > + }, > + > + startScroll: function(event) { > + > + this._scrollable = this.findNearestScrollableElement(event.originalTarget); (In reply to neil@parkwaycc.co.uk from comment #27) > (In reply to Arpad Borsos from comment #24) > > this._scrollable = this.findNearestScrollingElement(event.originalTarget); > > > > Might be clearer instead of having that method set the var as a side-effect. > It also sets _scrolldir as a side-effect, so having it set one and not the > other might be even less clear. You are absolutely right. `[this._scrollable, this._scrolldir] = …` then? ::: toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js @@ +37,5 @@ > </body></html>'}, > {elem: 'i', expected: expectScrollVert}, // bug 695121 > {dataUri: 'data:text/html,<html><head><meta charset="utf-8"></head><style>html, body { width: 100%; height: 100%; overflow-x: hidden; overflow-y: scroll; }</style>\ > <body id="j"><div style="height: 2000px"></div>\ > <iframe id="iframe" style="display: none;"></iframe>\ I just see that the other tests also include a display: none iframe. Apparently I added those, can’t remember why though. @@ +107,5 @@ > + > + if (test.testwindow) { > + ok((scrollVert && gBrowser.contentWindow.scrollY > 0) || > + (!scrollVert && gBrowser.contentWindow.scrollY == 0), > + 'Window should'+(scrollVert ? '' : ' not')+' have scrolled vertically'); 'Window '+test.elem+' so we know which one it is in case of a failure.

(In reply to neil@parkwaycc.co.uk from comment #27) > (In reply to Arpad Borsos from comment #24) > > this._scrollable = this.findNearestScrollingElement(event.originalTarget); > > > > Might be clearer instead of having that method set the var as a side-effect. > It also sets _scrolldir as a side-effect, so having it set one and not the > other might be even less clear. I'm going to leave this as it is for now, because it seems controversial.

(In reply to Arpad Borsos (Swatinem) from comment #29) > ::: toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js > @@ +37,5 @@ > > </body></html>'}, > > {elem: 'i', expected: expectScrollVert}, // bug 695121 > > {dataUri: 'data:text/html,<html><head><meta charset="utf-8"></head><style>html, body { width: 100%; height: 100%; overflow-x: hidden; overflow-y: scroll; }</style>\ > > <body id="j"><div style="height: 2000px"></div>\ > > <iframe id="iframe" style="display: none;"></iframe>\ > > I just see that the other tests also include a display: none iframe. > Apparently I added those, can’t remember why though. This is to make sure the "Page load in background causes autoscroll popup to disappear" bug does not reappear.

I addressed comment 26, comment 28 and the last comment in comment 29 in here.

Comment on attachment 8460837 [details] [diff] [review] 251903_v3.diff (Surprisingly I only have a two-button mouse right now so I'm just making a style nit. I would have liked to avoid the recursion but I couldn't work out how to do it without reindenting the code, and hgweb's blame won't ignore whitespace yet.) >+ } else if (this._scrollable.frameElement) { >+ this.findNearestScrollableElement(this._scrollable.frameElement); >+ return; This return is unnecessary.

(In reply to neil@parkwaycc.co.uk from comment #33) > Comment on attachment 8460837 [details] [diff] [review] > 251903_v3.diff > > (Surprisingly I only have a two-button mouse right now so I'm just making a > style nit. I would have liked to avoid the recursion but I couldn't work out > how to do it without reindenting the code, and hgweb's blame won't ignore > whitespace yet.) > > >+ } else if (this._scrollable.frameElement) { > >+ this.findNearestScrollableElement(this._scrollable.frameElement); > >+ return; > This return is unnecessary. I'll remove it with a new patch, but won't bother with a new patch, until you've review the patch completely. Let me know if you're able to review the patch (there is software, I believe, that allows you to mimic middle-click)

This is with the unnecessary return; removed. Pushed to try server: https://tbpl.mozilla.org/?tree=Try&rev=9c3247660887

Tryserver builds that people can test with are here: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mwargers@mozilla.com-9c3247660887/

Tryserver is green.

This caused https://tbpl.mozilla.org/php/getParsedLog.php?id=44585672&tree=Fx-Team#error0 02:02:58 INFO - 22395 INFO TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_paste_selection.html | global event fired 02:02:58 INFO - TEST-INFO | expected PASS 02:02:58 INFO - 22396 INFO TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_paste_selection.html | data pasted properly from global clipboard - got "", expected "CLIPBOARD" so it has to be backed out. I guess I also should have carried out mochitest-plain on try server.

(In reply to Martijn Wargers [:mwargers] (QA) from comment #39) > This caused > https://tbpl.mozilla.org/php/getParsedLog.php?id=44585672&tree=Fx-Team#error0 > so it has to be backed out. > I guess I also should have carried out mochitest-plain on try server. yeah did the backout now in https://tbpl.mozilla.org/?tree=Fx-Team&rev=e8f997d315e0

It turned out that test was buggy, it relied on this bug. I had to add this to the test to turn off the autoscroll icon to appear: + // This is to remove autoscroll for platforms that have it enabled by default + synthesizeMouse(pasteArea, 8, 8, { button: 0 }); An alternative would be to turn off autoscroll for this test with the "general.autoscroll" pref. For the rest, the patch is the same. I've now pushed a total mochitest run on try server for this: https://tbpl.mozilla.org/?tree=Try&rev=26e5fbb5c4e4

(In reply to Martijn Wargers from comment #41) > It turned out that test was buggy, it relied on this bug. I had to add this > to the test to turn off the autoscroll icon to appear: > + // This is to remove autoscroll for platforms that have it enabled by > default > + synthesizeMouse(pasteArea, 8, 8, { button: 0 }); > > An alternative would be to turn off autoscroll for this test with the > "general.autoscroll" pref. It looks like the bug that created the test only applies to Linux, in which case I'm not sure why the test is running on other platforms at all. I'm going to defer to the author of the test to see what the correct intent is here.

(In reply to Martijn Wargers [:mwargers] (QA) from comment #41) > I've now pushed a total mochitest run on try server for this: > https://tbpl.mozilla.org/?tree=Try&rev=26e5fbb5c4e4 Tryserver is green.

That test should run on all platforms. On Linux it tests that the selection clipboard works. On other platforms, it tests that it isn't handled.

Comment on attachment 8462511 [details] [diff] [review] 251903_v4.diff So, there are in fact two bugs here. The first bug is that the existing test fails on Android with the previous patch. The test should pass, but there's a bug in the patch which means that it fails: the patch incorrectly fails to block autoscroll even though we are middle-clicking a textarea. The second bug is that the test is assuming that the autoscroll won't trigger because nothing in the iframe is scrollable. So I think the correct thing to do as regards autoscroll is to cancel autoscroll only in the case where we don't expect the paste to happen.

(In reply to neil@parkwaycc.co.uk from comment #45) > The first bug is that the existing test fails on Android with the previous > patch. The test should pass, but there's a bug in the patch which means that > it fails: the patch incorrectly fails to block autoscroll even though we are > middle-clicking a textarea. Did you test it locally on Android? I don't see it on https://tbpl.mozilla.org/?rev=edfd65cef207&tree=Fx-Team . There doesn't seem to be any Android mochitests there, at all. The next run after it, still had this faulty patch checked in, and there the Android mochitests were run, but I couldn't see any orange there at all: https://tbpl.mozilla.org/?tree=Fx-Team&rev=fc26cfe5b2ca Anyway, I don't think that Android supports autoscroll. If it did react to middle-click, that code should probably be removed or the general.autoScroll pref should be false. My patch shouldn't influence anything regarding autoscroll blocking, that is done by the "isAutoscrollBlocker" function. This tests if the middlemouse.paste pref is set to false or true. On Android it is set to true: http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/init/all.js#3173 This was done in bug 568700. But that patch forgot there was another instance of the middlemouse.paste pref in "all.js" at: http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/init/all.js#3439 But that one is wrapped inside an #ifndef ANDROID statement. It makes me wonder what the desired outcome for Android is in this regard. > The second bug is that the test is assuming that the autoscroll won't > trigger because nothing in the iframe is scrollable. > > So I think the correct thing to do as regards autoscroll is to cancel > autoscroll only in the case where we don't expect the paste to happen. I don't know, this test is supposed to be about middlemouse paste, not autoscroll. I would prefer to add some tests to the browser-chrome test that test this autoscroll blocking feature on middlemouse paste. And I would just flip the general.autoScroll pref to false for the test_paste_selection.html test.

(In reply to Martijn Wargers from comment #46) > My patch shouldn't influence anything regarding autoscroll blocking, that is > done by the "isAutoscrollBlocker" function. This tests if the > middlemouse.paste pref is set to false or true. Sorry, I thought it would fail on Android because it has both autoscroll and middlemouse paste prefs set, but I must have misread the function. > I would just flip the > general.autoScroll pref to false for the test_paste_selection.html test. Fair enough, but I thought you preferred your original version that dismissed the autoscroll.

I think we should set "middlemouse.paste" to false by default on Android, especially if it helps get this test working. Unlike X11, Android does not use middle mouse button to paste as a standard platform behavior.

(In reply to Matt Brubeck (:mbrubeck) from comment #48) > I think we should set "middlemouse.paste" to false by default on Android, > especially if it helps get this test working. Unlike X11, Android does not > use middle mouse button to paste as a standard platform behavior. Actually, by setting this pref to false, it might make the test_paste_selection.html test fail. I filed bug 1046398 for setting the "middlemouse.paste" pref to false.

(In reply to Martijn Wargers from comment #49) > Actually, by setting this pref to false, it might make the > test_paste_selection.html test fail. I filed bug 1046398 for setting the > "middlemouse.paste" pref to false. It won't fail but currently Android is the only place the middlemouse.paste pref gets tested when there is only one clipboard.

Ok, this adds some testing to make sure that autoscroll is blocked on input, textarea when the middlemouse.paste pref is true and that autoscroll is not blocked on input, textarea when that pref is false. Also some testing for the area and a tag, to make sure that autoscroll is always blocked on those.

And I've set the general.autoscroll pref to false in the test_paste_selection.html file in the patch. (In reply to neil@parkwaycc.co.uk from comment #47) > Fair enough, but I thought you preferred your original version that > dismissed the autoscroll. I don't have a strong preference for either way, disabling autoscroll entirely seems a little bit better to me.

Comment on attachment 8465170 [details] [diff] [review] 251903_v5.diff >+ if (test.disablemousepaste) >+ Services.prefs.setBoolPref("middlemouse.paste", false); ... >+ if (test.disablemousepaste) >+ Services.prefs.setBoolPref("middlemouse.paste", true); Wouldn't it be easier simply to set the pref value before each middle mouse click to the appropriate value depending on the test's preference? >+ // remove 2 tabs that were opened by middle-click on links >+ gBrowser.removeTab(gBrowser.tabs[gBrowser.visibleTabs.length - 1]); >+ gBrowser.removeTab(gBrowser.tabs[gBrowser.visibleTabs.length - 1]); Are these preexisting tabs that the existing test fails to clean up?

(In reply to neil@parkwaycc.co.uk from comment #53) > Wouldn't it be easier simply to set the pref value before each middle mouse > click to the appropriate value depending on the test's preference? Ok, done that in this patch. > >+ // remove 2 tabs that were opened by middle-click on links > >+ gBrowser.removeTab(gBrowser.tabs[gBrowser.visibleTabs.length - 1]); > >+ gBrowser.removeTab(gBrowser.tabs[gBrowser.visibleTabs.length - 1]); > Are these preexisting tabs that the existing test fails to clean up? No, those are tabs that were created by the middle-click on the area link and the normal link in this automated test (this is for testing that autoscroll doesn't show up in those cases). I have to clean up those tabs, otherwise it causes test failures.

It's the general.autoScroll pref, not the general.autoscroll (without camel case) pref. That took me quite some time to figure out, why it didn't seem to work in my previous patch.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=e0e0fd9b7f36

Ok, tryserver green