298677 - regression: Vertical scrollbar draws over toolbar and tabs when location or search bar has focus during scrolling

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Mac, defect)

Description

[Wayne Woods]

UI drawing bug. I think it's Mac-only.

- Navigate to a page long enough to make the vertical scrollbar appear, e.g.
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/
- Place the cursor focus into the location bar or search bar
- Using the mouse, grab the scroller and move it up and down.
- Shortly you should notice a slight glitch as the scroller jumps unexpectedly.
- Look at the top left side of the browser window. You should notice a section
of the scrollbar has been drawn over the top of the toolbars and tabs.

This can also happen if you move a scrollbar associated with a form element. In
which case it can often draw over the top of the web page, rather than the
toolbar. Sometimes it only seems to happen as soon as I let go of the scroller,
though.

Focusing the box for entering the bug description (which I'm currently typing
into), I even managed to get the scrollbar graphics to draw all the way down the
left side of the browser window, by moving the window scrollbar enough.

It first occurred in the 20050622 Firefox nightly build. I haven't tested
Seamonkey yet. Using Mac OS X 10.4.1. Fresh profile. No extensions or themes.

Comment 1

[Asaf Romano (gone)]

Also happens here... :-/

Is this a recent regression?

Comment 2

[Wayne Woods]

Yeah, the 20050622 Fx nightly is the first one to display it. It still exists in
the 20050623 nightly. 

I just confirmed it exists in Seamonkey as well (20050623 nightly).

Comment 3

[Stefan [:stefanh]]

In SeaMonkey 2005062410 (mac) I can get a duplicate scrollbar in the browser
window, Chatzilla window, MailNews... This also happens with the horizontal
scrollbar. See also bug 298679.

Comment 4

[philippe (part-time)]

Similar problem: advanced search page here on bugzilla, the < input> at the top
has focus on page-load. Just move the scrollbar on one of the (any, really)
select boxes. A short vertical scrollbar appears at the top left of the window,
just under the tabbar.
Putting the focus on the location bar, and that scrollbar disappeared.

Comment 5

[Stefan [:stefanh]]

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

Comment 6

[jhp (no longer active)]

This was caused by the CFRunLoop checkin.  Any idea why that would cause this issue?

Comment 7

[Simon Fraser [no longer active]]

Bug 205873 may inspire a fix for this.

Comment 8

[Josh Aas]

Attachment: gdb bt, catch scrollbar redrawing outside of StartDraw()/EndDraw() pair

just posting this in case it gives anyone any ideas

Comment 9

[Wayne Woods]

The scroll widget also jumps a bit within its normal track, i.e. jumps out from
under the mouse. I don't think it's been noted that when it does so, the whole
page also jumps. So I guess it's more than just a redrawing effect, as the
browser is treating the move as a valid scroll. It's like when some other field
has the focus, that the browser is trying to treat that field as the entire
window for the scroll, and of course failing badly.

Comment 10

[Simon Fraser [no longer active]]

Attachment: Patch

This patch installs a Carbon Event handler for drawing events on native Mac
controls, which ensures that the port origin is set correctly before calling
the next handler (which will actually do the drawing). It's essentially the
patch from bug 205873.

Comment 11

[Josh Aas]

Simon's patch solves half the problem. No more drawing where it shouldn't, but
the scrollbar still skips in sync with the cursor. As said in the summary but
more exactly, do this:

1. load slashdot.org (its nice and long)
2. click in the google search field in the toolbar so that the caret blinks there
3. put your mouse down on the scrollbar for the page and slowly move down the page
4. notice that when you do this, it "skips" based on the caret drawing

Comment 12

[Josh Aas]

I meant that it skips in sync with the caret drawing (not the cursor), and only
when the caret is drawing outside the content area, like in the URL or search
fields.

Comment 13

[Mark Mentovai]

Jumping around when the caret is going is also an origin problem.  If you
collapse all of the toolbars above a vertical scrollbar, that scrollbar won't
skip.  If you open a sidebar to the left of a horizontal scrollbar, that
scrollbar will skip.  Open a find bar and put the caret there if you're nervous
about not being able to see it.

When the caret is going, it seems like something is causing the scrollbars to be
read (GetControl32BitValue?) without going through the expected code path with
the proper origin set.  The immediate solution I thought of was an
StOriginSetter in nsNativeScrollbar.cpp, but that won't work.  I wanted to use
libSystem_debug to see what else might be touching that control and do other
neat debugging things, but was prevented by bug 299661.

