Build Identifier: Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110222 Firefox/4.0b12pre ID:20110222030357 Arrow of Edit Bookmarks Panel is missing Reproducible: Always Steps to Reproduce: 1. Start Minefield with new profile 2. Open any page 3. Click Star UI Actual Results: Arrow of Edit Bookmarks Panel is missing Expected Results: Arrow should be drawn properly

Please add the fplowing step to STR 4. Toggle "Show all Bookmarks Folder"

Alice, Works for me, i checked on following versions with new profiles: Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110222 Firefox/4.0b12pre Built from http://hg.mozilla.org/mozilla-central/rev/42e7f9088975 BuildID=20110222191702 Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110222 Firefox/4.0b12pre BuildID=20110222030357 Built from http://hg.mozilla.org/mozilla-central/rev/1da3405c74fd Followed the same steps 1-4, please update.

Between, checked this on Ubuntu Distribution

http://www.youtube.com/watch?v=E58sMn1kPew I can still reproduce the issue on latest m-c nightly with new profile. http://hg.mozilla.org/mozilla-central/rev/42e7f9088975 Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110222 Firefox/4.0b12pre ID:20110222191702 Ubuntu 10.04 LTS + GNOME 2.30.2 and Ubuntu 10.04 LTS + GNOME 2.30.2 on VMware (HOST OS Windows7)

yes alice.sorry my mistake.did not toggle "show all bookmarks folder" reproducible very much.Thanks for this update

I can reproduce under kwin and metacity with different X server versions. The window is visible behind where the arrow should be. (i.e. it is not just the border that is missing.)

Dao/Shorlander: can you take a look?

This may be a Widget:GTK bug, so I'll investigate there.

We only update the shape of the window when we paint. What seems to be happening is that we don't get expose events for the hidden part of the window (as there is no point in drawing to the hidden part of the window), so we don't draw there, so we don't update the shape of the window. I need to look into whether we are actually invalidating the hidden part of the window at the right time, but we may end up needing to track some invalid regions ourselves. (It would be unfortunate to make the region visible just so that we get expose events.)

This is gross. If it's our problem, we need to fix it. If it's on Ubuntu's side, it would still be nice to fix it.

On windows, the arrow jumps to the left, then situates itself back into place. Happy to make a screencast if no one else can repro, but would need instructions on what to use to do so.

