Autoscroll activates when middle-clicking document scrollbars

VERIFIED FIXED in Firefox1.0beta

Status

()

Firefox
General
P3
normal
VERIFIED FIXED
15 years ago
12 years ago

People

(Reporter: Chad Daelhousen, Assigned: Alexey Chernyak)

Tracking

(Blocks: 1 bug, {fixed-aviary1.0})

unspecified
Firefox1.0beta
fixed-aviary1.0
Points:
---
Dependency tree / graph
Bug Flags:
blocking-aviary1.0 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 10 obsolete attachments)

(Reporter)

Description

15 years ago
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

15 years ago
Confirming bug with 20030907 build on W2K.
Taking QA Contact. Setting Platform/OS to All.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
QA Contact: asa → bugzilla
Hardware: PC → All

Comment 2

14 years ago

*** This bug has been marked as a duplicate of 216899 ***
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → DUPLICATE

Comment 3

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

Comment 4

14 years ago
Reopening.  I agree with Dean.  This is still around and has nothing to do
ContentLoadURL (the bug it was duped against).
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---

Comment 5

14 years ago
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).
Assignee: firefox → steffen.wilberg
Status: REOPENED → NEW
Depends on: 213250
Target Milestone: --- → Firefox0.9

Comment 6

14 years ago
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

14 years ago
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

14 years ago
Created attachment 141487 [details] [diff] [review]
patch

This is what it looks like.

Comment 9

14 years ago
Made Bug 212273 depend on this.
Blocks: 212273

Comment 10

14 years ago
CCing some people from bug 213250.
Status: NEW → ASSIGNED

Comment 11

14 years ago
Created attachment 141825 [details] [diff] [review]
patch for the new code

Here's the patch I announced in comment 5. Comments welcome!
Attachment #141487 - Attachment is obsolete: true

Comment 12

14 years ago
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

14 years ago
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?
(Assignee)

Comment 14

14 years ago
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

14 years ago
I've found out something very very simple. I'll make a new patch later today.
(Assignee)

Comment 16

14 years ago
Created attachment 141898 [details] [diff] [review]
new patch

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

How does this look?
Attachment #141825 - Attachment is obsolete: true

Comment 17

14 years ago
Created attachment 141917 [details] [diff] [review]
simple patch

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

Comment 18

14 years ago
Comment on attachment 141917 [details] [diff] [review]
simple patch

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

Comment 19

14 years ago
Created attachment 141936 [details] [diff] [review]
new patch v1.1

made middleclick-autoscroll in scrollbars depend on
middlemouse.scrollbarPosition property, as per IRC discussion.
Added handling of <scrollcorner> as well.
Assignee: steffen.wilberg → alexey
Attachment #141898 - Attachment is obsolete: true
Attachment #141917 - Attachment is obsolete: true

Comment 20

14 years ago
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.
(Assignee)

Comment 21

14 years ago
(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.
(Assignee)

Comment 22

14 years ago
Created attachment 141953 [details] [diff] [review]
patch v1.2

1. Prevented autoscroll checks from being run on left click.
2. Cleaned up by removing trailing whitespaces across the document.
Attachment #141936 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #141953 - Flags: review?(firefox)
(Assignee)

Comment 23

14 years ago
Created attachment 141954 [details]
functions code

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

Comment 24

14 years ago
> 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

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

Please make a new patch that doesn't change so much whitespace.
Attachment #141953 - Flags: review?(firefox)
(Assignee)

Comment 26

14 years ago
Created attachment 142001 [details] [diff] [review]
patch v1.2.1

same patch
much less whitespace cleaning.
Attachment #141953 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #142001 - Flags: review?(firefox)
(Assignee)

Comment 27

14 years ago
Created attachment 142355 [details] [diff] [review]
patch v1.3

2 nits:
made the scrollbar check clearer ro understand and set it aside into an 'else'
Attachment #141954 - Attachment is obsolete: true
Attachment #142001 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #142001 - Flags: review?(firefox)
(Assignee)

Updated

14 years ago
Attachment #142355 - Flags: review?(firefox)

Updated

14 years ago
Flags: blocking1.0?
+ing to invoke review
Flags: blocking1.0? → blocking1.0+
(Assignee)

Comment 29

14 years ago
Created attachment 148586 [details] [diff] [review]
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.
Attachment #142355 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #142355 - Flags: review?(firefox)
(Assignee)

Updated

14 years ago
Attachment #148586 - Flags: review?(firefox)
Priority: -- → P3
Target Milestone: Firefox0.9 → Firefox1.0
(Assignee)

Comment 30

14 years ago
Created attachment 149150 [details] [diff] [review]
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.
Attachment #148586 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #148586 - Flags: review?(firefox)
(Assignee)

Updated

14 years ago
Attachment #149150 - Flags: review?(firefox)
(Assignee)

Comment 31

14 years ago
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

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

Moving review request to Mike. Patch is from alexey@optus.net. Applies cleanly
and works fine.
Attachment #149150 - Flags: review?(firefox) → review?(mconnor)
Comment on attachment 149150 [details] [diff] [review]
patch v1.5

will get to this sometime this week
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.
Attachment #149150 - Flags: review?(mconnor) → review+

Updated

14 years ago
Attachment #149150 - Flags: approval-aviary?

Comment 35

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

a=asa (on behalf of aviary drivers) for checkin to the aviary branch.
Attachment #149150 - Flags: approval-aviary? → approval-aviary+

Comment 36

14 years ago
Checked in br & trunk 07/30/2004 05:14, without the whitespace/formatting-changes.
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago14 years ago
Keywords: fixed-aviary1.0
Resolution: --- → FIXED
Target Milestone: Firefox1.0 → Firefox1.0beta

Updated

12 years ago
Status: RESOLVED → VERIFIED
QA Contact: bugzilla → general
You need to log in before you can comment on or make changes to this bug.