Closed Bug 295977 Opened 19 years ago Closed 15 years ago

Autoscrolling does not work for elements such as div using overflow

Categories

(Toolkit :: UI Widgets, enhancement)

Product:
Toolkit
Component:
UI Widgets
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.9.3a1

People

(Reporter: destian, Assigned: Swatinem)

References

(Depends on 1 open bug,
URL
)

Details

(Keywords: testcase, Whiteboard: [polish-hard][polish-interactive][polish-p4])

Attachments

(7 files, 14 obsolete files)

Testcase
933 bytes, text/html
Details
Testcase (with varying overflow styles)
4.10 KB, text/html
Details
testcase
577 bytes, text/html
Details
Multi-area testcase webpage using <pre> and <textarea> elements
5.87 KB, text/html
Details
patch, v3 [pushed: comment 67]
10.47 KB, patch
neil
: review+
Details | Diff | Splinter Review
browser-chrome tests + non-multiple select fixup as pushed [pushed: comment 79]
6.46 KB, patch
Swatinem
: review+
Details | Diff | Splinter Review
folded patch for 1.9.2, updated
15.46 KB, patch
Swatinem
: review+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050529 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050529 Firefox/1.0+

The element seems to have focus, as mousescrolling works fine (so do the up/down
page up/down arrows). Autoscroll however does not work.

Reproducible: Always

Steps to Reproduce:
1. Create a <div> with overflow:auto or overflow:scroll which contains enough
data to require scrolling
2. Put the mouse over any point in the <div> and attempt to autoscroll
Actual Results:  
Nothing

Expected Results:  
Autoscrolling
Status: UNCONFIRMED → NEW
Component: Layout → General
Ever confirmed: true
Product: Core → Firefox
-> Firefox per Bug 97283 Comment 242
QA Contact: layout → general
Depends on: 97283
Can you explain what 'autoscrolling' is ?. I've read bug 97283, but I'm no
clearer to understanding what you're trying to achieve.

Autoscrolling is when you middle click and move the mouse up and down to scroll.
(You know, the little white thing comes up with the arrows, and you move your
mouse pointer in relation to scroll.) As this bug states, it doesn't work in a
<div> with overflow: auto or scroll.
Severity: normal → major
You need Tools > Options > Advanced > General > Browsing > Use Auto Scroll
ticked to be able to use autoscroll. Some extensions (all-in-1, et al) interfere
with this option, so best to use a new profile if you're unsure.
This was switched from "Core" to "Firefox," but the bug it stemmed from was a
"Core" bug. Has it been confirmed that this is a Fx only problem? Also, can
anyone confirm that this bug exists on another OS so maybe it will get a little
more attention? It doesn't seem like something that would be confined to XP only.

I use autoscrolling every time I scroll (am I the only one?), so it's killing me
that something as simple as using a div with overflow doesn't work. I never
thought Fx would be the browser limiting my validated xhtml1.1/css design. There
must be some webdevs out there who share my pain.
*** Bug 298269 has been marked as a duplicate of this bug. ***
Attached file Testcase — 
OS: Windows XP → All
Hardware: PC → All

    
    

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

    
    

    
  
The normal mousewheel works on such divs, so vertical scrolling is possible. But horizontal scrolling is only possible by dragging the scrollbar.
On a side note: Opera has this autoscroll problem too.

    
    

    
  
This works in IE7 (not sure about 6) so someone might want to add parity-ie to the whiteboard

    
    

    
  


  
The tree-walking is a hack.  For one thing, it won't work for non-HTML.  The reason I check for the HTML element is that the computed height of <HTML> of a small document can be less than the height of the browser window, but the scrollHeight of the <HTML> is the height of the browser window.  I need a cleaner way to detect an overflowing element (and I should probably verify that it isn't overflow: hidden)
Assignee: nobody → cst
Status: NEW → ASSIGNED
Severity: major → enhancement
Component: General → XUL Widgets
Product: Firefox → Toolkit
Target Milestone: --- → mozilla1.9alpha6
QA Contact: general → xul.widgets

    
    

    
  
