34400 - [skins/xbl] behavior property doesn't work with <window> tag

Status: VERIFIED FIXED

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

Description

[David Hyatt]

If you try to attach behavior to the <window> tag it fails.  The new style
context that is built for the gfx scroll frame somehow loses the information.  
Seems to be a problem with the scroll frame's style resolution.

Comment 1

[David Hyatt]

To test, you can add a style rule for window...

window {
  behavior: url(whatever.you.want);
}

Then break in nsCSSFrameConstructor at COnstructDocElementFrame... the 
styleContext prior to BeginBuildScrollFrame will have, in its mUserInterface 
struct, the correct value for mBehavior.

After you call BuildScrollFrame, though, and set styleContext = newContext, then 
mBehavior is now empty.  Something is going wrong.

Pierre, if you have any ideas, or can help out, it would be much appreciated.

Comment 2

[Peter Trudelle]

Adding 'skins' keyword to appropriate bugs en masse, sorry about any 
mistakes...

Comment 3

[Pierre Saslawsky]

I put a breakpoint on BeginBuildingScrollFrame() inside 
ConstructDocElementFrame() but I never hit it. I tried pages with list elements 
and dropdown lists too. Could you describe more exactly the steps to reproduce?

Comment 4

[David Hyatt]

I wonder if it's just for XUL... hmmm... 

If you go to resource:/res/samples/widgets.xul and MANUALLY EDIT the file
to add overflow: auto to the <window> tag (just use inline style), you should
hit the breakpoint.

Comment 5

[David Hyatt]

You can also edit the file and put a behavior: on there and watch how it is gone
on the new context.

Comment 6

[Pierre Saslawsky]

I have a fresh tree (around 9pm) and I applied the following patch:

Index: widgets.xul
===================================================================
RCS file: /m/pub/mozilla/xpfe/browser/samples/widgets.xul,v
retrieving revision 1.2
diff -r1.2 widgets.xul
33c33
<         style="width: 250px;"
---
>         style="width: 250px; overflow:auto; behavior: url(whatever.you.want);"

Then I put a breakpoint in nsCSSFrameConstructor.cpp line 3219, which is the call 
to BeginBuildingScrollFrame() inside ConstructDocElementFrame(), launched the 
app, went to the Bugzilla Query page and played with some widgets but I still did 
not hit the breakpoint.

Please make sure you have a clean tree and send me the patch to apply in 
widgets.xul.

Comment 7

[Peter Trudelle]

Should this bug be targetted for M16 as part of the skinnability push for B2?  
Could you please set the target so we know whether to expect it?

Comment 8

[Peter Trudelle]

Mass-adding beta2 keyword to all skins bugs.

Comment 9

[Phil Peterson]

PDT would like to know what required beta2 work this blocks.

Comment 10

[Peter Trudelle]

cc hangas. still need info on what this blocks.

Comment 11

[leger]

Putting on [nsbeta2-] radar.  

Comment 12

[Peter Trudelle]

Mass-moving nsbeta2- bugs to M20

Comment 13

[Pierre Saslawsky]

This bug looks completely dead. It was opened one day, almost 2 months ago, in 
the middle of the night. Comments shows that it could not be reproduced during 
the next couple of hours and ever since only the managers have added comments 
wondering what to do with the goddamm thing. Reassigned to the reporter :-)

Comment 14

[Peter Trudelle]

need info: is this bug still a problem?  We need to either nominate for nsbeta3
(with a compelling argument as to why we should plus it) or retarget to Future.

Comment 15

[Peter Trudelle]

->future

Comment 16

[David Hyatt]

Attachment: Patch to make XBL work with the root element of a doc.

Comment 17

[David Hyatt]

Wasn't CSS' fault as it turns out.  (Go CSS!) :)

All that was wrong here is that the XBL code needed to be fixed to match the
same ordering that's used for normal frames.  Here's the patch.

Looking for r= and a= for trunk landing.

Comment 18

[Brendan Eich [:brendan]]

+  const nsStyleUserInterface* ui= (const nsStyleUserInterface*)
+        styleContext->GetStyleData(eStyleStruct_UserInterface);

NS_STATIC_CAST, please.  I know, the other casts in there are old C-style.  It 
still seems worthwhile to plant a good seed.

Could you say more about why it's important to CreateAnonymousFrames before 
SetInitialChildList here:

+     // Create any anonymous frames the doc element frame requires
+    CreateAnonymousFrames(aPresShell, aPresContext, nsnull, aState, 
aDocElement, contentFrame,
+                          childItems);
+
     // Set the initial child lists
     contentFrame->SetInitialChildList(aPresContext, nsnull,
                                       childItems.childList);
  
-    // Create any anonymous frames the doc element frame requires
-    CreateAnonymousFrames(aPresShell, aPresContext, nsnull, aState, 
aDocElement, contentFrame,
-                          childItems);

Looks ok to me otherwise, but I'm new to nsCSSFrameConstructor.cpp, which is WAY 
too big for me to review and understand.  How about getting waterson to bless 
your change too?

/be

Comment 19

[David Hyatt]

Sure.  XBL hands you back a new child list of anonymous nodes and might even
move the explicit children (from the original list) down underneath the
anonymous nodes.  Because I had the ordering wrong, all anonymous frames were
being lost, since I'd set the list before they were even made!

Comment 20

[Brendan Eich [:brendan]]

Ok, thanks -- that makes sense, I needed to wrap my brain around this top-level 
contentFrame case.

cc'ing waterson for ultimate final sr=.  Man is that file big!  I don't feel so 
bad about js_Interpret now.

/be

Comment 21

[David Hyatt]

r=waterson.

Comment 22

[David Hyatt]

Fixed.

Comment 23

[Christine Hoffman]

Verified per reporter comments