Closed Bug 160936 Opened 22 years ago Closed 22 years ago

Image opacity does not change when hovered with border

Categories

(Core Graveyard :: GFX, defect, P2)

Product:
Core Graveyard
Component:
GFX
Platform:
x86
All
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: jaagup.irve, Assigned: roc)

References

(
URL
)

Details

Attachments

(1 file, 8 obsolete files)

Revised patch
203.41 KB, patch
roc
: review+
kinmoz
: superreview+
Details | Diff | Splinter Review 
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
OS: Windows 98 → All
Confirmed 2002080404/win98se.

Note that if you change 1px to 0px in the testcase, it starts working properly.

No dupe found ==> NEW
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch One possible fix (obsolete) — — Splinter Review 
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.....
->bz
Assignee: kmcclusk → bzbarsky
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.
Attached patch simplified testcase (obsolete) — — Splinter Review 
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.
Attached file Better testcase (obsolete) — 
Sorry, I was using a branch build. This testcase is better.
Attachment #94066 - Attachment is obsolete: true
Attached file Better yet (obsolete) — 
This testcase will continue to show the bug even if the broken optimization in
nsStyleContext is fixed.... ;)
Attachment #94074 - Attachment is obsolete: true
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.
Priority: -- → P2
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.
Assignee: bzbarsky → roc+moz
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
Attachment #93933 - Attachment is obsolete: true
Attachment #94077 - Attachment is obsolete: true
Attachment #94700 - Attachment is obsolete: true
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?
Attached patch changes to nsStyleConsts.h (obsolete) — — Splinter Review 
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)>
Attached patch Promised new patch (obsolete) — — Splinter Review 

        Attachment #95556 -
        Attachment is obsolete: true

        Attachment #95601 -
        Attachment is obsolete: true

    
    

    
  
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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Revised patch
        — 
          — Splinter Review
      

    


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

        Attachment #95816 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 96372 [details] [diff] [review]
Revised patch

carrying over dbaron's review

        Attachment #96372 -
        Flags: review+

    
    

    
  
Comment on attachment 96372 [details] [diff] [review]
Revised patch

sr=kin@netscape.com

        Attachment #96372 -
        Flags: superreview+

    
    

    
  
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.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: