218686 - Autoscroll activates when middle-clicking document scrollbars

Status: VERIFIED FIXED

(Firefox :: General, defect, P3)

Description

[Chad Daelhousen]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5b) Gecko/20030904 Firebird/0.6.1+
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5b) Gecko/20030904 Firebird/0.6.1+

Observed in gtk1 & gtk2 cvs builds, and official gtk2 nightlies: middle-clicking
a scrollbar on the main document to activate the "jump to location" behavior
also activates autoscroll. This also happens in the blank square where the
scrollbars meet.

Reproducible: Always

Steps to Reproduce:
1. Ensure autoscroll is being used.
2. Navigate to a page that can be scrolled.
3. Middle-click one of the main scrollbars. (TEXTAREA scrollbars do not exhibit
the problem.)
Actual Results:  
Autoscroll activates as well as the expected jump of the scrollbar.

Expected Results:  
Only a jump of the scrollbar should have occurred.

Comment 1

[Simon Paquet [:sipaq]]

Confirming bug with 20030907 build on W2K.
Taking QA Contact. Setting Platform/OS to All.

Comment 2

[Ari Pollak]

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

Comment 3

[:deanis74]

I don't think this is a duplicate of that bug.  I suggest re-opening this.

Comment 4

[Robert Parenton]

Reopening.  I agree with Dean.  This is still around and has nothing to do
ContentLoadURL (the bug it was duped against).

Comment 5

[Steffen Wilberg]

I have a fix for this. We need to block autoscroll if 
|findParentNode(node, "scrollbar")| is true. I'll make a patch as soon as bug
213250 is fixed, which changes the same code (method isLink, to be renamed in
isAutoscrollBlocker, in browser.xul).

Comment 6

[:deanis74]

I haven't looked at the autoscroll code at all, but can we do it in a similar
manner to how the click-hold timer ignores scrollbars, by checking the tag and
comparing it against nsXULAtoms::scrollbar?

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsEventStateManager.cpp&branch=&root=/cvsroot&subdir=mozilla/content/events/src&command=DIFF_FRAMESET&rev1=1.244&rev2=1.245

Comment 7

[Steffen Wilberg]

The autoscroll code entirely lives in
mozilla/toolkit/content/widgets/browser.xml, especially the handlers mousedown,
mousemove, mouseup. Autoscroll starts on mousedown, which checks if autoscroll
is enabled, and if you didn't click on a link or similar items which block
autoscroll (method isLink). The actual scrolling is done in the method
autoScrollLoop, which does a scrollBy(x, y).

The code I suggested was inspired by this:
http://lxr.mozilla.org/mozilla/source/browser/base/content/browser.js#3518

Comment 8

[Steffen Wilberg]

Attachment: patch

This is what it looks like.

Comment 9

[Robert Parenton]

Made Bug 212273 depend on this.

Comment 10

[Steffen Wilberg]

CCing some people from bug 213250.

Comment 11

[Steffen Wilberg]

Attachment: patch for the new code

Here's the patch I announced in comment 5. Comments welcome!

Comment 12

[Jon Henry]

Works over here on Linux. Could there, in theory, be a false positive on this if
you're viewing some random XML dialect which contains an element called scrollbar? 

Comment 13

[Steffen Wilberg]

Sure, the patch prevents autoscrolling from any scrollbars, that is the main
scrollbars, in-document scrollbars and just any <scrollbar> element. 
Put this in a file:
  <p>Test 1</p>
  <scrollbar>Test 2</scrollbar>
You can autoscroll on Test 1 (or anywhere else), but you can't on Test 2.
Do you think that is a problem?

Comment 14

[Alexey Chernyak]

Comment on attachment 141825 [details] [diff] [review]
patch for the new code

Ok, the behaviour of autoscroll with non-html XML data is questionable and open
for debate.

I see one big problem with this patch. The function complexity unnessesarily
becomes O(n^2).

Not only the node tree gets traversed from node to it's parent by the function
itself, but findParentNode() additionally traverses it for every single node in
every recursive call of this function.

Also now I'm thinking that perhaps recursion is not the best way to implement
this function in the first place. It places unnecessary hit on the stack (the
parameters for each recursive function call should be stored somewhere,
right?), which could be avoided with a simple while loop.

Still the question of appropriate behaviour for non HTML documents remains.
Should autoscroll work in non-HTML documents?
Should autoscroll be blocked only in XUL namespace <scrollbar>, or in every XML
<scrollbar> element even when we know nothing about that element and it's
functionality but it's name?

Also if you create another patch, could you please change node.nodeName to
node.localName in the current code.

Comment 15

[Steffen Wilberg]

I've found out something very very simple. I'll make a new patch later today.

Comment 16

[Alexey Chernyak]

Attachment: new patch

I completely reworked the function.
In this patch the autoscroll is blocked by XUL <scrollbar> elements only.

How does this look?

Comment 17

[Steffen Wilberg]

Attachment: simple patch

WTF!? This is what I found out (after experimenting with a while loop and a
couple of dumps).

Comment 18

[Alexey Chernyak]

Comment on attachment 141917 [details] [diff] [review]
simple patch

That'll still activate autoscroll when middle-clicking on scrollbar arrow
buttons.

Comment 19

[Alexey Chernyak]

Attachment: new patch v1.1

made middleclick-autoscroll in scrollbars depend on
middlemouse.scrollbarPosition property, as per IRC discussion.
Added handling of <scrollcorner> as well.

Comment 20

[Steffen Wilberg]

Ouch, it took me some time to see the recursion in the original function. :-/
Sure, let's have a while loop.

If you add this inside your while loop:
  dump("nodeName: "+node.nodeName+" localName: "+node.localName+"\n");
and comment out the scrollbar check, you get this output on the console when
clicking on the scrollbar:
  nodeName: xul:slider localName: slider
  nodeName: scrollbar localName: scrollbar
  nodeName: HTML localName: HTML
  nodeName: #document localName: null

If you click on a scrollcorner, you get this:
  nodeName: xul:scrollbarbutton localName: scrollbarbutton
  nodeName: scrollbar localName: scrollbar
  nodeName: HTML localName: HTML
  nodeName: #document localName: null

So your check for "scrollcorner" doesn't find anything, but it works
nonetheless, because the check for "scrollbar" catches both the slider and the
scrollcorner/scrollbarbutton.
But I'd suggest to check if the nodeName (localName doesn't contain the "xul:"
part) is "xul:slider" or "xul:scrollbarbutton", because these _are_ XUL elements.
The advantage is that you don't need to check the namespaceURI:
  if (mmScrollbarPosition && (node.nodeName == "xul:slider" || node.nodeName ==
"xul:scrollbarbutton"))
    return true;

While working on this, I also noticed that isAutoscrollBlocker is called on
left-clicks as well. We could add "event.button != 1 ||" to the if in the
mousedown handler (line 745 with your patch applied) to prevent that.

Comment 21

[Alexey Chernyak]

(In reply to comment #20)
> If you click on a scrollcorner, you get this:
>   nodeName: xul:scrollbarbutton localName: scrollbarbutton
>   nodeName: scrollbar localName: scrollbar
>   nodeName: HTML localName: HTML
>   nodeName: #document localName: null

You are confusing a scrollbutton with a scrollcorner.

> But I'd suggest to check if the nodeName (localName doesn't contain the "xul:"
> part) is "xul:slider" or "xul:scrollbarbutton", because these _are_ XUL elements.

node.localName will never contain "xul:" part. No checks are needed.

> The advantage is that you don't need to check the namespaceURI:

The namespace is checked to make sure the <scrollbar> element is in fact from
XUL realm, and not some custom XML. The behaviour of autoscroll while
representing unknown XML data is outside of the scope of this bug.

> While working on this, I also noticed that isAutoscrollBlocker is called on
> left-clicks as well. We could add "event.button != 1 ||" to the if in the
> mousedown handler (line 745 with your patch applied) to prevent that.

I have also noticed this, but didn't look into it.
I'll attach a patch to fix this as well.

Comment 22

[Alexey Chernyak]

Attachment: patch v1.2

1. Prevented autoscroll checks from being run on left click.
2. Cleaned up by removing trailing whitespaces across the document.

Comment 23

[Alexey Chernyak]

Attachment: functions code

The patch is a bit hard to read, here is the code of changed functions.

Comment 24

[Steffen Wilberg]

> You are confusing a scrollbutton with a scrollcorner.
Ah, indeed. Scrollcorner is where the horizontal and vertical scrollbars meet.
It's only visible if both scrollbars are shown...

> node.localName will never contain "xul:" part. No checks are needed.
Of course it doesn't. My idea was to use nodeName instead of localName in the
if-clause, because the "xul:" in "xul:slider" and "xul:scrollbarbutton" shows
that they're xul elements. But this is not the case with the scrollcorner, i.e.
it's nodeName is just "scrollcorner" and not "xul:scrollcorner".

Egg on my head, your patch is just fine!

Comment 25

[Blake Ross]

Comment on attachment 141953 [details] [diff] [review]
patch v1.2

Please make a new patch that doesn't change so much whitespace.

Comment 26

[Alexey Chernyak]

Attachment: patch v1.2.1

same patch
much less whitespace cleaning.

Comment 27

[Alexey Chernyak]

Attachment: patch v1.3

2 nits:
made the scrollbar check clearer ro understand and set it aside into an 'else'

Comment 28

[Ben Goodger (use ben at mozilla dot org for email)]

+ing to invoke review

Comment 29

[Alexey Chernyak]

Attachment: patch v1.4

Removed unnecessary double call to getElementsByTagName('body') and instead
inserted the autoscroll image into document.documentElement.
As a result, removed _clientFrameBody field.

Comment 30

[Alexey Chernyak]

Attachment: patch v1.5

Redone the clicked element check again and reduced its code.
Got rid of string comparisons.
Got rid of special XHTML handling and lower case conversion.

Comment 31

[Alexey Chernyak]

I noticed in Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8a)
Gecko/20040424 Firefox/0.8.0+ that autoscroll icon appears below scrollbars and
is hidden by them.
I see less and less reason to keep autoscroll functioning while clicking on
scrollbars.

Comment 32

[Steffen Wilberg]

Comment on attachment 149150 [details] [diff] [review]
patch v1.5

Moving review request to Mike. Patch is from [e-mail]. Applies cleanly
and works fine.

Comment 33

[Mike Connor [:mconnor]]

Comment on attachment 149150 [details] [diff] [review]
patch v1.5

will get to this sometime this week

Comment 34

[Mike Connor [:mconnor]]

Comment on attachment 149150 [details] [diff] [review]
patch v1.5

>-         
>-             if(Math.abs(x) > 0 && Math.abs(y) > 0)
>-             {
>+
>+             if (Math.abs(x) > 0 && Math.abs(y) > 0) {
>                if (this._scrollCount++ % 2)

r=me minus all of the random whitespace/formatting changes, like this.

>-         
>+

and this, etc.

Comment 35

[Asa Dotzler [:asa]]

Comment on attachment 149150 [details] [diff] [review]
patch v1.5

a=asa (on behalf of aviary drivers) for checkin to the aviary branch.

Comment 36

[Steffen Wilberg]

Checked in br & trunk 07/30/2004 05:14, without the whitespace/formatting-changes.