I have a page that does not apply opacity right when hovered with a border 1. Go to page http://www.hot.ee/irve/moz/bannerbug/iframe2.html 2. Hover image with mouse. Expected: Image changing to opaque when hovered Result: Image only getting a border

Linux, 2002073022 -> too

Confirmed 2002080404/win98se. Note that if you change 1px to 0px in the testcase, it starts working properly. No dupe found ==> NEW

I threw in the nsStyleContext change because it's the right thing to do....

So the basic problem here is that the image gets reflown (to handle the border). And there are no backgrounds set anywhere. So no painting happens. Oddly enough, taking out the inline the image lives in makes things work; I suspect that's because the image's view is twiddled by the blockframe it ends up in.... In any case, I think this is the right fix.....

Comment on attachment 93933 [details] [diff] [review] One possible fix r/sr=dbaron

->bz

I'm a bit scared about the performance implications of this. What do you guys think?

I think it's fine. In the current design, a repaint hint is subsumed by a reflow hint, but that reflow hint might end up not moving anything, which could mean that there's no repainting, which would be bad. Since this should just be doing invalidates, at worst, we'll do a few more invalidates than we would have otherwise.

The main thing I'm worried about is what while loading a page and appending to it, we'll keep repainting the darn thing over and over. Currently we don't do that and there is hacky code in nsContainerFrame::SyncFrameViewAfterReflow do make sure we don't (basically, if the frame grows vertically but not horizontally, only repaint the difference in bounds rects, otherwise repaint everything). This is going to repaint everything. In general it would be nice for reflow to only have to repaint stuff that changed, and this defeats that.

ProcessRestyledFrames isn't called during page loading, it's only called when we process style changes. The other alternative to this would be turning the style change hints into a bitfield (as you suggested in the past), although that's a good bit more work and would require removing the optimizations in nsStyleContext::CalcDifference.

Oh, ProcessRestyledFrames. OK. I do think we should change style change hints into a bitset so you can have any or all of VISUAL, VIEWSYNC, REFLOW (subsumes VIEWSYNC), REFRAME (subsumes all the others). I was planning to do this a while ago, because it fixes several bugs, but I never finished it. Mea culpa. Actually, I'm suspicious of bz's explanation of this particular case. nsContainerFrame::SyncFrameViewAfterReflow should be calling SetViewOpacity which forces a repaint if opacity changes. Furthermore it looks to me like if I force a repaint while the element is hovered, it still comes up with opacity .1

As far as nsContainerFrame goes.... I just poked through that. As far as I can tell, inline frames don't ever call nsContainerFrame::FinishReflowChild or nsContainerFrame::SyncFrameViewAfterReflow. This seems wrong.

SyncFrameViewAfterReflow gets called in a few places (e.g. PlaceFrameView in nsBlockFrame.cpp, which gets called when lines are moved around) but not in every reflow. This is definitely a problem here.

With this testcase you can see that in the initial reflow, the span gets its opacity set correctly, but if you click to set its opacity to 1, the opacity does not change; the style has changed but the view has not been updated.

See, that's the thing... that testcase worksforme in a current tip build on Linux.

Sorry, I was using a branch build. This testcase is better.

This testcase will continue to show the bug even if the broken optimization in nsStyleContext is fixed.... ;)

These testcases show that it is not a problem with missing invalidates, but rather that the view for the frame is not being fully synced during line reflow. bz's patch works because it forces view sync as well as invalidate after style changes. I guess it's OK but I don't like the duplication of code and work.

I've decided to do this the hard way and move to a bitset for style change hints. Patch forthcoming. Should we do this here or in another bug?

Here it is. This patch introduces the type nsStyleHint and a few operations on the type, and converts all current users of style hints over to use the new type. It does NOT introduce any different behaviour (I hope) except that ProcessRestyledFrames both reflows and updates frames given NS_STYLE_HINT_REFLOW (like bz's patch), thus fixing this bug. The reason for all this heavy lifting is that it allows us to do other things relatively easily: 1) separate non-geometric view synchronization out of reflow and repaint, to be performed only by nsStyleHint_SyncFrameView. 2) use finer-grained hints for style changes, e.g., nsStyleHint_ReflowFrame but NOT nsStyleHint_RepaintFrame for changes that require reflow but not necessarily painting the entire frame. Use only nsStyleHint_SyncFrameView for changes to styles such as "clip:" which may not require any painting or reflowing. 3) Introduce new more specialized hint bits, e.g. nsStyleHint_ForceViewForFrame which will force a frame change if the frame doesn't already have a view yet This patch is large, but because it's supposed to preserve existing behaviour, any regressions should be relatively easy to track down. Future patches to change behaviour for the above issues should be much smaller. I just realized that my patch doesn't include the fixes to the CalcDifference code that bz's patch has. I'll include those fixes in another iteration.

reassigning to roc, since he's doing all the work.

bz, dbaron: tell me your thoughts, and if suitable, your reviews :-).

bz going offline ... switch to Netscape backup unit

This is the same as the last patch, but I have included the fix to the optimization in nsStyleContext

