212002 - autoscroll icon appears on pages that cannot scroll (have no scrollbars)

Status: RESOLVED WORKSFORME

(Firefox :: General, defect)

Description

[Will Levine]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5a) Gecko/20030706 Mozilla Firebird/0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5a) Gecko/20030706 Mozilla Firebird/0.6

On a page that whose content is small enough so that there is no scrolling,
middle-click produces a useless four-way pointed icon.

Reproducible: Always

Steps to Reproduce:
1. go to about:blank
2. middle-click
3.

Actual Results:  
icon appeared

Expected Results:  
icon should not have appeared

this is the way it is done in all the other programs i've used autoscroll, in so
it would make sense for us to do it like that also.

Comment 1

[noririty]

checked in

Comment 2

[Will Levine]

verified

Comment 3

[noririty]

backing out

Comment 4

[Phil Ringnalda (:philor)]

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

Comment 5

[Daniel C]

(In reply to comment #3)
> backing out

Using a 1.8b2 Gecko/20050305 Firefox/1.0+ build this bug occurs. A page with no
scrollbars displays the 4-way autoscroll cursor. A page with horizontal
scrollbars only also displays a 4-way autoscroll cursor.

Related to https://bugzilla.mozilla.org/show_bug.cgi?id=216035?

Comment 6

[Martijn Wargers (dead)]

Attachment: patch

This works for me.
However, you still get to see the autoscroll marker in cases where the root
element (or the body element in quirks mode) has style overflow:hidden.

Comment 7

[Martijn Wargers (dead)]

Comment on attachment 185430 [details] [diff] [review]
patch

Anyway, this is good enough. The overflow:hidden case can be solved, but I'll
do that in an eventuel other patch.

Comment 8

[Martijn Wargers (dead)]

Comment on attachment 185430 [details] [diff] [review]
patch

The patch in bug 251903 would override this patch.

Comment 9

[Martijn Wargers (dead)]

Attachment: patchv2

Ok, with the fix for bug 296315, it is now possible to detect if a document has
scrollbars or not.
This patch makes use of that.
It is really a ridiculous long line of code in one place, but I don't think I
can avoid that.

Comment 10

[Wayne Mery (:wsmwk)]

Martijn, is there an issue with the patch? Or is it only hung up on review?  (removed about:blank from URL - AFAICT it is no longer a valid testcase)

Comment 12

[Martijn Wargers (dead)]

Comment on attachment 186608 [details] [diff] [review]
patchv2

It's "hung up on review".

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 186608 [details] [diff] [review]
patchv2

>Index: toolkit/content/widgets/browser.xml

>+              this._clientFrameDoc = event.originalTarget.ownerDocument;
>+              var scrollObject = this._clientFrameDoc.defaultView.QueryInterface(Components.interfaces.nsIInterfaceRequestor).getInterface(Components.interfaces.nsIWebNavigation).QueryInterface(Components.interfaces.nsIDocShell).QueryInterface(Components.interfaces.nsIScrollable);
>+              var h={}; var v={};
>+              scrollObject.getScrollbarVisibility(v,h);
>+              if (!this.autoscrollEnabled ||
>+                 ((this._clientFrameDoc.defaultView.scrollMaxY == 0)
>+                 && (this._clientFrameDoc.defaultView.scrollMaxX == 0)) ||
>+                 ((h.value == false) && (v.value == false)) ||
>+                 this.isAutoscrollBlocker(event.originalTarget))
>+                return;

It would probably be a good idea to create a local variable for this._clientFrameDoc.defaultView, with a shorter name (defView?). You can remove the QI to nsIDocShell, it isn't needed.

I'd also use !(h.value || v.value) instead of ((h.value == false) && (v.value == false)). Do we need to worry about getScrollbarVisibility throwing?

>             this._clientFrameDoc = event.originalTarget.ownerDocument;

This should be removed now that you have it already above.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

I also wouldn't bother with the scrollObject variable and just call scrollObject directly, and indent that long line appropriately.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #14)
> and just call scrollObject directly

I mean "call getScrollbarVisibility directly", of course.

Comment 16

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

I can't reproduce this in Firefox 2.0.0.x or my trunk build... when did it get fixed?

Comment 17

[Martijn Wargers (dead)]

Attachment: testcase

There is still the case when autoscroll appears when body has overflow: hidden.
However, I tend to think it's better to not fix that.

Comment 18

[Romain BEZUT]

Cannot reproduce that bug either.
This bug could be marked as fixed.

Comment 19

[u212413]

you are right, this bug themes to be fixed.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Resolving WORKSFORME since no specific checkin is known to have fixed it. If someone really feels like figuring out what fixed it, feel free to marked it fixed noting that bug.