See http://weblogs.mozillazine.org/roc/archives/2005/11/frame_display_lists.html for an overview. My patch for this now compiles although there's still a long way to go in terms of testing it and getting it working. I want to put a copy of the patch in Bugzilla for backup purposes. The original vision pretty much held up during implementation. There were several pain points: XUL hacks in GetFrameForPoint/Paint that need to be emulated; table background painting (which really wants borders and backgrounds to be two separate layers); recreating the view manager's scroll analysis algorithm; and more that I probably won't find until I've actually tested this thing. This patch will not remove a lot of view-related code that could be removed. E.g., a bunch of view flags and synchronization with frames are no longer used. I'm trying to bound the size of this patch. Even so, this patch removes more lines than it adds (and I think the new lines have a higher comment/line ratio than the old lines...). To simplify this job, the patch makes fixed-pos elements not have widgets. To compensate for that, the scroll analysis algorithm always allows the scrolling view to bitblt, but invalidates extra areas as necessary. This is the right way to go anyway. I've changed the 'opacity' implementation to work in a way that's pathologically bad for nested translucent elements --- and won't work at all on Mac. This will be easily fixed when we move to cairo.

BTW a bunch of the comments in this patch are marked "REVIEW:". This means that they're only intended for review and should be deleted before checkin. These comments typically describe changes I'm making and only make sense in the context of comparing the old and new code ... i.e. they don't make sense in the context of the new code alone.

This patch builds against trunk, runs, and produces a very usable browser. I've tested all kinds of things and fixed a lot of regressions. I currently know of a couple of minor regressions, no doubt there are more left. It fixes the bugs it's supposed to fix including the last acid2 red region. Next I'm going to run a battery of automated tests against the test suites we have to see what renderings have changed. I'll also run Trender tests to see how performance is doing. On my machine there is no visible perf degredation but this machine is too fast to show it.

BTW the patch is gzipped.

The results are very noisy so they're hard to interpret. However, if anything, Trender improves slightly with frame display lists. The only really huge change is the improvement in misc-trans. The improvement there is probably due to not having widgets for position:fixed elements,which reduces the number of paints we have to do.

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

Okay, I've run visual regression tests against all *.*ml files under layout/html/tests. Here's a summary of the results: -- Border dotting/dashing has changed -- Radio button rasterization has changed -- Rounded border rasterization has changed -- Some gnome-web-photo glitches with fixed-pos -- Some existing bugs (regressions?) fixed, including -- overflow:hidden table clipped to content-edge, not padding-edge -- table row frames honour 'overflow:hidden' -- table border painted above caption content -- abs-pos children of columns don't display properly (due to existing bug with placeholders not being in the right place) -- Regression: Fieldset border errors (FIXED) -- Regression: Positioned inlines not painting their absolute children (FIXED) -- Regression: Comboboxes not clipping their display text (FIXED) -- Regression: overflow:hidden Table outer frames not clipping properly (FIXED by moving clipping to table inner frames) -- Regression: table cell quirks-mode %-overheight clip area not correctly positioned (FIXED) -- Regression: table row background at the root of a stacking context doesn't paint row-spanning cells correctly (FIXED) I'll upload a new patch, updated to trunk, shortly ... thus with the new patch we should have no regressions under layout/html/tests. Next I'll test against Hixie's test suite.

I fixed border drawing so it matches existing builds.

I ran against a subset of Hixie's tests ... the ones I successfully downloaded and that worked at all. About 3600 tests altogether. Summary of results: -- Regression: crash when page has no root frame (?) (FIXED) -- Regression: crash in nsComputedDOMStyle::GetStyleData when aFrame is null (FIXED) -- couldn't reproduce some issues (border alignment) -- many many bugs fixed by patch! -- lots of timing-dependent false positives -- Regression: error calculating region clipped by abs-pos clipping (FIXED) -- Regression: error in repeating tiled backgrounds (FIXED) -- Regression: error in relatively positioned cell backgrounds (FIXED) -- Regression: error stacking a float with 'opacity' (FIXED) -- Regression: root scrollframe not clipping fixed-pos elements (FIXED) I've also added a lot of documentation in nsDisplayList.h. Along the way I realized that GetUnderlyingFrame() was subtly broken in some edge cases so I tightened up the definition and made related changes.

This is the latest version of the patch, updated to trunk (gzipped because it's too big to attach directly).

This one actually fits in the bugzilla limit :-).

Now, the question is, how are we going to review this. :-) One idea I have is that maybe we should identify someone to "own" or at least "peer" this code --- NOT dbaron or bz --- and have them read through it checking it for sanity. Have dbaron and/or bz check the core code and interfaces in nsDisplayList.cpp/.h, nsIFrame.h, and nsFrame.cpp and whatever else they feel up to.

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

Well, I tried to apply the updated patch, but I get some errors: http://wargers.org/mozilla/rejected.txt I'm fairly certain I have a reasonable clean build.

Can you try patching with the -l option?

I'll publish linux build with this patch for testers if it's ok for you.

patch failed hunks against current tree. url: http://www.e-gandalf.net/mozilla/patch2-rej

Hmm... I synchronized the patch with the trunk but it fails in nsTreeBodyFrame.cpp because of: if (oldPageCount != mPageLength || mHorzWidth != CalcHorzWidth()) { // Schedule a ResizeReflow that will update our info properly. nsBoxLayoutState state(mPresContext); MarkDirty(state); } /server/cvs/mozilla/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:2248: error: `mPresContext' undeclared (first use this function)

updated patch.

replaced mPresContext with GetPresContext() This patch compiles.

Linux test build available at http://www.e-gandalf.net/mozilla/firefox-bug-317375.tar.bz2

With this patch, gtk primitives used for native tab drawing are no longer clipped correctly.

Okay, I'll take a look when I get a chance. I guess we're discovering just how fast this patch rots

Julien, which theme, and what else can you say about the problem?

Any theme I tested exhibits the problem. I think it is not specific to tabs (I saw gliches on checkboxes too), but since tabs heavily use clipping to get the work done, it is very visible on them. It is even more visible with my patch (currently only on my custom build, bug 265698 I think) that changes the native rendering of tabs to remove the border between the selected tab and the underlying tabpanel. _FrnchFrgg_

To be precise : gtk primitives are no longer clipped to the XUL widget's rect. Probably a missing intersection before passing the cliprect to the native drawing methods.

Shouldn't this patch solve bug 191830 ? The testcase in it still renders wrong (or wrongly claims it is wrong, I have not checked).

I put some additionnal printf statements in gtk2drawing.c, and indeed, the cliprect passed to moz_gtk_widget_paint (which is passed unmodified from nsNativeThemeGTK::DrawWidgetBackground) always is the full invalidate rect. For example, if I invalidate the entire "Page Info" window, every call to DrawWidgetBackground for the tabs is done with a (x=0,y=0,w=416,h=485) cliprect.

Just intersecting aDirtyRect with aBorderArea before calling DrawWidgetBackground in layout/style/base/nsCSSRendering.cpp does the trick. I do not know where you'd want to put the intersection, though. If the background/border of an object cannot spit out of its border area, then I think it is sane to intersect the border area with the cliprect before attempting to paint; at least the speed is increased, since the painting can be avoided if IntersectRect returns false. I thought that always were the case, but I am not Mozilla-litterate enough to get a grasp of every edge-case. In any case, clipping to the BorderArea rect for -moz-appearance widgets not only raises no issue, but also is assumed by rendering code (at least for tabs on gtk and on windows). The glitch on checkboxes has almost certainly nothing to do with this problem, since I still see it. _FrnchFrgg_

The problem with the checkboxes is that a unthemed checkbox is painted on top of the themed one. I did not investigate on why.

(In reply to comment #28) > Shouldn't this patch solve bug 191830 ? The testcase in it still renders wrong > (or wrongly claims it is wrong, I have not checked). That testcase is broken; CSS2.1 says elements with opacity < 1 do float above other in-flow siblings in the stacking order.

Zbigniew, your patch mixes in some other changes :-( I've fixed the checkbox (and radiobutton) and tab clipping issues. I'm still seeing an issue with groupbox borders not painting right. I'll get into that soon and post a new patch.

(In reply to comment #33) > Zbigniew, your patch mixes in some other changes :-( Seems so :( Sorry for that. I had also SMIL patch applied. Shame on me. Do you have new version of the patch? I'd like to test it on my brand fresh tree and publish another build.

Comment on attachment 206103 [details] [diff] [review] patch from attachment 205915 [details] updated to the trunk (2) forget this one.

Update patch to trunk. This contains the fixes to the checkbox and tabpanel clipping problems, and also fixes the groupbox issue I saw (which turned out to be existing bad code in nsCSSRendering's RoundedRect::Set).

Robert, it seems for me that your tree is not synced. gandalf@gandalf /server/cvs/mozilla/mozilla $ patch -p0 --dry-run < ~/projects/mozilla/patch2 (...) patching file layout/base/Makefile.in Hunk #1 FAILED at 111. 1 out of 1 hunk FAILED -- saving rejects to file layout/base/Makefile.in.rej (...) patching file layout/forms/nsButtonFrameRenderer.h Hunk #1 FAILED at 48. Hunk #2 FAILED at 104. 2 out of 2 hunks FAILED -- saving rejects to file layout/forms/nsButtonFrameRenderer.h.rej patching file layout/forms/nsComboboxControlFrame.cpp (...) patching file layout/generic/nsFrameSetFrame.cpp Hunk #7 FAILED at 1751. 1 out of 8 hunks FAILED -- saving rejects to file layout/generic/nsFrameSetFrame.cpp.rej (...) patching file layout/generic/nsHTMLFrame.cpp Hunk #2 FAILED at 103. 1 out of 4 hunks FAILED -- saving rejects to file layout/generic/nsHTMLFrame.cpp.rej (...) patching file layout/mathml/base/src/Makefile.in Hunk #1 FAILED at 47. 1 out of 1 hunk FAILED -- saving rejects to file layout/mathml/base/src/Makefile.in.rej patching file layout/mathml/base/src/nsMathMLChar.cpp Hunk #2 FAILED at 1934. 1 out of 2 hunks FAILED -- saving rejects to file layout/mathml/base/src/nsMathMLChar.cpp.rej (...) patching file layout/tables/nsTableColGroupFrame.h Hunk #1 FAILED at 76. 1 out of 2 hunks FAILED -- saving rejects to file layout/tables/nsTableColGroupFrame.h.rej (...) patching file layout/xul/base/src/nsImageBoxFrame.cpp Hunk #2 FAILED at 376. 1 out of 2 hunks FAILED -- saving rejects to file layout/xul/base/src/nsImageBoxFrame.cpp.rej

Try with -l? I synced just before I posted that patch.

I tried patching with -l option. I still get the same errors as Zbigniew mentioned in comment 38.

Try this one. I just updated about an hour ago.

Sorry, still getting the same errors with the "new patch?" patch :(

Works for me with both new patches with -l (not -1 (letter l vs. digit 1) ;)

Sorry for the confusion, comment 42 is wrong, it is now working for me also with the -l option.

I tested everything I could think about during my limited free time, and didn't notice any glitches. Some of dbaron's tests rendered correctly contrary to without the patch. Robert, do you have some hints as to where to find more relevant test suites and what to test ?

I haven't run the W3C CSS tests, but I don't expect them to show much of interest. Hixie's test suite is fantastic but I've already tested that for regressions. At this point I'd be most concerned about regressions in areas not covered by my automated rendering regressions tests; namely, XUL, MathML, and event handling. I've done some manual testing of all of these, of course, but the more the better. Thanks!

I think I see a regression with the latest patch, see: http://wargers.org/iframegoogle.htm With a patched build, I see the iframe with visibility:hidden;. With the latest trunk build not. Also, it is expected that fixed positioned elements 'lag' when scrolling the page, right? (something that you are planning to fix later in the game)

New patch. This just changes one line in nsFrame::BuildDisplayListForChild: // and CSS visibility is atomic over them too - if (!IsVisibleForPainting(aBuilder)) + if (!aChild->IsVisibleForPainting(aBuilder)) return NS_OK; This fixes the bug detected by Martijn. Good work Martijn :-).

> This just changes one line in nsFrame::BuildDisplayListForChild: Well, it also updates the patch to trunk. The flicker you see with fixed-pos elements is expected. It will get fixed later.

Oops, I had incorrectly merged updates to nsObjectFrame. This corrects the problem.

With the patched build, I get an assertion with this page: http://wargers.org/assertion.htm "Did not find ourselves on the placeholder's ancestor chain" The page basically consists of this: <div style="position: fixed;"> <div style="border: 1px solid black; position: absolute;"> </div> </div>

Fixes the assertion. The only change is in MarkOutOfFlowChild; I moved the test "if (f == aFrame)" before "if (f->GetStateBits() & NS_FRAME_OUT_OF_FLOW)".

With the patch the background disappears in this test case when you scroll down a bit https://bugzilla.mozilla.org/attachment.cgi?id=90636

This url is basically the same issue as mentioned in the previous comment: http://wargers.org/317375_bgfixed.htm But maybe it makes it a bit clearer of what is going on. It seems this patch also regresses this example: http://wargers.org/317375_abs_invisible_in_float.htm (I'm seeing this problem on http://www.zianet.com/zyloo/paper_radio/index.htm )

Another bug (could be the same as the previous background bug): http://wargers.org/317375_td_background.htm

Another bug: http://wargers.org/317375_multipleselect.htm When focusing something in a multi-select box, the focus outline is positioned too high and too far to the left.

At https://bugzilla.mozilla.org/attachment.cgi?id=207493 (careful, this testcase can crash your browser), it's from bug 322348. the text doesn't show up anymore with the patch. Also I get to see a load of assertions.

roc, apparently with that patch the text in the testcase for bug 322348 doesn't paint. Not an emergency yet since our style context tree is all screwed up there, but I should have a patch for that soon, I hope.

With every svg page (like this one for example: http://www.croczilla.com/svg/samples/arcs1/arcs1.svg), you don't get to see the autoscroll image anymore when middle-clicking in Firefox.

> This url is basically the same issue as mentioned in the previous comment: > http://wargers.org/317375_bgfixed.htm > But maybe it makes it a bit clearer of what is going on. > Another bug (could be the same as the previous background bug): > http://wargers.org/317375_td_background.htm Both fixed with some more rework of nsCSSRendering::PaintBackgroundWithSC. > It seems this patch also regresses this example: > http://wargers.org/317375_abs_invisible_in_float.htm > (I'm seeing this problem on http://www.zianet.com/zyloo/paper_radio/index.htm ) Fixed with some changes to MarkOutOfFlowChild and BuildDisplayListForChild. (In reply to comment #56) > Another bug: > http://wargers.org/317375_multipleselect.htm > When focusing something in a multi-select box, the focus outline is > positioned too high and too far to the left. Fixed with changes to nsListControlFrame::PaintFocus. > With every svg page (like this one for example: > http://www.croczilla.com/svg/samples/arcs1/arcs1.svg), you don't get to see > the autoscroll image anymore when middle-clicking in Firefox. Autoscroll on Firefox works by inserting a position:fixed HTML IMG element as a child of the document's root element: http://lxr.mozilla.org/mozilla/source/toolkit/content/widgets/browser.xml#728 Inserting this as a child of a root SVG element *should not* work, so I'm not going to treat this as a bug. In the 1.9 world it should become possible to put the autoscroll marker in the containing XUL document where it belongs.

I'm ignoring bug 322348 for now as well.

Updated as mentioned in the previous comments, and also updated to latest trunk.

Martijn, when this patch lands we should check the various tests that caught issues in it into our regression tests. If you haven't gotten a CVS account by then, send me a list of URLs and I'll do it?

(In reply to comment #60) > Inserting this as a child of a root SVG element *should not* work, so I'm not > going to treat this as a bug. Sorry, I have to know, why should it not work?

In case that anyone is interested in today's trunk with this patch on Linux - http://www.e-gandalf.net/mozilla/firefox-trunk-20060109-bug317375-and-places.tar.bz2

I see some glitches during the testcase from bug 140789 (http://www.e-gandalf.net/mozilla/bugs/140789/dynl-c31.html). It seems that the text disappears sometimes, I don't know if it's the trunk falut or this patch.

(In reply to comment #64) > (In reply to comment #60) > > Inserting this as a child of a root SVG element *should not* work, so I'm not > > going to treat this as a bug. > Sorry, I have to know, why should it not work? Because an IMG child of an SVG element should not be displayed. You'd have to wrap it in foreignobject or something.

(In reply to comment #66) > I see some glitches during the testcase from bug 140789 > (http://www.e-gandalf.net/mozilla/bugs/140789/dynl-c31.html). > It seems that the text disappears sometimes, I don't know if it's the trunk > falut or this patch. Which test phase causes that?

(In reply to comment #68) > Which test phase causes that? I see it. There is indeed a bug. New patch coming up.

New patch. This changed only nsIFrame::BuildDisplayListForStackingContext, to get the abs-pos clip rect into the right coordinate system before intersecting it with the dirty area. This was causing child content to not be drawn in some cases where an element with 'opacity' or non-auto 'z-index' also had 'clip' applied.

(In reply to comment #67) > Because an IMG child of an SVG element should not be displayed. You'd have to > wrap it in foreignobject or something. Ah, I see, makes sense, thanks. I've tried the latest updated patch (of 2006-01-09). All issues I mentioned are solved with it, but I've found a new one: http://wargers.org/317375_bordertestcase.htm The right and bottom border of the inner, green bordered iframe disappears when scrolling with this patch.

I suspect this isn't a bug, but something that the patch fixes: http://wargers.org/Well,%20I'm%20Back.htm Without the patch, you see only green, with the patch, you also see a red block. In case the patch did fix this, then you need to fix your blog, because with the patch, the :hover trick doesn't work anymore.

Probably related to the issue mentioned in comment 71: http://wargers.org/317375_iframe_border.htm The scrollbars don't show up fine here, and you can't scroll with it.

A fix in nsSubdocumentFrame::BuildDisplayList fixes the IFRAME issues. Your testcase from my blog shows a bug being fixed by this patch; inline content goes above floating siblings.

http://wargers.org/317375_no_hover_td.html Hovering over a table cell doesn't seem to work with the patch.

Fixed that bug by adding a HitTest method to nsDisplayTableCellBackground.

See http://wargers.org/317375_replaced-overflow-auto.xhtml There are already some issues with this testcase, see bug 230863, but the frame display lists patch introduces a new one, when scrolling down in the replaced, overflow:auto img, and then hovering over it, you get a flickering behavior, the img disappear and then appear again.

See http://wargers.org/317375_svg_image_clipping.xml I'm seeing a much larger box than without the patch.

http://wargers.org/317375_iframe_for_testcase.xul After switching tabs and returning, the yellow focus outline isn't painted well anymore (you have to focus one of the buttons first to get the yellow focus outline). http://wargers.org/317375_focus_outline_iframe.xul When clicking on two buttons, I get two focus outlines with the patch. There should only be one (the iframe contains the previous mentioned url).

(In reply to comment #79) > http://wargers.org/317375_focus_outline_iframe.xul > When clicking on two buttons, I get two focus outlines with the patch. There > should only be one. Same problem with checkboxes: http://wargers.org/317375_focus_outline_checkbox.xul

http://wargers.org/317375_painting_bug_absolute.html Painting bug with the patch. You should only see a small red and a small green box. Resizing the window fixes it.

http://wargers.org/317375_outline.htm I see that outline is now always at the top, I guess that's on purpose, right? (I think I like it). They are transparent to events, I noticed.

(In reply to comment #77) > See http://wargers.org/317375_replaced-overflow-auto.xhtml > There are already some issues with this testcase, see bug 230863, but the > frame display lists patch introduces a new one, when scrolling down in the > replaced, overflow:auto img, and then hovering over it, you get a flickering > behavior, the img disappear and then appear again. That testcase causes us to endlessly recreate frames for the content while the mouse is over it, and I can't even scroll it most of the time, so I'm going to ignore it until the underlying bug is fixed (which I don't want to roll into this patch). (In reply to comment #78) > See http://wargers.org/317375_svg_image_clipping.xml > I'm seeing a much larger box than without the patch. Fixed with a change to nsSVGOuterSVGFrame::Paint (In reply to comment #79) > http://wargers.org/317375_iframe_for_testcase.xul > After switching tabs and returning, the yellow focus outline isn't painted > well anymore (you have to focus one of the buttons first to get the yellow > focus outline). This is reproducible on trunk, filed as bug 323255. > http://wargers.org/317375_focus_outline_iframe.xul > When clicking on two buttons, I get two focus outlines with the patch. There > should only be one (the iframe contains the previous mentioned url). Fixed with changes to PresShell::HandleEvent and nsViewManager::DispatchEvent. (In reply to comment #80) > Same problem with checkboxes: > http://wargers.org/317375_focus_outline_checkbox.xul Couldn't reproduce that, but should be fixed now. (In reply to comment #81) > http://wargers.org/317375_painting_bug_absolute.html > Painting bug with the patch. You should only see a small red and a small > green box. Resizing the window fixes it. Present on trunk, please file a new bug. (In reply to comment #82) > http://wargers.org/317375_outline.htm > I see that outline is now always at the top, I guess that's on purpose, > right? Yes, that is recommended by the spec. > (I think I like it). They are transparent to events, I noticed. That was my choice. We can easily change it if desired.

updated as indicated

(In reply to comment #83) > (In reply to comment #81) > > http://wargers.org/317375_painting_bug_absolute.html > > Painting bug with the patch. You should only see a small red and a small > > green box. Resizing the window fixes it. > > Present on trunk, please file a new bug. Oops! I filed it as bug 323291.

(Canvas is working fine with the patch) Minor bug: http://wargers.org/317375_listbox.htm With this testcase, you see with the patch the right part of the focus outline out of the select box.

This fixes that issue.

http://wargers.org/317375_flash_wmode.html There seems to be an issue (with the patch) with embedded flash that has wmode="transparent" set, it doesn't appear in the correct place or doesn't appear at all.

Updated to trunk. I can't test windowless plugins, they're only on Windows. I've made some changes to this patch in nsObjectFrame to make the code for painting windowless plugins closer to what it was --- I think it will fix at least a couple of bugs I spotted. Let me know if it helps.

That helps, I can also see the left blue circle now. But I can't interact with the flash now, it seems like it is painted in the background. See previous testcase, when hovering over the circle the background color of the circle should change into red, that's not happening with the left one with this patch.

OK, that's because we weren't calling DisplayBackgroundBorderOutline in nsObjectFrame::BuildDisplayList, which most frames use to create a background that receives events (and for replaced elements that's enough because they're atomic in z-order). The reason I wasn't calling DisplayBackgroundBorderOutline there is that nsObjectFrame::Paint currently doesn't paint borders, backgrounds or outlines, so in fact they aren't supported on OBJECT or EMBED elements. Amazingly I don't recall this being reported before. Anyway, this patch fixes that as well as (hopefully) the Flash event issue.

So this fixes bug 236089, basically? Note that borders were not painted because of issues with widget/view/frame coords lining up, etc -- the plugin widget needs to be the content-box, but the view is the border-box, etc.

I think nsObjectFrame's reflow code needs to be changed to handle borders as well. See also the XXXbz comment in nsObjectFrame::GetDesiredSize.

Ok, the latest patch fixes the flash event issue. I see an issue with frameborders, they look different with the patch (somethink like 2px solid black instead of the normal brownish rounded border): http://wargers.org/317375_frameborder.html

Yeah, this patch makes the borders and outline paint, but incorrectly because of the plugin positioning and reflow issues. But background will paint correctly and I think we should do this as a step towards really fixing bug 236089.

If we don't want to risk borders/outlines on OBJECTs causing trouble in the interim, I can easily change the DisplayBackgroundBorderOutline call to something that just does the background only.

Currently, the border of a collapsed-border table isn't repainted if only the outer half is invalidated. See testcase.

s/Currently/With the patch (yesterday's version, I didn't have time to compile again my tree).

https://bugzilla.mozilla.org/attachment.cgi?id=147434 from bug 242253 should show "foo" as text, this doesn't happen anymore in my debug build with the patch.

(In reply to comment #94) > Ok, the latest patch fixes the flash event issue. > I see an issue with frameborders, they look different with the patch > (somethink like 2px solid black instead of the normal brownish rounded > border): > http://wargers.org/317375_frameborder.html Fixed in nsHTMLFramesetBorderFrame::PaintBorder. (In reply to comment #97) > Created an attachment (id=209030) [edit] > Missing redraw of outer half of table Fixed by adding nsDisplayTableBorderBackground::GetBounds to let it fluff out to the table's overflow area. (In reply to comment #99) > https://bugzilla.mozilla.org/attachment.cgi?id=147434 from bug 242253 should > show "foo" as text, this doesn't happen anymore in my debug build with the > patch. This appears to be an incremental reflow issue. The table cell frame seems to be positioned outside the table; forcing a reflow (e.g., by resizing the window) makes the text come back. Not sure why this doesn't show up on trunk. Let's file a separate bug about that, perhaps after this lands.

Updated patch as indicated. Thanks for all the testing work, this is most valuable.

(In reply to comment #100) > Let's file a separate bug about that, perhaps after this lands. I'll keep an eye on it, I can't reproduce it on current trunk builds, so I don't think it would make much sense to file a bug right now. This behavior has changed: http://wargers.org/317375_bordercell.htm A border-collapse table, when hovering over the borders, they should turn green. This doesn't work well now, but the patch changes the behavior slightly.

(In reply to comment #102) > This behavior has changed: > http://wargers.org/317375_bordercell.htm > A border-collapse table, when hovering over the borders, they should turn > green. > This doesn't work well now, but the patch changes the behavior slightly. Some of the gliches seem to be related to bug 286797 (I'm not trying to force that bug upon you, even if it looks as such ;)

http://wargers.org/317375_vishidden.htm You should be able to select the text and when hovering over the black bordered box, it should turn green, doesn't happen here with my build with the patch.

(In reply to comment #102) > This behavior has changed: > http://wargers.org/317375_bordercell.htm > A border-collapse table, when hovering over the borders, they should turn > green. > This doesn't work well now, but the patch changes the behavior slightly. I'm not going to worry about this one. With the patch I'm about to submit, behaviour is very similar to current (broken). (In reply to comment #104) > http://wargers.org/317375_vishidden.htm > You should be able to select the text and when hovering over the black > bordered box, it should turn green, doesn't happen here with my build with > the patch. Fixed by not catching events in nsDisplayTableBackground and instead constructing per-frame nsDisplayBackground items specifically for catching events. I also noticed that in some cases we would paint outlines on table parts even when the frame was hidden, so I fixed that, making nsFrame::DisplayOutline check visibility to be consistent with DisplayBackgroundBorderOutline. I added nsFrame::DisplayOutlineUnconditional for callers who've already checked visibility.

I had to apply some parts of the patch by hand, due to the landing of the pixels to twips patch, so I am not sure if I introduced errors, but : With the patch, the unselected tabs cannot be clicked onto. Keyboard navigation continue to work. There is no problem with the trunk without the patch. I am certain I have a pure clean source tree, since I use unionfs to manage my different custom patched trees. Clicking on the selected tab while in "click to find node" mode of DOMI selects the whole hbox child of tabbrowser-tabs, while clicking on unselected ones doen't do anything -- DOMI doen't even quit the "click-find" mode.

Some forgotten infos : This behaviour is shown with the latest trunk (I strongly suspect it is related to the brand new tab code), and the latest patch. The previous one did also exibit this behaviour, but the way I applied it was sort of ugly (I just took the list of files that failed and unapplied every checkin affecting those one after the orhter until I succeded in applying the patch, and I dit it quickly, perhaps forgetting some files)

Ah, and the problem is only present for the tab strip of the browser, not in "normal" tabs (as in preferences dialog). Sorry for bugspam -- I'll try in the future not to split a post in three due to lacking memory :(

Okay, this is updated to trunk. I've removed the 'mousethrough=always' attribute on tabs that was causing the problem. On trunk, that attribute was being ignored because there was no non-ancestor element below the tab title in the DOM --- that's just how 'mousethrough' works on trunk. IMHO that behavior is completely bogus; with this patch, 'mousethrough' simply means 'this element is transparent to mouse events'. Removing the useless mousethrough=always attribute fixes the problem and doesn't seem to break anything.

So I'm looking at nsIFrame::BuildDisplayListForChild, and I'm concerned about a bunch of things. First of all, NS_FRAME_REPLACED_ELEMENT probably doesn't match the CSS definition of replaced element. But, more importantly, Appendix E only refers to the "replaced content" painting atomically -- that doesn't include the border or background, which are painted in step 2 for blocks and still need to follow block/inline layer rules, I think. It would also be helpful if the return in the middle that happens to be the 99% case had a slightly bigger comment marking it. Why is the variable extraPositionedDescendants needed? It seems that if it's needed to fix a bug for the positive-z-index-child case, it would cause a bug for the negative-z-index-child case. Also, "pseudo" is misspelled in the comment on the 6th line.

A bunch of comments refer to BuildDisplayListForChildren in places outside the box code, but that function is only in nsBoxFrame. What should they refer to? What does BuildDisplayListForInlineChildren really mean? I'm confused about two things: * why do nsSimplePageSequenceFrame and nsViewportFrame use it? * Why do nsContainerFrame::BuildDisplayList and nsHTMLContainerFrame::DisplayTextDecorationsAndChildren both call it? Can that cause duplication?

(In reply to comment #111) > First of all, NS_FRAME_REPLACED_ELEMENT probably doesn't match the CSS > definition of replaced element. But, more importantly, Appendix E only > refers to the "replaced content" painting atomically -- that doesn't include > the border or background, which are painted in step 2 for blocks and still > need to follow block/inline layer rules, I think. Okay, I will change that. > It would also be helpful if the return in the middle that happens to be the > 99% case had a slightly bigger comment marking it. OK > Why is the variable extraPositionedDescendants needed? It seems that if it's > needed to fix a bug for the positive-z-index-child case, it would cause a bug > for the negative-z-index-child case. You mean instead of just appending to aChild's positioned kids to aLists.PositionedDescendants() directly? That would put aChild after its positioned kids in the aLists.PositionedDescendants() list, which is not correct. In fact there would be no bug because the stacking context will sort its entire PositionedDescendants list by z-index and content order anyway, but it would make the sort routine do more work. So I guess extraPositionedDescendants is just a little optimization ... perhaps not worth it? > Also, "pseudo" is misspelled in the comment on the 6th line. Ok, fixed.

(In reply to comment #112) > A bunch of comments refer to BuildDisplayListForChildren in places outside > the box code, but that function is only in nsBoxFrame. What should they > refer to? nsContainerFrame::BuildDisplayListForInlineChildren > What does BuildDisplayListForInlineChildren really mean? I'm confused about > two things: > * why do nsSimplePageSequenceFrame and nsViewportFrame use it? It's poorly named. It just processes all children, putting their backgrounds and borders and contents into the content list. This happens to be how inlines behave, so that's the name I gave it. I'm open to suggestions for a better name. > * Why do nsContainerFrame::BuildDisplayList and > nsHTMLContainerFrame::DisplayTextDecorationsAndChildren both call it? Can > that cause duplication? Frames that call nsHTMLContainerFrame::DisplayTextDecorationsAndChildren do not also call nsContainerFrame::BuildDisplayList.

(In reply to comment #113) > > Why is the variable extraPositionedDescendants needed? It seems that if it's > > needed to fix a bug for the positive-z-index-child case, it would cause a bug > > for the negative-z-index-child case. > > You mean instead of just appending to aChild's positioned kids to > aLists.PositionedDescendants() directly? That would put aChild after its > positioned kids in the aLists.PositionedDescendants() list, which is not > correct. In fact there would be no bug because the stacking context will sort > its entire PositionedDescendants list by z-index and content order anyway, but > it would make the sort routine do more work. So I guess > extraPositionedDescendants is just a little optimization ... perhaps not worth > it? Then could you add a comment saying that things are added in that order so that the sort routine can do less work?

(In reply to comment #115) > Then could you add a comment saying that things are added in that order so that > the sort routine can do less work? Sure. At one time you wanted this to be blocked by bug 310985, is that still so?

(In reply to comment #116) > At one time you wanted this to be blocked by bug 310985, is that still so? No.

updated for dbaron's comments. I renamed BuildDisplayListForInlineChildren to BuildDisplayListForNonBlockChildren and fixed the comments. I complete removed the handling of NS_FRAME_REPLACED_ELEMENT in BuildDisplayListForChild. We were doing two things in there: checking visibility and forcing atomicity in terms of z-order. In most cases it is not necessary for replaced elements to specially do an early exit if !IsVisbibleForPainting, all they need to do is ensure that they don't add any display items of their own if they're hidden. As for their children, for elements like <select> that can have user-provided content, there's nothing wrong with displaying visible child elements (I think); for replaced element frames with anonymous child content creating child frames, we don't override visibility in those child frames so there is no issue. As for z-ordering atomicity, I don't think there are any issues there either. The existing from control paint layering code was added in bug 95826 and bug 107244, apparently to ensure that the backgrounds of those form frames were painted in the FOREGROUND layer, like inline frames. This patch removes special z-order handling in those frames but doesn't regress the testcases in those bugs, because inline-level frames now get their backgrounds put on the right child list by their parent. There could still be a problem if we had a block-level replaced-element frame with a block-level anonymous child frame with a background --- it might not be painted atomically in z-order. As far as I can tell this would currently only apply to list control frames for a <select style="display:block">, but are they really replaced elements for this purpose? Should we stop their option block children from drawing their backgrounds in the block background layer?

> There could still be a problem if we had a block-level replaced-element frame > with a block-level anonymous child frame true for a block-level or inline-level replaced-element frame, I guess.

(In reply to comment #118) > child list by their parent. There could still be a problem if we had a > block-level replaced-element frame with a block-level anonymous child frame > with a background --- it might not be painted atomically in z-order. As far as > I can tell this would currently only apply to list control frames for a <select > style="display:block">, but are they really replaced elements for this purpose? > Should we stop their option block children from drawing their backgrounds in > the block background layer? Things like select and input should probably be forced to be treated like inline-block and inline-table (which I think you don't do yet in the current patch).

(In reply to comment #120) > Things like select and input should probably be forced to be treated like > inline-block and inline-table (which I think you don't do yet in the current > patch). That makes sense in an inline context, but what if they're display:block?

Okay. This modifies nsFrame::BuildDisplayListForChild so you can tell it you're in an inline context. In that case, if aChild is a block or table frame, we make it a pseudo-stacking-context. This takes care of <select>, most <input>s, and will take care of inline-table and inline-block. While testing I also noticed a subtle bug with repainting and roundoff. The problem turned out to be a situation like this: we repaint an dirty area whose rect.y=130 pixels. nsViewManager::Refresh does a Translate(..., -130*pixelsToTwips) before drawing to the offscreen buffer. This creates a translation of -130.0000001 pixels in the transform matrix because we multiply by 14.0 and then the scale factor, which is 1/14.0 (not represented perfectly by the float). Now it turns out that the dirty thing we're painting (a combobox dropdown button) is 12 pixels high and has a background image centered inside it, and the image is 7 pixels high. Therefore it draws the image at 130 + (12 - 7)/2 = 132.5 pixels. After translation and rounding, the device coordinate is 2.49999 -> 2 pixels, so after the backbuffer is copied back to the damage area, the rendering appears at 132 pixels. HOWEVER if we do a full redraw of the page, then the dirty area y=0, the transform matrix's translation is 0, and the image is drawn at round(132.5) = 133 pixels. To fix this, I have modified gfx to add a SetTranslation method to nsIRenderingContext, which is used when double-buffering to ensure that the initial translation to account for the origin of offscreen buffer is exactly right; any subsequent error accumulation will at least be independent of the damage rect. This bug probably exists independent of the frame display list patch, but it doesn't manifest on trunk. (Or maybe it does, in some uninvestigated rounding/rendering filed bug.)

With the approval of dbaron and mrbkap, I landed this. Let's see how it goes.

This seemed to regress Tp a bit on btek an luna at least. Looking at the log, it's across-the-board type increase, not on specific pages...

Yes, and a codesize increase of 16K on luna is unwelcome too. I think I should focus on the codesize issue. If we fix that, the Tp numbers may come back down. Trender/Tdhtml numbers didn't really move so I don't think we actually made painting slower directly.

I want to look at the codesize numbers in detail to see where we grew. Making some nsDisplayList.h inline functions out-of-line might help. Another thing to do is to remove more of the no-longer-used view code. E.g. I think the "is transparent" view flag is no longer used and all the code that maintains it can be removed.

The //-style comments in ua.css cause an error to console: CSS Error (resource://gre/res/ua.css :72.4): Expected declaration but found '/'. syntax error near unexpected token `(' Skipped to next declaration.

This checking has caused MathML-enabled builds to fail to compile. I don't have access to the specific errors right now, but it's due to 19-20 unresolved externals. I know stephend has also seen these same errors. roc: Do you want me to start a seperate bug for this or should I just post the compiler output here when I get home tonight?

(In reply to comment #128) > This checking has caused MathML-enabled builds to fail to compile. I don't have > access to the specific errors right now, but it's due to 19-20 unresolved > externals. I know stephend has also seen these same errors. > > roc: Do you want me to start a seperate bug for this or should I just post the > compiler output here when I get home tonight? 2006-01-25 23:06 dbaron%dbaron.org mozilla/view/public/nsIViewObserver.h 3.20 1/1 Another attempt at the Windows bustage. b=317375 that fixed it for me...

Turns out activex was picking up nsRect.h through nsIPresShell.h... which was an unnecessary #include, so I've removed it. But bz says that nsIPresShell should probably just forward-declare nsRect

I believe this may have caused bug 324763.

(In reply to comment #122) > Created an attachment (id=209534) [edit] > updated patch > > While testing I also noticed a subtle bug with repainting and roundoff. The > will at least be independent of the damage rect. This bug probably exists > independent of the frame display list patch, but it doesn't manifest on trunk. > (Or maybe it does, in some uninvestigated rounding/rendering filed bug.) > From my testing with today's build vs. a build from a couple of days ago, this appears to have fixed bug 296025 and bug 289929.

These are the codesize results from luna's log, saved for posterity.

OH, and unless I am mistaken, this has fixed another issue with the Acid 2 test. Strange it was not listed as a dependncy on the ACID 2 meta-bug.

> Strange it was not listed as a dependncy on the ACID 2 meta-bug. It was, just not directly. See the dependency tree of this bug. The Acid2 metabug is in it, via actual bugs that existed that this patch fixed.

My build from today now shows a CSS parsing error on startup in ua.css: CSS Error (resource://gre/res/ua.css :72.4): Expected declaration but found '/'. Skipped to next declaration. Related to this patch? I didn't think you could put C++ style comments in a CSS file :) Trees in thunderbird no longer seem to be painting correctly this morning either but I'm not sure if that's related to this bug or not yet. I'll keep looking.

I filed Bug 324866 for the tree widget painting regression I'm seeing in Thunderbird today. I put a screen shot in that bug. This seems to be the only bug that touched tree frames yesterday. But the patch is so big, I'm hesitant to try to back it out in my tree to prove that it causes the painting problems :)

(In reply to comment #136) > My build from today now shows a CSS parsing error on startup in ua.css: > > CSS Error (resource://gre/res/ua.css :72.4): Expected declaration but found > '/'. > Skipped to next declaration. > > Related to this patch? I didn't think you could put C++ style comments in a CSS > file :) Scott: already mentioned in comment 127. > Trees in thunderbird no longer seem to be painting correctly this morning > either but I'm not sure if that's related to this bug or not yet. I'll keep > looking. >

(In reply to comment #127) > The //-style comments in ua.css cause an error to console: That comment was REVIEW: only, so I just checked in a patch to simply remove it. I took the liberty of bypassing review on this trivial patch.

What is the expected performance impact of this patch so soon after the landing? Scrolling is slower now and feels quite sluggish compared to builds without this patch.

Dennis, please file a new bug on that issue and make it blocking this bug. (mention the site(s) where you see the sluggish scrolling)

Seems there is a regression with css background when "no-repeat" is used. In the attached testcase, image isn't displayed.

Regis, please file a new bug for that and make it blocking this bug.

(In reply to comment #143) > Regis, please file a new bug for that and make it blocking this bug. Done (#324960)

Looks like fixing bug 324969 fixed the Tp hit from this bug (and on luna actually made it into a 1% or so Tp win).

Is it possible that this checkin caused bug 325492?

I filed bug 326551 for the issue mentioned in comment 99 (see also comment 100).

Any chance the check-in on 20060125 caused bug 327259? Not sure if that bug's Mac-only or not at this stage.