130078 - integrate iframe into chrome view hierarchy (link view managers / trees between chrome and content)

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P1)

Description

[Mike Lee]

Follow on from bug 91516 quote:

"IFRAME or BROWSER children of chrome do not get integrated into the view
hierarchy so they won't interact properly with siblings in terms of z-index and
overlap. This was a deliberate decision because I didn't want to change the
behavior of the main browser window (too risky). We can change this policy after
Mozilla 1.0 ships."

Thanks!

Comment 1

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

This is trivial to correct, it just needs testing to make sure it doesn't cause
regressions.

Comment 2

[Chris Petersen]

Changing Priority to P4

Comment 3

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

Moving this out...

Comment 4

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

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

Comment 5

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

Attachment: Untested patch

This patch might work :-)

Comment 6

[Wang Xianzhu]

I tried the pactch.  It works well.  Thanks.

I reported bug 216835 similar to 204278 yesterday.  It seems duplicate of bug
204278, if they are of the same cause.
 

Comment 7

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

OK. We'll see about getting this into 1.6. It's "the right thing to do" but
there may be some performance impact.

Comment 8

[Wang Xianzhu]

Will 1.6a fix bug 204278?  I'm looking forward to it.:)

Comment 9

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

Wang, please check if this patch fixes bug 216835 and bug 204278. If it does,
mark those bugs duplicates of this bug. Thanks!!

Comment 10

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

Note: this patch breaks the way that PaintBackground sets the canvas colour to
the user's preference color. With this patch, PaintBackground doesn't detect the
"root document canvas" correctly, because really the only root documents are the
ones that have their own windows or are embedded in a non-Mozilla context.

With this patch, an HTML document which does not set the background color is
simply transparent and the background of the <browser> XUL element shows
through. This is actually very good. The right way to set the canvas background
would be to simply set the background color of the <browser> element. I'll have
to add that.

Comment 11

[Wang Xianzhu]

This patch seems no effect on bug 204278 and bug 216835.

Comment 12

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

There's a good bit of information in bug 227361 about this, since I tried to do
it (see bug 227361 comment 40 and bug 227361 comment 43).

Fixing this is likely to make it easier to fix bug 83858 and bug 301408
consistently across platforms (especially Mac, and GTK2 for the former).

Comment 13

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

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

Comment 14

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

Attachment: fix

This patch is somewhat tested. Things seem to work fine. There may be
performance issues but they aren't visible on this machine.

The default background color code in nsCSSRendering is still there so Gecko
embedders will still get the default background painted on their content
shells. But Gecko no longer paints the default background on root chrome shells
or any of their descendants: in the former case, we make the widget transparent
instead, and in the latter case, the content shells are transparent. This patch
preserves the functionality of the default background preference by setting the
background color of the <browser> binding to the default background. I've done
things this way so that XUL authors have the flexibility to make transparent
content shells inside chrome.

Comment 15

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

This is for 1.9 of course.

CCing mconnor because of the toolkit change.

Comment 16

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

Comment on attachment 191565 [details] [diff] [review]
fix

r+sr=dbaron, but please use the name nsIPrefBranch2 instead of
nsIPrefBranchInternal (see the headers for why).

Comment 17

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

checked in.

Comment 18

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

This caused performance regressions, so I backed it out. 40ms on btek, 90ms on
luna, 10ms on beast. Also, a 30ms Tdhtml regression on luna and 10ms Txul on
comet (80ms on luna).

Comment 19

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

My current thinking is that I might just go for full view manager removal and do
this as part of that.

Comment 20

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

That seems risky.  If you can't make this work without a performance regression,
why do you think you'll be able to do the view manager removal without one?

Comment 21

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

There should be a win from not having to maintain the view tree or bounce
between frames and views that could compensate for this.

I guess it would be better to somehow eliminate the overhead here. OK.

Comment 22

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

Attachment: updated to trunk

Comment 23

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

Attachment: alternative patch

The orginal patch painted the default background color by making it the background color on the XUL tabpanel. This is elegant in some ways, because the default background color for content documents is entirely a feature of the browser chrome, but this meant that the background of the content window is often transparent, which forces us to traverse frames in the browser chrome every time we paint, and possibly interferes with scrolling optimization.

This patch takes a different approach, hewing closer to the existing code. We have a new view flag to control whether the default background color should be painted. We set this flag for the root views whose view managers would have been root view managers in the old system. We use that flag to control whether the view paints the default background color or not. I hope that this will improve performance.

One problem is that trunk and branch have just diverged due to my landing of the frame display list patch. This patch here will apply to both trunk and branch, but it's more likely to be effective on branch. On trunk it may not be effective because we'll have to traverse XUL frames to build the display list. We could land it anyway, but it would be good to know what it does to Tp on branch and I'm not sure we have sensitive enough branch tinderboxes.

Comment 24

[Martijn Wargers (dead)]

Robert, I'm a bit confused, it seems to me that this bug is already fixed by your frame display lists patch, not?
See bug 204278 and bug 303813 for example, which are definetely fixed now.

Comment 25

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

This may be fixed for some <browser> elements, but I don't believe it's fixed for "content" <browser>s like you'd find in Firefox tabs.

Comment 26

[Pete Collins (MDG)]