(In reply to comment #12) > On windows, the arrow jumps to the left, then situates itself back into place. > Happy to make a screencast if no one else can repro, but would need > instructions on what to use to do so. Lucy, this is when you expand the folder disclosure widget thing? I see that too on windows. It's ugly. I suspect that's a bug for post Firefox 4 though and not this bug.

Yes, under the same STR as this bug, I see an issue with the same thing that's broken here, albeit not exactly the same issue. Actually watching Alice's video the arrow does indeed jump to the left and then doesn't redraw properly when it goes back to place.

when did this regress?

It's our problem, and I assume it has been there since arrow panel styling was turned on (Bug 604257 comment 9). (This is years-old code that was rarely used until then.)

(In reply to comment #10) > We only update the shape of the window when we paint. > What seems to be happening is that we don't get expose events for the hidden > part of the window (as there is no point in drawing to the hidden part of the > window), so we don't draw there, so we don't update the shape of the window. You mean the expose event region excludes the hidden part of the window? How about if, for translucent windows, we ignore the expose event region and always paint a backbuffer the size of the entire window? This should be reasonably efficient now since the layer system will have the unchanged layer contents cached.

Works for me on both Linux Mint and Arch Linux, so this seems to be Ubuntu-specific.

Works for me on Ubuntu 10.04 LTS, Nvidia drivers, amd64, Gnome, Clearlooks theme (not the default Ubuntu theme).

This doesn't reproduce on the Ubuntu 11 or 10.10 machines I checked. moving to .x because it doesn't affect all systems. Renominate if you disagree.

WFM, too, on meercat + unity/2d updates from natty, old nvidia h/w, neuveau driver, gnome, ambience theme,

Sorry if this is an uneducated question, but is it possible this is an acceleration thing?

Once the arrow disappears it is gone for good. (A new main window will have get an arrow for its panel until leaving expanded folder view.) I can't think this how this can ever work on any system with GTK version >= 2.18. It is not Ubuntu-specific. But only appearance is affected. There is no functionality lost.

(In reply to comment #17) > You mean the expose event region excludes the hidden part of the window? Yes, and there will be no expose event if only the hidden part is invalidated. > How about if, for translucent windows, we ignore the expose event region and > always paint a backbuffer the size of the entire window? We'd also need to make sure to invalidate a non-hidden part of the window (assuming some part is not hidden). > This should be > reasonably efficient now since the layer system will have the unchanged layer > contents cached. It won't be efficient because we read back to analyse the alpha values. I also note that, when we do update the shape mask, GDK uploads the mask to a pixmap on the server, and applies it to a temporary window to read the shape back as a region (set of rectangles), then applies the shape region to our window (as well as storing that region client-side). We could create the depth-1 bitmap (from alpha painted alpha values) on the server and then fetch the shape region ourselves. (This would change the cutoff from 1/255 to 128/255, so could affect styling.) We could also record info from UpdateTransparentRegion() so that we don't analyse alpha values when only opaque regions are being painted. But for fixing this bug, I see two sensible approaches: 1. If we did all the toplevel window shape setting through X rather than through GDK, then gdk_window_invalidate_rect would behave as expected. Perhaps we could even apply that bitmap as a shape on every paint, but that would be relying on the X server to not send expose events when the shape doesn't change (which would create an infinite loop). Therefore we probably should still fetch the region (as rectangles) and keep the current shape region. 2. nsWindow::Invalidate() can use gdk_window_get_update_area() to check whether the rectangle was entirely within the current shape region. If not, it can schedule a task to paint all or part of the window. This may be more future-proof against GDK thinking of new ways to interact with shape regions.

option 1 sounds good. GDK's behavior sounds terrible.

Can you have a look at this please, Michael? Bear in mind that Xfixes has a number of potentially useful shape/region/bitmap operations[1], and it may be worth checking gtk3 apis to see whether any gtk2 methods that we use (or will choose to use) are still supported in gtk3. [1] http://cgit.freedesktop.org/xorg/proto/fixesproto/tree/fixesproto.txt (One day we can use windows with RGBA visuals while compositing window managers are running, but we'll still need to have a shape-based solution for when there is no compositor.)

I've been looking at this all morning, so let me see if I've got this correct. The problem is the way GDK does things? The important call is in nsWindow::ApplyTransparencyBitmap. Assuming there's no problems with mTransparencyBitmap, I can simply replace the gtk_widget_shape_combine_mask() call with XShapeCombineMask(), assuming I can convert mTransparencyBitmap to an X Pixmap. With this approach there's no need to use regions or XFixes. karlt, did I miss anything? (BTW out of curiosity, how did you manage to track down the issue? Did you turn on any debug flags within the code?)

(In reply to comment #27) > The problem is the way GDK does things? The way GDK does things is a problem to Gecko but not because there's anything wrong with what GDK does. It's just that Gecko thinks about things differently. GDK/X think about shapes and drawing quite separately, except that drawing and invalidation is confined to the visible region. Gecko OTOH thinks about shapes as the alpha channel of drawing. The problem is that invalidations don't cause us to draw outside the visible region (shape). Using X instead of GDK to apply the shape would mean that GDK doesn't know that part of the window is shaped-out and so would always give us expose events in response to invalidations of invisible regions. There's a number of inefficiencies in the process, and I think comment 25 was directed at what GDK does when we apply the shape mask, but there are also inefficiencies on our side. > The important call is in > nsWindow::ApplyTransparencyBitmap. Assuming there's no problems with > mTransparencyBitmap, I can simply replace the > gtk_widget_shape_combine_mask() call with XShapeCombineMask(), assuming I > can convert mTransparencyBitmap to an X Pixmap. With this approach there's > no need to use regions or XFixes. mTransparencyBitmap can be converted to an X Pixmap (and that is what gtk_widget_shape_combine_mask does), so that would work. There would still be inefficiencies. Currently we: 1) Ask the server to draw, which happens in graphics memory. 2) Ask the server for the results, so the server reads the results back to server system memory, and we copy that into client (system) memory to analyse the bitmap and check for changes. 3) If the shape has changed, then we upload the new bitmap (back) to the server. I was thinking we could instead ask the server to copy the alpha values into the server-side bitmap, and read the result back as a region instead of a bitmap (bringing in Xfixes) to check for changes (or even do region arithmetic on the server, to reduce what needs to be read back). Advantages of this approach are that regions are remembered instead of bitmaps and that all the bitmap to region logic happens on the server. However, the server would still most likely be reading back from graphics memory to system memory when working with a bitmap. There are also scarey issues about how well using a 128/255 cutoff would work with premultiplied color values. I'm now thinking it may be best to stick with keeping an mTransparencyBitmap (do what you suggest to fix this bug) and deal with the readback inefficiency by drawing client-side onto image surfaces (which actually need not necessarily be dealt with in this bug). > (BTW out of curiosity, how did you manage to track down the issue? Did you > turn on any debug flags within the code?) I can't remember, for certain but I likely used NSPR_LOG_MODULES=Widget:5,WidgetDraw:5 or similar to get dumps of expose rectangles.

Oh, I see now. I thought it was already all done client side. Cairo's API is hiding all that from me... IIRC GTK3 uses RGBA windows by default when on a composited server. So in the GTK3 port we can skip this code entirely when composited, and try your region approach when we aren't. Incidentally, in GTK3 you can only set a shape using a cairo_region_t, you can't use a mask anymore.

I also noticed that when this bug is happening, the popup also has an incorrect rectangular shadow (for WMs that do that). I thought shadows were only removed when the window is set to RGBA, but I'll have to look into that further.

Comment on attachment 530980 [details] [diff] [review] Patch Thanks, Michael. Outside of GDK, GDK_WINDOW_XDISPLAY and GDK_WINDOW_XID are both function calls, so it would make sense to call them only once, using temporary variables for their results. >+ Pixmap maskPixmap = XCreateBitmapFromData(GDK_WINDOW_XDISPLAY(mShell->window), >+ GDK_WINDOW_XID(mShell->window), >+ mTransparencyBitmap, >+ mTransparencyBitmapWidth, mTransparencyBitmapHeight); Can you wrap at 80 columns, please?

(In reply to comment #30) > I also noticed that when this bug is happening, the popup also has an > incorrect rectangular shadow (for WMs that do that). I thought shadows were > only removed when the window is set to RGBA, but I'll have to look into that > further. Sounds similar to bug 514182.

I meant bug 514182. (attachment 514182 [details])

bug 635897, even.

I don't think we should be worrying much about inefficiencies dealing with shapes and window regions. RGBA windows are the future and we should focus on that.

Comment on attachment 530980 [details] [diff] [review] Patch Given that this small patch fixes a visual defect I'm hoping I can get this into 5.0. (By the time 6.0 gets released we'll probably default to RGBA windows anyway making this fix needless)

Comment on attachment 530980 [details] [diff] [review] Patch Not primary UI and while this is ugly, it's not the kind of high visibility we'd take this late in the game.

(In reply to comment #39) > Comment on attachment 530980 [details] [diff] [review] [review] > Patch > > Not primary UI and while this is ugly, it's not the kind of high visibility > we'd take this late in the game. What is "primary UI" if not a panel with a (default, non-removable,) highly visible "button" on the browser? Just saying...

(In reply to comment #40) > What is "primary UI" if not a panel with a (default, non-removable,) highly > visible "button" on the browser? Just saying... The primary UI in this case is the bookmarks star in the address bar. That is the mechanism for bookmarking a site and that looks and behaves as expected. The secondary UI is what you get when you click the star a second time and it brings up the arrow panel. That UI also looks and works just fine. It is the tertiary UI, what happens when you click the start twice, then you toggle the "Show all Bookmarks Folder" button to expand the folder control. I am in no way saying that this isn't a bug. I am saying that this is a bug which is already fixed and will be shipping in Firefox 6 and does not rise to the level of visibility that we're going to consider taking the change on the already stabilized Aurora branch for Firefox 5.

Mozilla/5.0 (X11; Linux i686; rv:8.0a1) Gecko/20110707 Firefox/8.0a1 Verified issue using the STR from Comment 1 and Comment 2 on Ubuntu 10.10 and 11.04. Setting resolution to VERIFIED FIXED.