Closed Bug 130078 Opened 22 years ago Closed 14 years ago

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

Categories

(Core :: Web Painting, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla2.0b5
Tracking Status
blocking2.0 --- beta5+
fennec 2.0b1+ ---

People

(Reporter: tseng_mike, Assigned: roc)

References

(Depends on 7 open bugs, Blocks 4 open bugs)

Details

(Whiteboard: [not needed for 1.9][dbaron-1.9:RpCe][personas-needs][:bane][tabcandy])

Attachments

(10 files, 15 obsolete files)

565 bytes, application/vnd.mozilla.xul+xml
Details
25.48 KB, image/gif
Details
3.84 KB, text/plain
Details
1016 bytes, application/vnd.mozilla.xul+xml
Details
30.10 KB, image/png
Details
6.70 KB, text/plain
Details
991 bytes, patch
roc
: review+
Details | Diff | Splinter Review
3.08 KB, patch
Details | Diff | Splinter Review
1.03 KB, patch
Details | Diff | Splinter Review
58.26 KB, image/png
Details
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!
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: integrate iframe into chrome view → integrate iframe into chrome view hierarchy
This is trivial to correct, it just needs testing to make sure it doesn't cause
regressions.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.0.1
Changing Priority to P4
Priority: -- → P4
Moving this out...
Target Milestone: mozilla1.0.1 → mozilla1.2alpha
Component: Layout → Layout: View Rendering
QA Contact: cpetersen0953 → ian
*** Bug 216840 has been marked as a duplicate of this bug. ***
Attached patch Untested patch (obsolete) — Splinter Review
This patch might work :-)
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.
 
OK. We'll see about getting this into 1.6. It's "the right thing to do" but
there may be some performance impact.
Target Milestone: mozilla1.2alpha → mozilla1.6alpha
Will 1.6a fix bug 204278?  I'm looking forward to it.:)
Wang, please check if this patch fixes bug 216835 and bug 204278. If it does,
mark those bugs duplicates of this bug. Thanks!!
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.
This patch seems no effect on bug 204278 and bug 216835.
No longer blocks: 204278
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).
Summary: integrate iframe into chrome view hierarchy → integrate iframe into chrome view hierarchy (link view managers / trees between chrome and content)
*** Bug 190115 has been marked as a duplicate of this bug. ***
Attached patch fix (obsolete) — Splinter Review
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.
Attachment #130172 - Attachment is obsolete: true
Attachment #191565 - Flags: superreview?(dbaron)
Attachment #191565 - Flags: review?(dbaron)
This is for 1.9 of course.

CCing mconnor because of the toolkit change.
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).
Attachment #191565 - Flags: superreview?(dbaron)
Attachment #191565 - Flags: superreview+
Attachment #191565 - Flags: review?(dbaron)
Attachment #191565 - Flags: review+
checked in.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
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).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
My current thinking is that I might just go for full view manager removal and do
this as part of that.
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?
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.
Attached patch updated to trunk (obsolete) — Splinter Review
Attachment #191565 - Attachment is obsolete: true
Blocks: 324437
Attached patch alternative patch (obsolete) — Splinter Review
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.
Blocks: 204278
Blocks: 303813
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.
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.
> 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.
Blocks: 329726
I need to do a test-landing of this patch and see what the damage is.
I've been running w/ it for a while on a trunk build and things appear to be solid.
It's all about performance really.
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>
> Does fixing this bug mean that you should be able to overlay the content of an
> iframe like so? 