Is there any chance that https://bugzilla.mozilla.org/show_bug.cgi?id=257938 had an effect on what needs to be done to fix this?

    
    

    
  
Not really, unless it is implemented in a way that provides a consistent way to check if something is scrollable.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        proof-of-concept patch (obsolete)
        — 
          — Splinter Review
      

    


  
This is a simplistic proof-of-concept with no pretense of its utility.

    
    

    
  
Comment on attachment 293809 [details] [diff] [review]
proof-of-concept patch

Well this is a great start - it almost works, even on textareas!

>-      <field name="_scrollingView">null</field>
>+      <field name="_target">null</field>
I'd name this something more obvious, such as _scrollingElement

>+            this._target = event.originalTarget;
>+            while (this._target.clientHeight >= this._target.scrollHeight ||
>+                   this._target.clientWidth >= this._target.scrollWidth) {
>+                this._target = this._target.parentNode;
>+            }
>+            if (!this._target)
>+                this._target = event.originalTarget.ownerDocument.documentElement;
OK, so this is your problem - you're not finding the correct element. I can see a number of issues here:
1. You're not actually checking that your element is an nsIDOMNSHTMLElement.
2. In fact, you're not even null-testing it inside the loop!
  (And when you reach null, return immediately, no documentElement hacks.)
3. You're requiring elements to have both horizontal and vertical scrolling.

    
    

    
  
Comment on attachment 293809 [details] [diff] [review]
proof-of-concept patch

>             this._scrollingView = event.originalTarget.ownerDocument.defaultView;
This is superfluous.

>+            var scroll_vert = this._target.clientHeight >= this._target.scrollHeight;
>+            var scroll_horz = this._target.clientWidth >= this._target.scrollWidth;        
>+            if (scroll_vert) {
>+              this._autoScrollPopup.setAttribute("scrolldir", scroll_horz ? "NSEW" : "EW");
>             }
>+            else if (scroll_horz) {
>               this._autoScrollPopup.setAttribute("scrolldir", "NS");
>             }
You've got your logic back-to-front. You're setting scroll_vert if you can't scroll vertically, and so this has no chance of doing the right thing.

You've also got some trailing whitespace on some of your lines.

    
    

    
  
This is the what the code looks like once I'd got it working (note that this is still POC so I didn't bother renaming _target):

for (this._target = event.originalTarget; this._target;
     this._target = this._target.parentNode) {
  if (this._target instanceof NSHTMLElement) {
    var vert = this._target.clientHeight < this._target.scrollHeight;
    var horz = this._target.clientWidth < this._target.scrollWidth;
    if (horz) {
      this._autoScrollPopup.setAttribute("scrolldir", vert ? "NSEW" : "EW");
    }
    else if (vert) {
      this._autoScrollPopup.setAttribute("scrolldir", "NS");
    }
    else {
      continue;
    }

    document.popupNode = null;
    this._autoScrollPopup.openPopupAtScreen(event.screenX,
                                            event.screenY, false);
/* etc... */
    this._autoScrollTimer = setInterval(function(self) {
        self.autoScrollLoop(); }, 20, this);
    break;
  }
}
Assignee: cst → mozilla-bugzilla
Status: ASSIGNED → NEW

    
    

    
  

      
      
      
      

        Attached patch
          
          
        corrected autoscroll patch (obsolete)
        — 
          — Splinter Review
      

    


  
Here is a corrected patch. The script is still sometimes trying to scroll the wrong elements. (For example, I encountered this when trying to scroll on certain parts of Google News. I think it happens with elements that use overflow: hidden, but I need to debug more to find out.)

I see two solutions. The first is a function like this:

function canScrollX(elem) {
  // Check whether the element is already scrolled.
  if (elem.scrollLeft) {
    return true;
  }

  // Try scrolling the element.
  elem.scrollLeft++;
  if (elem.scrollLeft) {
    elem.scrollLeft = 0;
    return true;
  }
  else {
    return false;
  }
}

