337545 - [Mac] expanded view button in "Add Bookmark" dialog disappears on first click

Status: VERIFIED FIXED

(Core Graveyard :: GFX: Mac, defect)

Description

[Josh Aas]

1. Go to a web page with an atom/rss feed and click on the RSS icon in the URL bar
2. On the RSS preview page, hit "Subscribe Now"
3. In the dialog/sheet that comes up, hit the button to the right of the dropdown menu, the button with the downward black triangle
4. View expands, black triangle button disappears

Even though the button has disappeared, you can still click where it used to be and it'll work.

Comment 1

[Josh Aas]

I am seeing this in a debug build of the FF2 branch on Mac OS X.

Comment 2

[Ben Goodger (use ben at mozilla dot org for email)]

I don't see this, today's branch nightly, OS X 10.4.6. This is the add bookmark dialog btw, not anything to do with the RSS preview :-)

Does this still happen? What about a fresh nightly using your same profile?

Comment 3

[Phil Ringnalda (:philor)]

I don't see this in the livemark subscription dialog, which doesn't have a microsummary picker (I can't remember whether or not it did when Josh filed this), but I was just about to file a duplicate of it against Bookmarks, because I do see it by adding a bookmark: first click it disappears, click the blank spot and the tree is correctly hidden and from then on the button properly changes from down to up. Comment out the microsummary picker in addBookmark2.xul, and it works fine first time, though my first guess would be that microsummaries are just exposing it, not actually causing it.

Resummarizing and moving: thump me if I'm wrong, Josh.

Comment 4

[Adam Guthrie]

regression range:
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=2005-07-20+06%3A00&maxdate=2005-07-21+08%3A00&cvsroot=%2Fcvsroot

Doesn't occur on non-Mac platforms, so I'm assuming it's one of the Mac widget check-ins during that range. CCing the people who made the patches.

Comment 5

[Mark Mentovai]

Doesn't seem to be caused by 289353, 300095, or 289973.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(My note from triage meeting:  Also seems a little scary because if it's a regression from widget changes, there could be other regressions.)

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Looks like this is still happening now that the microsummary picker has been integrated with the Name field and is an editable menulist, as I see the buggy behavior on today's nightly.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

This is fugly, so if we could get you to look at it for beta2, josh ...

Comment 9

[Josh Aas]

I can verify that it wasn't 289353 or 300095 that caused the regression, but things have changed so much since 289973 that it is difficult to back out.

Comment 10

[Josh Aas]

BTW, I can verify the regression range given in comment #4. The build from 2005.7.18 works and 2005.7.21 is broken.

Comment 11

[Josh Aas]

It looks like the problem is we're not painting reliably during window resize and the cause is almost certainly the fix for bug 289973, but simply backing that out may not work at this point.

Comment 12

[Mark Mentovai]

I'll do this.  Josh, if you really want it, take it back from me.

Comment 13

[Mark Mentovai]

Attachment: An interesting picture

Note that it's not just the expander button that's gone.  The folder icon that normally appears in the "Create in:" drop-down menu is missing too.  Both of these images are drawn by CGContextDrawImage (nsImageMac::Draw).

Now, look at the "fewb" folder in the list.  Note that it has the top of the folder icon that should have been drawn in the drop-down.  All the way over to the right is the top of the expander button.  Both of these images (the portions that are visible) are shifted horizontally 19 pixels and vertically 97 pixels from where they belong.

(19,97) is, not coincidentally, the position of the white box in the window.  The white box is a widget, and we paint widgets by setting the port origin so that (0,0) corresponds to the widget's top-left.  We could definitely see this sort of behavior if we were to draw the images in question while the origin is set in such a way.  Except the origin is fine when we draw these images, and we don't even change the origin until after they're drawn.

Now, for some interesting findings:

The problem only occurs the first time the window is resized to make it larger.  If, through evil trickery, I force the window to initially display at the expanded size and remain that size throughout its life, everything draws in the proper location reliably.

If we skip the LockPortBits and UnlockPortBits calls while handling an update, the problem disappears.  LockPortBits is supposed to be an optimization, used at the urging of tn2051 (http://developer.apple.com/technotes/tn/tn2051.html).  Note that the HTML version of tn2051 now makes no mention of calling LockPortBits, although the PDF version still mentions it.  Getting rid of LockPortBits will regress Tp, but in my tests, it's by less than 1%.

I'm almost certain that this bug is the same as bug 314900, in which the reload button sometimes doesn't appear.  That one's been elusive, because there's no easily-reproducible testcase.

I'm pretty sure that this was initially caused by bug 289973, although probably only because it prevented sheets from updating more often than necessary.  I made more changes to update handling (bug 106695) that should prevent any window from being updated more than necessary, so simply bypassing 289973 won't solve the problem any longer, and it wouldn't be the right thing to do anyway*.

* 289973 may not have been the optimal approach, but it solved the important resize-in-update problem, and we're not bumping up against any of its shortcomings here.

Comment 14

[Mark Mentovai]

Attachment: How it should look

For the pixel-hunters out there, this is how the add bookmark sheet should appear, and how it does appear after you expand/collapse/expand the button.

Comment 15

[Mark Mentovai]

The best way I've found so far to fix this is to skip the LockPortBits call.  We should probably do that and file a bug with Apple.  LockPortBits causes trouble with the QuickTime plugin and possibly others, and as I mentioned above, it's no longer listed at all on Apple's QuickDraw performance technote.  I measured pageload impact at under 1%.

Comment 16

[Josh Aas]

sounds good to me

Comment 17

[Mark Mentovai]

Attachment: v1 fix

This gets rid of LockPortBits.

I found that we were calling QDRegionToRects twice during each update: once with a callback that counts the number of rectangles in the region, and then if that number was below a certain threshhold (the most common case), a second time with a callback to actually collect the rectangles.  QDRegionToRects, as you might guess, is an expensive call.  The new strategy is to call it only once.  The callback populates the rectangle list until it's at the threshhold, at which point it stops and indicates to the caller that the threshhold has been exceeded.

This new optimization more than makes up for the loss of the small performance gain that LockPortBits gave us.

There was also a lot of related dead code, some of which hasn't been used since the pre-Carbon builds.  It was never #ifdefed, so it escaped the guillotine.

Comment 18

[Mark Mentovai]

Checked in on the trunk.

Comment 19

[pcbjrmail]

If it's fixed and there is a patch, where is it?  I'm no teche, so I need some "See Spot run" instructions.  The reload button is still disappearing (OS 10.4.6; Firefox 1.5.0.5).

Comment 20

[Mark Mentovai]

The patch is attachment 231417 [details] [diff] [review].  If you're not doing your own builds, you'll need to wait for a nightly that has it.

The patch was checked in today to the trunk, which is where Firefox 3 development happens.  The trunk currently has a different "add bookmarks" sheet, so reproducing this bug is much more difficult.  We mark bugs FIXED when the patch is checked in to the trunk.  If the patch works well on the trunk, we can then put it on a more stable branch.

I expect this patch to be checked in to the 1.8 branch soon, for Firefox 2 development.  Once you see a message from me about being "checked in to the 1_8 branch", you can get a Firefox 2 nightly build that should have the fix.  Firefox 2 nightlies are available at http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla1.8/ .  To ensure that the build has the fix, you should wait a day after the patch is checked in to that branch before testing a build.

As an alternative, you could wait for Firefox 2 beta 2, which I expect will contain this fix.

This patch isn't targeted at the 1.5 series of releases.

Comment 21

[Darin Fisher]

Is there a simple unit tests that can be constructed (in xpcom/tests) that would help us ensure that we are only making the change that we intend to nsLocalFileOSX?

Comment 22

[Mike Connor [:mconnor]]

Comment on attachment 231417 [details] [diff] [review]
v1 fix

a=mconnor on behalf of drivers for checkin to 1.8 branch for 1.8.1

Comment 23

[Darin Fisher]

> Is there a simple unit tests that can be constructed (in xpcom/tests) that
> would help us ensure that we are only making the change that we intend to
> nsLocalFileOSX?

Sorry, I meant to put this comment in bug 294584.

Comment 24

[Mark Mentovai]

Checked in on MOZILLA_1_8_BRANCH for 1.8.1b2.

Comment 25

[Adam Guthrie]

Verified FIXED using Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1b1) Gecko/20060808 BonEcho/2.0b1.