388583 - Autocomplete dropdown doesn't update when scrolling with scrollbar

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Toh Yanxi]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1.5) Gecko/20070713 Firefox/2.0.0.5
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1.5) Gecko/20070713 Firefox/2.0.0.5

in the place where u enter url(s), when u click for the dropdown menu, the scrollbar can move, but the url(s) don't change. It's as if they were stuck.

Reproducible: Always

Steps to Reproduce:
1.open firefox
2.click on the dropdown menu
3.scroll
Actual Results:  
nothing happens.

Expected Results:  
display all my previously browsed addresses.

Comment 1

[Adam Guthrie]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.5) Gecko/20070713 Firefox/2.0.0.5

Confirmed regression from Firefox 2.0.0.4. I'll work on finding a regression range.

Comment 2

[Adam Guthrie]

This regressed between 2007-07-02-03 and 2007-07-03-04 on branch. http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=MOZILLA_1_8_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-07-02+02&maxdate=2007-07-03+05&cvsroot=%2Fcvsroot

Looks like it's fallout from bug 344228.

Comment 3

[Stefan [:stefanh]]

fwiw, I don't seem to be able to reproduce this on SeaMonkey (latest branch build).

Comment 4

[Stefan [:stefanh]]

(Mac OS 10.3.9)

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I see it on FIrefox. This is a pretty bad branch regression.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(should be Mac only though, at least)

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: a guess fix.

I don't have a mac, so can't test whether this works. Anyone willing to try?
Does the problem occur only when dragging scrollbar?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

If the patch fixes this, the regression is from bug 382444.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

This is the right fix.

The problem is that nsNativeScrollbarFrame::Hookup is called from
nsNativeScrollbarFrame::GetPrefSize. The first time it runs it interposes
itself as the scrollbar mediator: it keeps a reference to the old scrollbar
mediator, makes itself the new scrollbar mediator, and forwards messages from
the scrollbar to the old mediator.

But the fix for bug 382444 is setting the scrollbar mediator after GetPrefSize has run for the first time. So nsNativeScrollbarFrame thinks there is no mediator and doesn't install itself as a mediator, so never passes events to the tree's mediator even after the tree has installed it.

The right thing to do is to just always make the scrollframe a mediator always. Every time a mediator event happens we recheck whether there's a real mediator (this already happens in nsNativeScrollbarFrame::FindParts). We also need to make nsNativeScrollbar always perform regular scrolling *and* mediator notification.

Good thing native scrollbars are gone on trunk!

