635903 - Arrow of Edit Bookmarks Panel is missing after toggling the expanded folder view

Status: VERIFIED FIXED

(Core :: Widget: Gtk, defect)

Description

[Alice0775 White]

Attachment: Screenshot

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

Comment 1

[Alice0775 White]

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

Comment 2

[aravindm]

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.

Comment 3

[aravindm]

Attachment: Screenshot shows Edit Bookmarks dialog

Comment 4

[aravindm]

Between, checked this on Ubuntu Distribution

Comment 5

[Alice0775 White]

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)

Comment 6

[aravindm]

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

Comment 7

[Karl Tomlinson (:karlt)]

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.)

Comment 8

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

Dao/Shorlander: can you take a look?

Comment 9

[Karl Tomlinson (:karlt)]

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

Comment 10

[Karl Tomlinson (:karlt)]

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.)

Comment 11

[Johnathan Nightingale [:johnath]]

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.

Comment 12

[Majken Connor [:Kensie]]

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.

Comment 13

[Asa Dotzler [:asa]]

(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.

Comment 14

[Majken Connor [:Kensie]]

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.

Comment 15

[Andreas Gal :gal]

when did this regress?

Comment 16

[Karl Tomlinson (:karlt)]

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.)

Comment 17

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

(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.

Comment 18

[Patrick Walton (:pcwalton)]

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

Comment 19

[Mihai Sucan [:msucan]]

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

Comment 20

[Robert Sayre]

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.

Comment 21

[aja+bugzilla]

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

Comment 22

[Majken Connor [:Kensie]]

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

Comment 23

[Karl Tomlinson (:karlt)]

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.

Comment 24

[Karl Tomlinson (:karlt)]

(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.

Comment 25

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

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

Comment 26

[Karl Tomlinson (:karlt)]

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.)

Comment 27

[Michael Ventnor]

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?)

Comment 28

[Karl Tomlinson (:karlt)]

(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.

Comment 29

[Michael Ventnor]

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.

Comment 30

[Michael Ventnor]

Attachment: Patch

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 31

[Karl Tomlinson (:karlt)]

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?

Comment 32

[Karl Tomlinson (:karlt)]

(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.

Comment 33

[Karl Tomlinson (:karlt)]

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

Comment 34

[Karl Tomlinson (:karlt)]

bug 635897, even.

Comment 35

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

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 37

[Michael Ventnor]

http://hg.mozilla.org/mozilla-central/rev/cc4677956189

Comment 38

[Michael Ventnor]

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 39

[Asa Dotzler [:asa]]

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.

Comment 40

[Nochum Sossonko [:Natch]]

(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...

Comment 41

[Asa Dotzler [:asa]]

(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.

Comment 42

[Simona Badau, Desktop QA]

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.