Last Comment Bug 317375 - Refactor painting/event handling to use frame display lists
: Refactor painting/event handling to use frame display lists
Status: RESOLVED FIXED
: perf
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal with 8 votes (vote)
: ---
Assigned To: Robert O'Callahan (:roc) (email my personal email if necessary)
:
Mentors:
Depends on: 325911 433744 446670 465900 161155 202628 317513 317514 324763 324812 324819 324836 324866 324883 324896 324915 324921 324940 324960 324963 324969 324980 325093 325276 325296 325407 325681 325702 326011 326040 326158 326251 326551 326732 326758 326827 327259 328285 328296 328480 329330 330905 331590 331809 333481 335140 336121 336153 336582 343495 344894 345609 347641 348621 349477 350148 352851 354243 354298 360129 362045 362356 365831 367443 368596 372365 377065 400015 405305 409994 411585 416735 423823 428156 433478 436352 436492 437220 438017 442304 445072 452209 455152 458651 491180
Blocks: 236089 266122 305568 317447 47742 78087 154926 191830 198062 204278 219147 225419 257458 266933 278033 279677 284115 287813 287940 289929 293638 293967 293977 296025 303813 304295 307811 308408 310761 311284 312639 315076 315519 317137 318784 320435 322440 323497 389623 391230
  Show dependency treegraph
 
Reported: 2005-11-21 20:34 PST by Robert O'Callahan (:roc) (email my personal email if necessary)
Modified: 2013-02-14 11:44 PST (History)
49 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
first pass (1002.87 KB, patch)
2005-11-21 20:43 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
mostly working (218.76 KB, patch)
2005-11-28 15:55 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
baseline Trender test results (-O2) (6.38 KB, text/plain)
2005-12-01 20:28 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
Trender results after applying frame display list patch (6.39 KB, text/plain)
2005-12-01 20:34 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
updated patch (215.95 KB, application/octet-stream)
2005-12-14 17:39 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
diff -w for review (1014.70 KB, patch)
2005-12-14 17:41 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
patch from attachment 205915 updated to the trunk (824.32 KB, patch)
2005-12-16 08:34 PST, Zibi Braniecki [:gandalf][:zibi]
no flags Details | Diff | Splinter Review
patch from attachment 205915 updated to the trunk (2) (824.34 KB, patch)
2005-12-16 09:25 PST, Zibi Braniecki [:gandalf][:zibi]
no flags Details | Diff | Splinter Review
updated patch (227.00 KB, application/octet-stream)
2005-12-20 18:55 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
diff -w (1016.21 KB, patch)
2005-12-20 18:58 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
new patch? (227.01 KB, application/octet-stream)
2005-12-21 15:36 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
updated patch (227.40 KB, application/octet-stream)
2005-12-29 13:36 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
updated patch (227.35 KB, application/octet-stream)
2005-12-29 14:26 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
updated patch (227.35 KB, application/octet-stream)
2005-12-29 19:28 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
updated patch (228.60 KB, application/octet-stream)
2006-01-08 19:54 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
updated patch (229.11 KB, application/octet-stream)
2006-01-09 14:11 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
updated patch (229.13 KB, application/octet-stream)
2006-01-10 14:23 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
updated patch (229.17 KB, application/octet-stream)
2006-01-11 12:46 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
updated patch (228.66 KB, application/octet-stream)
2006-01-12 20:25 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
updated patch (228.89 KB, application/octet-stream)
2006-01-16 12:52 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
updated patch (228.96 KB, application/octet-stream)
2006-01-18 13:22 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
updated patch (228.95 KB, application/octet-stream)
2006-01-19 11:31 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
Missing redraw of outer half of table (571 bytes, text/html)
2006-01-19 15:58 PST, Julien "_FrnchFrgg_" RIVAUD
no flags Details
updated patch (229.12 KB, application/octet-stream)
2006-01-19 18:12 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
updated patch (229.69 KB, application/octet-stream)
2006-01-22 15:52 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
updated patch (229.75 KB, application/octet-stream)
2006-01-23 16:05 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
updated patch (229.95 KB, application/octet-stream)
2006-01-23 19:58 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
updated patch (232.66 KB, application/octet-stream)
2006-01-24 18:13 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
luna codesize results (73.25 KB, text/plain)
2006-01-26 11:47 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
css background with "no-repeat" problem (535 bytes, text/html)
2006-01-27 09:41 PST, Régis Caspar
no flags Details

Description Robert O'Callahan (:roc) (email my personal email if necessary) 2005-11-21 20:34:57 PST
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.
Comment 1 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-11-21 20:43:42 PST
Created attachment 203888 [details] [diff] [review]
first pass
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-11-21 20:45:41 PST
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.
Comment 3 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-11-28 15:55:40 PST
Created attachment 204392 [details] [diff] [review]
mostly working

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.
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-11-28 15:57:05 PST
BTW the patch is gzipped.
Comment 5 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-12-01 20:28:56 PST
Created attachment 204756 [details]
baseline Trender test results (-O2)
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-12-01 20:34:57 PST
Created attachment 204758 [details]
Trender results after applying frame display list patch

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.
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-12-11 13:23:23 PST
*** Bug 257458 has been marked as a duplicate of this bug. ***
Comment 8 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-12-11 17:32:47 PST
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.
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-12-14 16:15:04 PST
I fixed border drawing so it matches existing builds.
Comment 10 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-12-14 16:17:44 PST
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.
Comment 11 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-12-14 17:39:09 PST
Created attachment 205915 [details]
updated patch

This is the latest version of the patch, updated to trunk (gzipped because it's too big to attach directly).
Comment 12 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-12-14 17:41:54 PST
Created attachment 205916 [details] [diff] [review]
diff -w for review

This one actually fits in the bugzilla limit :-).
Comment 13 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-12-14 17:50:42 PST
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.
Comment 14 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-12-15 12:41:46 PST
*** Bug 320435 has been marked as a duplicate of this bug. ***
Comment 15 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-12-15 18:39:09 PST
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.
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-12-15 19:06:14 PST
Can you try patching with the -l option?
Comment 17 Zibi Braniecki [:gandalf][:zibi] 2005-12-16 03:24:09 PST
I'll publish linux build with this patch for testers if it's ok for you.
Comment 18 Zibi Braniecki [:gandalf][:zibi] 2005-12-16 05:04:03 PST
patch failed hunks against current tree.

url: http://www.e-gandalf.net/mozilla/patch2-rej
Comment 19 Zibi Braniecki [:gandalf][:zibi] 2005-12-16 08:30:20 PST
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)
Comment 20 Zibi Braniecki [:gandalf][:zibi] 2005-12-16 08:34:05 PST
Created attachment 206091 [details] [diff] [review]
patch from attachment 205915 [details] updated to the trunk

updated patch.
Comment 21 Zibi Braniecki [:gandalf][:zibi] 2005-12-16 09:25:37 PST
Created attachment 206103 [details] [diff] [review]
patch from attachment 205915 [details] updated to the trunk (2)

replaced mPresContext with GetPresContext()
This patch compiles.
Comment 22 Zibi Braniecki [:gandalf][:zibi] 2005-12-16 09:46:25 PST
Linux test build available at http://www.e-gandalf.net/mozilla/firefox-bug-317375.tar.bz2
Comment 23 Julien "_FrnchFrgg_" RIVAUD 2005-12-16 10:12:34 PST
With this patch, gtk primitives used for native tab drawing are no longer clipped correctly.
Comment 24 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-12-16 14:14:35 PST
Okay, I'll take a look when I get a chance.

I guess we're discovering just how fast this patch rots
Comment 25 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-12-16 14:15:22 PST
Julien, which theme, and what else can you say about the problem?
Comment 26 Julien "_FrnchFrgg_" RIVAUD 2005-12-16 14:20:41 PST
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_
Comment 27 Julien "_FrnchFrgg_" RIVAUD 2005-12-16 14:25:56 PST
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.
Comment 28 Julien "_FrnchFrgg_" RIVAUD 2005-12-16 14:41:22 PST
Shouldn't this patch solve bug 191830 ? The testcase in it still renders wrong (or wrongly claims it is wrong, I have not checked).
Comment 29 Julien "_FrnchFrgg_" RIVAUD 2005-12-16 15:44:51 PST
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.
Comment 30 Julien "_FrnchFrgg_" RIVAUD 2005-12-16 16:20:59 PST
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_
Comment 31 Julien "_FrnchFrgg_" RIVAUD 2005-12-16 16:35:33 PST
The problem with the checkboxes is that a unthemed checkbox is painted on top of the themed one. I did not investigate on why.
Comment 32 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-12-18 20:06:39 PST
(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.
Comment 33 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-12-18 21:27:48 PST
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.
Comment 34 Zibi Braniecki [:gandalf][:zibi] 2005-12-18 21:29:53 PST
(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 35 Zibi Braniecki [:gandalf][:zibi] 2005-12-18 21:33:43 PST
Comment on attachment 206103 [details] [diff] [review]
patch from attachment 205915 [details] updated to the trunk (2)

forget this one.
Comment 36 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-12-20 18:55:23 PST
Created attachment 206460 [details]
updated patch

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).
Comment 37 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-12-20 18:58:23 PST
Created attachment 206461 [details] [diff] [review]
diff -w
Comment 38 Zibi Braniecki [:gandalf][:zibi] 2005-12-21 13:33:40 PST
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
Comment 39 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-12-21 13:38:44 PST
Try with -l? I synced just before I posted that patch.
Comment 40 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-12-21 15:31:43 PST
I tried patching with -l option. I still get the same errors as Zbigniew mentioned in comment 38.
Comment 41 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-12-21 15:36:21 PST
Created attachment 206553 [details]
new patch?

Try this one. I just updated about an hour ago.
Comment 42 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-12-21 15:46:19 PST
Sorry, still getting the same errors with the "new patch?" patch :(
Comment 43 Zibi Braniecki [:gandalf][:zibi] 2005-12-21 15:57:45 PST
Works for me with both new patches with -l (not -1 (letter l vs. digit 1) ;)
Comment 44 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-12-21 16:32:18 PST
Sorry for the confusion, comment 42 is wrong, it is now working for me also with the -l option.
Comment 45 Julien "_FrnchFrgg_" RIVAUD 2005-12-28 06:26:03 PST
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 ?
Comment 46 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-12-28 13:29:39 PST
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!
Comment 47 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-12-29 09:16:34 PST
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)
Comment 48 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-12-29 13:36:18 PST
Created attachment 207123 [details]
updated patch

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 :-).
Comment 49 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-12-29 13:37:37 PST
> 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.
Comment 50 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-12-29 14:26:43 PST
Created attachment 207127 [details]
updated patch

Oops, I had incorrectly merged updates to nsObjectFrame. This corrects the problem.
Comment 51 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-12-29 16:00:56 PST
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>
Comment 52 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-12-29 19:28:27 PST
Created attachment 207149 [details]
updated patch

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)".
Comment 53 Olli Pettay [:smaug] 2005-12-29 22:36:22 PST
With the patch the background disappears in this test case 
when you scroll down a bit
https://bugzilla.mozilla.org/attachment.cgi?id=90636
Comment 54 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-01-04 11:53:05 PST
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 )
Comment 55 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-01-04 15:10:42 PST
Another bug (could be the same as the previous background bug):
http://wargers.org/317375_td_background.htm
Comment 56 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-01-04 16:58:04 PST
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.
Comment 57 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-01-05 12:47:31 PST
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.
Comment 58 Boris Zbarsky [:bz] 2006-01-05 12:49:15 PST
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.
Comment 59 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-01-05 15:01:40 PST
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.
Comment 60 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-01-08 19:16:47 PST
> 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.
Comment 61 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-01-08 19:18:07 PST
I'm ignoring bug 322348 for now as well.
Comment 62 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-01-08 19:54:06 PST
Created attachment 207960 [details]
updated patch

Updated as mentioned in the previous comments, and also updated to latest trunk.
Comment 63 Boris Zbarsky [:bz] 2006-01-08 19:54:58 PST
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?
Comment 64 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-01-09 04:44:35 PST
(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?
Comment 65 Zibi Braniecki [:gandalf][:zibi] 2006-01-09 08:15:44 PST
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
Comment 66 Zibi Braniecki [:gandalf][:zibi] 2006-01-09 08:18:40 PST
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.
Comment 67 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-01-09 13:06:20 PST
(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.
Comment 68 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-01-09 13:10:55 PST
(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?
Comment 69 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-01-09 14:09:01 PST
(In reply to comment #68)
> Which test phase causes that?

I see it. There is indeed a bug. New patch coming up.
Comment 70 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-01-09 14:11:40 PST
Created attachment 208017 [details]
updated patch

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.
Comment 71 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-01-10 05:59:20 PST
(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.
Comment 72 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-01-10 06:51:02 PST
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.
Comment 73 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-01-10 09:19:58 PST
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.
Comment 74 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-01-10 14:23:10 PST
Created attachment 208131 [details]
updated patch

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.
Comment 75 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-01-11 05:56:36 PST
http://wargers.org/317375_no_hover_td.html
Hovering over a table cell doesn't seem to work with the patch.
Comment 76 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-01-11 12:46:57 PST
Created attachment 208221 [details]
updated patch

Fixed that bug by adding a HitTest method to nsDisplayTableCellBackground.
Comment 77 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-01-12 04:33:54 PST
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.
Comment 78 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-01-12 14:03:17 PST
See http://wargers.org/317375_svg_image_clipping.xml
I'm seeing a much larger box than without the patch.
Comment 79 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-01-12 14:46:15 PST
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).
Comment 80 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-01-12 15:24:16 PST
(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

Comment 81 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-01-12 17:09:57 PST
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.
Comment 82 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-01-12 18:54:40 PST
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.
Comment 83 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-01-12 20:23:31 PST
(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.
Comment 84 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-01-12 20:25:21 PST
Created attachment 208345 [details]
updated patch

updated as indicated
Comment 85 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-01-13 04:22:15 PST
(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.
Comment 86 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-01-16 08:55:45 PST
(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.
Comment 87 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-01-16 12:52:11 PST
Created attachment 208662 [details]
updated patch

This fixes that issue.
Comment 88 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-01-17 11:04:22 PST
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.
Comment 89 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-01-18 13:22:14 PST
Created attachment 208893 [details]
updated patch

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.
Comment 90 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-01-19 03:25:26 PST
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.
Comment 91 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-01-19 11:31:13 PST
Created attachment 208990 [details]
updated 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.
Comment 92 Boris Zbarsky [:bz] 2006-01-19 13:03:08 PST
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.
Comment 93 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-01-19 13:04:03 PST
I think nsObjectFrame's reflow code needs to be changed to handle borders as well.  See also the XXXbz comment in nsObjectFrame::GetDesiredSize.
Comment 94 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-01-19 15:06:40 PST
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
Comment 95 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-01-19 15:20:31 PST
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.
Comment 96 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-01-19 15:27:49 PST
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.
Comment 97 Julien "_FrnchFrgg_" RIVAUD 2006-01-19 15:58:45 PST
Created attachment 209030 [details]
Missing redraw of outer half of table

Currently, the border of a collapsed-border table isn't repainted if only the outer half is invalidated. See testcase.
Comment 98 Julien "_FrnchFrgg_" RIVAUD 2006-01-19 16:02:37 PST
s/Currently/With the patch (yesterday's version, I didn't have time to compile again my tree).
Comment 99 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-01-19 17:32:11 PST
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. 
Comment 100 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-01-19 18:11:04 PST
(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.
Comment 101 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-01-19 18:12:23 PST
Created attachment 209043 [details]
updated patch

Updated patch as indicated.

Thanks for all the testing work, this is most valuable.
Comment 102 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-01-20 08:34:20 PST
(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.
Comment 103 Julien "_FrnchFrgg_" RIVAUD 2006-01-20 09:07:00 PST
(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 ;)
Comment 104 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-01-20 10:07:36 PST
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.
Comment 105 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-01-22 15:51:39 PST
(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.
Comment 106 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-01-22 15:52:52 PST
Created attachment 209310 [details]
updated patch
Comment 107 Julien "_FrnchFrgg_" RIVAUD 2006-01-23 11:43:28 PST
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.
Comment 108 Julien "_FrnchFrgg_" RIVAUD 2006-01-23 11:49:45 PST
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)
Comment 109 Julien "_FrnchFrgg_" RIVAUD 2006-01-23 11:52:18 PST
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 :(
Comment 110 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-01-23 16:05:21 PST
Created attachment 209411 [details]
updated patch

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.
Comment 111 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-01-23 17:04:39 PST
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.
Comment 112 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-01-23 17:15:25 PST
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?
Comment 113 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-01-23 17:23:45 PST
(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.
Comment 114 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-01-23 17:28:57 PST
(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.
Comment 115 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-01-23 17:53:57 PST
(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?
Comment 116 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-01-23 18:14:21 PST
(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?
Comment 117 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-01-23 18:45:24 PST
(In reply to comment #116)
> At one time you wanted this to be blocked by bug 310985, is that still so?

No.

Comment 118 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-01-23 19:58:00 PST
Created attachment 209419 [details]
updated patch

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?
Comment 119 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-01-23 20:01:55 PST
> 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.
Comment 120 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-01-23 22:26:04 PST
(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).
Comment 121 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-01-24 01:16:06 PST
(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?
Comment 122 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-01-24 18:13:02 PST
Created attachment 209534 [details]
updated patch

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.)
Comment 123 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-01-25 18:34:53 PST
With the approval of dbaron and mrbkap, I landed this. Let's see how it goes.
Comment 124 Boris Zbarsky [:bz] 2006-01-25 22:06:00 PST
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...
Comment 125 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-01-25 23:15:59 PST
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.
Comment 126 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-01-25 23:37:54 PST
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.
Comment 127 Simon Montagu :smontagu 2006-01-26 04:44:32 PST
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.
Comment 128 Ryan VanderMeulen [:RyanVM] 2006-01-26 08:12:40 PST
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?
Comment 129 Stephen Donner [:stephend] 2006-01-26 08:24:47 PST
(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...
Comment 130 Benjamin Smedberg [:bsmedberg] 2006-01-26 08:44:34 PST
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
Comment 131 bugzilla 2006-01-26 09:47:19 PST
I believe this may have caused bug 324763.
Comment 132 Bill Gianopoulos [:WG9s] 2006-01-26 11:17:58 PST
(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.
Comment 133 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-01-26 11:47:50 PST
Created attachment 209739 [details]
luna codesize results

These are the codesize results from luna's log, saved for posterity.
Comment 134 Bill Gianopoulos [:WG9s] 2006-01-26 11:54:11 PST
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.
Comment 135 Boris Zbarsky [:bz] 2006-01-26 12:07:54 PST
> 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.
Comment 136 Scott MacGregor 2006-01-26 14:20:28 PST
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.
Comment 137 Scott MacGregor 2006-01-26 14:39:54 PST
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 :)
Comment 138 Stephen Donner [:stephend] 2006-01-26 14:40:12 PST
(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.
> 
Comment 139 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-01-26 14:53:58 PST
(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.
Comment 140 Dennis Jacobfeuerborn 2006-01-26 17:13:24 PST
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.
Comment 141 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-01-26 17:23:00 PST
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)
Comment 142 Régis Caspar 2006-01-27 09:41:40 PST
Created attachment 209859 [details]
css background with "no-repeat" problem

Seems there is a regression with css background when "no-repeat" is used. In the attached testcase, image isn't displayed.
Comment 143 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-01-27 09:59:16 PST
Regis, please file a new bug for that and make it blocking this bug.
Comment 144 Régis Caspar 2006-01-27 10:46:38 PST
(In reply to comment #143)
> Regis, please file a new bug for that and make it blocking this bug.
Done (#324960)

Comment 145 Boris Zbarsky [:bz] 2006-01-29 14:22:10 PST
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).
Comment 146 Jerry Baker 2006-02-01 15:06:31 PST
Is it possible that this checkin caused bug 325492?
Comment 147 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-02-09 08:00:38 PST
I filed bug 326551 for the issue mentioned in comment 99 (see also comment 100).
Comment 148 Wayne Woods 2006-02-15 01:35:38 PST
Any chance the check-in on 20060125 caused bug 327259? Not sure if that bug's Mac-only or not at this stage.
Comment 149 neil@parkwaycc.co.uk 2006-08-28 15:52:31 PDT
I'm guessing here from CVS blame, but this bug appears to have regressed window transparency - the issue is that nsViewManager is no longer painting the white context buffer. I managed to figure out a way of persuading it to, but that doesn't help me create transparent popups, because it's painting the window's background over the popup. Note that things appear to work OK (including, with my test patch, transparent popups) on branch builds, although there I'm running into a bug with the windows widget code that's creating a window that's incorretly parented and appears as a dummy taskbar button.
Comment 150 Ria Klaassen (not reading all bugmail) 2006-12-06 00:03:16 PST
*** Bug 362883 has been marked as a duplicate of this bug. ***
Comment 151 walter sheets 2007-01-29 11:10:18 PST
(In reply to comment #123)
> With the approval of dbaron and mrbkap, I landed this.
> Let's see how it goes.

Sorry, I just found a regression a year later which is still in trunk (but not in 2.0.0.1 release):

http://finance.yahoo.com and click on the small chart of the
NASDAQ average to enlarge it.

Immediately above the enlarged chart there will be a banner
"Try our new charts now in beta".  Click on that (requires
Flash plugin).  The problem begins when you enter a non-
existent stock symbol (try 'xyz') in the upper left corner
where you see 'Enter symbols' and click on 'Get Chart'.

After a few seconds you should see a pop-up saying No Data
Available.  You need to dismiss that pop-up before the
window will function properly (a stupid design decision).

On trunk, that pop-up never appears because the response
(apparently) never returns from Yahoo.  No idea why.
Comment 152 Boris Zbarsky [:bz] 2007-01-29 11:12:00 PST
That's a regression from this bug?  You're sure?
Comment 153 Steve England [:stevee] 2007-01-29 11:19:23 PST
Re: comment 151
WFM Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a2pre) Gecko/20070128 Minefield/3.0a2pre ID:2007012809 [cairo] & Shockwave Flash 9.0 r28
Popup appears ("No data available for requested time period: xyz") and [OK] button dismisses the dialog.
Comment 154 walter sheets 2007-01-29 11:26:33 PST
I just spent several hours narrowing it down to this range:
#mk_add_options MOZ_CO_DATE="25 Jan 2006 19:00"
#mk_add_options MOZ_CO_DATE="25 Jan 2006 18:00"

I have not actually tried backing out individual commits 
because there are too many of them.  I'd be happy to start
trying that if someone can give me a likely suspect out of
all of those to choose from.

This is linux, BTW.  I've not tried Windows trunk.
Comment 155 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2007-01-29 11:51:51 PST
Could you file the regression as a separate bug report?  It's hard to track bugs when they're all in the same report.
Comment 156 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-01-29 14:04:27 PST
The issue mentioned in comment 151 (and further) is now filed as bug 368596.
Comment 157 Aaron Leventhal 2007-04-16 13:42:25 PDT
I finally tracked this as the cause of accessibility regression bug 348621 -- no events inside iframes.
Comment 158 Juraj Lonc 2007-09-30 15:01:33 PDT
I am creating some page elements via DOM. And it is possible to drag&drop those elements (they have transparent background). When I drag and move some element the page under this element is shaking (invalid redrawing). 
This can be seen in FF2, but not in MSIE, Opera, FF3. 

http://taskee.bluecobra.net 
(just click "Open" button, an element is opened, it can be dragged and moved)

When I put those elements directly into HTML code it works without those redrawing problems. Problem shows up only when I build exactly the same elements through DOM createElement.
Comment 159 Boris Zbarsky [:bz] 2007-09-30 15:57:47 PDT
Doesn't sound like your issue has anything to do with this bug (except maybe this bug fixing it).

Note You need to log in before you can comment on or make changes to this bug.