iframe invisible when window is transparent

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Core
Layout: View Rendering
--
major
RESOLVED FIXED
13 years ago
8 years ago

People

(Reporter: cedricv, Assigned: XiaoXiaoHU)

Tracking

({fixed1.9.1})

Trunk
mozilla1.9.2a1
x86
Windows XP
fixed1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 -
blocking1.8.1.15 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(13 attachments, 3 obsolete attachments)

40.18 KB, image/png
Details
149 bytes, text/css
Details
3.91 KB, patch
Details | Diff | Splinter Review
7.93 KB, image/jpeg
Details
11.70 KB, patch
Details | Diff | Splinter Review
1.44 KB, patch
Details | Diff | Splinter Review
6.79 KB, patch
Details | Diff | Splinter Review
4.87 KB, patch
Details | Diff | Splinter Review
5.92 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.91 KB, patch
roc
: review+
Details | Diff | Splinter Review
6.27 KB, patch
Details | Diff | Splinter Review
202 bytes, text/plain
Details
26.97 KB, text/plain
Details
(Reporter)

Description

13 years ago
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!
(Reporter)

Comment 1

13 years ago
Created attachment 187389 [details]
screenshot
(Reporter)

Comment 2

13 years ago
Created attachment 187393 [details]
userChrome.css

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

Updated

13 years ago
Attachment #187393 - Attachment description: window transparent → userChrome.css
(Reporter)

Comment 3

13 years ago
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.

(Reporter)

Comment 4

13 years ago
Created attachment 188547 [details] [diff] [review]
first patch

oops sorry :/	 here it is
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.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 7

12 years ago
Bug confirmed in Songbird. See attachment. 

http://songbirdnest.com/

Comment 8

12 years ago
Created attachment 205460 [details]
See red circled area.  Corner should be transparent.
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

12 years ago
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
aaaah, that makes sense! thanks.
Created attachment 206057 [details] [diff] [review]
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

