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!

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

Changing Priority to P4

Moving this out...

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

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.

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.

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

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

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.

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

checked in.

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

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.

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.

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.

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>

Yes.

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

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.

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

This is the patch as checked in.

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

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.

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.

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

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

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

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

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

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

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

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.

Comment on attachment 285029 [details] [diff] [review] Revised patch using GetSameTypeParent as suggested by bz r+sr=bzbarsky

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.

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

Targeting M10.

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

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

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.

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.

I have created bug 433048 for followup work.

This bug should remain open.

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.

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.

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.

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?

Mats could do it.

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

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.

Tryserver builds with those patches will show up here: https://build.mozilla.org/tryserver-builds/rocallahan@mozilla.com-try-673a79f73633/

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

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

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

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

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.

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

Updated to trunk.

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.

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.

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

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.

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

Comment on attachment 453999 [details] [diff] [review] Part 1 refreshed I guess these never got reviewed.

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

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

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.

part 1 rotted again, looks like

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.