I guess basically all tree scrolling is broken on Mac in 2.0.0.5 :-(

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The blame for this is mainly mine. My patch changed things so that SetScrollbarMediator has to be called before GetPrefSize is called on the scrollbar (an unintentional and stupid requirement). No-one noticed because all our code was doing that. Then Olli checked in his tree patch and violated the requirement.

Comment 11

[Martijn Wargers (dead)]

Thunderbird 2.0.0.5 hasn't been released yet. I guess this is something of a show-stopper for that, right?

Comment 12

[Marcia Knous [:marcia]]

roc: Tomcat and I don't see scrolling broken on the Thunderbird 2.0.0.5pre build. We are able to scroll the message pane as well as the account pane (not great experience but the scroll bar chunks through). Since this is core code I guess we are puzzled why it wouldn't break in both products. Can you enlighten us?

(In reply to comment #9)
> Created an attachment (id=272879) [details]
> fix
> 
> This is the right fix.
> 
> The problem is that nsNativeScrollbarFrame::Hookup is called from
> nsNativeScrollbarFrame::GetPrefSize. The first time it runs it interposes
> itself as the scrollbar mediator: it keeps a reference to the old scrollbar
> mediator, makes itself the new scrollbar mediator, and forwards messages from
> the scrollbar to the old mediator.
> 
> But the fix for bug 382444 is setting the scrollbar mediator after GetPrefSize
> has run for the first time. So nsNativeScrollbarFrame thinks there is no
> mediator and doesn't install itself as a mediator, so never passes events to
> the tree's mediator even after the tree has installed it.
> 
> The right thing to do is to just always make the scrollframe a mediator always.
> Every time a mediator event happens we recheck whether there's a real mediator
> (this already happens in nsNativeScrollbarFrame::FindParts). We also need to
> make nsNativeScrollbar always perform regular scrolling *and* mediator
> notification.
> 
> Good thing native scrollbars are gone on trunk!
> 
> I guess basically all tree scrolling is broken on Mac in 2.0.0.5 :-(
> 

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Did you test on Mac? This is Mac only. If it works on Mac then I'm confused.

Comment 14

[Marcia Knous [:marcia]]

Yes, I tested on an Intel Mac running 10.4.10.

(In reply to comment #13)
> Did you test on Mac? This is Mac only. If it works on Mac then I'm confused.
> 

Comment 15

[Stephen Donner [:stephend] Not actively reading bugmail]

This also affects the "Message Filters" dialog in Thunderbird on Mac (2.0.0.5 candidates), if you overflow the message criteria, and try to scroll in a folder picket which has scrollbars, like the "Move message to" criterion.

Comment 16

[Dave Miller [:justdave]]

Preed told me to mention this here....

This affects the Password Manager also on Mac...

I renamed a username for a login account on a site I manage tonight, and then logged out and logged back in to try to save the new username in password manager.  Then I went to the preferences and into Password Manager to attempt to delete the old username since it'll never be used again, and the scrollbar seemed to start scrolling then stopped, and wouldn't start scrolling again.  So I never did get to that username to delete it.

Comment 17

[Dave Miller [:justdave]]

ah, to clarify, the scrollbar itself works, but the content stops moving.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You can probably select an entry and then use keyboard navigation to get down to the one you want. Mousewheel also works if you have one.

I'm just saying that for completeness .... this regression sucks.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 272879 [details] [diff] [review]
fix

>Index: widget/src/mac/nsNativeScrollbar.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/widget/src/mac/nsNativeScrollbar.cpp,v
>retrieving revision 1.12.4.2
>diff -u -p -1 -2 -r1.12.4.2 nsNativeScrollbar.cpp
>--- widget/src/mac/nsNativeScrollbar.cpp	8 May 2006 20:53:48 -0000	1.12.4.2
>+++ widget/src/mac/nsNativeScrollbar.cpp	18 Jul 2007 23:05:09 -0000
>@@ -229,31 +229,29 @@ nsNativeScrollbar::DoScrollAction(Contro
>       return;
>   }
> 
>   if (buttonPress) {
>     //
>     // For the up/down buttons, scroll up or down by the line height and 
>     // update the attributes on the content node (the scroll frame listens
>     // for these attributes and will scroll accordingly). However,
>     // if we have a mediator, we're in an outliner and we have to scroll by
>     // lines. Outliner ignores the indexes in ScrollbarButtonPressed() except
>     // to check if one is greater than the other to indicate direction.
>     //
>+    UpdateContentPosition(newPos);
>     if (mMediator) {
>       BoundsCheck(0, newPos, mMaxValue);
>       mMediator->ScrollbarButtonPressed(mScrollbar, oldPos, newPos);
>     }
>-    else {
>-      UpdateContentPosition(newPos);
>-    }
>   }
>   else {
>     //
>     // For page up/down and dragging the thumb, scroll by the page height
>     // (or directly report the value of the scrollbar) and update the attributes
>     // on the content node (as above). If we have a mediator, we're in an
>     // outliner so tell it directly that the position has changed. Note that
>     // outliner takes the new position as a signed reference, so we have to
>     // convert our unsigned to signed first.
>     //
>     UpdateContentPosition(newPos);

Couldn't you have just one UpdateContentPosition(newPos), right before |if (buttonPress)|?

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes.

Comment 22

[Robert Kaiser]

I added this fix to SeaMonkey 1.1.3 (I did minibranch the files for that but unfortunately hit the wrong branch when wanting to check in on that minibranch, I did back out immediately - sorry!)

Comment 25

[Mats Palmgren (inactive)]

Comment on attachment 272879 [details] [diff] [review]
fix

sr=mats

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 272879 [details] [diff] [review]
fix

Need branch approvals for this major regression.

Comment 34

[Mike Connor [:mconnor]]

Comment on attachment 272879 [details] [diff] [review]
fix

a=mconnor on behalf of drivers, please land on GECKO181_20070712_RELBRANCH for
2.0.0.6 and MOZILLA_1_8_BRANCH for 2.0.0.7

Comment 35

[Jon Gateley]

(In reply to comment #0)
> User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US;
> rv:1.8.1.5) Gecko/20070713 Firefox/2.0.0.5
> Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US;
> rv:1.8.1.5) Gecko/20070713 Firefox/2.0.0.5
> 
> in the place where u enter url(s), when u click for the dropdown menu, the
> scrollbar can move, but the url(s) don't change. It's as if they were stuck.
> 
> Reproducible: Always
> 
> Steps to Reproduce:
> 1.open firefox
> 2.click on the dropdown menu
> 3.scroll
> Actual Results:  
> nothing happens.
> 
> Expected Results:  
> display all my previously browsed addresses.
> 

I got mine to work using the mousewheel!  You may have to jiggle the scrollbar a bit to get mousewheel to work.

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked into GECKO181_20070712_RELBRANCH and MOZILLA_1_8_BRANCH.

Comment 38

[Carsten Book [:Tomcat]]

verified fixed 1.8.1.6 using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.6) Gecko/2007072301 Firefox/2.0.0.6 autocomplete dropdown/scrollbar works now as expected. Adding verified keyword

Comment 39

[JOHN DIELE]

MIGHT BE NICE IF FIREFOX FIXED IT.  ALL THESE CC'S ARE GREEK AND SERVE NO PURPOSE.

Comment 40

[Russell Gillette]

(In reply to comment #38)
> verified fixed 1.8.1.6 using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US;
> rv:1.8.1.6) Gecko/2007072301 Firefox/2.0.0.6 autocomplete dropdown/scrollbar
> works now as expected. Adding verified keyword
> 

what about PPC-based Mac?

Comment 41

[Marcia Knous [:marcia]]

Verified fixed on PPC using Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1.6) Gecko/2007072301 Firefox/2.0.0.6. Scrollbars work lovely in the URL bar and elsewhere.
> 
> what about PPC-based Mac?
> 

Comment 45

[Chris F.]

Alright in all honesty this is greek to most of us. How do you implement the fix, alot of us are having issues because we don't know how to, so how about a walk through on that.

Comment 46

[Robert Kaiser]

Chris:
If you don't compile builds yourself, then either go back to Firefox 2.0.0.4 (which is bad as it has security issues), wait for Firefox 2.0.0.6, which should be available soon (there might test builds somewhere already), or use SeaMonkey 1.1.3 which has the fix (not that I want to force anyone to convert from Firefox to SeaMonkey here though).
Your best option is probably to wait for Firefox 2.0.0.6 which should be available soon.

Comment 47

[Chris F.]

Robert,

Thanks. I've compiled builds, but never for firefox and never for a mac. If 2.0.0.6 will be out soon I can just wait. Thanks for the help.

Chris

Comment 50

[Daniel Veditz [:dveditz]]

Comment on attachment 272879 [details] [diff] [review]
fix

approved for 1.8.0.13, a=dveditz for release-drivers

Comment 51

[Daniel Veditz [:dveditz]]

Fix checked into 1.8.0.13

Comment 52

[juan becerra [:juanb]]

Verified on MacIntel using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.7pre) Gecko/2007082105 BonEcho/2.0.0.7pre, and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.7pre) Gecko/2007082103 Thunderbird/2.0.0.7pre

Adding verified keyword.

I was not able to observe the problem on Thunderbird 15012 or 15013. 

Comment 53

[Marcia Knous [:marcia]]

minusing my original nomination for in-litmus as I don't feel we need a test case for this.