Closed
Bug 90099
Opened 23 years ago
Closed 22 years ago
cells with fixed backgrounds don't repaint when needed
Categories
(Core :: Layout: Tables, defect)
Core
Layout: Tables
Tracking
()
RESOLVED
FIXED
Future
People
(Reporter: phil, Assigned: roc)
References
()
Details
(Whiteboard: [awd:tbl])
Attachments
(1 file, 3 obsolete files)
5.62 KB,
patch
|
dbaron
:
review+
roc
:
superreview+
asa
:
approval1.3a+
|
Details | Diff | Splinter Review |
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•23 years ago
|
||
seeing this on linux build 2001-07-09-21 as well.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows NT → All
Hardware: PC → All
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.
Summary: Backgroundimages with the property background-attachment:fixed; are not displayed in the cell of a table: → cells with fixed backgrounds don't repaint when needed
Comment 3•23 years ago
|
||
Temporarily moving to future until a milestone can be assigned.
Status: NEW → ASSIGNED
Target Milestone: --- → Future
*** Bug 172831 has been marked as a duplicate of this bug. ***
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.)
Assignee | ||
Comment 6•22 years ago
|
||
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)
Assignee | ||
Comment 7•22 years ago
|
||
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.
Assignee | ||
Comment 8•22 years ago
|
||
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.
Attachment #101860 -
Attachment is obsolete: true
Assignee | ||
Comment 9•22 years ago
|
||
Taking... Looks like we need views for generated content too (see bug 92723). I'll add that...
Assignee | ||
Comment 11•22 years ago
|
||
Attachment #101861 -
Attachment is obsolete: true
Comment 12•22 years ago
|
||
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.
Assignee | ||
Comment 13•22 years ago
|
||
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.
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.)
Assignee | ||
Comment 15•22 years ago
|
||
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•22 years ago
|
||
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?
Assignee | ||
Comment 17•22 years ago
|
||
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•22 years ago
|
||
Comment on attachment 101935 [details] [diff] [review] best patch yet r/sr=kin@netscape.com with dbaron's issue addressed.
Attachment #101935 -
Flags: superreview+
Assignee | ||
Comment 19•22 years ago
|
||
Attachment #101935 -
Attachment is obsolete: true
Assignee | ||
Comment 20•22 years ago
|
||
Comment on attachment 102157 [details] [diff] [review] revised fix marking with kin's sr since this patch address dbaron's comment
Attachment #102157 -
Flags: superreview+
Comment on attachment 102157 [details] [diff] [review] revised fix r=dbaron
Attachment #102157 -
Flags: review+
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? :-)
Assignee | ||
Comment 23•22 years ago
|
||
Er ... yeah :-). I'll check this in in a couple of weeks when I get back from my holiday :-).
Assignee | ||
Updated•22 years ago
|
Attachment #102157 -
Flags: approval1.3a?
Comment 24•22 years ago
|
||
Comment on attachment 102157 [details] [diff] [review] revised fix a=asa for checkin to 1.3a
Attachment #102157 -
Flags: approval1.3a? → approval1.3a+
Assignee | ||
Comment 25•22 years ago
|
||
Fix checked in.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 26•22 years ago
|
||
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 27•22 years ago
|
||
Fix checked in again :-).
Status: REOPENED → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•