12 years ago
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.
(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

12 years ago
???

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

12 years ago
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.
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?
(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.).
Status: NEW → ASSIGNED
Ah, I just realized: The slowness and CPU hogging occur without roc's patch applied, as well. Separate bug?
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

12 years ago
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

12 years ago
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.
(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.
Dainis, do you feel like tracking down the problem you mentioned in comment #22?

Comment 26

12 years ago
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.
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

12 years ago
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.
Created attachment 206599 [details] [diff] [review]
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

12 years ago
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!
Created attachment 206651 [details] [diff] [review]
patch to fix GetAlphas error

Try this additional patch. It should fix the BlendingBuffer error.
Comment on attachment 206651 [details] [diff] [review]
patch to fix GetAlphas error

ooops, that contained unrelated junk
Attachment #206651 - Attachment is obsolete: true
Created attachment 206652 [details] [diff] [review]
patch to fix GetAlphas error

Just this one.

Comment 34

12 years ago
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
Created attachment 208652 [details] [diff] [review]
1-bit transparency autodetect v1.1

Oops, just realized that mAlphaMask doesn't always exist... This should fix some crashes. Thoughts on this?
Attachment #206599 - Attachment is obsolete: true
Attachment #208652 - Flags: review?(roc)

Comment 36

12 years ago
Do not introduce new member variable mIsOneBitTranslucent. It is equal to !IsAlphaTranslucencySupported().
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;
  }
(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.
(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

12 years ago
Created attachment 225380 [details] [diff] [review]
fix? (roc's attachment 206057 [details] [diff] [review] for 1_8_0 branch)

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

Comment 41

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

Comment 42

12 years ago
(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? 
Blocks: 357052

Comment 43

11 years ago
(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 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.
Attachment #208652 - Flags: review?(roc)

Comment 45

10 years ago
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.
Flags: blocking1.9?
Flags: blocking1.8.1.14?

Comment 46

10 years ago
(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.
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.
Flags: blocking1.9? → blocking1.9-

Comment 48

10 years ago
(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

10 years ago
(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
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.
Flags: blocking1.8.1.15? → blocking1.8.1.15-
(Assignee)

Comment 51

9 years ago
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.
(Assignee)

Comment 52

9 years ago
Created attachment 363480 [details] [diff] [review]
patch to fix invisible iframe/browser when background is transparent

See above comments for details.
(Assignee)

Updated

9 years ago
Attachment #363480 - Flags: review?(roc)
That's a pretty good patch! I wouldn't want to take it on a branch, but we could take it on trunk.
So, please give me the trunk version to review. Also, it would be good if you can use -p in your diffs. Thanks!
(Assignee)

Comment 55

9 years ago
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?
(Assignee)

Comment 56

9 years ago
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.
(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.
(Assignee)

Comment 58

9 years ago
Created attachment 363674 [details] [diff] [review]
Fix for the mozilla-central trunk
[Checkin: Comment 68]

The patch for the mozilla-central trunk version.
Attachment #363480 - Attachment is obsolete: true
Attachment #363674 - Flags: review?(roc)
Attachment #363480 - Flags: review?(roc)
(Assignee)

Comment 59

9 years ago
Created attachment 363675 [details] [diff] [review]
The patch for the mozilla-1.9.1 branch

The patch for the mozilla-1.9.1 branch.
Attachment #363675 - Flags: review?(roc)
(Assignee)

Comment 60

9 years ago
Created attachment 363676 [details] [diff] [review]
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

9 years ago
(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)
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.
(Assignee)

Comment 63

9 years ago
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.
(Assignee)

Comment 64

9 years ago
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.
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.
(Assignee)

Comment 66

9 years ago
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 on attachment 363674 [details] [diff] [review]
Fix for the mozilla-central trunk
[Checkin: Comment 68]

OK
Attachment #363674 - Flags: superreview+
Attachment #363674 - Flags: review?(roc)
Attachment #363674 - Flags: review+
Keywords: checkin-needed
Whiteboard: [needs landing]
Comment on attachment 363674 [details] [diff] [review]
Fix for the mozilla-central trunk
[Checkin: Comment 68]


http://hg.mozilla.org/mozilla-central/rev/9f8224db57f5
Attachment #363674 - Attachment description: Fix for the mozilla-central trunk → Fix for the mozilla-central trunk [Checkin: Comment 68]
Assignee: roc → gotmyname
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
QA Contact: ian → layout.view-rendering
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla1.9.2a1
(Assignee)

Comment 69

9 years ago
Robert, what about the 1.9.1 branch?
Attachment #363675 - Flags: review?(roc) → review+
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.
(Assignee)

Comment 71

9 years ago
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.
Attachment #363675 - Flags: approval1.9.1?
(Assignee)

Comment 72

9 years ago
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

9 years ago
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?
(Assignee)

Comment 74

9 years ago
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?
You mean approval?
(Assignee)

Comment 76

9 years ago
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.
Attachment #363675 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 363675 [details] [diff] [review]
The patch for the mozilla-1.9.1 branch

a1.9.1=dbaron
Keywords: checkin-needed
Whiteboard: [needs 191 landing]
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/34d1d4206402
Keywords: checkin-needed → fixed1.9.1
Whiteboard: [needs 191 landing]
(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.
Keywords: fixed1.9.1
(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!
}
(Assignee)

Comment 81

9 years ago
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.
(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
(Assignee)

Comment 83

9 years ago
I'm sorry, what's the crash means here, a real program crash or just return some test result not as expected?
(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.
Do full logs now have stack backtraces? I saw one the other day...

/be
(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.
(Assignee)

Comment 87

9 years ago
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.
(Assignee)

Comment 88

9 years ago
Created attachment 367461 [details]
My mozconfig for linux mochiteset test
(Assignee)

Comment 89

9 years ago
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.
(Assignee)

Comment 90

9 years ago
Created attachment 367463 [details]
Backtrace for crashed test
(Assignee)

Comment 91

9 years ago
Oh, an correction to my previous comments, the inner_window is not NULL, but was already destroyed, but my judgement remain the same.
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?
(Assignee)

Comment 93

9 years ago
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 :)

Updated

9 years ago
Depends on: 451341
(Assignee)

Comment 94

9 years ago
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.
They will get checked in.
(Assignee)

Comment 96

9 years ago
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.
Whiteboard: [needs 191 landing]
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2fbdb814c364
Keywords: fixed1.9.1
Whiteboard: [needs 191 landing]

Updated

8 years ago
Depends on: 514788

Comment 98

8 years ago
can someone check Bug 530964 as possible fall out from this change?
You need to log in before you can comment on or make changes to this bug.