Closed Bug 196603 Opened 21 years ago Closed 21 years ago - menus and "customize" do not work in recent nightlies


(Core :: CSS Parsing and Computation, defect, P1)






(Reporter: marcoos, Assigned: dbaron)




(Keywords: regression, testcase, Whiteboard: [patch])


(6 files)

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 does not work properly in recent nightlies.
It uses Eric Meyer's CSS menu, but modified with some JavaScript to show the
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 

Reproducible: Always

Steps to Reproduce:
1. Go to
2. Hover the "Customize" button [1] or the menu ("View Source | Archive | Tech
Centrall") [2]

Actual Results:  
Nothing happens.

Expected Results:  
Show the <div> with option to change the stylesheet [1]
or display the contents of the menu [2].

I was also notified by MozillaZine readers ( ), 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

==> Layout
Assignee: asa → other
Component: Browser-General → Layout
Ever confirmed: true
Keywords: regression
QA Contact: asa → ian
Attached file testcase
mousing over "International" works the first time, mousing out hides it, but
after that it stops working.
regression from bug 171830?
Keywords: testcase
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
bug appears).

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'
MaxHint: 14
ParsePropertyValue 'left' to '6px' Old Value '6px'
MaxHint: 14
ParsePropertyValue 'top' to '22px' Old Value '22px'
MaxHint: 14
ParsePropertyValue 'visibility' to 'visible' Old Value 'hidden'
MaxHint: 14

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")
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
OS: Linux → All
Attached file smaller testcase
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:
94 (mouseover)
this: 0x8765d44, other: 0x8765834, vis: 0x8765ca4, othervis: 0x8765860, diff:
94 (mouseout)

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)
Hardware: PC → All
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
dissapear. This problem also dissapears if I disable javascript for navigator
for the moment.
Flags: blocking1.4a+
You can't set that flag if you're not a driver.  Or rather you can, but you'll
be ignored.

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...
Flags: blocking1.4a+
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.
Depends on: 171830
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).
Assignee: other → dbaron
Component: Layout → Style System
Priority: -- → P1
Whiteboard: [patch]
Target Milestone: --- → mozilla1.4alpha
Attached patch patchSplinter 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...
Attachment #117333 - Flags: superreview?(bzbarsky)
Attachment #117333 - Flags: review?(bzbarsky)
But when I check in I should remember to add the accessor to nsStyleSVG, since
for some reason it doesn't have one.
Attachment #117333 - Flags: superreview?(roc+moz)
Attachment #117333 - Flags: superreview?(bzbarsky)
Attachment #117333 - Flags: review?(roc+moz)
Attachment #117333 - Flags: review?(bzbarsky)
Comment on attachment 117333 [details] [diff] [review]

nice! But I also wonder about the portability of the explicit template
Attachment #117333 - Flags: superreview?(roc+moz)
Attachment #117333 - Flags: superreview+
Attachment #117333 - Flags: review?(roc+moz)
Attachment #117333 - Flags: review+
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.
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
wrong thing.
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).
Closed: 21 years ago
Resolution: --- → FIXED
Attached patch macro-izationSplinter Review
Is this better or worse?  It probably messes up debuggers more, but it should
reduce the codesize penalty...
Attachment #117359 - Flags: superreview?(roc+moz)
Attachment #117359 - Flags: review?(roc+moz)
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.
Attachment #117359 - Flags: superreview?(roc+moz)
Attachment #117359 - Flags: review?(roc+moz)
You need to log in before you can comment on or make changes to this bug.