Closed
Bug 196603
Opened 22 years ago
Closed 22 years ago
devedge.netscape.com - menus and "customize" do not work in recent nightlies
Categories
(Core :: CSS Parsing and Computation, defect, P1)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla1.4alpha
People
(Reporter: marcoos, Assigned: dbaron)
References
()
Details
(Keywords: regression, testcase, Whiteboard: [patch])
Attachments
(6 files)
716 bytes,
text/html
|
Details | |
1.09 KB,
text/html
|
Details | |
840 bytes,
text/html
|
Details | |
14.76 KB,
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
6.51 KB,
patch
|
Details | Diff | Splinter Review | |
7.40 KB,
patch
|
Details | Diff | Splinter Review |
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.
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 http://devedge.netscape.com/
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 (
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.
Comment 1•22 years ago
|
||
WFM branch build:
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.3) Gecko/20030308
Reporter | ||
Comment 2•22 years ago
|
||
Yes, I forgot to mention here, that this problem exists only in 1.4a trunk
builds, not in the 1.3 branch.
Comment 3•22 years ago
|
||
confirmed with linux turnk 20030308
regression bewteen linux trunk 2003030605 and 2003030705
==> Layout
Assignee: asa → other
Status: UNCONFIRMED → NEW
Component: Browser-General → Layout
Ever confirmed: true
Keywords: regression
QA Contact: asa → ian
Comment 4•22 years ago
|
||
mousing over "International" works the first time, mousing out hides it, but
after that it stops working.
Comment 6•22 years ago
|
||
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....
Comment 7•22 years ago
|
||
backing out bug 171830 fixed the URL and testcase
Assignee | ||
Comment 8•22 years ago
|
||
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?
Comment 9•22 years ago
|
||
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
properly...
Another possibility is that the hint from step #2 _is_ being processed fully,
but just not requesting all the structs that it probably should....
Comment 10•22 years ago
|
||
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
it....
Comment 11•22 years ago
|
||
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")
Comment 12•22 years ago
|
||
with this testcase, it should be easier to see what is and is not happening.
Comment 13•22 years ago
|
||
Thanks! Yeah, I see the problem with the first testcase if I go up.
Will try to debug some more later this week....
Comment 14•22 years ago
|
||
Seeing the problem with build 2003031208 on WinXP too, changing OS to ALL
OS: Linux → All
Comment 15•22 years ago
|
||
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.
Notes:
1) I can reproduce the bug with visibility or -moz-opacity, but not with
color;
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,
vis->CalcDifference(*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:
94
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....
Comment 17•22 years ago
|
||
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+
Comment 18•22 years ago
|
||
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+
Comment 19•22 years ago
|
||
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"
optimization.
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
NS_STYLE_HINT_NONE.
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).
Comment 20•22 years ago
|
||
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
GetStyleData.
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
Assignee | ||
Comment 21•22 years ago
|
||
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?
Comment 22•22 years ago
|
||
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.
Assignee | ||
Comment 23•22 years ago
|
||
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
Assignee | ||
Comment 24•22 years ago
|
||
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
typing.
Assignee | ||
Comment 25•22 years ago
|
||
Actually, hold on, there's a slightly better way to do this...
Assignee | ||
Comment 26•22 years ago
|
||
Actually, never mind.
Assignee | ||
Updated•22 years ago
|
Attachment #117333 -
Flags: superreview?(bzbarsky)
Attachment #117333 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 27•22 years ago
|
||
But when I check in I should remember to add the accessor to nsStyleSVG, since
for some reason it doesn't have one.
Assignee | ||
Updated•22 years ago
|
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]
patch
nice! But I also wonder about the portability of the explicit template
parameter.
Attachment #117333 -
Flags: superreview?(roc+moz)
Attachment #117333 -
Flags: superreview+
Attachment #117333 -
Flags: review?(roc+moz)
Attachment #117333 -
Flags: review+
Assignee | ||
Comment 29•22 years ago
|
||
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.
Assignee | ||
Comment 30•22 years ago
|
||
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
it).
Assignee | ||
Comment 31•22 years ago
|
||
I used the potential bustage fix since IRIX and OS/2 VACPP didn't like the other
way.
Assignee | ||
Comment 32•22 years ago
|
||
And from the codesize diffs it looks like VC++ (Windows) compiled it but did the
wrong thing.
Assignee | ||
Comment 33•22 years ago
|
||
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).
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 34•22 years ago
|
||
Is this better or worse? It probably messes up debuggers more, but it should
reduce the codesize penalty...
Assignee | ||
Updated•22 years ago
|
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
Assignee | ||
Comment 37•22 years ago
|
||
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 38•22 years ago
|
||
Comment on attachment 117359 [details] [diff] [review]
macro-ization
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.
/be
Assignee | ||
Comment 39•22 years ago
|
||
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.
Description
•