And it works quite wonderfully ... 
> 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.  
(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?
Attached patch updated patch (obsolete) — Splinter Review
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.
Attachment #220348 - Flags: superreview?(dbaron)
Attachment #220348 - Flags: review?(dbaron)
> 
> 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 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?
(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.
OK, I'll trust you on that, so r+sr=dbaron with the change to remove the else clause.
Attached patch updated patch (obsolete) — Splinter Review
This is the patch as checked in.
Attachment #220348 - Attachment is obsolete: true
Attachment #220737 - Flags: superreview+
Attachment #220737 - Flags: review+
Attachment #220348 - Flags: superreview?(dbaron)
Attachment #220348 - Flags: review?(dbaron)
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 
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
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
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...
Blocks: 337801
Flags: blocking1.9?
Attached patch Updated patch to trunk (obsolete) — Splinter Review
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.
Flags: blocking1.9? → blocking1.9+
I tried landing this again. Let's see how it goes.
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.
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.
Status: REOPENED → RESOLVED
Closed: 19 years ago18 years ago
Resolution: --- → FIXED
(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)
No longer blocks: 360731
Depends on: 360731
*** Bug 355078 has been marked as a duplicate of this bug. ***
Depends on: 360789
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.
Backed out
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Blocks: 341950
(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?
Blocks: 362968
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".
Target Milestone: mozilla1.6alpha → ---
Keywords: helpwanted
Why is this a P5? On http://wiki.mozilla.org/Firefox3/Gecko_Feature_List it is listed as a P1.
(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.
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.
Supposedly the "ghost button" issue will go away once view removal is complete. It seems to be a variant of bug 324819.
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.
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
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.
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.
Tested and reproducable with XULRunner: Mozilla/5.0 (Windows; U; Windows NT 5.1; de-DE; rv:1.9a6pre) Gecko/20070627
Blocks: 385609
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.
chrome-to-chrome should already be hooked up
(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.
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?
(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.
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.
Attachment #285010 - Flags: superreview?(bzbarsky)
Attachment #285010 - Flags: review?(bzbarsky)
Whiteboard: [needs review]
Attachment #285010 - Attachment is obsolete: true
Attachment #285029 - Flags: superreview?(bzbarsky)
Attachment #285029 - Flags: review?(bzbarsky)
Attachment #285010 - Flags: superreview?(bzbarsky)
Attachment #285010 - Flags: review?(bzbarsky)
Comment on attachment 285029 [details] [diff] [review]
Revised patch using GetSameTypeParent as suggested by bz

r+sr=bzbarsky
Attachment #285029 - Flags: superreview?(bzbarsky)
Attachment #285029 - Flags: superreview+
Attachment #285029 - Flags: review?(bzbarsky)
Attachment #285029 - Flags: review+
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.
Attachment #285029 - Flags: approval1.9?
Whiteboard: [needs review] → [needs approval]
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.

Whiteboard: [needs approval] → [needs approval][dbaron-1.9:RpCe]
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.
Attachment #285029 - Flags: approval1.9? → approval1.9-
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).
Attachment #285029 - Flags: approval1.9- → approval1.9+
Targeting M10.
Target Milestone: --- → mozilla1.9 M10
Whiteboard: [needs approval][dbaron-1.9:RpCe] → [needs landing][dbaron-1.9:RpCe]
So - why do we need this at this point in the 1.9 cycle?  
See comment #76. If you need more details, Neil will have to provide them.
(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?
Checking in layout/base/nsDocumentViewer.cpp;
/cvsroot/mozilla/layout/base/nsDocumentViewer.cpp,v  <--  nsDocumentViewer.cpp
new revision: 1.552; previous revision: 1.551
done
Status: REOPENED → RESOLVED
Closed: 18 years ago17 years ago
Keywords: helpwanted
Resolution: --- → FIXED
Whiteboard: [needs landing][dbaron-1.9:RpCe] → [dbaron-1.9:RpCe]
Flags: in-testsuite?
Sorry, this bug isn't fully fixed and won't be in 1.9. It needs the compositor work.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
There's nothing left here that blocks 1.9.
Flags: blocking1.9+
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.
Status: REOPENED → RESOLVED
Closed: 17 years ago16 years ago
Resolution: --- → INCOMPLETE
I have created bug 433048 for followup work.
This bug should remain open.
Status: RESOLVED → REOPENED
Resolution: INCOMPLETE → ---
No longer depends on: 433048
Whiteboard: [dbaron-1.9:RpCe] → [not needed for 1.9][dbaron-1.9:RpCe]
Any chance of seeing this fixed in 1.9.2? It may block work on Taskfox.
Depends on how fast I make progress on compositor pieces and what the 1.9.2 schedule is.
No longer blocks: 385609
Blocks: 294060
QA Contact: ian → layout.view-rendering
Target Milestone: mozilla1.9beta2 → ---
Whiteboard: [not needed for 1.9][dbaron-1.9:RpCe] → [not needed for 1.9][dbaron-1.9:RpCe][personas-needs]
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.
blocking2.0: --- → ?
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.
blocking2.0: ? → alpha1
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.
roc: can you recommend someone with the time and expertise to implement your suggestion in bug 494238, comment 4?
Maybe that should go in a different bug and this bug should continue to be for the general fix?
(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.
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 "-").
blocking2.0: alpha1 → ?
This is on top of the patch in bug 532569. It hooks up the view manager trees unconditionally.
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
* ?
What we'd need are tree tests for iframes I think. Alex, would you agree?
(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.
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?
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.
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.
(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?
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.
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.
The try server builds do have symbols available via the symbol server.
Depends on: 545593
Depends on: 546178
In addition, using JAWS, I get the same reproducible crash. So it's not dependent on NVDA alone.
Attached file 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.
Rob, looks like tab previews need to be retooled for the post-child-widgets world...
Depends on: 546572
Blocks: 543934
(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?
Blocks: 458407
No longer blocks: 458407
blocking2.0: ? → beta1+
Priority: P5 → P1
Depends on: 563878
Depends on: 567702
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.
Depends on: 571640
Depends on: 572599
Blocks: 564978
After discussing this with Johnath and dbaron, nothing for beta 1 depends on this on the Firefox side.  Bumping to beta 2.
blocking2.0: beta1+ → beta2+
Whiteboard: [not needed for 1.9][dbaron-1.9:RpCe][personas-needs] → [not needed for 1.9][dbaron-1.9:RpCe][personas-needs][:bane]
Whiteboard: [not needed for 1.9][dbaron-1.9:RpCe][personas-needs][:bane] → [not needed for 1.9][dbaron-1.9:RpCe][personas-needs][:bane][tabcandy]
Blocks: 574217
Attached patch Part 1 refreshed (obsolete) — Splinter Review
Updated to trunk.
Attachment #425945 - Attachment is obsolete: true
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
I've not seen that. Do you have steps to reproduce or a stack of the assertion?
testing on m-c 46301:911af4c07fd4, + jimm changes enabled for linux build:
DocumentViewerImpl::MakeWindow
Also applied part1/2 patches from this patch.
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.
Attachment #454426 - Attachment is obsolete: true
Depends on: 575567
Attached patch patch post 513162 (obsolete) — Splinter Review
No need to check for a container view in the parent widget anymore.
> patch post 513162
is this patch replacing  "Part 1 refreshed" or additional? changes seems overlapping each other
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?
This will not make beta 2. All the dependent bugs are still needed, plus some more that aren't filed yet.
Blocks: 580002
blocking2.0: beta2+ → beta3+
(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/tnikkel@gmail.com-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.
Blocks: 577208
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.
Unfortunately this will not make beta 3 code freeze.
Moving to betaN for now, but roc/dbaron, we need to figure out how and when we'll contain this fix.
blocking2.0: beta3+ → betaN+
Blocks: 583462
Depends on: 584193
This is jimm's change applied on top of part 1 so that we can separate it out and get review.
Attachment #455299 - Attachment is obsolete: true
Attachment #462530 - Flags: review?(roc)
Attachment #462530 - Flags: review?(roc) → review+
Comment on attachment 453999 [details] [diff] [review]
Part 1 refreshed

I guess these never got reviewed.
Attachment #453999 - Flags: review?(dbaron)
Attachment #425946 - Flags: review?(dbaron)
Depends on: 584516
Comment on attachment 425946 [details] [diff] [review]
Part 2: remove widgets from all subframes

r=dbaron
Attachment #425946 - Flags: review?(dbaron) → review+
Comment on attachment 453999 [details] [diff] [review]
Part 1 refreshed

r=dbaron
Attachment #453999 - Flags: review?(dbaron) → review+
No longer blocks: 574217
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.
Attachment #206053 - Attachment is obsolete: true
Attachment #209787 - Attachment is obsolete: true
Attachment #220737 - Attachment is obsolete: true
Attachment #241004 - Attachment is obsolete: true
Attachment #285029 - Attachment is obsolete: true
Attached patch Part 1 refreshedSplinter Review
Attachment #453999 - Attachment is obsolete: true
tracking-fennec: --- → 2.0b1+
Blocks: 586679
Blocks: 582816
Blocks: 582818
roc: I think that this needs to block b5; agree?
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.
(In reply to comment #143)
> roc: I think that this needs to block b5; agree?

Yes.
blocking2.0: betaN+ → beta5+
Attached patch Part 2 refreshed (obsolete) — Splinter Review
Attachment #425946 - Attachment is obsolete: true
Depends on: 587534
Depends on: 587539
Depends on: 587542
Depends on: 587944
Depends on: 587960
Blocks: 423014
Depends on: 587610
(In reply to comment #147)
> Latest try server builds are at
> http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/tnikkel@gmail.com-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?
(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.
Depends on: 588337
Depends on: 588402
Depends on: 588403
Depends on: 588407
Depends on: 588663
Depends on: 588684
Depends on: 588692
Depends on: 588693
Attached patch Part 2 refreshedSplinter Review
Attachment #465752 - Attachment is obsolete: true
Depends on: 589078
Blocks: 532106
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
Depends on: 589396
Blocks: 583135
Depends on: 589529
Depends on: 589535
No longer depends on: 589535
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.
Depends on: 590403
Blocks: 449734
Depends on: 591435
Depends on: 591554
Depends on: 591558
Blocks: 591559
(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
Disregard, misread the date.
Depends on: 591570
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?
Yes, this landed. I haven't had a spare moment in order to mark all the bugs resolved and paste the changeset links yet.
Status: REOPENED → RESOLVED
Closed: 16 years ago14 years ago
Resolution: --- → FIXED
Blocks: 507006
Depends on: 591754
Target Milestone: --- → mozilla2.0b5
Depends on: 591784
Depends on: 591671
Depends on: 591834
Depends on: 591805
Depends on: 591861
Depends on: 591874
Depends on: 592131
Depends on: 592663
Depends on: 592954
No longer blocks: 583135
No longer depends on: 592954
Depends on: 592954
Depends on: 593466
Depends on: 593577
Depends on: 593286
Depends on: 593892
Blocks: 593577
No longer depends on: 593577
Depends on: 593959
Depends on: 594267
Depends on: 594271
Depends on: 594791
Depends on: 595709
No longer depends on: 595709
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
That's probably something else - this landed in beta 5.
Blocks: 596441
No longer blocks: 596441
Depends on: 597031
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!
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.
No longer blocks: 507006
Depends on: 600396
Depends on: 601547
Depends on: 603550
Depends on: 604338
Depends on: 595132
Depends on: 605357
Depends on: 606980
Depends on: 612024
Depends on: 604303
Depends on: 602080
Depends on: 626403
Depends on: 628157
Depends on: 481737
Depends on: 648477
Depends on: 654643
Depends on: 626963
Depends on: 684431
Blocks: 263160
Depends on: 626813
Depends on: 690282
No longer depends on: 690282
Depends on: 713315
Depends on: 669511
Depends on: 1112470
Restrict Comments: true
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.