Index: content/html/content/src/nsGenericHTMLElement.cpp - (NS_STYLE_HINT_CONTENT < impact), + (impact & (nsStyleHint_AttrChange | nsStyleHint_Aural + | nsStyleHint_Content)) != 0, Looks like you forgot a "~". nsCSSParser.cpp (2 occurrences): - if (aErrorCode == NS_ERROR_ILLEGAL_VALUE) { // bug 47138 + if (aErrorCode == (PRInt32)NS_ERROR_ILLEGAL_VALUE) { // bug 47138 I'd rather not. All those |aErrorCode| values should really just be changed to |nsresult| since that's what they are, but, please, not in this patch. It's big enough already. :-) If you want to move nsStyleConsts.h, make sure to have it done as a repository-copy rather than a |cvs add| (but then |cvs remove| as normal). I'd like to see a diff of that file, though. (Perhaps it would be better to move that as part of a different bug? There are a bunch of other misplaced files as well...) I've looked at everything up to (but not including) nsFrameManager.cpp. Comments on the rest of the patch (but not nsStyleConsts.h) in a bit...

nsStyleConsts.h has to be moved because it has to be visible outside content+layout (primarily because nsIDocument depends on nsStyleHint in the signature of attribute change notifications). Who coordinates intra-repository copies?

Ah, and you don't want to go through the whole tree changing REQUIRES? Seems reasonable. The best way to get a copy in the repository is probably to email leaf, but not until you're close to being able to check in.

Another option would be to create a little .h file just for nsStyleHint and put that in content/shared/public. I actually like that idea. What do you think?

Here's the changes to nsStyleConsts.h according to the current patch

A separate file seems fine for something like this that has to be included a lot. (I did that for nsCompatibility.h)

Comment on attachment 95601 [details] [diff] [review] changes to nsStyleConsts.h + aDest = r; + return (int)r == (int)aDest; This seems like a problem. (And why the casts, too?)

> Looks like you forgot a "~". Yep. > I'd rather not. OK... > This seems like a problem. Yeah, it should be (int)r == (int)aSrc. The casts are required because one isn't supposed to use the <,>,==,!=,<=,=> operators on these hints (hence the #ifdef DEBUG_roc operator overloading, which catches such problems). I'd better make that clearer in the new nsStyleHint.h...

> Yeah, it should be (int)r == (int)aSrc Well that doesn't quite work, but you know what I mean :-).

Comment on attachment 95601 [details] [diff] [review] changes to nsStyleConsts.h Another issue with this patch is that I don't like the definition of NS_STYLE_HINT_UNKNOWN -- unknown doesn't really seem equivalent to all -- it is used in a somewhat different way in the existing code, although I haven't completely figured out how. The other thing is that it's sometimes seemed to me that NS_STYLE_HINT_CONTENT was used to mean something similar to NS_STYLE_HINT_NONE, although I could be wrong about that. I'd need to look at the existing code a bit more...

Index: layout/html/base/src/nsFrameManager.cpp + (nsStyleHint_ReconstructFrame | nsStyleHint_ReconstructDoc | nsStyleHint_ReflowFrame))) && Perhaps list ReflowFrame first, to be consistent? - else { - // XXXldb |oldContext| is null by this point, so this will - // never do anything. - if (pseudoTag && pseudoTag != nsHTMLAtoms::mozNonElementPseudo && - aAttribute && (aMinChange < NS_STYLE_HINT_REFLOW) && - HasAttributeContent(oldContext, aAttrNameSpaceID, aAttribute)) { - aChangeList.AppendChange(aFrame, content, NS_STYLE_HINT_REFLOW); - } - } I'd prefer |#if 0| to removal, since it has to be replaced at some point, I think.

> Another issue with this patch is that I don't like the definition of > NS_STYLE_HINT_UNKNOWN OK, I've changed it to reflect the current code more closely. > The other thing is that it's sometimes seemed to > me that NS_STYLE_HINT_CONTENT was used to mean something similar to > NS_STYLE_HINT_NONE, although I could be wrong about that. Sometimes, but not always. What I have reflects what the current code does. > Perhaps list ReflowFrame first, to be consistent? OK. > I'd prefer |#if 0| to removal, since it has to be replaced at some point, I > think. OK. New patch coming up. The new patch addresses all your comments so far and also updates to the trunk as of a couple of days ago. I called the new .h file "nsChangeHint.h" and renamed nsStyleHint to nsChangeHint since I think nsChangeHint is closer to reality (the hints are generated by attribute changes as well as style changes)>

Comment on attachment 95816 [details] [diff] [review] Promised new patch You fixed the missing ~ I mentioned in comment 25 in only one of the three locations where it occurred. Other than that, r=dbaron.

Oops ... 2 other sites fixed as per dbaron's comment

Comment on attachment 96372 [details] [diff] [review] Revised patch carrying over dbaron's review

Comment on attachment 96372 [details] [diff] [review] Revised patch sr=kin@netscape.com

Fix checked in. Note that the patch here missed a few places in the Mac menu bar code which were implementing nsIDocumentObserver ... I added a bustage-fix for that.