> One problem is that trunk and branch have just diverged due to my landing of
> the frame display list patch. This patch here will apply to both trunk and
> branch, but it's more likely to be effective on branch. On trunk it may not be
> effective because we'll have to traverse XUL frames to build the display list.
> We could land it anyway, but it would be good to know what it does to Tp on
> branch and I'm not sure we have sensitive enough branch tinderboxes.

I tested on the trunk and this patch works exactly as expected.  We can really use this checked into the trunk and branch.

Comment 27

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

I need to do a test-landing of this patch and see what the damage is.

Comment 28

[Pete Collins (MDG)]

I've been running w/ it for a while on a trunk build and things appear to be solid.

Comment 29

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

It's all about performance really.

Comment 30

[Ben Goodger (use ben at mozilla dot org for email)]

Does fixing this bug mean that you should be able to overlay the content of an iframe like so? 

<stack>
  <iframe src="http://www.homestarrunner.com/sbemail58.html"/>
  <label>Burninate!</label>
</stack>

Comment 31

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

Yes.

Comment 32

[Pete Collins (MDG)]

> Does fixing this bug mean that you should be able to overlay the content of an
> iframe like so? 

And it works quite wonderfully ... 

Comment 33

[Pete Collins (MDG)]

> And it works quite wonderfully ... 
 
Well for the most part. I do see some problems arise from time to time. For example dynamically resizing XUL controls causes problems that don't appear on unpatched builds.  

Comment 34

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