Comment 14

[Michael Graubart]

Personally, I am less and less convinced that Bug 298679 is a duplicate of this.
But this is only a hunch, based on the two very different behaviours.

Comment 15

[jhp (no longer active)]

Comment on attachment 188003 [details] [diff] [review]
Patch

Unfortunately, I'm not a superreviewer, but I will review this patch anyway.

+  ControlRef theControl;
+  ::GetEventParameter(inEvent, kEventParamDirectObject, typeControlRef, NULL,
sizeof(ControlRef), NULL, &theControl);
+  
+  CGrafPtr controlPort;   // port we're drawing into (usually the control's
port)
+  if (::GetEventParameter(inEvent, kEventParamGrafPort, typeGrafPtr, NULL,
sizeof(CGrafPtr), NULL, &controlPort) != noErr)
+    controlPort = ::GetWindowPort(::GetControlOwner(theControl));

Since |theControl| is only needed in the case where the second
|::GetEventParameter| fails, why not just get |theControl| in the failure case.

In bug 205873, you mentioned that this patch may allow you to get rid of many
of the |StartDraw/EndDraw| calls.  So in this code, the calls would then just
go down the |!macControl->IsDrawing()| path, correct?  Would be still need to
keep around the |IsDrawing()| case?

Nit: Tabbing issues.  Some lines use them, others don't.  I know the file is
already riddled with them, but it would be best not to add any more.

Nit: Some lines over 80 chars in length.

With that fixed, r=jhpedemonte.  Since the next alpha is imminent, can we get
this patch in to fix the phantom scrollbars, and keep the bug open to continue
work on the remaining issues?

Comment 16

[Mark Mentovai]

Attachment: Hack to handle jumping scrollbar

I don't like this hack.  Nobody who's heard about it likes it.	Nobody who
looks at it will like it.  I'm only putting it here for coordination, as a
last-ditch possibility.

The call to GetControl32BitValue() is giving bad results.  As suspected, it's
got something to do with the origin.  Resetting the origin doesn't help, but
it's possible to test that the origin is what it's supposed to be and discard
data known to be faulty.  That's what this hack does.

Installing a Carbon event handler on the scrollbar and using that to handle
tracking doesn't help, either.

Comment 17

[Josh Aas]

Comment on attachment 188003 [details] [diff] [review]
Patch

looks good

Comment 18

[Mike Shaver (:shaver emeritus)]

Comment on attachment 188003 [details] [diff] [review]
Patch

a=shaver for 1.8b3

Comment 19

[Simon Fraser [no longer active]]

Comment on attachment 188003 [details] [diff] [review]
Patch

This patch checked in. Keeping bug open for the jumpy scrolling.

Comment 20

[Mark Mentovai]

Gross.  I get the same goofy scrollbar behavior at www.apple.com.  Must be that
JavaScript redrawing.  No carets necessary.

Comment 21

[Michael Graubart]

Interestingly, the 'goofy behaviour' doesn't happen if one uses the up- or
down-arrow; only if one actually slides the scrolling slider up or down with the
mouse.

Comment 22

[Mark Mentovai]

We're only calling GetControl32BitValue() when the user moves the thumb
indicator directly.  For clicks on the up or down buttons, or in the scrollbar
above or below the thumb, we adjust the last-known position by the size of a
line or the viewable area, respectively.

Comment 23

[Wayne Woods]

FYI bug 300095 appeared at the same time as this one. Does it share the same
root cause?

Comment 24

[Michael Graubart]

BugZilla says 'Zarro boogs' when I try to find Bug 300095.

BTW, when will a new nightly build become available?

Comment 25

[Asaf Romano (gone)]