Admittedly, this is still a bit of a hack, since it will trigger an onscroll event and will cause the page to jump when using autoscroll.

The other option is to check the computed style to see if "overflow" is "hidden".

Other ideas?

        Attachment #293809 -
        Attachment is obsolete: true

    
    

    
  


  
Here is an updated patch that handles the problem I mentioned by checking this._scrollingElement's clientWidth and clientHeight against 0.

        Attachment #293915 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 293921 [details] [diff] [review]
check for clientHeight/clientWidth of 0

>+                if (horz) {
>+                  this._autoScrollPopup.setAttribute("scrolldir", vert ? "NSEW" : "EW");
>+                  break;
>+                }
>+                else if (vert) {
>+                  this._autoScrollPopup.setAttribute("scrolldir", "NS");
>+                  break;
>+                }
Nit: else is unnecessary after a break;

    
    

    
  

      
      
      
      

        Attached patch
          
          
        remove extraneous "else" (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #293921 -
        Attachment is obsolete: true

    
  
Target Milestone: mozilla1.9alpha6 → ---

    
    

    
  
I think that it is necessary to consider "border-*-width" for compare with "client*" and "scroll*".  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        to consider "border-*-width" (obsolete)
        — 
          — Splinter Review
      

    


  
I make a patch.
In the range that I used, It seems to function without a problem.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-us; rv:1.9pre) Gecko/2008050506 Minefield/3.0pre

    
    

    
  
Why do you leave a bug for three years?  
I regard this bug as the omission of a basic function. 
A certain patch does not seem to have any problem, but is there already a special reason what it is? 
I want you to wake up an action to be please in time for release of Firefox3.0!

    
    

    
  
Hi Alice,

I share your frustration with the GUI bugs in Firefox that aren't getting fixed.

You can view the list of bugs blocking Firefox 3 RC1 (release candidate 1) by clicking on "Blocking bugs" on this page...
http://wiki.mozilla.org/Releases/Firefox_3.0rc1

The direct link was just too long to post.

I think you *may* be able to get your bug fix added once they start taking fixes for RC2...but I honestly do not know how they handle that.

I would recommend inquiring in the Mozillazine build forum here...
http://forums.mozillazine.org/viewforum.php?f=23

Best of luck! I really want to see this bug fixed too! :-)

    
    

    
  

      
      
      
      

        Attached patch
          
          
         to consider "border-*-width"  (obsolete)
        — 
          — Splinter Review
      

    


  
Sorry, Because the range of the patch was wrong, I revised it

        Attachment #319415 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 319441 [details] [diff] [review]
 to consider "border-*-width" 

Neil, could you take a look at the updated patch?

        Attachment #319441 -
        Flags: review?(neil)

    
    

    
  
Comment on attachment 319441 [details] [diff] [review]
 to consider "border-*-width" 

That scroll element detection code scares me so I'll defer on that to roc!

        Attachment #319441 -
        Flags: superreview?(roc)

    
    

    
  
I made a small extension to fix this Bug.
https://addons.mozilla.org/ja/firefox/addon/7201 (In Sandbox)

    
    

    
  
I don't know why this border calculation is needed. clientWidth is defined to be the width of the padding-box, minus any scrollbars. scrollWidth is defined to be the width of the padding plus the widths of the children.
http://www.w3.org/TR/cssom-view/#scroll-attributes
So when scrollWidth <= clientWidth, things fit and scrolling is not needed. When scrollWidth > clientWidth, scrolling is needed. I'll attach a testcase that seems to confirm this.

    
    

    
  

      
      
      
      

        Attached file
          
        testcase
        — 
      

    


  
The testcase calibrates itself so that the inner DIV exactly fills the available space. Make the inner DIV a little wider, and horizontal scrolling is enabled. The results show that just before horizontal scrolling is enabled, scrollWidth==clientWidth.

    
    

    
  