(In reply to comment #33)
> > And it works quite wonderfully ... 
> 
> Well for the most part. I do see some problems arise from time to time. For
> example dynamically resizing XUL controls causes problems that don't appear on
> unpatched builds.  

What sort of problems?

Comment 35

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

Attachment: updated patch

This patch is updated to fix a problem where the display item for a document's default background wasn't being recognized as opaque, so we were unnecessarily painting the chrome tabpanel's background.

It also only paints a default background on content subdocuments of chrome documents, instead of all subdocuments of chrome documents.

I would like to land this on trunk to see what the impact is.

Comment 36

[Pete Collins (MDG)]

> 
> I would like to land this on trunk to see what the impact is.


I'm going to apply the new patch and I'll report any specific problems I find.

Comment 37

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

Comment on attachment 220348 [details] [diff] [review]
updated patch

This seems fine except for this function:

> PRBool
> nsDisplayBackground::IsOpaque(nsDisplayListBuilder* aBuilder) {
>   // theme background overrides any other background
>   if (mFrame->IsThemed())
>     return PR_FALSE;
> 
>   PRBool isCanvas;
>   const nsStyleBackground* bg;
>+  nsPresContext* presContext = mFrame->GetPresContext();
>   PRBool hasBG =
>-    nsCSSRendering::FindBackground(mFrame->GetPresContext(), mFrame, &bg, &isCanvas);
>-  if (!hasBG || (bg->mBackgroundFlags & NS_STYLE_BG_COLOR_TRANSPARENT) ||
>-      bg->mBackgroundClip != NS_STYLE_BG_CLIP_BORDER ||
>-      HasNonZeroBorderRadius(mFrame->GetStyleBorder()) ||
>-      NS_GET_A(bg->mBackgroundColor) < 255)
>+    nsCSSRendering::FindBackground(presContext, mFrame, &bg, &isCanvas);
>+  if (!hasBG)
>     return PR_FALSE;
>-  return PR_TRUE;
>+  PRBool isTranslucentColor;
>+  if (isCanvas) {
>+    nsIView* rootView;
>+    presContext->GetViewManager()->GetRootView(rootView);
>+    if (rootView->IsUsingDefaultBackgroundColor()) {
>+      isTranslucentColor = NS_GET_A(presContext->DefaultBackgroundColor()) < 255;
>+    }
>+  } else {
>+    isTranslucentColor = NS_GET_A(bg->mBackgroundColor) < 255 ||
>+      (bg->mBackgroundFlags & NS_STYLE_BG_COLOR_TRANSPARENT);
>+  }
>+  return !isTranslucentColor &&
>+    bg->mBackgroundClip == NS_STYLE_BG_CLIP_BORDER &&
>+    !HasNonZeroBorderRadius(mFrame->GetStyleBorder());
> }

For a start, you're leaving isTranslucentColor uninitialized in one of the branches here.  But it also seems odd that we're checking background color and border-radius for the canvas default background color case -- if it's painted as a backstop by the view manager (or is it?), do those have any effect?  Maybe this should be rewritten to use an early return inside the nested if, and then no else?

Comment 38

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

(In reply to comment #37)
> For a start, you're leaving isTranslucentColor uninitialized in one of the
> branches here.

Indeed, I should just set it to "NS_GET_A(bg->mBackgroundColor) < 255 ||
(bg->mBackgroundFlags & NS_STYLE_BG_COLOR_TRANSPARENT)" at the top and scratch the "else" clause.

> But it also seems odd that we're checking background color and
> border-radius for the canvas default background color case -- if it's painted
> as a backstop by the view manager (or is it?), do those have any effect?  

This is accounting for the backstop color change by nsCSSRendering::PaintBackground:
http://lxr.mozilla.org/mozilla/source/layout/base/nsCSSRendering.cpp#2791
which uses presContext->DefaultBackgroundColor(). The bg->mBackgroundClip and border-radius would be honoured by PaintBackgroundWithSC, if they were set, which they only would be in pathological situations of course.

Comment 39

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

OK, I'll trust you on that, so r+sr=dbaron with the change to remove the else clause.

Comment 40

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

Attachment: updated patch

This is the patch as checked in.

Comment 41

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

Here are some Tp numbers:
argo: ~365 to ~410
luna: ~900 to ~933
btek: ~950 to ~965

luna Tdhtml: ~1565 to ~1580

Nothing else seems to have changed much.

Here are raw Tp numbers from argo:
BEFORE:
Avg. Median : 364 msec
  Average     : 381 msec
  Minimum     : 146 msec
  Maximum     : 2352 msec
  IDX PATH                           AVG    MED    MAX    MIN  TIMES ...
    0 home.netscape.com         	   323    303    401    293    401    321    300    303    293 
    1 my.netscape.com           	   324    318    368    306    368    310    306    320    318 
    2 www.aol.com               	   570    359   1419    346    380    359   1419    349    346 
    3 www.mapquest.com          	   213    211    231    208    211    231    208    208    211 
    4 www.moviefone.com         	   204    203    224    190    224    190    201    203    203 
    5 www.digitalcity.com       	   236    237    244    231    244    234    231    237    237 
    6 www.iplanet.com           	   270    229    437    223    233    229    437    229    223 
    7 web.icq.com               	   441    419    533    400    533    400    418    436    419 
    8 www.compuserve.com        	   233    219    266    219    266    245    219    219    219 
    9 www.msnbc.com             	   185    177    219    172    219    185    175    172    177 
   10 www.yahoo.com             	   196    202    205    175    203    199    202    175    205 
   11 bugzilla.mozilla.org      	   309    300    332    300    300    300    300    315    332 
   12 www.msn.com               	   313    308    336    302    336    302    304    318    308 
   13 slashdot.org              	   326    331    333    313    331    313    331    325    333 
   14 www.nytimes.com           	   381    383    411    356    411    386    383    356    370 
   15 www.nytimes.com_Table     	   571    566    600    553    600    553    566    566    570 
   16 www.w3.org_DOML2Core      	  1127   1105   1186   1088   1088   1096   1105   1186   1161 
   17 lxr.mozilla.org           	   735    734    754    720    731    737    754    720    734 
   18 espn.go.com               	   546    533    596    521    596    533    531    552    521 
   19 www.voodooextreme.com     	  1636   1459   2352   1444   2352   1480   1459   1444   1447 
   20 www.wired.com             	   490    472    530    458    530    458    470    472    522 
   21 hotwired.lycos.com        	   289    288    300    279    284    288    279    295    300 
   22 www.ebay.com              	   224    225    245    203    225    245    203    221    226 
   23 www.apple.com             	   159    161    168    146    168    160    163    161    146 
   24 www.amazon.com            	   266    260    294    253    294    260    253    264    259 
   25 www.altavista.com         	   210    209    224    200    224    209    200    209    212 
   26 www.zdnet.com_Gamespot.com	   404    349    623    348    623    349    348    348    355 
   27 www.spinner.com           	   327    349    367    251    314    251    357    349    367 
   28 www.microsoft.com         	   208    205    232    192    205    192    216    196    232 
   29 www.time.com              	   285    267    358    265    358    265    266    273    267 
   30 www.travelocity.com       	   289    286    320    274    320    274    279    289    286 
   31 www.expedia.com           	   326    311    389    307    389    311    307    310    317 
   32 www.quicken.com           	   264    266    283    224    266    266    281    224    283 
   33 www.zdnet.com             	   469    457    528    443    528    457    443    466    453 
   34 www.excite.com            	   281    277    315    265    315    280    265    277    271 
   35 www.google.com            	   155    151    164    149    164    151    149    151    161 
   36 www.tomshardware.com      	   437    425    482    415    482    424    425    415    442 
   37 www.cnn.com               	   556    539    637    522    637    522    528    556    539 
   38 news.cnet.com             	   297    303    317    278    317    303    278    304    284 
   39 www.sun.com               	   186    184    196    179    184    184    179    196    187 

AFTER:
  Starting Page Load Test
  Test id: 4459842D4D
  Avg. Median : 408 msec
  Average     : 426 msec
  Minimum     : 159 msec
  Maximum     : 2628 msec
  IDX PATH                           AVG    MED    MAX    MIN  TIMES ...
    0 home.netscape.com         	   367    304    642    286    642    286    299    304    305 
    1 my.netscape.com           	   302    307    314    289    289    307    291    312    314 
    2 www.aol.com               	   489    496    506    469    474    500    496    469    506 
    3 www.mapquest.com          	   207    197    253    191    253    197    195    191    200 
    4 www.moviefone.com         	   281    222    392    194    392    194    207    222    392 
    5 www.digitalcity.com       	   236    235    246    229    229    234    246    240    235 
    6 www.iplanet.com           	   287    289    303    269    269    295    303    289    283 
    7 web.icq.com               	   453    424    571    421    571    424    431    421    422 
    8 www.compuserve.com        	   250    229    344    219    344    229    229    219    229 
    9 www.msnbc.com             	   216    214    231    207    231    217    214    212    207 
   10 www.yahoo.com             	   205    202    223    188    195    202    188    223    218 
   11 bugzilla.mozilla.org      	   423    422    452    403    452    422    426    403    415 
   12 www.msn.com               	   329    319    373    312    373    312    319    326    317 
   13 slashdot.org              	   348    344    372    337    372    342    344    345    337 
   14 www.nytimes.com           	   406    393    476    375    476    400    386    393    375 
   15 www.nytimes.com_Table     	   595    595    638    557    638    595    557    606    583 
   16 www.w3.org_DOML2Core      	  1202   1186   1280   1156   1186   1280   1176   1215   1156 
   17 lxr.mozilla.org           	   735    725    764    716    725    748    722    764    716 
   18 espn.go.com               	   696    689    795    631    795    689    631    709    659 
   19 www.voodooextreme.com     	  1756   1525   2628   1499   2628   1611   1518   1499   1525 
   20 www.wired.com             	   523    514    574    500    574    500    503    525    514 
   21 hotwired.lycos.com        	   352    332    474    289    342    332    474    323    289 
   22 www.ebay.com              	   268    249    326    228    294    249    326    245    228 
   23 www.apple.com             	   187    178    225    172    225    189    172    178    174 
   24 www.amazon.com            	   266    259    292    251    292    251    259    258    271 
   25 www.altavista.com         	   223    215    244    211    237    212    244    215    211 
   26 www.zdnet.com_Gamespot.com	   419    375    603    360    603    360    375    362    399 
   27 www.spinner.com           	   548    556    588    512    512    556    588    561    527 
   28 www.microsoft.com         	   225    237    242    196    242    196    242    237    209 
   29 www.time.com              	   386    380    472    316    380    434    332    472    316 
   30 www.travelocity.com       	   303    271    400    269    400    305    269    271    271 
   31 www.expedia.com           	   327    302    429    286    429    302    286    325    297 
   32 www.quicken.com           	   268    253    337    246    337    248    257    253    246 
   33 www.zdnet.com             	   742    728    807    711    807    711    743    723    728 
   34 www.excite.com            	   298    299    338    264    338    264    299    300    289 
   35 www.google.com            	   168    169    180    159    169    161    159    174    180 
   36 www.tomshardware.com      	   558    515    735    510    735    515    515    518    510 
   37 www.cnn.com               	   627    609    714    579    714    603    579    609    632 
   38 news.cnet.com             	   331    322    364    299    364    299    313    357    322 
   39 www.sun.com               	   250    250    268    235    268    256    250    245    235 

Comment 42

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

I backed out, BTW.

Planetoid Tp had gone up from ~367 to ~402.

On argo, the slowdown seems to affect some sites and not others, which is very interesting. The following sites show a consistent, significant slowdown:

www.aol.com
www.iplanet.com
www.msnbc.com
bugzilla.mozilla.org
espn.go.com
www.spinner.com
www.time.com
www.zdnet.com
www.tomshardware.com
www.sun.com

Comment 43

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

On planetoid the most consistently affected sites could be

www.aol.com
www.moviefone.com
web.icq.com
www.compuserve.com
bugzilla.mozilla.org
espn.go.com
www.wired.com
hotwired.lycos.com
www.time.com
www.zdnet.com
www.tomshardware.com

Comment 44

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

I profiled the aol.com content (to be precise, the result of doing "Save As" on axolotl's content) on my laptop, using a cairo build like argo does, and the differences between results before and after the patch are in the noise :-(.

Maybe I should try to get argo configured to run jprof during its tests...

Comment 45

[daniel newton]

Attachment: Updated patch to trunk

I wanted to use this patch on the latest trunk and had to update a couple of lines  so I figured I may as well post it here.

Comment 46

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

I tried landing this again. Let's see how it goes.

Comment 47

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

This did cause a performance regression on bl-bldlnx01. I have a patch that will let us dump display lists into the Tinderbox log, which may help diagnose the problem. I'll check it in temporarily when that machine comes back online.

Comment 48

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

Vlad suggests that the Tp2 regression was in the noise and the Tdhtml regression, while probably real, is also tiny, so we should leave this landed. So, that's what I'll do.

Comment 49

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

(We do have to traverse more frames and build a slightly larger display list, that was always clear, so a tiny performance regression was actually inevitable)

Comment 50

[daniel newton]

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

Comment 51

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

I think we'll want to back this out for 1.9alpha1. It's interacting with view update batching and that stuff is fragile. I'll back it out tomorrow.

Comment 52

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

Backed out

Comment 53

[Devon Jensen]

One thing I noticed while briefly testing a nightly with this patch in it - If I stacked an xul button on top of the firefox browser it would show up great, but when I would scroll the webpage quickly, I would often see a 'ghost' button at a different spot (basically one 'scroll height' away).  

I don't know if this is possible to fix

Can we count on this patch being in firefox 3.0?  I'd like to use this for one of my extensions

Comment 54

[Michael Ventnor]

(In reply to comment #53)
> Can we count on this patch being in firefox 3.0?  I'd like to use this for one
> of my extensions
> 

According to the Blocking1.9+ flag I assume so. But I'd like to know whether this bug is still being worked on. After looking at some dependency trees, this bug is blocking quite a few important bugs.
Should the "helpwanted" keyword be set?

Comment 55

[Gábor Stefanik]

Mozilla 1.6 alpha is long gone, removing tarhet milestone. Also, is this bug abandoned? The latest patch I can see is from 2006-10-02. I second the need for "helpwanted".

Comment 56

[Michael Ventnor]

Why is this a P5? On http://wiki.mozilla.org/Firefox3/Gecko_Feature_List it is listed as a P1.

Comment 57

[Bill Gianopoulos [:WG9s]]

(In reply to comment #56)
> Why is this a P5? On http://wiki.mozilla.org/Firefox3/Gecko_Feature_List it is
> listed as a P1.
> 

Unless you have something useful to contribute.  Please stop spamming this bug.

Comment 58

[Gábor Stefanik]

Just a quick note: The latest patch can still be applied on the recent trunk, however the issues mentioned in this bug still need resolving.

Comment 59

[Gábor Stefanik]

Supposedly the "ghost button" issue will go away once view removal is complete. It seems to be a variant of bug 324819.

Comment 60

[Daniel Kirsch]

I see the problem on the latest XULRunner 1.9a4. When using <xul:iframe type="content"> it works. When using no type attribute an iframe is always rendered on top.

Comment 61

[Daniel Kirsch]

Attachment: Testcase for iframe with and without type=content attribute

Comment 62

[Daniel Kirsch]

Attachment: Screenshot of the iframe testcase

Comment 63

[Daniel Kirsch]

After bug 361600 is fixed the problem described in comment 60 is still visible.
Tested with Mozilla/5.0 (Windows; U; Windows NT 5.1; de-DE; rv:1.9a4pre) Gecko/20070412

Comment 64

[Steffen Wilberg]

The testcase from comment 61 looks fixed here. Contrary to the screenshot from comment 62, the lower iframe displays the red border and the text as well.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a6pre) Gecko/20070628 Firefox/3.0a6pre

But the safe browsing notification is still hidden by the content area (bug 341950). I guess we could use another testcase here.

Comment 65

[Daniel Kirsch]

The testcase always worked when displayed in the browser, as the browser is allready a content window. Testing the testcase from a chrome environment with the latest XULRunner nightly still does NOT work.

Comment 66

[Daniel Kirsch]

Tested and reproducable with XULRunner: Mozilla/5.0 (Windows; U; Windows NT 5.1; de-DE; rv:1.9a6pre) Gecko/20070627

Comment 67

[Neil Deakin]

roc, can we at least still hook up chrome->chrome hierarchies for 1.9? This is needed for bug 395670 to fix some popup bugs, and handle frames inside popups, such as the customize dialog on Mac.

Comment 68

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

chrome-to-chrome should already be hooked up

Comment 69

[Neil Deakin]

(In reply to comment #68)
> chrome-to-chrome should already be hooked up
> 

roc, OK maybe I misunderstood what you described recently. I want to make sure that iframes in popups get events targeted to them. Disabling the 'don't hook up chrome parents' at http://lxr.mozilla.org/mozilla/source/layout/base/nsDocumentViewer.cpp#2212 causes that work, otherwise mouse events just get ignored.

Comment 70

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

ah ok, I was wrong, we're not hooking up any children of chrome shells. We probably should hook up chrome children of any shell, and that's easy, so do you want to sling me a patch for that?

Comment 71

[Neil Deakin]

(In reply to comment #70)
> ah ok, I was wrong, we're not hooking up any children of chrome shells. We
> probably should hook up chrome children of any shell, and that's easy, so do
> you want to sling me a patch for that?
> 

That would be great.

Comment 72

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

Attachment: hook up view hierarchies for chrome children of chrome

Simple patch. Fairly conservative as it leaves content children of chrome unhooked, which is sad but the way we'll have to ship 1.9, I think. It will help people messing with chrome child frames.

Boris, I didn't know what to do about typeChromeWrapper and typeContentWrapper. They will generally lead to things being hooked up. Let me know if that's wrong; I'm not sure what they mean.

Comment 73

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

Attachment: Revised patch using GetSameTypeParent as suggested by bz

Comment 74

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 285029 [details] [diff] [review]
Revised patch using GetSameTypeParent as suggested by bz

r+sr=bzbarsky

Comment 75

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

Comment on attachment 285029 [details] [diff] [review]
Revised patch using GetSameTypeParent as suggested by bz

I'm going to request approval because this patch does not address the bulk of this bug, and should therefore be evaluated on its own merits.

Neil, it would be useful if you could explain exactly how this will benefit.

Comment 76

[Neil Deakin]

There are cases where child frames are used in popups. The toolbar customization dialog uses a child frame. Also, the places tag editor wants an <editor> inside a popup. The mouse events don't get targeted properly unless the chrome views are connected.

Comment 77

[Damon Sicore (:damons)]

Comment on attachment 285029 [details] [diff] [review]
Revised patch using GetSameTypeParent as suggested by bz

a-'ing for M9.  We will make another pass through the -'s right after we unfreeze and process them again.  So, if you leave it a minus, we'll get to it ASAP.

Comment 78

[Damon Sicore (:damons)]

Comment on attachment 285029 [details] [diff] [review]
Revised patch using GetSameTypeParent as suggested by bz

OK.  Sorry for all the status changes here, but we're still working out the best way to process approval requests for specific milestones.

So, on that note:  

Setting approval1.9+, targeting for M10, and you are clear to land once the tree is open for M10 (i.e., after tree opens, after beta).

Comment 79

[Damon Sicore (:damons)]

Targeting M10.

Comment 80

[Mike Schroepfer]

So - why do we need this at this point in the 1.9 cycle?  

Comment 81

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

See comment #76. If you need more details, Neil will have to provide them.

Comment 82

[Mike Schroepfer]

(In reply to comment #81)
> See comment #76. If you need more details, Neil will have to provide them.
> 

Right - I see that this bug does some good - but is there a driver to get this fixed in 1.9 - e.g. some new way in which this problem is exposed?   Neil?

Comment 83

[Reed Loden [:reed]]

Checking in layout/base/nsDocumentViewer.cpp;
/cvsroot/mozilla/layout/base/nsDocumentViewer.cpp,v  <--  nsDocumentViewer.cpp
new revision: 1.552; previous revision: 1.551
done

Comment 84

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

Sorry, this bug isn't fully fixed and won't be in 1.9. It needs the compositor work.

Comment 85

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

I can't think of a straightforward way to write a test for the patch that was checked in. We'd have to make a mochitest that runs as a chrome-type docshell and I don't know how to do that.

Comment 86

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

There's nothing left here that blocks 1.9.

Comment 89

[Chris Pearce [:cpearce (Not reading bugmail)]]

We want to get this bug off the "bugs which are have patch approved for FF3 but bug is still open" list, so I'm resolving-incomplete this bug and I will reopen another for followup work.

Comment 90

[Chris Pearce [:cpearce (Not reading bugmail)]]

I have created bug 433048 for followup work.

Comment 91

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

This bug should remain open.

Comment 92

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Any chance of seeing this fixed in 1.9.2? It may block work on Taskfox.

Comment 93

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

Depends on how fast I make progress on compositor pieces and what the 1.9.2 schedule is.

Comment 94

[Myk Melez [:myk] [@mykmelez]]

The latest work this bug blocks is the implementation of panels in Jetpack (bug 494238), which is a high priority feature for Jetpack.

Ideally, a version of Jetpack that supports panels would be available in conjunction with the release of Firefox 3.6, and absent a fix for this bug, we'll explore every possible hack that can get us somewhat there.

But regardless of how we hack it for Firefox 3.6, we need a robust, functional implementation of panels in Jetpack by the time Firefox 3.7 rolls around, so I'm requesting that this bug be considered a Gecko 1.9.3 blocker and prioritized accordingly.

Comment 95

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

Roc: I'm tentatively marking this as blocking 1.9.3 to get it on your radar. It's needed for JetPack (see comment 94) capability which is a priority for Firefox 3.7.

Comment 96

[David Dahl :ddahl]

Just tried to apply this patch on the current trunk (as an excersize for jetpack) - it did not apply cleanly (or at all). The current source looks like it has implemented parts of this patch but does nto return the same object. Not knowing ANYTHING about layout code, this is as far as I can go.

Comment 97

[Myk Melez [:myk] [@mykmelez]]

roc: can you recommend someone with the time and expertise to implement your suggestion in bug 494238, comment 4?

Comment 98

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

Mats could do it.

Comment 99

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

Maybe that should go in a different bug and this bug should continue to be for the general fix?

Comment 100

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #99)
> Maybe that should go in a different bug and this bug should continue to be for
> the general fix?

Sure.  I filed bug 532569 on this.

Comment 101

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

I don't think *this* bug blocks 1.9.3 now that bug 532569 has been separated, so setting the blocking nomination back to "?" (suggesting that it go to "-").

Comment 102

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

Attachment: Part 1: hookup view managers unconditionally

This is on top of the patch in bug 532569. It hooks up the view manager trees unconditionally.

Comment 103

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

Attachment: Part 2: remove widgets from all subframes

Comment 104

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

These patches work surprisingly well. Part 1 breaks plugins (because plugins whose nearest enclosing widget doesn't correspond to the root of a view manager tree are hidden), but part 2 fixes them. The biggest obvious issue is that zooming is completely broken (as expected).

What remains to be done:
* fix zooming
* make sure that chrome elements that are over windowed plugins obscure those plugins instead of being obscured by them
* remove nsViewManager::EnableRefresh, which is probably going to cause subtle problems here
* carefully measure the increase in display list construction overhead for small paints in the content window, and if it's significant, figure out how to ameliorate it
* test accessibility carefully
* run tryserver tests and get them all to pass
* ?

Comment 105

[Marco Zehe (:MarcoZ)]

What we'd need are tree tests for iframes I think. Alex, would you agree?

Comment 106

[alexander :surkov (:asurkov)]

(In reply to comment #105)
> What we'd need are tree tests for iframes I think. Alex, would you agree?

yes, iframe, browser and etc, anything related with nsDocAccessible and nsOuterDocAccessible.

Comment 107

[Marco Zehe (:MarcoZ)]

We do have tests for iframe (test_iframe.html in the "trees" folder), and also we do have test_tabbrowser.xul. Anything else this code touches?

Comment 108

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

The main thing I'm worried about is that AT tools might be distinguishing "chrome" native windows (HWNDs) from "content" native windows. With these patches, there's basically only one native window per toplevel window, which will look like a chrome window.

Comment 109

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

In particular, we currently have code on Windows that sets the HWND class to MozillaUIWindowClass, MozillaContentWindowClass, or MozillaWindowClass. After these patches, that code becomes irrelevant; there is generally only one window, whose class will be MozillaUIWindowClass.

Comment 110

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

Tryserver builds with those patches will show up here:
https://build.mozilla.org/tryserver-builds/[e-mail]-try-673a79f73633/

Comment 111

[alexander :surkov (:asurkov)]

(In reply to comment #108)
> The main thing I'm worried about is that AT tools might be distinguishing
> "chrome" native windows (HWNDs) from "content" native windows. With these
> patches, there's basically only one native window per toplevel window, which
> will look like a chrome window.

I think we should be fine once we got fit only top level windows have HWND. Marco, could ensure please if it doesn't affect on AT?

Comment 112

[Marco Zehe (:MarcoZ)]

I hope all screen readers have switched to using MSAA/IA2 to gain access to our windows. Otherwise, if they still use the algorithm of searching for MozillaContentWindowClass and then down to get to the acc tree, we might be in big trouble. I'll give all our supported ATs a spin with the try server builds.

Comment 113

[Marco Zehe (:MarcoZ)]

The Windows try server build crashes upon launch when NVDA tries to access it. I don't get Crash Reporter since try is built without symbols, but the crash is 100% reproducible. Have NVDA running and launch the build, and it will crash.

Comment 114

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

The try server builds do have symbols available via the symbol server.

Comment 115

[Marco Zehe (:MarcoZ)]

In addition, using JAWS, I get the same reproducible crash. So it's not dependent on NVDA alone.

Comment 116

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: crash stack

(In reply to comment #114)
> The try server builds do have symbols available via the symbol server.

Sorry for the delay. I tried this today and it crashes for me on startup (windows 7) with no screen reader. Stack attached. Ping me anytime to try a new build or get more debugging info. BTW the patches don't apply cleanly on trunk for me.

Comment 117

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

Rob, looks like tab previews need to be retooled for the post-child-widgets world...

Comment 118

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #115)
> In addition, using JAWS, I get the same reproducible crash. So it's not
> dependent on NVDA alone.

Marco, can you file a dependency a11y bug for this one?

Comment 119

[Daniel Kirsch]

Attachment: Testcase for iframe overflowing when scrolled

I cannot reproduce the wrong behaviour from comment 61 anymore with the latest trunk. However I see an iframe with type="content" still on top of other elements when a surrounding container gets scrolled.

The new testcase should show this when loaded within a chrome environment. Simply scroll down the lower area. The white content iframe is still visible on top of the above red container while the non content iframe (and the labels) gets hidden as expected.

Comment 120

[Daniel Kirsch]

Attachment: Screenshot of testcase iframe overflowing when scrolled

Comment 121

[Damon Sicore (:damons)]

After discussing this with Johnath and dbaron, nothing for beta 1 depends on this on the Firefox side.  Bumping to beta 2.

Comment 122

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

Attachment: Part 1 refreshed

Updated to trunk.

Comment 123

[Oleg Romashin (:romaxa)]

Playing with last 2 patches, and have this output in recursive loop
###!!! ASSERTION: Root view had a parent, but it has the same view manager: 'mRootViewManager != this', file view/src/nsViewManager.cpp, line 182

Comment 124

[Timothy Nikkel (:tnikkel)]

I've not seen that. Do you have steps to reproduce or a stack of the assertion?

Comment 125

[Oleg Romashin (:romaxa)]

Attachment: Backtrace with accertion log

testing on m-c 46301:911af4c07fd4, + jimm changes enabled for linux build:
DocumentViewerImpl::MakeWindow
Also applied part1/2 patches from this patch.

Comment 126

[Oleg Romashin (:romaxa)]

Attachment: all my changes against 46301:911af4c07fd4 m-c

Comment 127

[Timothy Nikkel (:tnikkel)]

Comment on attachment 454426 [details] [diff] [review]
all my changes against 46301:911af4c07fd4 m-c

Marking this obsolete since it's not really part of this bug.

Comment 128

[Jim Mathies [:jimm]]

Attachment: patch post 513162

No need to check for a container view in the parent widget anymore.

Comment 129

[Oleg Romashin (:romaxa)]

> patch post 513162
is this patch replacing  "Part 1 refreshed" or additional? changes seems overlapping each other

Comment 130

[Justin Dolske [:Dolske]]

Is this still planned to land for beta 2 (which freezes Monday)?

Is the tiny patch from comment 128 all that's needed now, or are some of the other patches here (and in dependent bugs) still needed?

Comment 131

[Timothy Nikkel (:tnikkel)]

This will not make beta 2. All the dependent bugs are still needed, plus some more that aren't filed yet.

Comment 132

[u88484]

(In reply to comment #131)
> This will not make beta 2. All the dependent bugs are still needed, plus some
> more that aren't filed yet.

I tried the tryserver build listed at http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-5385f18dfbc5/tryserver-win32/ and the fonts on the ftp pages look horrible as well as the colors.  See screenshot: http://img841.imageshack.us/img841/8617/82319408.png

Left side is tryserver build and right side is today's nightly build with the same profile for each.

Comment 133

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

Do we expect this to hit code freeze on Monday, Aug 2, 23:00 PT? From the number of dependencies I suspect not, but it was marked a b3 blocker 10 days ago.

Comment 134

[Timothy Nikkel (:tnikkel)]

Unfortunately this will not make beta 3 code freeze.

Comment 135

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

Moving to betaN for now, but roc/dbaron, we need to figure out how and when we'll contain this fix.

Comment 136

[Timothy Nikkel (:tnikkel)]

Attachment: part 1.1 - update part 1 to deal with bug 513162

This is jimm's change applied on top of part 1 so that we can separate it out and get review.

Comment 137

[Timothy Nikkel (:tnikkel)]

Comment on attachment 453999 [details] [diff] [review]
Part 1 refreshed

I guess these never got reviewed.

Comment 138

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

Comment on attachment 425946 [details] [diff] [review]
Part 2: remove widgets from all subframes

r=dbaron

Comment 139

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

Comment on attachment 453999 [details] [diff] [review]
Part 1 refreshed

r=dbaron

Comment 140

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Note, both fennec and desktop would really like this, but right now this breaks fennec due to 577579.  That bug should probably be fixed at the same time this lands.

Comment 141

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

part 1 rotted again, looks like

Comment 142

[Timothy Nikkel (:tnikkel)]

Attachment: Part 1 refreshed

Comment 143

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

roc: I think that this needs to block b5; agree?

Comment 144

[Aza Raskin [:aza]]

For what it is worth, this bug also blocks hardware acceleration of the kinds of animations we use in Tab Candy (bug 564978) and we'd love to see it in b5.

Comment 145

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

(In reply to comment #143)
> roc: I think that this needs to block b5; agree?

Yes.

Comment 146

[Timothy Nikkel (:tnikkel)]

Attachment: Part 2 refreshed

Comment 147

[Timothy Nikkel (:tnikkel)]

Latest try server builds are at http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-f9eb580dd56b/

Comment 148

[Matheus Kerschbaum]

(In reply to comment #147)
> Latest try server builds are at
> http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-f9eb580dd56b/

I noticed several regressions with this build (some fonts are aliased, smooth scrolling is broken, scrolling on some pages can cause rendering artifacts..), should I file individual bugs and mark them as blocking this one?

Comment 149

[Timothy Nikkel (:tnikkel)]

(In reply to comment #148)
> I noticed several regressions with this build (some fonts are aliased, smooth
> scrolling is broken, scrolling on some pages can cause rendering artifacts..),
> should I file individual bugs and mark them as blocking this one?

Yes please.

Comment 150

[Timothy Nikkel (:tnikkel)]

Attachment: Part 2 refreshed

Comment 151

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Mostly note to self: I landed these patches on projects/cedar temporarily so that fennec+layers work can continue in parallel.  I'm going to back them out when this lands on m-c.

http://hg.mozilla.org/projects/cedar/rev/d9181da95392
http://hg.mozilla.org/projects/cedar/rev/6d16789c6aeb
http://hg.mozilla.org/projects/cedar/rev/d28c105c1d32

Comment 152

[Timothy Nikkel (:tnikkel)]

Latest try server build: http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-cf64eea45e07/

Comment 153

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: screenshot of the problem

With this tryserver build, on Windows 7, if I middle-click on a page and scroll up or down, a piece of the page gets pulled with the "scroll" icon.
The piece of the page that gets pulled with the scroll icon stays on the page after you click out of scroll mode, but goes away when you scroll on the page with the mousewheel or keyboard, so I guess triggering a repaint fixes it.

Comment 154

[Leman Bennett [Omega]]

(In reply to comment #153)
> Created attachment 468409 [details]
> screenshot of the problem
> 
> With this tryserver build, on Windows 7, if I middle-click on a page and scroll
> up or down, a piece of the page gets pulled with the "scroll" icon.
> The piece of the page that gets pulled with the scroll icon stays on the page
> after you click out of scroll mode, but goes away when you scroll on the page
> with the mousewheel or keyboard, so I guess triggering a repaint fixes it.

That's Bug 588403

Comment 155

[Leman Bennett [Omega]]

Disregard, misread the date.

Comment 156

[Bobby Holley (:bholley)]

It appears that this landed? I noticed the effects in the latest nightly.

part 1: http://hg.mozilla.org/mozilla-central/rev/ffacf623a9df
part 1.1: http://hg.mozilla.org/mozilla-central/rev/687b70fea4d0
part 2: http://hg.mozilla.org/mozilla-central/rev/7804b5cf4313

Should this be resolved fixed?

Comment 157

[Timothy Nikkel (:tnikkel)]

Yes, this landed. I haven't had a spare moment in order to mark all the bugs resolved and paste the changeset links yet.

Comment 158

[CruNcher]

There is some heavy thing going on 

http://forums.mozillazine.org/viewtopic.php?f=23&t=1990331

http://img801.imageshack.us/img801/566/namorokafine.png    (Namoroka Nightly)

http://img831.imageshack.us/img831/6941/minefieldprob.png (Minefield Nightly and starting with Beta 2/ Beta 1 is ok no Kernel Explosion visible)

currently this site http://hcm.24h.com.vn/ almost kills Gecko 2.0b it stalls the GUI because of the Windows Kernel Peaking

Comment 159

[Gábor Stefanik]

That's probably something else - this landed in beta 5.

Comment 160

[Marco Zehe (:MarcoZ)]

In the aftermatt of this bug, I received a question on my blog: http://www.marcozehe.de/2010/09/23/whats-up-with-all-those-windows/comment-page-1/#comment-63851
Basically, the developer wants to know how he can find the sidebar from inside an XPCOM extension now that we've taken away the hwnd for it. Can someone more knowledgeable than me help out here? Thanks!

Comment 161

[Timothy Nikkel (:tnikkel)]

I think file a new bug and get that commenter on the bug and we can figure out what exactly he needs and we can map out how to get it with existing APIs or what API we need to add to do that.