90099 - cells with fixed backgrounds don't repaint when needed

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Philipp Metzler]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:0.9.2) Gecko/20010628
BuildID:    2001062815

Description: Backgroundimages with the property background-attachment:fixed; are
not displayed in the cell of a table:



Reproducible: Always
Steps to Reproduce:
example on this page: http://www.goli.at/phil/mozilla/bugs.html

Comment 1

[Boris Zbarsky [:bzbarsky]]

seeing this on linux build 2001-07-09-21 as well.

Comment 2

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

Note that the correct behavior of displaying the background image should be that
we only display it (I think -- see bug 1045) when the page is horizontally
scrolled so that the table cell with the fixed background image is at the left
edge of the page.  I see it *some* of the time there, but it's not being painted
reliably.  I suspect table cells don't have the logic needed to trigger repaints
when the have 'background-attachment: fixed' -- see bug 68499.

Comment 3

[karnaze (gone)]

Temporarily moving to future until a milestone can be assigned. 

Comment 4

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

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

Comment 5

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

It looks like the main problem here is that |nsTableCellFrame|s never get views.
 (At least that's what it looks like from a quick look in the code, and what I'm
seeing from "Dump Views" on the two testcases in bug 172831.)

Comment 6

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

Attachment: fix

I audited nsCSSFrameConstructor and added a call to
nsHTMLContainerFrame::CreateViewForFrame after InitAndRestoreFrame in the
places where I think we need one:
--- after table-related frames have been constructed (except for cols and
colgroups, because I'm not sure if that will screw up the view hierarchy, and I
don't even know that they're relevant)
--- after inline frames have been constructed, even if they're not positioned
(e.g., if they only have an "opacity" or "background-attachment:fixed" setting,
we still need a view)

Comment 7

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

The latter might impact page loads a little bit because I'm adding a small
amount of code to what I suspect is a very frequently executed path.

I'll attach a patch that should improve the speed of CreateForViewFrame a little
bit.

Comment 8

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

Attachment: better patch

Like the previous patch, but I tweaked CreateViewForFrame to check for an
existing view only after we've determined the frame's style demands a view.
This is because the existing view check almost always fails, because
CreateViewForFrame is almost always called on a fresh frame. Since most frames
don't need a view we want the "no view present or required" path to be as short
as possible.

Comment 9

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

Taking...

Looks like we need views for generated content too (see bug 92723). I'll add that...

Comment 10

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

really taking

Comment 11

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

Attachment: best patch yet

Comment 12

[kinmoz]

Do we really want to create views for every inline unconditionally? This sounds
like it has the potential to create lots of views when loading html pages with
lots of markup (links, font, bold, etc). Is there any impact to the event
system? My concern is that there may be frame based event code that may now be
bypassed because the view manager will send events directly to views associated
with the inlines.

Comment 13

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

nsHTMLContainerFrame::CreateViewForFrame only creates a view if the frame's
style says it needs one. See nsContainerFrame::FrameNeedsView. For inline
frames, that's only if the frame has -moz-opacity != 1.0, if it has a background
with background-attachment:fixed, or if it's relatively positioned.

We have to be able to create views for such frames, so if we encounter problems
with the views in the event system or wherever, then those are bugs in the event
system that have to be fixed. The same bugs would probably apply to relatively
positioned frames and so might already have been noticed, although since our
relative positioning is pretty buggy I'm not sure they would have been noticed :-).

Having said all that, I think the risk is really quite low since the two new
situations where we'll create a view (-moz-opacity, background-attachment:fixed)
are quite rare and we currently don't handle them correctly anwyay.

Comment 14

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

Is there a spot in the patch where you're creating a view for a text node?  (I
see |textStyleContext|.)  That seems evil.  (It could happen, given -moz-opacity.)

Comment 15

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

That call is for generated content.

Yes, it's evil. However, the real evil is that -moz-opacity inherits. Yes, it's
on my list :-).

Nevertheless, you're right that a non-element should never need a view, so I can
remove that call to CreateViewForFrame. I'll do that.

Comment 16

[kinmoz]

Comment on attachment 101935 [details] [diff] [review]
best patch yet

==== This approach would mean that the children in list1 and list3 could not be
on the same line as inline content that could be before and after the inline.
Imagine this example:  <body>bar<font>foo<p>ptext</p>foo</font>bar</body>


+  // XXX really, I think we should create an anonymous block frame
representing
+  // the ENTIRE element and attach a view to that, then put the list1, list2
and list3
+  // frames as child of that block --- roc


Are you going to post a new patch addressing dbaron's issue?

Comment 17

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

I will, but it's a trivial change so feel free to continue your review on the
assumption I'll get it right :-).

Comment 18

[kinmoz]

Comment on attachment 101935 [details] [diff] [review]
best patch yet

r/sr=[e-mail] with dbaron's issue addressed.

Comment 19

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

Attachment: revised fix

Comment 20

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

Comment on attachment 102157 [details] [diff] [review]
revised fix

marking with kin's sr since this patch address dbaron's comment

Comment 21

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

Comment on attachment 102157 [details] [diff] [review]
revised fix

r=dbaron

Comment 22

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

Does this also fix the fixed backgrounds in 
http://tom.me.uk/2002/11/css-pseudo-fixed-bg.xhtml ?  (They work under certain
conditions, but most of the time don't.)

roc:  Are you planning to check this in? :-)

Comment 23

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

Er ... yeah :-). I'll check this in in a couple of weeks when I get back from my
holiday :-).

Comment 24

[Asa Dotzler [:asa]]

Comment on attachment 102157 [details] [diff] [review]
revised fix

a=asa for checkin to 1.3a

Comment 25

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

Fix checked in.

Comment 26

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

I backed out the fix while trying to determine whether it had caused a Tp
regression. It appears to not be the cause of the regression. I will reland the
fix tomorrow.

Comment 27

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

Fix checked in again :-).