(In reply to comment #24)
> BugZilla says 'Zarro boogs' when I try to find Bug 300095.

Clicking on the bug link isn't that hard!

Comment 26

[Michael Graubart]

In dismissal of Comment #25: What a fine line in sarcasm! But it would have been
more helpful, and saved the Mozilla system time, to say that the link (which
does NOT appear in the e-mail notification) can be found under Bug #298677.

Comment 27

[Asaf Romano (gone)]

FYI, this patch has caused bug 300058.

Comment 28

[jhp (no longer active)]

Mark, since Alpha2 is wrapping up right now, are you confident enough in your
'hack' patch to check it in for now, so it at least works in Alpha2?  Then we
can continue to work on this in the trunk to find the 'correct' fix.  What's
everyone think of that?

Comment 29

[Mark Mentovai]

Comment on attachment 188402 [details] [diff] [review]
Hack to handle jumping scrollbar

I have complete confidence in the hack, I just don't like the approach.  It's
better than nothing, and since nothing is exactly what we were able to come up
with so far, I'd set it loose on a2.

Comment 30

[Simon Fraser [no longer active]]

Comment on attachment 188402 [details] [diff] [review]
Hack to handle jumping scrollbar

Does this patch do the right thing for a scrollbar  inside an iframe (like the
one on the Edit Attachment page)?

Comment 31

[jhp (no longer active)]

Debugging the jittery scrollbars:  everytime I see the |portBounds| get set to
(0,0) in |DoScrollAction()|, preceding it I see the |::SetOrigin(0,0)| call in
|nsWindow::WidgetToScreen()|.

Comment 32

[jhp (no longer active)]

Hmm, scratch that, looks like it's just a coincidence.

Comment 33

[Mark Mentovai]

Comment on attachment 188402 [details] [diff] [review]
Hack to handle jumping scrollbar

Simon, scrollbars in iframes work properly with the hack, but now that your
event-based drawing patch is in, I'm seeing drawing artifacts.	The thumb, or
part of it, is now drawing in the "wrong" position, but the hack prevents the
document from jumping.	Prior to the event-based redraws, the hack prevented
document jumping AND incorrect scrollbar appearance.

We'd need something better than this for a2, so I'm cancelling the review
requests.

Comment 34

[Simon Fraser [no longer active]]

Scrollbar drawing turds are bug 300058. See also bug 300095.

Comment 35

[Mark Mentovai]

Comment on attachment 188003 [details] [diff] [review]
Patch

This is in, it shouldn't be obsolete.

Comment 36

[Mark Mentovai]

Attachment: Patch for jumping scrollbar

I finally found where the origin was being set for the caret and not restored
for the scrollbars.  It was in the rendering context.

Once upon a time, which is to say, before January 2002, nsRenderingContextMac
would save and restore the origin and a few other key settings.  I'm bringing
that behavior back, wrapped in #ifdef MOZ_WIDGET_COCOA, because we don't care
about the origin with Cocoa.  If you don't believe me, ask Camino.

This fixes the jumpy scrollbars with the caret.  They're still jumping with
that JavaScript counter at www.apple.com.  That's most likely another failure
to restore the origin.

It would be nice to sneak this into 1.8b3/1.1a2 if there's still time for that.
 It's a pretty high-profile thing to have broken.

Comment 37

[Mark Mentovai]

Note that in addition to fixing the jumping, the new patch also handles
scrollbars drawing in the wrong spot.  The event handler-based drawing patch in
attachment 188003 [details] [diff] [review] to this bug can be backed out, which will resolve dependent
bug 300058 (scrollbar turds).

Just as the new patch doesn't solve the jumpy behavior at www.apple.com, it
doesn't prevent the scrollbar from drawing in the wrong place at that page.  The
event handler at least fixes the drawing there.  But since the scrollbar is
pretty useless on that page anyway, it might be worth dropping the event handler
to flush the turds.

Comment 38

[Wayne Woods]

(In reply to comment #36)
> Once upon a time, which is to say, before January 2002, nsRenderingContextMac
> would save and restore the origin and a few other key settings.  I'm bringing
> that behavior back, wrapped in #ifdef MOZ_WIDGET_COCOA, because we don't care
> about the origin with Cocoa.  If you don't believe me, ask Camino.
> 
> This fixes the jumpy scrollbars with the caret.  They're still jumping with
> that JavaScript counter at www.apple.com.  That's most likely another failure
> to restore the origin.

Sounds great! Why was that behaviour removed in the first place? :)

Comment 39

[Mark Mentovai]

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

Comment 40

[Mike Shaver (:shaver emeritus)]

Comment on attachment 188812 [details] [diff] [review]
Patch for jumping scrollbar

Yeah, this is a disaster on the Mac right now; a=shaver for checkin in the next
24 hours, pending reviews.

Comment 41

[Simon Fraser [no longer active]]

(In reply to comment #38)
> Sounds great! Why was that behaviour removed in the first place? :)

It was removed because it didn't make sense in a world in which
creation/destruction of RCs is not cleanly nested. We moved from a
'save/restore' model to a 'set before use' model (which worked just fine until
CFRunLoop came along).

Comment 42

[Simon Fraser [no longer active]]

The history of the nsRenderingContextMac behavior can be seen by looking at the
CVS log:
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/gfx/src/mac/nsRenderingContextMac.cpp

The changes we're talking about here were mostly added in version 1.136. Bug
113480#13 has a comment describing what changed. Essentially, this is when we
moved to a "set before use" pattern in the rendering context. When testing for
regressions, look at bug 103234 bug 104781, bug 120228.

The reason that the save/restore stuff was removed was that it doesn't make
sense in a world in which RC creation/destruction is not cleanly nested. It's
analagous to the NSQuickDrawView issues we've also been having. Although I don't
disagree with this patch in principle, it might bring back some of those other
rendering bugs.

Comment 43

[Mark Mentovai]

I can't reproduce any of the problems in bug 113480, bug 103234, bug 105946, bug
104871, or bug 120228.

Since most of gfx assumes that it'll receive a useless origin, there's a lot of
origin resetting going on anyway.  Restoring the origin in the destructor can
suck for long-lived rendering contexts, but it's OK in the caret case for now
(see DONT_REUSE_RENDERING_CONTEXT in nsCaret.cpp).  A long-lived rendering
context may be why this patch doesn't work for www.apple.com.

Maybe it's better to restore when the last graphic state is popped?

(Trying that...)

Yeah, that works too, but it still doesn't solve www.apple.com.

Comment 44

[Mark Mentovai]

I'm still able to force ludicrous scrollbar behavior and drawing in the wrong
parts of the window if the scrollbar is manipulated while reloading.  In fact,
the scrollbar appears (incorrectly) disconnected from the document while
reloading, but that doesn't prevent it from being jittery.  See also bug 299384
(crash when using scrollbar and reloading), bug 299643 (content not updating
when using widgets and loading).  This occurs with any combination of the draw
event patch and the origin restoration patch.  The jumping and drawing is
probably yet another origin in need of restoration.  The scrollbar not being
attached is probably something else.

I don't think we should let this hold up what we've got so far.

Comment 45

[Mark Mentovai]

Attachment: v3, handle apple.com case too

I can resolve the apple.com case by turning the unrestoring SetOrigin calls in
nsWindow into StOriginSetters that restore the origin when they're done.  The
port is already being restored there by StPortSetters.

The draw event handler is now mostly redundant.  It should be removed because
it caused bug 300058, and because it wasn't really immune to all problems
either.  Carbon is being really adamant about restoring evil port states.  I
haven't taken it out as a part of this patch yet because it's still saving us
from drawing the scrollbar in the wrong spot if the scrollbar is being
manipulated while reloading.

Comment 46

[Mark Mentovai]

Attachment: v4, handle all known cases

This last patch handles scrollbars redrawing in the wrong location during/after
reload.  This is implemented by calling ::SetControlVisibility to make controls
invisible without redraw when Destroy() is called.  The comment tells why other
methods of tracking Destroy() won't work.

This last case was occurring even with the draw event handler, but the
scrollbar was drawing at the top-right instead of the top-left.

The draw event handler is now gone.

There may be some more origin-related problems lurking after switching to
CFRunLoop.  Areas to watch out for: clipboard, plugins, Java.  There shouldn't
be anything else scrollbar-specific.  I think we're done here.

Comment 47

[Mark Mentovai]

Comment on attachment 189081 [details] [diff] [review]
v4, handle all known cases

+	NS_IMETHOD			Destroy();
+

Extra newline won't be checked in.

Comment 48

[Simon Fraser [no longer active]]

Comment on attachment 189081 [details] [diff] [review]
v4, handle all known cases

Good work.

Comment 49

[jhp (no longer active)]

Is it too late to get this in for 1.8b3/DeerPark a2?

Comment 50

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 189081 [details] [diff] [review]
v4, handle all known cases

r=pink

sorry for the delay.

Comment 51

[Mark Mentovai]

Checked in.

Comment 52

[Wayne Woods]

FYI, seemingly related (but not identical) bug still occurring: bug 304607