Comment on attachment 319441 [details] [diff] [review]
 to consider "border-*-width" 

Need response to comment #37

        Attachment #319441 -
        Flags: superreview?(roc) → superreview-

    
    

    
  
Comment on attachment 319441 [details] [diff] [review]
 to consider "border-*-width" 

Also, please don't remove the scrollMaxX and scrollMaxY code, because it's needed for this case:
<html><head></head><body><div style="float: left; height: 3000px;"></div></body></html>
So you need to take care this case works too.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        revised patch (obsolete)
        — 
          — Splinter Review
      

    


  
It corrected according to the comment #37 and comment #40.

The check of border was removed. Instead, overflow attribute confirmed whether to have been visible or not. 
scrollMaxX and scrollMaxY was checked. 

This patch is for Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090108 Minefield/3.2a1pre ID:20090108040418.

        Attachment #319441 -
        Attachment is obsolete: true

        Attachment #319441 -
        Flags: review?(neil)

    
    

    
  
Alice, do you plan to ask for review for the revised patch?

    
    

    
  
(In reply to comment #44)
> Alice, do you plan to ask for review for the revised patch?

Sorry, 
I am not familiar with source tree.
Moreover, it is difficult to write the code for a test in my environment.

Please please ask professional person.

    
    

    
  


  
me:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9.0.5) Gecko/2008120122 Firefox/3.0.5 Ubiquity/0.1.5

A few comments if I may:

- 1) Along with this comment, I'm attaching a custom-made testcase webpage (similar to the one in the now-duped Bug 409032) containing 4 different scrollable areas, using <pre> and <textarea> element tags so that users can test horizontal autoscrolling, vertical autoscrolling, and 4-way(all-way) autoscrolling.

-2) It was suggested a short while ago to add 'polish' Whiteboard terms to outstanding Bugs that we would like to see fixed for Firefox 3.1 -- therefore, could someone with permissions please add the Whiteboard term "[polish-interactive]" as I am unable to.

- 3) FYI: Alice White has published an extension called 'Div AutoScroll' to 'fix' this Bug - the current version is based on the 'revised patch' posted in this Bug Thread (Comment #43). The extension is available here: https://addons.mozilla.org/en-US/firefox/addon/7201

I installed the extension and tested it with my testcase webpage -- it seems to work PERFECTLY! (on XP, anyhow). Thanks Alice!!

-4) Comment #44 and Comment #45 seem to indicate that the 'revised patch' has not yet been flagged for Review.

Alice White did not do so as it seems he/she is not familiar with the 'proper' procedure around here, which is not surprising as: a) if you submit a patch, isn't it obvious you want someone to review it?!; b) the 'requesting for review' procedure is not well documented; and c) English isn't Alice White's first language.

As a week has gone by since Alice White expressed a desire for someone who knows what they're doing to do what's needed next -- and as I'm unable to -- could I ask someone who has permission to do so to set the 'review' flag for Alice's 'revised patch'??

Thanks! (My once long-list of outstanding *really-annoying* Bugs is happily down to just a few).

    
    

    
  
****.. I always mess something up...
re: #2 in Comment #46, that should be add "polish" as a Keyword as "[polish-interactive]" as a Whiteboard term.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Revised patch (hg diff) (obsolete)
        — 
          — Splinter Review
      

    


  
Ok, lets step a bit forward with the bug to get it fixed hopefully soon. Thanks Alice for the patch! I've integrated it into my hg tree and here is the hg-wise patch. And thanks to RenegadeX for the testcase. While testing the functionality everything works fine. Gavin, do you mind reviewing this patch?

        Attachment #356194 -
        Attachment is obsolete: true

        Attachment #357325 -
        Flags: review?(gavin.sharp)

    
    

    
  
I've created a tryserver build. Everybody is encouraged to have a test-run. Please give feedback if it works for you. The builds will appear shortly in this folder:

https://build.mozilla.org/tryserver-builds/2009-01-16_01:31-hskupin@mozilla.com-bug295977/

    
    

    
  
The XP tryserver build works for me using your testcase and Google Reader for testing.

    
    

    
  
Gavin, ping?
Whiteboard: [has patch][needs review gavin]
Flags: blocking1.9.1?

    
    

    
  
This does not block the final release of Firefox 3.1, though we'd accept a reviewed patch (which should probably come with tests, if possible) that's nominated for approval1.9.1?

Enn: can you take this review off Gavin's plate?
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1+

    
  
Flags: blocking1.9.1+

    
    

    
  
Comment on attachment 357325 [details] [diff] [review]
Revised patch (hg diff)

The other Neil seems to have been reviewing earlier iterations of this patch.

        Attachment #357325 -
        Flags: review?(gavin.sharp) → review?(neil)

    
    

    
  
Comment on attachment 357325 [details] [diff] [review]
Revised patch (hg diff)

As I don't have a scroll mouse handy, let's start with some style nits.

>+              if (this._scrollingElement instanceof NSHTMLElement) {
>+                //Prevent selection the child by middle button during scroll
>+                if (this._scrollingElement.localName.toLowerCase() == "select" ){
Not an ideal test, although you have at least already checked that the element is an HTML element, but instanceof HTMLSelectElement would be better.

>+                  this._scrollingElement.parentNode.focus();
What does this achieve?

             }
At this point the indentation seems to go all awry.

>+                // consider border width
I don't think it does any more, actually.

>+                try {
>+                  var doc = this._scrollingElement.ownerDocument;
I think you should get the defaultView outside of this loop.

>+                  var overflowx = doc.defaultView.getComputedStyle(this._scrollingElement, '').
>+                              getPropertyValue('overflow-x');
>+                  var overflowy = doc.defaultView.getComputedStyle(this._scrollingElement, '').
>+                              getPropertyValue('overflow-y');
>+                } catch(ex) {
>+                  var overflowx = "";
>+                  var overflowy = "";
I'd put this before the try/catch (and not repeat the var inside).

>+                                                     (vert &&
>+                                                      overflowy != "hidden" &&
>+                                                      overflowy != "visible") ? "NSEW" : "EW");
>+                  break;
>+                }
>+                if (vert && overflowy != "hidden" && overflowy != "visible") {
To avoid repeating this I'd add the overflow checks into the horz/vert vars.

>+                //if body or html then , default view should use.
What's the benefit of this? I'd say it's better to try the default view if no scrollable element was found.

>+            if ("scrollBy" in elm) {
Another instance of a lack of instanceof.
Whiteboard: [has patch][needs review gavin]

    
    

    
  
Alice, are you willed to work out the latest review comments? That would be fantastic!

    
    

    
  
(In reply to comment #54)
>(From update of attachment 357325 [details] [diff] [review])
>As I don't have a scroll mouse handy, let's start with some style nits.
I got around to trying this on a machine with a scroll mouse, and the patch at least seems to be functionally complete.

>>+                  this._scrollingElement.parentNode.focus();
>What does this achieve?
To answer my own question, this seems to be designed to focus a textarea. Unfortunately it doesn't focus other scrollable regions like a click would.

    
  
Whiteboard: [polish-hard][polish-interactive]

    
    

    
  
P4 - Polish issue that is rarely encountered, and is easily identifiable.

Clearly very broken if you commonly use autoscroll and try to do this, however not that many users commonly take advantage of autoscroll, so that reduces the extent to which this bug will be encountered.
Priority: -- → P4

    
    

    
  
(switching to whiteboard for polish-relative priorities)
Priority: P4 → --
Whiteboard: [polish-hard][polish-interactive] → [polish-hard][polish-interactive][polish-p4]

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  
Unaware of the progress made in this bug, I tried implementing this myself.
Tryserver builds are also awailable: https://build.mozilla.org/tryserver-builds/arpad.borsos@googlemail.com-try-021af6321299/

Not sure if I should request review or let the former patch get updated...?

    
  

        Attachment #394005 -
        Flags: review?(neil)

    
    

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

[I haven't tried this out yet, these are just style nits]

>+            this._scrollingView = parent.ownerDocument.defaultView;
>+            do {
>+              if (parent.ownerDocument && parent == parent.ownerDocument.documentElement)
>+                continue;
You don't need to check parent.ownerDocument, you can hardly click an element without a document and you know its defaultView anyway.

>+              if (parent.namespaceURI && parent.namespaceURI != 'http://www.w3.org/1999/xhtml')
>+                continue;
>+              if (!parent.scrollWidth)
>+                continue;
These contortions are more easily written !(parent instanceof NSElement)

>+            } while (parent = parent.parentNode);
Nit: JavaScript strict warning: "test for equality mistyped as assignment?" To avoid annoying everyone who turns strict warnings on, please separate the assignment and test into separate statements.

>+            if (foundScrollable) {
>+              this._scrollingView = parent;
Hmm, not such a good name for it now...

>+           if (this._scrollingView.scrollBy)
Again, I'd prefer an instanceof test.

>+             this._scrollingView.scrollLeft+= actualScrollX;
>+             this._scrollingView.scrollTop+= actualScrollY;
Nit: space before +=

    
    

    
  
(In reply to comment #60)
> Tryserver builds are also awailable:
> https://build.mozilla.org/tryserver-builds/arpad.borsos@googlemail.com-try-021af6321299/

In Test case of  Comment #26,
The first DIV element (which has overfllow:hidden)should not autoscroll.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch, v2 (obsolete)
        — 
          — Splinter Review
      

    


  
I added the overflow-x/y detection from the former patch and also added checks to make sure that autoscroll does not work on overflow: hidden elements.
Also taken from the former patch is to abort autoscroll inside select elements. That can be very ugly indeed :D

Also added another check to fix the behavior for elements with overflow-x/y: auto; overflow-y/x: hidden;
It should only scroll in the direction that is allowed to scroll.

Other nits should be fixed now.
Assignee: mozilla-bugzilla → arpad.borsos

        Attachment #394005 -
        Attachment is obsolete: true
Status: NEW → ASSIGNED

        Attachment #394045 -
        Flags: review?(neil)

        Attachment #394005 -
        Flags: review?(neil)

    
  
Depends on: 510323

    
    

    
  
(In reply to comment #63)
> Also taken from the former patch is to abort autoscroll inside select elements.
> That can be very ugly indeed :D
I think that's a <select> bug, so I filed bug 510323 on that.

    
    

    
  
Comment on attachment 394045 [details] [diff] [review]
patch, v2

I still think it might be an idea to rename this._scrollingView (I only named it that for consistency with event.target.ownerDocument.defaultView) and maybe just calling it this._scrollable would make more sense now.

>+              if ((parent instanceof Document) || (parent instanceof HTMLHtmlElement) ||
>+                  !(parent instanceof HTMLElement)) {
Nit: redundant test for Document since it's not an HTMLElement.

>+              if (parent instanceof HTMLSelectElement) {
[See my previous comment]

>+              var overflowx = parent.ownerDocument.defaultView.getComputedStyle(parent, '').
>+                              getPropertyValue('overflow-x');
[We've got an unresolved thread in the newsgroups about this... my preference is to have the . at the start of the next line, since otherwise you can accidentally delete it and it will still be syntactially value JavaScript. Should you change this, I would want you to align the . with the previous line, maybe along these lines, or whatever fits in 80 columns, or something:
var overflowx = parent.ownerDocument.defaultView
                      .getComputedStyle(parent, '')
                      .getPropertyValue('overflow-x');
]

>+              var scrollVert = (parent.scrollHeight > parent.offsetHeight &&
>+                                overflowy != 'hidden' && overflowy != 'visible');
Nit: don't need those ()s here.

>+            if (foundScrollable) {
>+              this._scrollingView = foundScrollable;
I'm not sure it helps to set this.scrollingView before the loop. If you didn't, then you could just write this as
if (!this._scrollingView) {
  this._scrollingView = event.target.ownerDocument.defaultView;
  if (this._scrollingView.scrollMaxX > 0) {
etc. thus eliminating foundScrollable.

>+           if (scrolldir == 'EW')
>+             actualScrollY = 0;
>+           if (scrolldir == 'NW')
>+             actualScrollX = 0;
Typo, and this looks ugly to me. I'd rather do something like
if (scrolldir != "NS")
  this._scrollingView.scrollLeft += actualScrollX;

    
    

    
  


  
This can now autoscroll select boxes, yay :-)
The direction check should be more clear now.

        Attachment #268906 -
        Attachment is obsolete: true

        Attachment #293936 -
        Attachment is obsolete: true

        Attachment #357325 -
        Attachment is obsolete: true

        Attachment #394045 -
        Attachment is obsolete: true

        Attachment #394790 -
        Flags: review?(neil)

        Attachment #357325 -
        Flags: review?(neil)

        Attachment #394045 -
        Flags: review?(neil)

    
  

        Attachment #394790 -
        Flags: review?(neil) → review+

    
    

    
  
Comment on attachment 394790 [details] [diff] [review]
patch, v3 [pushed: comment 67]

http://hg.mozilla.org/mozilla-central/rev/859f85e86b0d

        Attachment #394790 -
        Attachment description: patch, v3 → patch, v3 [pushed: comment 67]

    
    

    
  
According to http://mxr.mozilla.org/mozilla-central/source/toolkit/content/tests/browser/Makefile.in#53 synthesizing middle button clicks on linux is not possible. :(
I'll still try to come up with an automated test for this.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite?
Resolution: --- → FIXED

    
    

    
  
Verfied fixed with builds on OS X, Linux and Windows like Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20090821 Minefield/3.7a1pre ID:20090821030931

Arpad, you should ask for approval1.9.2 on your latest patch so we can get the fix into 1.9.2 too.
Status: RESOLVED → VERIFIED
Target Milestone: --- → mozilla1.9.3a1

    
    

    
  
You would have to ask for approval on bug 510323 too.

    
    

    
  


  
I found a small glitch: It tried to autoscroll select elements which were NOT multiple.
Also added automated browser-chrome tests. Despite the comment in the makefile, they work fine on linux... well half of the time at least.
I have also pushed the patch to tryserver to verify the tests on other platforms.

I will fold the patches together and request approval for 1.9.2 once I get this fix and tests into m-c

        Attachment #396053 -
        Flags: review?(neil)

    
    

    
  
Comment on attachment 396053 [details] [diff] [review]
browser-chrome tests + non-multiple select fixup

>+  
Nit: whitespace

>+      gBrowser.contentWindow.setTimeout(function() {
Using setTimeout doesn't look like a good idea to me but I'm not sufficiently familiar with tests to have any better ideas; please double-check with someone else?

        Attachment #396053 -
        Flags: review?(neil) → review+

    
    

    
  


  
> Using setTimeout doesn't look like a good idea to me but I'm not sufficiently
> familiar with tests to have any better ideas; please double-check with someone
> else?

setTimeouts are ugly, indeed. I added a comment explaining why I believe they are necessary. Also unified/simplified the test code a bit.

        Attachment #396053 -
        Attachment is obsolete: true

        Attachment #396435 -
        Flags: review+

    
    

    
  
Comment on attachment 396435 [details] [diff] [review]
browser-chrome tests + non-multiple select fixup, v2 [backed out]

http://hg.mozilla.org/mozilla-central/rev/72c1f97df4c6

        Attachment #396435 -
        Attachment description: browser-chrome tests + non-multiple select fixup, v2 → browser-chrome tests + non-multiple select fixup, v2 [pushed: comment 74]

    
    

    
  

      
      
      
      

        Attached patch
          
          
        folded patch for 1.9.2 (obsolete)
        — 
          — Splinter Review
      

    


  
Folded together patch v3 and browser-chrome tests.
The original patch has been baking for more than a week, the tests patch was checked in just now.

Performance impact: none
Risk: none

        Attachment #397232 -
        Flags: review+

        Attachment #397232 -
        Flags: approval1.9.2?
Flags: in-testsuite? → in-testsuite+

    
    

    
  
Mac U and E boxes have these failures:
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_bug471962.js | TypeError: gBrowser.contentDocument.getElementById("postForm") is null
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_keyevents_during_autoscrolling.js | Timed out

Windows is fine. Linux skips those tests (see comment 68)

Not sure if the mac failures are affected by my changes. Mac tryserver did not have those failures.
I will leave this in for at least another cycle.

    
    

    
  
(In reply to comment #74)
> (From update of attachment 396435 [details] [diff] [review])
> http://hg.mozilla.org/mozilla-central/rev/72c1f97df4c6

(In reply to comment #76)
> Mac U and E boxes have these failures:
> TEST-UNEXPECTED-FAIL |
> chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_bug471962.js
> | TypeError: gBrowser.contentDocument.getElementById("postForm") is null
> TEST-UNEXPECTED-FAIL |
> chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_keyevents_during_autoscrolling.js
> | Timed out
> 
> Windows is fine. Linux skips those tests (see comment 68)
> 
> Not sure if the mac failures are affected by my changes. Mac tryserver did not
> have those failures.
> I will leave this in for at least another cycle.

Backed out, as it kept failing in most runs on OS X. Maybe related to bug 512100?

    
  

        Attachment #396435 -
        Attachment description: browser-chrome tests + non-multiple select fixup, v2 [pushed: comment 74] → browser-chrome tests + non-multiple select fixup, v2 [backed out]
Flags: in-testsuite+ → in-testsuite?

    
    

    
  
Comment on attachment 397232 [details] [diff] [review]
folded patch for 1.9.2

Clearing approval flag until test issues are sorted out.

        Attachment #397232 -
        Flags: approval1.9.2?

    
    

    
  


  
I updated the patch with the same changes dao did in bug 521557 and pushed it.
Tryserver passed the tests so it shouldn't be a problem on m-c.

http://hg.mozilla.org/mozilla-central/rev/c19711e6eef2

        Attachment #396435 -
        Attachment is obsolete: true

        Attachment #406676 -
        Flags: review+

    
    

    
  
This appears to have gotten a 1.7 - 2% Ts win on windows vista.

    
    

    
  
It seems to me it would be very bad if this would give Ts wins.

    
    

    
  
The recently checked in patch was a oneliner + tests, highly unlikely of causing a Ts win. Are you sure there isn't something wrong with the script that automatically detects wins/regressions?
Flags: in-testsuite? → in-testsuite+

    
    

    
  


  
Again, folded both the patch itself and the tests together.

The patch has been in for two month now without any complaits, the tests are running since two days without any problems.

The tests depend on bug 521557 and bug 486342 to apply/run cleanly.

        Attachment #397232 -
        Attachment is obsolete: true

        Attachment #406905 -
        Flags: review+

        Attachment #406905 -
        Flags: approval1.9.2?

    
    

    
  
Comment on attachment 406905 [details] [diff] [review]
folded patch for 1.9.2, updated

approval1.9.2 requests aren't currently being monitored, since we're nearing RC freeze and there are too many outstanding requests, so I'm clearing this request. Feel free to re-request approval if you are confident that it's worth drivers' time to consider whether this non-blocker needs to land for 1.9.2 at this stage.

        Attachment #406905 -
        Flags: approval1.9.2?
Depends on: 535907
Depends on: 545970
Depends on: 558945

    
  
Depends on: 561989
Depends on: 614643

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: