289973 - since 20050331:[Mac] White space shows up in XUL menu after redrawing

Status: VERIFIED FIXED

(Core Graveyard :: Widget: Mac, defect, P2)

Description

[firefox]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050330 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050330 Firefox/1.0+

Open in Tabs will show an empty white spot on a second level folder after
selecting.  Selecting the empty white spot will still perform the Open in Tabs
function, but it may be confusing for some users.  This has nothing to do with
Live Bookmarks, and as far as I know, occurs only on Macs.  This has been
"broken" since 3/31's build.

Reproducible: Always

Steps to Reproduce:
1. Go to a second level folder on the Bookmarks Toolbar
2. Select "Open in Tabs"
3. Go back and try to select it again.

Actual Results:  
White spot appears where Open in Tabs should be

Expected Results:  
Open in Tabs is displayed

Default theme used.  Occurs on clean profile, once you add bookmarks to a second
level folder.  I will upload a photo ASAP.

Comment 1

[firefox]

Attachment: Picture of white space

Attachment of what i've referred to as "white space."  Again note this does not
include any Live Bookmarks

Comment 2

[Asaf Romano (gone)]

Confirmed regression. (Actually, no need to click on "Open In Tabs", just reopen
the menu)

Comment 3

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050412
Firefox/1.0+

I can't reproduce this on windoze.

Comment 4

[Asaf Romano (gone)]

-> me for investigation.

Comment 5

[Asaf Romano (gone)]

regressed in the 3103 nightly.

Comment 6

[Asaf Romano (gone)]

The only releavan difference between mac an non-mac is the way we hide the Open
In Tabs menu item (bookmarksmenus.js):

#ifdef XP_MACOSX
  // FIXME: Work around a Mac menu bug with onpopuphidden.  onpopuphiding/hidden
on mac fire
  // before oncommand executes on a menuitem.  For now we're applying a XUL
workaround
  // since it's too late to fix this in the Mac widget code for 1.5.
  hideOpenInTabsMenuItem: function (aTarget)
  {
    setTimeout(function() { BookmarksMenu.realHideOpenInTabsMenuItem(aTarget);
}, 0);
  },
#else
  /////////////////////////////////////////////////////////////////////////////
  // hides the 'Open in Tabs' on popuphidden so that we won't duplicate it -->
  hideOpenInTabsMenuItem: function (aTarget)
  {
    BookmarksMenu.realHideOpenInTabsMenuItem(aTarget);
  },
#endif

Comment 7

[Asaf Romano (gone)]

While the setTimeout hack used here (comment 6) is bogus for XUL (the mac
popuphidden bug only applies to native menus), were there any changes to the way
popuphidden/hiding works in that period (20050330-20050331), from looking at
bonsai, the checkins to content/xul are only bug 233641 and bug 288117.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Asaf, what are the pull dates (including hour) of before/after builds for this
regression?

Comment 9

[Asaf Romano (gone)]

(In reply to comment #8)
> Asaf, what are the pull dates (including hour) of before/after builds for this
> regression?

The regression area: http://tinyurl.com/8gnr6

Comment 10

[Boris Zbarsky [:bzbarsky]]

Bug 288222, perhaps?  Or bug 287592?

Comment 11

[Asaf Romano (gone)]

Neither :|

Comment 12

[Boris Zbarsky [:bzbarsky]]

Nothing else in the range looks like it could affect this.  I assume a pull by
date for "2005-03-31 07:52" Pacific time shows the bug?  And that a pull by date
for "2005-03-30 07:47" Pacific time does not?

Comment 13

[Dan]

FWIW, when the "Add Bookmark Here" extension is installed and enabled, the "Add
Bookmark Here" entry is "whited out" exactly like the "Open In Tabs" entry.  Not
sure if that might help identify the problem. I originally thought that the
extension might be causing a problem before I retested with a clean profile.

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050531
Firefox/1.0+ 

Comment 14

[Asa Dotzler [:asa]]

Josh, can you look at this?

Comment 15

[Asaf Romano (gone)]

Using latest builds, i also see that in top-level folders (under the bookmarks
toolbar).

Comment 16

[Asaf Romano (gone)]

I also see this in the pref window (starting from the point Steffen checked in
the context-sensative help fix, _which also changes the hide/shows element_),
when i switch from the General pane to the Advance pane.

Another place is the autocomplete widget (in both content and xul).

In all cases, the area itslef is "clickable", so this has to be a drawing issue.

roc, might it be a regression from Bug 288117?

Comment 17

[Simon Fraser [no longer active]]

Maybe the checkin for bug 288117 regressed this? It had a bunch of other
repercussions. The other candidate may be 288222, but Asaf has tested that already.

Comment 18

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

It's entirely possible. If the problem is easily reproducible, someone should be
able to construct a reduced test case...

Comment 19

[Gary]

This is what I know so far:

When you open a Bookmark Toolbar Folder (in the Bookmark Toolbar) it looks fine.
When you close it and open it again, the options at the bottom are whited out
(by default, the only one existing is 'Open in Tabs' but I also have 'Add
Bookmark Here' - which is whited out as well. The divider line is also whited out.)

I don't think it matters as to if it's top level, second level, etc.

If the problem happens to me on a top level folder, I simply open another folder
and move back, and it is fixed (only for that moment.) This doesn't work for
second and lower levels, though.

Comment 20

[Gary]

This problem now appears in the list that shows up when clicking on the
down-arrow next to the 'back' and 'forward' history buttons. You know, where it
lists the pages that you visited previously? Except, the problem regarding this
area seems to be fairly random for now. I can't find a common reason for this
occurence.

Also, the white space that covers up the text seems to be fairly random as well
(it still covers up normally like the bookmarks, but this time, the # of items
it covers seems to be fairly random. Or maybe I'm not paying enough attention.)

Comment 21

[Asaf Romano (gone)]

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

Comment 22

[Asaf Romano (gone)]

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

Comment 23

[Simon Fraser [no longer active]]

This is the same "invalidate inside of update" issue that we had to fix in cocoa
widgets.

Comment 24

[Simon Fraser [no longer active]]

Actually, it's a little more complex. The window is actually being resized in
the middle of an update, causing some internal dirty region tracking to get
messed up.

Comment 25

[Simon Fraser [no longer active]]

So what is actually happening here is that we've called BeginUpdate() on the
window. This causes Carbon to replace the window's visRgn with the intersection
of the visRgn and updateRgn (and stash the visRgn off somewhere). We then go to
resize the window, which calls SizeWindow(), which calculates a new visRgn. Then
we pop out to EndUpdate(), which restores the original visRgn (which is now stale).

Comment 26

[Simon Fraser [no longer active]]

Attachment: Patch

This is a not very pretty patch to fix the bug. It does the following:
1. Convert update event handling to be Carbon Event-based. This is so that we
   can handle the BeginUpdate/EndUpdate on our side, rather than having the
   embedder (or nsMacMessagePump) do it. Note that it's redundant to handle
   both kEventWindowUpdate and kEventWindowDrawContent events.

2. Wrap the SizeWindow() call, if in BeginUpdate/EndUpdate pair, in calls that
   save the old update region, break out of the update, resize the window,
   then restore the old update region and go back into BeginUpdate. This fixes
   the bug.

Comment 27

[Jo Hermans]

(In reply to comment #24)
> Actually, it's a little more complex. The window is actually being resized in
> the middle of an update, causing some internal dirty region tracking to get
> messed up.

Could this cause bug 298502 under some circumstances (very slow cpu for instance) ?

Comment 28

[Simon Fraser [no longer active]]

(In reply to comment #27)
> (In reply to comment #24)
> > Actually, it's a little more complex. The window is actually being resized in
> > the middle of an update, causing some internal dirty region tracking to get
> > messed up.
> 
> Could this cause bug 298502 under some circumstances (very slow cpu for
instance) ?

It's possible, but unclear to me how. Also, you're the only person who has seen
bug 298502 as far as I can tell.

Comment 29

[jhp (no longer active)]

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

     // kEventWindowUpdate and kEventWindowDrawContent are only for sheets.
-    PRUint32 typeCount = mIsSheet ? GetEventTypeCount(windEventList) :
-				     GetEventTypeCount(windEventList) - 2;
+    PRUint32 typeCount = GetEventTypeCount(windEventList);

Doesn't look like the comment is necessary any more.  Can be removed.

Otherwise, looks good.

Comment 30

[Simon Fraser [no longer active]]

Taking.

Comment 31

[Mark Mentovai]

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

I'm good with this.  r=mento.

One thing: You killed the only thing that calls control_key_down(), which was
debug-only anyway.  Why not kill control_key_down() as well?

Comment 32

[Simon Fraser [no longer active]]

Checked in.

Comment 33

[:Gavin Sharp [email: [e-mail]]]

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

Comment 34

[Asaf Romano (gone)]

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

Comment 35

[Craig Hunter]

(In reply to comment #20)
> This problem now appears in the list that shows up when clicking on the
> down-arrow next to the 'back' and 'forward' history buttons. 

This bug is putting Firefox into a spinning beach-ball on my system, in the history button - when it happens - and in the bookmark bar - always.
using:
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

Mac OS X v. 10.2.8

500 MHz G3

Comment 36

[Craig Hunter]

(In reply to comment #35)
this comment really belongs to bug # 298502