384343 - Page does not render correctly (addons.mozilla.org)

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: addons.mozilla.org rendering problem

After clicking on a preview image and closing the preview, the dark overlay does not go away.  Other areas of the page will refresh on hover, but getting the dark overlay to go away entirely requires a page refresh.

Comment 1

[Martijn Wargers (dead)]

This seems to be MacOS specific, I can't reproduce this on windows.
Adam, can you reproduce (and make a testcase out of it)?

Alex, in general you want to file these types of bugs under Core->Layout (or maybe this is a thebes bug, not sure).

Comment 2

[Adam Guthrie]

I don't have a minimized testcase, but I do have a regression range, which is between 2007-05-30-04 and 2007-05-31-05. http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-05-30+03&maxdate=2007-05-31+06&cvsroot=%2Fcvsroot

I would guess maybe bug 373122? Seems like the only thing that would cause this, given that it's Mac-specific.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

I need a reliable testcase ... even a non-reduced one will do.  That
or specific instructions to reproduce.

Comment 4

[Adam Guthrie]

The steps I used when reproducing are as follows:

1. Load https://addons.mozilla.org/en-US/firefox/addons/previews/2448
2. Click on the image under "Google Bookmarks Menu".
(The image should show in the center of the page, surrounded by a opaque, black background.)
3. Click the "X" in the bottom right corner of the centered image.

After step 3 the page should look just like it does in step 1. In current trunk builds, however the popup will either fail to close or it does close, but leaves the opaque, black background in place.

A reduced testcase could probably be made, but it'd be a pain since this page is using jQuery APIs to do these effects.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

I can reproduce this problem, and find that it goes away when I make
the following change to the patch for bug 373122 (the changed code is
from nsChildView::TearDownView() in widget/src/cocoa/nsChildView.mm,
and partially rolls back the patch):

 if ([mView isEqual:[win contentView]]) {
   [mView release];
 } else {
-  // Stop NSView hierarchy being changed during [ChildView drawRect:]
-  [mView performSelectorOnMainThread:@selector(delayedTearDown)
-         withObject:nil waitUntilDone:false];
+  [self removeFromSuperviewWithoutNeedingDisplay];
+  [self release];
 }

The same change also makes bug 383821 go away.  So both bugs are
regressions "caused" by a part (the same part) of my patch for bug
373122.

This part of my patch is needed to stop a nasty problem happening when
a ChildView object (a subclass of NSView associated with an
nsChildView Gecko object) is removed from its superview during a call
to [ChildView drawRect:].  The call to
mGeckoChild->DispatchWindowEvent(paintEvent) in [ChildView drawRect:]
sometimes (indirectly) calls ~nsChildView::nsChildView() (and
nsChildView::TearDownView()).  My workaround is to postpone removing
the ChildView object from its superview until after the call to
nsChildView::TearDownView() is finished (thereby guaranteeing that it
never happens during a call to [ChildView drawRect:]).  I could find a
different workaround (for example I could call [ChildView
delayedTearDown] only when nsChildView::TearDownView() has been called
from inside [ChildView drawRect:]).  But I can't simply undo my patch.

Furthermore, I'm very puzzled how this part of my patch could have
triggered bug 383821 and this bug (384343).  I get the feeling that my
change may have unmasked other bugs.  Bug 383821 concerns Camino,
which I haven't been working on.  But I'm already working on the area
covered by this bug, so I'll assign it to myself and go looking for
the other bug(s) my bug 373122 patch might have unmasked.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix for bugs 384343 and 383821

Here's a patch that fixes the single problem reported here (in comment
#0) and all the problems that I could reproduce that were reported at
bug 383821 (those in bug 383821 comment #0, bug 383821 comment #8 and
bug 383821 comment #11).

It turns out that postponing removing ChildView objects from their
superviews did unmask one bug -- [ChildView resignFirstResponder]
should always call through to its superclass, but previously it didn't
do this if mGeckoChild had already been NULLed.  Changing this fixes
the keyboard focus problems (those I just mentioned that were reported
at bug 383821).

And there was also a bug in my patch for bug 373122:  [ChildView
delayedTearDown] should use [NSView removeFromSuperview] (instead of
[NSView removeFromSuperviewWithoutNeedingDisplay]), because at this
point there's no longer any danger of it being "invoked during
display".  This change fixes the problem reported here (at comment
#0).

Comment 7

[Steven Michaud [:smichaud] (Retired)]

landed on trunk