298889 - iframe invisible when window is transparent

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Cedric Vivier [:cedricv]]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041208 Firefox/1.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050531 Firefox/1.0+

When a window has CSS background property set to transparent, iframes's content
within the window are invisible.


Reproducible: Always

Steps to Reproduce:
1. add "window { background: transparent !important; } in your UserChrome.css
2. launch browser

Actual Results:  
the document iframe's content is invisible although it is "rendered" since you
can click on links.

Expected Results:  
render iframe's content correctly.

this bug applies also to any XUL window with a iframe inside!

Comment 1

[Cedric Vivier [:cedricv]]

Attachment: screenshot

Comment 2

[Cedric Vivier [:cedricv]]

Attachment: userChrome.css

just put this file in your Firefox Deerpark directory, then launch browser.

Comment 3

[Cedric Vivier [:cedricv]]

Attached a workaround patch adding a boolean pref to disable layered windows on
Windows 2K/XP (which by the way are SLOOW and un-needed for transparent windows
without translucency)

To disable layered windows, set config.disable_layered_windows to true.

It also have much better translucency without layered windows by using a more
diplomatic setting for opaque/transparent matching.

Comment 4

[Cedric Vivier [:cedricv]]

Attachment: first patch

oops sorry :/	 here it is

Comment 5

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

I don't think we'll fix this bug for 1.8. In 1.9 it should go away by itself
when we move to having just one widget for the entire application window.

Comment 6

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

Cedric, you have a bunch of stuff in that patch.

The last chunk looks like a good fix. I think we'd take that. But if (*pPixel >
128) should be >= 128, I think, and the other pixels should be set to zero.

I don't think we really want to introduce a new pref. If using the window region
is really preferred when translucency is not required, we could scan the alpha
map to see if there are any translucent pixels. If all alphas are 0 or 255, then
we could use the region.

-          if (!*pDest != !*pSrc)
+          if (*pDest != *pSrc)

This changes behaviour and I'm not sure you meant to do that. For example, if
*pDest is 2 and *pSrc is 3.

Comment 7

[rob lord]

Bug confirmed in Songbird. See attachment. 

http://songbirdnest.com/

Comment 8

[rob lord]

Attachment: See red circled area. Corner should be transparent.

Comment 9

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

Rob Lord, can you explain why you think this is the same bug? Maybe you could file a new bug about the Songbird issue with more details.

Comment 10

[mig]

This is the same bug because when we turn on the "background-color: transparent;" style on any OS-level <window> or <dialog> XUL element, any <browser> or <iframe> elements contained within that window or dialog do not paint.

My boss, however, does not understand how to file bug reports and misunderstood the actuality of the bug when creating his very helpful attachment.  We currently disable window transparency because having iframes is more important to our functionality.  If you let us have our iframes, then he gets his rounded corners and everyone walks away happy.

mig

Comment 11

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

aaaah, that makes sense! thanks.

Comment 12

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

Attachment: fix?

This patch might fix your troubles. Please apply it and see, and let me know the results. On GTK2 there are some rather ugly flickery transparency artifacts as we resize a window or flip between pages in the browser area (in a transparency-ized Firefox), but fixing those would probably build on this patch so if this patch works for you, I'd try to land it.

Comment 13

[mig]

So, this seems almost there?  

When it was rendering our .xul chrome in the iframe, everything was great.  When it was rendering remote html pages, it seems like it wouldn't repaint the window properly when we would do things like scroll.

Comment 14

[Brendan Eich [:brendan]]

