User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4a) Gecko/20030308
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4a) Gecko/20030308
The redesigned devedge.netscape.com does not work properly in recent nightlies.
pull down menus and to display a <div> with the option to change stylesheet.
This worked in 20030303 nightly build of Mozilla, and does not work
in the following builds:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4a) Gecko/20030308
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3b) Gecko/20030307 Phoenix/0.5
Steps to Reproduce:
1. Go to http://devedge.netscape.com/
2. Hover the "Customize" button  or the menu ("View Source | Archive | Tech
Show the <div> with option to change the stylesheet 
or display the contents of the menu .
I was also notified by MozillaZine readers (
http://www.mozillazine.org/forums/viewtopic.php?t=7089 ), that this problem is
reproducible also in:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3b) Gecko/20030308 Phoenix/0.5
but the page works fine in
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.4a) Gecko/20030306
so this has become broken probably on March 7th or March 8th.
WFM branch build:
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.3) Gecko/20030308
Yes, I forgot to mention here, that this problem exists only in 1.4a trunk
builds, not in the 1.3 branch.
confirmed with linux turnk 20030308
regression bewteen linux trunk 2003030605 and 2003030705
Created attachment 116737 [details]
mousing over "International" works the first time, mousing out hides it, but
after that it stops working.
regression from bug 171830?
Hmm.. ok, I see the problem on the devedge site. I have to try very hard to get
the testcase to reproduce it (have to move over to the right, not down; then
sometimes the mouseout fires without me actually mousing out of the <p> and the
ccing jkeiser in case he knows why the mouseout fires when it should not.....
In any case, I'm not likely to be able to seriously debug this for at least a
few more days....
backing out bug 171830 fixed the URL and testcase
The patch for bug 171830 *should* have changed what happens here from a reflow
to a repaint -- but could it somehow have caused a reframe instead?
OK, so I think I sorta know what's going on... Here's what I see when I stick
some printfs in the code (the ParsePropertyValue is in
nsDOMCSSAttrDeclaration.cpp, the other is in the frame constructor; MaxHint is
the hint the reresolve returned).
ParsePropertyValue 'padding-bottom' to '0' Old Value '0pt'
ParsePropertyValue 'left' to '6px' Old Value '6px'
ParsePropertyValue 'top' to '22px' Old Value '22px'
ParsePropertyValue 'visibility' to 'visible' Old Value 'hidden'
So the problem is in fact that we just don't repaint that area (going to a
different virtual terminal and coming back make the menu show up fine).
If I change nsStyleContext::CalcStyleDifference to use GetStyleData throughout
instead of using PeekStyleData, I get the right change hints and the right behavior.
What I suspect is happening is this.
1) We start out with a style context (Context1).
2) We change some inline style, creating Context2; diff context1 and context2
and _stop_ at the point when we find what the difference is (when the
optimizations in CalcStyleDifference kick in). We post a reflow or
invalidate or something for the frame.
3) Before this is processed, we change some more inline style creating
Context3; we diff context2 and context3. Now the reflow/repaint/whatever
has not happened yet, so we only have the style structs on context2 that
were created in step #2. None of those structs changed, so we move on down
the list and see no change at all.
E.g. if step #2 triggered a reflow and step #3 triggered a reflow or repaint
further down the list, the second reflow or repaint would not get processed
Another possibility is that the hint from step #2 _is_ being processed fully,
but just not requesting all the structs that it probably should....
Needless to say, a minimal testcase that demonstrates the problem consistently
(as the page does) would help immensely in debugging this.... ;) I'll try to
construct one tomorrow, but if someone has time before that and is willing to do
the testcase works consistently for me. The one thing to watch out for is that
if you mouseover the hidden element (Français), it will become visible. Also
the mouseover area is 100% width, so mouseover left-right is not real effective
unless your mouse actually leaves the Mozilla window. Coming in from the top
and then leaving through the top triggers the bug every time for me.
(I'm a bit confused by "have to move over to the right, not down")
Created attachment 116836 [details]
testcase that's more clear
with this testcase, it should be easier to see what is and is not happening.
Thanks! Yeah, I see the problem with the first testcase if I go up.
Will try to debug some more later this week....
Seeing the problem with build 2003031208 on WinXP too, changing OS to ALL
Created attachment 117059 [details]
OK, this one is probably about minimal...
This testcase, and others in this bug, fail even if I change from PeekStyleData
to GetStyleData, though that change fixes the devedge site.
1) I can reproduce the bug with visibility or -moz-opacity, but not with
I'm betting that's because the color struct is after the padding struct in
the order we compute changes in.
2) When I add a printf to the point right after I get the "otherVis" struct in
nsStyleContext::CalcStyleDifference as follows:
fprintf(stderr, "this: %p, other: %p, vis: %p, othervis: %p, diff: %d\n",
this, aOther, vis, otherVis,
I get the following when I mouseover the text:
this: 0x876c040, other: 0x876c5f4, vis: 0x862cfa8, othervis: 0x862cfa8, diff: 0
this: 0x876c768, other: 0x876c140, vis: 0x876c200, othervis: 0x876c7c8, diff: 0
and the following when I mouseout:
this: 0x876c140, other: 0x876c794, vis: 0x876c7c8, othervis: 0x876c200, diff:
If I take out the line that sets the padding, I instead get:
this: 0x87658e0, other: 0x8765d44, vis: 0x8765860, othervis: 0x8765ca4, diff:
this: 0x8765d44, other: 0x8765834, vis: 0x8765ca4, othervis: 0x8765860, diff:
It may be that caching the style structs themselves somewhere in the ruletree,
so that even though the content gets a new style rule and hence a new
rulenode/stylecontext it's ending up with the same struct as it used to
have.... Not sure why that's breaking things, though....
Setting platform to All (Mac/OS X)
I'm seeing this too. What I can do is hover over the menu(download etc) and
press ctrl+ -/= and the menu drops down, but when I go over them, they
for the moment.
You can't set that flag if you're not a driver. Or rather you can, but you'll
David, do you have any idea what's up here? I may be able to debug this on
Tuesday a bit, but then I'm gone till early April...
OK, here's what's going on:
1) We change padding on the parent node. This triggers a style reresolve down
the content/frame tree from that point. All the kids get new style
contexts, but none of those style contexts get any structs, because of the
"it's in the same position in the rule tree and has the same rulenode"
2) Since the change hint comes out to NS_STYLE_HINT_NONE, there is no actual
reflow or anything, so those structs remain unfilled (before bug 171830 the
hint here would have been REFLOW).
3) Another change comes along (for visibility) targeted at one of the kids and
the structs are either null (since they don't exist) or get computed based
on the nsCSSDeclaration. But the nsCSSDeclaration has _changed_ (since we
don't clone the decl). So our change hint here is always
Removing the optimization in item #1 fixes this bug (since the structs are now
filled). Just cloning the decl fixes some testcases, but not all. Just using
GetStyleData instead of PeekStyleData fixes some testcases, but not all.
Cloning the decl _and_ using GetStyleData does fix all the testcases (since it
forces the creation of structs with the right style data in them).
Ho-hum. So thoughts? Note that we've always ended up in this situation where
style structs are blown away from child contexts by a parent's inline style
changing and things are broken thereafter; it just didn't affect inline style
changes, since those didn't depend on the resolved change hint (but _did_
affect, eg, :hover, alternate sheet switches, etc).
So to sum up the situation, there are three possible approaches to the fix that
I can think of:
1) Go back to doing a reflow even in cases when it's not really needed
2) Make sure the new style contexts created for the kids in Step 1 of comment
19 have all the needed style structs. One way would be to change the
optimization to always GetStyleData if PeekStyleData returned something but
to only call CalcDifference if the rulenodes are different.
3) Make sure that we don't touch the old style data in any way (Clone() the
nsCSSDeclaration in inline style changes) and replace PeekStyleData with
I'm not actually sure which of these is most performant..... My personal
preference lies with #2, sort of, but real style rule immutability would sorta
point to #3.... Of course then cloning an nsCSSDeclaration needs to be cheap.
I think I prefer #2 as well. (Doesn't that just involve removing the
"break-out" optimization in CalcStyleDifference by moving the NS_IsHintSubset
tests in with the |struct != otherStruct| test?) How does that disagree with
style rule immutability?
Actually, it means combining the mRuleNode == aOther.mRuleNode test with the
struct != otherstruct test.
We could move the hint subset stuff there too, but if that test fails then we'll
be doing reflow or whatever and reconstructing the structs anyway (or so the
theory goes). It's safer to combine all three tests, of course....
The immutability issue is that in this case we _are_ mutating the old
nsIStyleRule. But I guess it does not matter since we also stop pointing to it
once the re-resolve completes...
OK, so let's go with #2. David, will you have time to do this in the next day
or two? If not, I can implement it on Tuesday.
I'll attach a patch to do #2 shortly (once my build finishes and I can test it).
Created attachment 117333 [details] [diff] [review]
OK, this is fun. If I have porting trouble with this, I'll add a bogus 6th
parameter to the inline function for type deduction and get rid of the explicit
Actually, hold on, there's a slightly better way to do this...
Actually, never mind.
But when I check in I should remember to add the accessor to nsStyleSVG, since
for some reason it doesn't have one.
Comment on attachment 117333 [details] [diff] [review]
nice! But I also wonder about the portability of the explicit template
We use the trick of adding an extra parameter to template functions all over in
the string code, so it clearly works on our set of compilers.
Created attachment 117356 [details] [diff] [review]
potential bustage fix
Here's what I'd do if there are problems with what I checked in (in case some
obscure port breaks in the middle of the night after 5 hours thinking about
I used the potential bustage fix since IRIX and OS/2 VACPP didn't like the other
And from the codesize diffs it looks like VC++ (Windows) compiled it but did the
Well, fix checked in to trunk (although given the state of optimization on
current C++ compilers, judging from codesize logs, I'm tempted to just convert
this to macros).
Created attachment 117359 [details] [diff] [review]
Is this better or worse? It probably messes up debuggers more, but it should
reduce the codesize penalty...
This is ugly ... are you going to just check this in to see what it does to code
size on various platforms?
I don't think you need to whack the names of 'this' and 'other', since they're
scoped to the enclosing block. That would reduce the ugliness a tiny bit.
The code looks good though, r+sr etc
I vaguely remember some compiler not liking variables of the same name across
scoped blocks. It's a pretty vague memory, though.
I'm pretty sure it will improve codesize. It looks like Visual C++ isn't inlining.
Comment on attachment 117359 [details] [diff] [review]
It's better style to use PR_BEGIN_MACRO and PR_END_MACRO instead of braces --
then you can use ; after each call, even in an unbraced then statement between
if (condition) and else.
bryner said Windows builds were having serious problems even with the second
patch (I presume due to some compiler stupidity), so I checked in the macroization.