263671 - nsStyleUserInterface::CalcDifference does wrong stuff if more than one property differs

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

if both mUserInput and mCursor differ, then NS_STYLE_HINT_VISUAL will be
returned, rather than NS_STYLE_HINT_REFLOW.

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

taking...

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

unfortunately I was unable to create a testcase showing this bug...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 161625 [details] [diff] [review]
patch

>Index: layout/html/style/src/nsCSSFrameConstructor.cpp
>+      if (hint & nsChangeHint_UpdateCursor) {
>+        nsIViewManager* viewMgr = aPresContext->GetViewManager();
>+        if (viewMgr)
>+          viewMgr->SynthesizeMouseMove(PR_FALSE);

So if there is a reframe we won't do this?  I guess that makes sense, but
should we perhaps put UpdateCursor before ReconstructFrame in nsChangeHint so
the "... subsumes ..." comment will reflect reality better?

Don't you also need to change nsStyleContext::CalcStyleDifference to add the
UpdateCursor hint to the initial maxHint value?  Otherwise, you should be
getting asserts whenever UpdateCursor is actually returned...

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

whoops, meant to mention that this patch contains some code from the layout
patch in bug 38447 (attachment 155496 [details] [diff] [review])

> So if there is a reframe we won't do this?  I guess that makes sense

oh. I can't remember why I put it into this else... I _think_ I actually want to
move it before the else.

>Don't you also need to change nsStyleContext::CalcStyleDifference to add the
>UpdateCursor hint to the initial maxHint value? 

probably, if you say so ;)

Comment 5

[Boris Zbarsky [:bzbarsky]]

(In reply to comment #4)
> whoops, meant to mention that this patch contains some code from the layout
> patch in bug 38447 (attachment 155496 [details] [diff] [review])

I assume that code is landing with this patch, right?  (Not the whole layout
patch, just the parts in the diff in this bug.)

> oh. I can't remember why I put it into this else... I _think_ I actually want
> to move it before the else.

Yeah, I think you do.  Or after the whole thing, better yet?

Though note that it may be possible that the initial reflow of the new frame
(which happens sometime later) will do its own mousemove stuff (per the code
David added).  In that case, this is correct.  The way to find out is to test,
of course.  Do something where off a timer you change the display of a div and
the cursor value of something, and then see whether the cursor changes before
the mouse moves.

> probably, if you say so ;)

I do say so. :)

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 161625 [details] [diff] [review]
patch

Marking r- per comments.

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #5)
> I assume that code is landing with this patch, right?  (Not the whole layout
> patch, just the parts in the diff in this bug.)

Indeed.

>  In that case, this is correct.  The way to find out is to test,

ok, will test

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: updated to files move

attaching this just for reference; it is unchanged from the above patch except
that it now patches the files in their new location

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

> Do something where off a timer you change the display of a div and
> the cursor value of something, and then see whether the cursor changes before
> the mouse moves.

does the ordering of these two actions matter? i.e. will this work:
    10
document.all.change2.style.cursor='url(http://www.mozilla.org/images/ico-ff.png),
pointer';
     11 document.all.change2.style.display='inline';

Comment 10

[Boris Zbarsky [:bzbarsky]]

In a recent build, order should not matter there.

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

hm... if I move the UpdateCursor check after the ReconstructFrame check, this
does not work... I get the cursor I specified for <html> instead of the one I
set in the .style.cursor... shouldn't this way always work?

Comment 12

[Boris Zbarsky [:bzbarsky]]

I'm not sure. What is "this way"?

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

this is "this way" :-)

Comment 14

[Boris Zbarsky [:bzbarsky]]

You also need to fix nsStyleContext::CalcDifference accordingly, no?

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #14)
> You also need to fix nsStyleContext::CalcDifference accordingly, no?

I can't seem to find such a function?
http://lxr.mozilla.org/seamonkey/search?string=nsStyleContext%3A%3ACalcDifferenc

I am patching nsStyleContext::CalcStyleDifference in this patch; are you saying
it's missing a change?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Er... I missed that part.  So yes, what you have there should work.

Perhaps the problem is that the newly-created frame has not been reflowed, so
has 0 size?  As a result, the mousemove that's synthesized doesn't see it?

But I'd expect the reflow to synthesize a mousemove too...

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 168467 [details] [diff] [review]
patch

hm.. that sounds like a different issue though... I still think this is a good
change.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 168467 [details] [diff] [review]
patch

So... the comments in nsChangeHint.h about "subsumes all of the above" are
wrong...  Also, there are short-circuit checks in ReResolveStyleContext that
bail out on nsChangeHint_ReconstructFrame.  So if a change causes a node to get
a new cursor and causes its parent to be reframed, we won't show the new
cursor...

In other words, we may need to do the cursor updating only in the "not
reframing" branch and fix things so frame construction under the pointer
updates the cursor....

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 168467 [details] [diff] [review]
patch

r+sr=bzbarsky if the issues in my last comment are addressed or have bugs filed
on them.

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #11)
> hm... if I move the UpdateCursor check after the ReconstructFrame check, this
> does not work... I get the cursor I specified for <html> instead of the one I
> set in the .style.cursor... shouldn't this way always work?

so, I don't know what happened there, but I now have a new testcase in which
this works just fine w/ the attached patch...

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: reframe + cursor change

Comment 22

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

and it also works inside the else of the ReconstructFrame if. this way, I
believe the "subsumes" comment is correct.

Comment 23

[Christian :Biesinger (don't email me, ping me on IRC)]

afaict all issues are addressed now, w/o filing new bugs. checked in.
Checking in layout/base/nsCSSFrameConstructor.cpp;
/cvsroot/mozilla/layout/base/nsCSSFrameConstructor.cpp,v  <-- 
nsCSSFrameConstructor.cpp
new revision: 1.1031; previous revision: 1.1030
done
Checking in layout/base/nsChangeHint.h;
/cvsroot/mozilla/layout/base/nsChangeHint.h,v  <--  nsChangeHint.h
new revision: 3.8; previous revision: 3.7
done
Checking in layout/style/nsStyleContext.cpp;
/cvsroot/mozilla/layout/style/nsStyleContext.cpp,v  <--  nsStyleContext.cpp
new revision: 3.251; previous revision: 3.250
done
Checking in layout/style/nsStyleStruct.cpp;
/cvsroot/mozilla/layout/style/nsStyleStruct.cpp,v  <--  nsStyleStruct.cpp
new revision: 3.115; previous revision: 3.114
done