Closed
Bug 218686
Opened 21 years ago
Closed 20 years ago
Autoscroll activates when middle-clicking document scrollbars
Categories
(Firefox :: General, defect, P3)
Firefox
General
Tracking
()
VERIFIED
FIXED
Firefox1.0beta
People
(Reporter: loonxtall, Assigned: alexeyc2003)
References
Details
(Keywords: fixed-aviary1.0)
Attachments
(1 file, 10 obsolete files)
6.13 KB,
patch
|
mconnor
:
review+
asa
:
approval-aviary+
|
Details | Diff | Splinter Review |
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•21 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•21 years ago
|
||
*** This bug has been marked as a duplicate of 216899 ***
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → DUPLICATE
I don't think this is a duplicate of that bug. I suggest re-opening this.
Comment 4•21 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•21 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
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•21 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•21 years ago
|
||
This is what it looks like.
Comment 11•21 years ago
|
||
Here's the patch I announced in comment 5. Comments welcome!
Attachment #141487 -
Attachment is obsolete: true
Comment 12•21 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•21 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•21 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•21 years ago
|
||
I've found out something very very simple. I'll make a new patch later today.
Assignee | ||
Comment 16•21 years ago
|
||
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•21 years ago
|
||
WTF!? This is what I found out (after experimenting with a while loop and a couple of dumps).
Assignee | ||
Comment 18•21 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•21 years ago
|
||
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•21 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•21 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•21 years ago
|
||
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•21 years ago
|
Attachment #141953 -
Flags: review?(firefox)
Assignee | ||
Comment 23•21 years ago
|
||
The patch is a bit hard to read, here is the code of changed functions.
Comment 24•21 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•21 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•21 years ago
|
||
same patch much less whitespace cleaning.
Attachment #141953 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #142001 -
Flags: review?(firefox)
Assignee | ||
Comment 27•20 years ago
|
||
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•20 years ago
|
Attachment #142001 -
Flags: review?(firefox)
Assignee | ||
Updated•20 years ago
|
Attachment #142355 -
Flags: review?(firefox)
Updated•20 years ago
|
Flags: blocking1.0?
Assignee | ||
Comment 29•20 years ago
|
||
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•20 years ago
|
Attachment #142355 -
Flags: review?(firefox)
Assignee | ||
Updated•20 years ago
|
Attachment #148586 -
Flags: review?(firefox)
Updated•20 years ago
|
Priority: -- → P3
Target Milestone: Firefox0.9 → Firefox1.0
Assignee | ||
Comment 30•20 years ago
|
||
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•20 years ago
|
Attachment #148586 -
Flags: review?(firefox)
Assignee | ||
Updated•20 years ago
|
Attachment #149150 -
Flags: review?(firefox)
Assignee | ||
Comment 31•20 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•20 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 33•20 years ago
|
||
Comment on attachment 149150 [details] [diff] [review] patch v1.5 will get to this sometime this week
Comment 34•20 years ago
|
||
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•20 years ago
|
Attachment #149150 -
Flags: approval-aviary?
Comment 35•20 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•20 years ago
|
||
Checked in br & trunk 07/30/2004 05:14, without the whitespace/formatting-changes.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago → 20 years ago
Keywords: fixed-aviary1.0
Resolution: --- → FIXED
Target Milestone: Firefox1.0 → Firefox1.0beta
Updated•19 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.
Description
•