251903 - autoscroll does not function as expected when in an iframe

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Luke Montague]

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.

Comment 1

[Luke Montague]

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

Comment 2

[Tuukka Tolvanen (sp3000)]

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

Comment 3

[Tuukka Tolvanen (sp3000)]

whoops. bad dup.

Comment 4

[Stefan Geisseler]

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...

Comment 5

[Kyle Krafka]

(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.

Comment 6

[Kyle Krafka]

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....

Comment 7

[Martijn Wargers (dead)]

Attachment: patchv1

Comment 8

[Martijn Wargers (dead)]

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.

Comment 9

[Martijn Wargers (dead)]

Attachment: patchv2

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.

Comment 10

[Adam Guthrie]

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

Comment 11

[Ria Klaassen (not reading all bugmail)]

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

Comment 12

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Neil, this affects us too.

Comment 13

[[e-mail]]

(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.

Comment 14

[Jonathan Protzenko [:protz]]

: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...

Comment 15

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

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

Comment 16

[natanji]

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.

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Comment 19

[Pekka Laihonen]

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.

Comment 20

[Martijn Wargers (dead)]

Attachment: 251903 _iframe_middleclick.html

testcase

Comment 21

[Martijn Wargers (dead)]

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

Comment 22

[Martijn Wargers (dead)]

Attachment: 251903 _iframe_middleclick2.html

Comment 23

[Martijn Wargers (dead)]

Attachment: 251903.diff

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 24

[Arpad Borsos [:Swatinem]]

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.

Comment 25

[Martijn Wargers (dead)]

Attachment: 251903_v2.diff

(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 26

[[e-mail]]

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
}

Comment 27

[[e-mail]]

(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 28

[[e-mail]]

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 29

[Arpad Borsos [:Swatinem]]

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 [e-mail] 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.

Comment 30

[Martijn Wargers (dead)]

(In reply to [e-mail] 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.

Comment 31

[Martijn Wargers (dead)]

(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.

Comment 32

[Martijn Wargers (dead)]

Attachment: 251903_v3.diff

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

Comment 33

[[e-mail]]

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.

Comment 34

[Martijn Wargers (dead)]

(In reply to [e-mail] 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)

Comment 35

[Martijn Wargers (dead)]

Attachment: 251903_v3.diff (for check-in)

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

Comment 36

[Martijn Wargers (dead)]

Tryserver builds that people can test with are here: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-9c3247660887/

Comment 37

[Martijn Wargers (dead)]

Tryserver is green.

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/1340475e6954

Comment 39

[Martijn Wargers (dead)]

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.

Comment 40

[Carsten Book [:Tomcat]]

(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

Comment 41

[Martijn Wargers (dead)]

Attachment: 251903_v4.diff

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

Comment 42

[[e-mail]]

(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.

Comment 43

[Martijn Wargers (dead)]

(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.

Comment 44

[Neil Deakin]

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 45

[[e-mail]]

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.

Comment 46

[Martijn Wargers (dead)]

(In reply to [e-mail] 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.

Comment 47

[[e-mail]]

(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.

Comment 48

[Matt Brubeck (:mbrubeck)]

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.

Comment 49

[Martijn Wargers (dead)]

(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.

Comment 50

[[e-mail]]

(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.

Comment 51

[Martijn Wargers (dead)]

Attachment: 251903_v5.diff

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.

Comment 52

[Martijn Wargers (dead)]

And I've set the general.autoscroll pref to false in the test_paste_selection.html file in the patch.

(In reply to [e-mail] 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 53

[[e-mail]]

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?

Comment 54

[Martijn Wargers (dead)]

Attachment: 251903_v6.diff

(In reply to [e-mail] 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.

Comment 55

[Martijn Wargers (dead)]

Attachment: 251903_v6.diff

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.

Comment 56

[Martijn Wargers (dead)]

Attachment: 251903_v6.diff (for check-in)

Comment 57

[Martijn Wargers (dead)]

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

Comment 58

[Martijn Wargers (dead)]

Ok, tryserver green

Comment 59

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1be6287cde8a

Comment 60

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1be6287cde8a