(In reply to comment #13)
> So, this seems almost there?  

Ideally, you guys build your own XULRunner with roc's patch, test, and testify.  If you have to ask, it sounds like you're waiting for someone else to test, but I don't know who else will test in the way that you care about.

/be

Comment 15

[mig]

???

No.  That was my response to trying the patch.  Along with a description of what we encountered when we did try the patch.

Since it mostly worked but there were some obvious update problems, it rates an "almost" with a question mark.

We do appreciate the effort placed in the patch, unquestionably, but we could not ship with it.

Comment 16

[Brendan Eich [:brendan]]

mig: oh, sorry -- I misread your comment.  If the patch doesn't totally fix this bug, but makes things better in some ways and never worse, it could be checked in once reviewed.

/be

Comment 17

[mig]

As said before, when applied, the patch causes some form of obvious update problem when loading remote HTML into the iframe (but seemed to render our XUL just fine).  

The easiest way to see this update problem was to attempt to scroll with the scrollbar -- it would scroll some 15-20 pixels and then seem to stop tracking the mouse.  If you changed the window size, even slightly, the iframe would then repaint with what appeared to be the proper scroll position.  From this, we assume someone's losing the paint messages.

It is worse to have webpages fail to draw properly than to have squared off corners, so, as stated, we could not ship with this patch installed.

Comment 18

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

Scrolling should work with that patch, because it basically turns all scrolls into repaints when the window is transparent.

Dainis, can you try this patch with a Firefox build with "window { background:transparent; }" in userChrome.css, and see if you can see what's going wrong?

Comment 19

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #18)

I manually applied attachment 206057 [details] [diff] [review] to a 1.8 branch build (I can post the 1.8 version if anyone wants it). Adding "window { background: transparent !important; }" to userChrome.css I am seeing behavior descibed exactly in bug 252067 comment 47 as well as the behavior in comment 17.

> Scrolling should work with that patch, because it basically turns all scrolls
> into repaints when the window is transparent.

Scrolling is definitely broken, and I'm inclied to believe that it's an update problem rather than an issue with the underlying objects. I'm basing this belief on the fact that the mouse positions that trigger a cursor change (like when mousing over a hyperlink) change the instant you move the scrollbar. The repainting does not occur, however, until you mouseover the new hyperlink location.

Also, Firefox behaves incredibly slowly with this patch applied. Quickly mousing in and out of hyperlinks easily sets my CPU usage above 80%. During this little test my console also spits out tons of NS_WARNINGs fired from nsViewManager::RenderViews, indicating that this function is being called multiple times for every mouse enter/leave event. This behavior is not limited to rendered html, either: I can get huge CPU usage by mousing in and out of any XUL widgets (menus, buttons, etc.).

Comment 20

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Ah, I just realized: The slowness and CPU hogging occur without roc's patch applied, as well. Separate bug?

Comment 21

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

Manipulating large, complex transparent windows is going to be slow because we have to keep recalculating alpha values in a very nasty way. That's just the way it is in 1.8. Things will get better in 1.9.

Comment 22

[Dainis Jonitis]

To simplify the testing I started with 1-bit transparency (as on Win9x). I can open web pages and even scroll them, but rather often the assertion in nsBlender::GetAlphas () line 380 is fired that "Mismatched bitmap formats (black/white) in Blender". Thats because blackSpan (76) != whiteSpan (72), but both blackBytesPerLine and whiteBytesPerLine are equal to 72. blackSurface.mBitmap.bmWidth = 19, but whiteSurface.mBitmap.bmWidth = 18 (at 32bits per pixel).
Do you see the same problem under Linux or is it yet another bug in nsDrawingSurfaceWin? Or could it be a rounding error that comes from nsViewManager when it created different size drawing surfaces? I think I get this only when page has vertical scrollbar.

Comment 23

[Dainis Jonitis]

Aha, and text cursor is not drawn because the nsCaret::GetCaretRectAndInvert() draws directly in mMemoryDC relying on XOR behaviour and does not call Invalidates - thus we do not know that we should update the screen. No idea how this can be fixed.

Comment 24

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

(In reply to comment #22)
> To simplify the testing I started with 1-bit transparency (as on Win9x). I
> can open web pages and even scroll them, but rather often the assertion in
> nsBlender::GetAlphas () line 380 is fired that "Mismatched bitmap formats
> (black/white) in Blender". Thats because blackSpan (76) != whiteSpan (72),
> but both blackBytesPerLine and whiteBytesPerLine are equal to 72.
> blackSurface.mBitmap.bmWidth = 19, but whiteSurface.mBitmap.bmWidth = 18 (at
> 32bits per pixel).
> Do you see the same problem under Linux or is it yet another bug in
> nsDrawingSurfaceWin? Or could it be a rounding error that comes from
> nsViewManager when it created different size drawing surfaces? I think I get
> this only when page has vertical scrollbar.

I don't see that on Linux.

The caret problem is will be fixed by mrbkap on trunk when we move caret painting to the regular paint path. We could hack in a solution for the branch, maybe.

Comment 25

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

Dainis, do you feel like tracking down the problem you mentioned in comment #22?

Comment 26

[Dainis Jonitis]

I was debugging the 256 level translucency problem, but still have no clue why it does not work. CPU usage is not 100%. First I have to better understand how scrolling works in normal windows. Unless you can recreate on Linux, I can also look at 1-bit translucency problem from comment #22.

  To my understanding layered windows were meant to some small things and are not well suited to display the entire application window. For example if we need blinking text cursor then for each blink we have to call ::UpdateLayeredWindow for _entrire_ window.
  Probably it is worth adding a hint to window which kind of transparency to use. By default it will be 256 level, but if application does not need this then it can specify that 1-bit transparency is enough. Unless transparency mask changes the shape all the time, it should be much more efficient and still sufficient to achieve rounded corners etc.
  I guess it can be specified by attribute of element. Something like in bug 253481, where XUL:textbox attribute newlines="stripsurroundingwhitespace" was hinting what to do with newlines when pasting the text from clipboard.

Comment 27

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

Could we look at the first alpha mask and if all pixels are either 0 or 255, use 1-bit transparency, otherwise use 8-bit?

Comment 28

[Dainis Jonitis]

What if one wants to implement fade-out effect? Initially all alphas will be 255.
I think only author knows and can specify what he wants.

Comment 29

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: 1-bit transparency autodetect v1.0

Dainis is right: layered windows are not a good solution for large windows, especially if all you need is rounded corners.

Here's a patch that I was playing around with last night that applies to the 1.8 branch. It's horribly inefficient (the scanning function gets called far to often), but it gets the job done, I think. I'm only posting it because I'm about to go on vacation and don't know when I'll be doing more moz work.

Using this patch I hit the assertion mentioned in comment 22 very frequently whenever there is a vertical scrollbar. There's definitely a bug lurking in there.

Comment 30

[Dainis Jonitis]

Robert,
I traced down the problem why warning "Mismatched bitmap formats (black/white) in Blender" appears. The problem is in difference how different code parts in nsViewManager.cpp scale twips to pixels.

Here is the detailed step-by-step recreation scenario:

1. nsViewManager::Refresh() line 854. 
  damegeRect={x=2160,y=1320,w=270,h=270}
  AppUnitsToDevUnits = 0.066666670
  widgetDamageRectInPixels={x=144,y=88,w=19,h=19}
2. nsViewManager::Refresh() line 888. 
  localcx->GetBackbuffer( {x=0,y=0,w=19,h=19} )
3. nsViewManager::Refresh() line 929.
  RenderViews() with damageRegion (damage rect = {x=2160,y=1320,w=270,h=270})
4. nsViewManager::RenderViews() line 1351 (same rect)
5. nsViewManager::CreateBlendingBuffers line 1569
  offscreenBounds = {0, 0, 18, 18}
6. nsViewManager::RenderViews() line 1443
  aRegion.GetBounds() = {x=2160,y=1320,w=270,h=270}
  bufferRect = {x=0,y=0,w=18,h=18}

Backbuffer is 19x19, but blending buffer 18x18!
All places except calculation of widgetDamageRectInPixels do not take into account x and y values when doing scaling. But nsRect::ScaleRoundOut produces different width and height depending of current value of upper left corner.
That inconsistency causes the creation of differently sized drawing surfaces.

Merry Christmas!

Comment 31

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

Attachment: patch to fix GetAlphas error

Try this additional patch. It should fix the BlendingBuffer error.

Comment 32

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

Comment on attachment 206651 [details] [diff] [review]
patch to fix GetAlphas error

ooops, that contained unrelated junk

Comment 33

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

Attachment: patch to fix GetAlphas error

Just this one.

Comment 34

[Dainis Jonitis]

Above patch fixes the warning, but to compile with VC++ 2003 you have to make these changes:

    PRUint32 width, height;
    aBorrowSurface->GetDimensions(&width, &height);
    offscreenBounds.SizeTo(width, height);

Otherwise build fails with error that cannot convert from *nscoord to *PRUint32

Comment 35

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: 1-bit transparency autodetect v1.1

Oops, just realized that mAlphaMask doesn't always exist... This should fix some crashes. Thoughts on this?

Comment 36

[Dainis Jonitis]

Do not introduce new member variable mIsOneBitTranslucent. It is equal to !IsAlphaTranslucencySupported().

Comment 37

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

Is it safe to change from layered-window to normal window and back again?

Does this patch work well for Songbird?

We can probably speed up the one-bit-ness test a lot. For one thing, runs of transparent pixels and opaque pixels usually occur together. So

  // Scan through the alpha mask looking for anything other than 0 or 255
  PRUint32 max = mBounds.height*mBounds.width;
  PRUint32 i;
  for (i = 0; i < (max >> 2); ++i)
  {
    PRUint32 v = ((PRUint32*)pPixel)[i];
    // v is probably either 0x00000000 or 0xFFFFFFFF
    if ((v + 1) < 2)
      continue;
    for (PRUint32 j = 0; j < 4; ++j)
    {
      if ((((v >> (j*8)) + 1) & 0xFF) >= 2)
        return PR_FALSE;
    }
  }
  for (i = i*4; i < max; ++i)
  {
    if (((pPixel[i] + 1) & 0xFF) >= 2)
      return PR_FALSE;
  }

Comment 38

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #36)
> Do not introduce new member variable mIsOneBitTranslucent. It is equal to
> !IsAlphaTranslucencySupported().

The idea is to just disable layered windows when you can get away with it, and that's not necessarily related to whether or not the OS will support layered windows. In the case of an alpha mask with all 0 or 255 values, for instance, the OS may support layered windows just fine and so you'd expect IsAlphaTranslucencySupported to return true in case you later wanted something to show up as translucent.

(In reply to comment #37)
> Is it safe to change from layered-window to normal window and back again?

In theory yes, but I seem to recall some issues in the past with windows repainting as big black rectangles when switching in and out of layered mode... Maybe MS fixed that by now?

> Does this patch work well for Songbird?

It's being investigated right now...

> We can probably speed up the one-bit-ness test a lot. For one thing, runs of
> transparent pixels and opaque pixels usually occur together.

Thanks for the optimization - I'll check it out.

Comment 39

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #37)
> Does this patch work well for Songbird?

Wow, sorry to have let this slip for so long. No, the patch did not work and we went with a native component to take the corners off until we have more time to investigate.

We're all booked up working on our linux and mac versions atm (our corners are present on these os'es, sadly), but once our 0.2 milestone has passed I'd love to get back to work on this. We could, of course, wait until 1.9 to fix this but I feel like this last patch was pretty close to getting a hack-ish solution for 1.8.

Thoughts?

Comment 40

[edwardbaafi]

Attachment: fix? (roc's attachment 206057 for 1_8_0 branch)

A version of roc's attachment id=206057 from comment #12 for the 1_8_0 branch.

Comment 41

[edwardbaafi]

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

Comment 42

[edwardbaafi]

(In reply to comment #16)
> If the patch doesn't totally fix
> this bug, but makes things better in some ways and never worse, it could be
> checked in once reviewed.
> 

So it seems this patch ((id=206057)) is a good start..  Reading the posts, it seems nobody is saying this makes things worse but just not good enough.. 

I filed a bug (see duplicate bug notice above) when I found that listbox elements wouldn't paint when running xulrunner with background:transparent. I later realized that content within deck elements wouldn't paint either.. There are probably numerous other xul elements that are affected by this!  

This patch seems to fix these problems..  As stated by other posters, content within iframe or browser elements have some problems.. But having painting problems has to be better than not painting at all..

I'd also like to be able to decide whether I wanted layered windows or window regions without having to take the hit from scanning the alpha mask.. I don't care if it's a pref or attribute..  Better yet, let me choose: layered, region or auto..  Too bad "background:transparent" already means "translucent if possible" because I'd like to choose "background:transparent" or "background:translucent"..  Not sure how cross platform this would be?  
 
The 1.8 branch is going to be around for a while so I would vote for CHOICE of layered vs regions + roc's patch even as is (after review of course) and better with a fix for the html rendering..  

Will test more but.. Thoughts? 

Comment 43

[[e-mail]]

(In reply to comment #37)
>We can probably speed up the one-bit-ness test a lot.

PRUint32 v = ((PRUint32*)pPixel)[i];
if (v != ((v & 0x01010101) * 0xFF))
  return PR_FALSE;

Comment 44

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

Comment on attachment 208652 [details] [diff] [review]
1-bit transparency autodetect v1.1

taking this off the radar.

I think the best way to handle this would probably be via a new attribute on the XUL root element --- "binarytransparency" or something, which forces the window to 1-bit transparency mode if it's transparent/translucent.

Comment 45

[Bertrand Janin]

Same problem here, my company is trying to create a skinned browser for one of our web applications. Unfortunately the <browser> disappears as soon as the window's background-color is transparent. We can of course disable the rounded corners but I feel the company will unfortunately go for an embed IE7 instead, just because of those bloody rounded corners. It is still a bug in 1.9b5 win32.

Comment 46

[mig]

(In reply to comment #45)
> Same problem here, my company is trying to create a skinned browser for one of
> our web applications. Unfortunately the <browser> disappears as soon as the
> window's background-color is transparent. We can of course disable the rounded
> corners but I feel the company will unfortunately go for an embed IE7 instead,
> just because of those bloody rounded corners. It is still a bug in 1.9b5 win32.
> 

Bertrand, we worked around this by applying our own subtractive Win32 regions to the HWND we get back from the API.  This works but it was going to be a chunk of work to go from hardcoded roundiness to overriding the CSS then reading from the CSS info to get and calculate a custom region from -moz-border-radius.

Comment 47

[Damon Sicore (:damons)]

Unfortunately it's too late in the release process for this to be considered and it's not a regression from 2.0.  Not going to block for this.

Comment 48

[Bertrand Janin]

(In reply to comment #46)
> Bertrand, we worked around this by applying our own subtractive Win32 regions
> to the HWND we get back from the API.  This works but it was going to be a
> chunk of work to go from hardcoded roundiness to overriding the CSS then
> reading from the CSS info to get and calculate a custom region from
> -moz-border-radius.
> 

Thank you for your answer, where in the src tree did you guys apply this patch?

Comment 49

[mig]

(In reply to comment #48)
> (In reply to comment #46)
> > Bertrand, we worked around this by applying our own subtractive Win32 regions
> > to the HWND we get back from the API.  This works but it was going to be a
> > chunk of work to go from hardcoded roundiness to overriding the CSS then
> > reading from the CSS info to get and calculate a custom region from
> > -moz-border-radius.
> > 
> 
> Thank you for your answer, where in the src tree did you guys apply this patch?
> 

Errr, we didn't.  We added it extra to our own Songbird API, made an xbl to wrap the API calls and tossed the xbl into any window upon which we wanted to round our corners.

http://publicsvn.songbirdnest.com/browser/trunk/components/integration/public/IWindowRegion.idl
http://publicsvn.songbirdnest.com/browser/trunk/components/integration/src/win32/WindowRegion.cpp
http://publicsvn.songbirdnest.com/browser/trunk/components/integration/src/NativeWindowFromNode.cpp

Comment 50

[Daniel Veditz [:dveditz]]

A fix would be nice, but we can't block on it for the branch hoping one will show up. Also would be bad for FF2 to suddenly improve when FF3 won't have the fix (comment 47) so it's most important to get a trunk fix first.

Comment 51

[XiaoXiaoHU]

I have made a patch to fix this bug, the attached patch files is made for and tested with xulrunner-1.9.0.6 branch, to patch latter version of mozilla (i.e. version above 1.9.1) just replace all GetTransparencyMode() with GetHasTransparentBackground() like the commented lines.

Comment 52

[XiaoXiaoHU]

Attachment: patch to fix invisible iframe/browser when background is transparent

See above comments for details.

Comment 53

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

That's a pretty good patch! I wouldn't want to take it on a branch, but we could take it on trunk.

Comment 54

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

So, please give me the trunk version to review. Also, it would be good if you can use -p in your diffs. Thanks!

Comment 55

[XiaoXiaoHU]

I will do it, but if I also want to see it's patched against the firefox 3.0.x branch, whether it means I should provide 2 patches, one for the cvs head, and one for the Mercurial trank?

Comment 56

[XiaoXiaoHU]

And it seems if I also want to see it's on the firefox 3.1.x branch, I also should provide a separate patch for the mozilla-1.9.1 branch.

Comment 57

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

(In reply to comment #55)
> I will do it, but if I also want to see it's patched against the firefox 3.0.x
> branch, whether it means I should provide 2 patches, one for the cvs head, and
> one for the Mercurial trank?

This fix would not meet our criteria for landing on the 1.9.0.x branch.

The patch for trunk should apply to the 1.9.1 branch also.

Comment 58

[XiaoXiaoHU]

Attachment: Fix for the mozilla-central trunk [Checkin: Comment 68]

The patch for the mozilla-central trunk version.

Comment 59

[XiaoXiaoHU]

Attachment: The patch for the mozilla-1.9.1 branch

The patch for the mozilla-1.9.1 branch.

Comment 60

[XiaoXiaoHU]

Attachment: The patch for the mozilla cvs head

Whatever, the patch for the cvs head (i.e. for firefox 3.0.x branch) is also provided.

Comment 61

[mig]

(In reply to comment #60)
> Created an attachment (id=363676) [details]
> The patch for the mozilla cvs head
> 
> Whatever, the patch for the cvs head (i.e. for firefox 3.0.x branch) is also
> provided.

That's okay, it's super useful to other teams that don't have the same requirements as the FF3.0 app.

Thanks!  (from someone who isn't supposed to care anymore)

Comment 62

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

In nsViewManager::UpdateWidgetArea:

+  if (widget->GetTransparencyMode() != eTransparencyTransparent) {
+    for (nsIWidget* childWidget = widget->GetFirstChild();
+         childWidget;
+         childWidget = childWidget->GetNextSibling()) {

This is assuming that the child widgets won't be visible. I don't think that's safe on all platforms. If you take this change out, do things work, maybe slower?

+  if (aMode == eTransparencyTransparent) {
+    style &= ~(WS_CAPTION | WS_THICKFRAME | WS_SYSMENU | WS_MINIMIZEBOX | WS_MAXIMIZEBOX);
+    exStyle &= ~(WS_EX_DLGMODALFRAME | WS_EX_WINDOWEDGE | WS_EX_CLIENTEDGE | WS_EX_STATICEDGE);
+  }

You added this since your last patch. Is this really necessary? This seems like something that HideWindowChrome should take care of.

Comment 63

[XiaoXiaoHU]

Yes, it's necessary. Otherwise even the caption bar is not shown (for a layered window, the WS_CAPTION will not make a visible caption bar), the mouse event will have wrong position which including the caption bar height, so without this modification, the result is every mouse click event happened to a position some offset below the real one.

Comment 64

[XiaoXiaoHU]

For the UpdateWidgetArea() changes, if not modified, the embeded iframe/browser will generate a WM_PAINT message to the window bind to their widget, but for layered window, the child window should not paint itself (because it paints on the DC, nothing will show), so instead, WM_PAINT message should going to the toplevel window which will paint the child window corretly.

Yes, here assurmed that the toplevel window knows how to paint the child window, this is not always true (but for iframe/browser child window it is true), for e.g. a flash embbed in the iframe/browser could not be painted, it's an another bug, but it's not easy to be fixed.

Comment 65

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

It might not be true on other platforms, that's what I'm worried about.

So if we remove the UpdateWidgetArea changes, everything still works, right? It's just slower.

Comment 66

[XiaoXiaoHU]

If the UpdateWidgetArea() not modified, the scrollbar inside the iframe/browser
will not update when you do scrolling, because the toplevel window don't
received a WM_PAINT message to do the updates, the message is send to the child
window, which has no effect according to the above comments.

Comment 67

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

Comment on attachment 363674 [details] [diff] [review]
Fix for the mozilla-central trunk
[Checkin: Comment 68]

OK

Comment 68

[Serge Gautherie (:sgautherie)]

Comment on attachment 363674 [details] [diff] [review]
Fix for the mozilla-central trunk
[Checkin: Comment 68]


http://hg.mozilla.org/mozilla-central/rev/9f8224db57f5

Comment 69

[XiaoXiaoHU]

Robert, what about the 1.9.1 branch?

Comment 70

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

Add the approval1.9.1? flag to your 1.9.1 patch. It would also help if you explain here in the bug why you need this, in as much detail as possible.

Comment 71

[XiaoXiaoHU]

Comment on attachment 363675 [details] [diff] [review]
The patch for the mozilla-1.9.1 branch

I'm working on an extension which depends this fix, i wish to publish my extension in the near future, so wait the trunk version take too long.
Since all the modification in this patch is surrounded by a if (is transparent window), so it's safe to include the patch and not break other part, and the original transparent window code is indeed not working, so it's also not possible to make things worse.

Comment 72

[XiaoXiaoHU]

And I believe the transparent window support will introduce many intersting and fancy new firefox extensions. Yeah, you know, I'm not just talking about my one :)

Comment 73

[WL]

Hi, would anyone kindly explain what's the relationship between this bug and bug 130078?
I've tested long time ago that XUL:Tree element won't rendered in a chrome transparent window, because of the iframes. Do you mean this patch fix it?

Comment 74

[XiaoXiaoHU]

I don't get time to investigate bug 130078, but just make a simple test with XUL:Tree in a chrome transparent window after this patch, it seems ok.

And Robert, what about the super reviewing progress of the patch for the 1.9.1 branch?

Comment 75

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

You mean approval?

Comment 76

[XiaoXiaoHU]

Yes, I had thought the approval process is the same as super reviewing.
So, when can I expect to get a final approval result? Thanks.

Comment 77

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

Comment on attachment 363675 [details] [diff] [review]
The patch for the mozilla-1.9.1 branch

a1.9.1=dbaron

Comment 78

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/34d1d4206402

Comment 79

[Dave Townsend [:mossop]]

(In reply to comment #78)
> http://hg.mozilla.org/releases/mozilla-1.9.1/rev/34d1d4206402

I've backed this out from the branch since it was causing the mochitest and mochichrome runs to crash on linux.

Comment 80

[Serge Gautherie (:sgautherie)]

(In reply to comment #79)
> it was causing the mochitest and mochichrome runs to crash on linux.

{
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.1/1237036915.1237045662.16615.gz
Linux mozilla-1.9.1 unit test on 2009/03/14 06:21:55

TEST-UNEXPECTED-FAIL | Exited with code 11 during test run
TEST-UNEXPECTED-FAIL | runtests-leaks | missing output line for total leaks!
TEST-UNEXPECTED-FAIL | Exited with code 11 during test run
TEST-UNEXPECTED-FAIL | runtests-leaks | missing output line for total leaks!
}

{
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1237036820.1237039423.32664.gz
Linux comm-central dep unit test on 2009/03/14 06:20:20

TEST-UNEXPECTED-FAIL | Exited with code 35584 during test run
TEST-UNEXPECTED-FAIL | runtests-leaks | missing output line for total leaks!
TEST-UNEXPECTED-FAIL | Exited with code 35584 during test run
TEST-UNEXPECTED-FAIL | runtests-leaks | missing output line for total leaks!
}

Comment 81

[XiaoXiaoHU]

Oh, I'm really sad to see this result, if someone is familiar with this unit test, could you kindly explain to me what this fail means and posible cause, then maybe I could also contribute to investigate it, I do want to see it go forward, thanks.

Comment 82

[Dave Townsend [:mossop]]

(In reply to comment #81)
> Oh, I'm really sad to see this result, if someone is familiar with this unit
> test, could you kindly explain to me what this fail means and posible cause,
> then maybe I could also contribute to investigate it, I do want to see it go
> forward, thanks.

It basically means that the tests for test_bug355213.xhtml and test_postMessage_chrome.html are doing something that causes your code change to crash. Since it is only on branch it also likely means there is some change on trunk that either makes your code change safe, or hides the problem there

Comment 83

[XiaoXiaoHU]

I'm sorry, what's the crash means here, a real program crash or just return some test result not as expected?

Comment 84

[Dave Townsend [:mossop]]

(In reply to comment #83)
> I'm sorry, what's the crash means here, a real program crash or just return
> some test result not as expected?

A real program crash.

Comment 85

[Brendan Eich [:brendan]]

Do full logs now have stack backtraces? I saw one the other day...

/be

Comment 86

[Dave Townsend [:mossop]]

(In reply to comment #85)
> Do full logs now have stack backtraces? I saw one the other day...

No. Maybe support for that has not made it to the 1.9.1 branch yet.

Comment 87

[XiaoXiaoHU]

I made the mochitest on my ubuntu machine, running a full test seems take too long, so I only make test one by one, the result is:

The first 2 unspected-fail tests, i.e.:
/tests/content/media/video/test/test_progress3.html
/tests/content/xml/document/test/test_bug355213.xhtml
passed the test successfully, so unable to reproduce the same result on my build.

My .mozconfig was attached.

Can anybody tell me how to run the 3rd test alone? i.e.: 
chrome://mochikit/content/chrome/dom/tests/mochitest/whatwg/test_postMessage_chrome.html

Thanks.

Comment 88

[XiaoXiaoHU]

Attachment: My mozconfig for linux mochiteset test

Comment 89

[XiaoXiaoHU]

Wow, I caught it, attached is the backtrace when ff crashs during --chrome testing,
it seems the nsViewManager try to do UpdateView() against a destroying widget (the widget's mDrawingarea->inner_window has already been destroyed), so when my patched code try to check whether the toplevel window is transparent, it can not access the toplevel window, because in the GetToplevelWidget(), finally get_gtk_widget_for_gdk_window(mDrawingarea->inner_window) was called, this NULL inner_window cause the crash.

So my rough judgement is: logically UpdateView should not be called in this case, so my patched code reveal a hidden bug, do you agree?

Whatevev, I will make further investigate to make it clear.

Comment 90

[XiaoXiaoHU]

Attachment: Backtrace for crashed test

Comment 91

[XiaoXiaoHU]

Oh, an correction to my previous comments, the inner_window is not NULL, but was already destroyed, but my judgement remain the same.

Comment 92

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

We're not crashing on central, so you could bisect between tip of central and where we branched 1.9.1 off central to see if there's a patch on central that fixed this crash. If there is, we might want it on 1.9.1. Maybe bug 451341?

Comment 93

[XiaoXiaoHU]

Yeah, lucky! It's exactly the same case as 451341, please also land the fix on 1.9.1, then my code could come back now :)

Comment 94

[XiaoXiaoHU]

Dear roc, since you have marked approval1.9.1+ for bug 451341, so whether it means someone who's monitoring this flag will check in the patches of 451341 and this 298889 soon? Should I do anything to request this check in, or just wait it to happend is enough?
I see firefox 3.5b4 is scheduled for mid of April, so I want to know whether this patch and the 451341 ones could be checked in before that. Thanks.

Comment 95

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

They will get checked in.

Comment 96

[XiaoXiaoHU]

Dear roc, since bug 451341 has been checked in for quite some time, could you also help to push this one to land before the code freezed (I see it's on April 15th), thanks a lot.

Comment 97

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

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2fbdb814c364

Comment 98

[chris hofmann]

can someone check Bug 530964 as possible fall out from this change?