7639 - XUL - nsXULElement::GetStyle() not implemented

Status: RESOLVED FIXED

(Core :: XUL, defect, P3)

Description

[hangas]

Using a box in a XUL file:
<box id="cvbName" align="vertical" class="CardView">
Unable to set the style on this box from JavaScript with the line:
node.style.display = "none";
The following error appears in the output window:
JavaScript error: node.style has no properties
however this does work fine with <html:div>

This is blocking Address Book UI development.

Comment 1

[Peter Trudelle]

reassigning to evaughan

Comment 2

[Eric Vaughan]

David,

This is a XUL thing. How would you do this?

Comment 3

[David Hyatt]

This isn't even implemented in our content model, seeing as how the STYLE
attribute is a property of nsIDOMHTMLElement (and therefore doesn't exist in
XUL).  I'm investigating what it will take to duplicate the HTML code over on
the XUL side (sigh).

Comment 4

[David Hyatt]

Slipping to M9.  This is not causing a crash or making someone's life miserable,
and so (given my current level of doomage), this can slip.

Comment 5

[David Hyatt]

*** Bug 8797 has been marked as a duplicate of this bug. ***

Comment 6

[David Hyatt]

Moving to M10.

Comment 7

[Chris Waterson]

*** Bug 12474 has been marked as a duplicate of this bug. ***

Comment 8

[David Hyatt]

All yours, baby.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Can I assume this will be fixed before beta for purposes of demos/documentation?

Comment 10

[Chris Waterson]

Probably not. I'd rather not implement this before beta; after beta we're going
to unify XUL and HTML ocntent model code. At which point we'll get this for
free. If there is a compelling reason that we need this for some functionality
before beta, somebody let me know.

Comment 11

[Paul MacQuiddy]

giving me rest of phillips open qa contact bugs, sorry for spam

Comment 12

[Paul MacQuiddy]

*IGNORE* - more massive spam, changing open XPToolkit bug's QA contact to
[e-mail]

Comment 13

[Paul MacQuiddy]

*IGNORE* - massive spam changing open XPToolkit bug's QA contact to
[e-mail]

Comment 14

[Chris Waterson]

doesn't this work now?

Comment 15

[John Morrison]

Attachment: testcase; setting elt.style.display for html:div and xul:box

Comment 16

[John Morrison]

It looks like this is not implemented. The attachment is a simple test
which toggles style.display = 'none|block' for html:div and xul:box. 
The xul:box causes this error in the console:

JavaScript Error: uncaught exception: [Exception... "Method not implemented"  
code: "-2147467263" nsresult: "0x80004001 (NS_ERROR_NOT_IMPLEMENTED)"  location: 
"http://bugzilla.mozilla.org/showattachment.cgi?attach_id=7086 Line: 12"]

Comment 17

[Chris Waterson]

Doh! nsXULElement::GetStyle() is most certainly unimplemented. Darn.

jst, pierre: how would you feel about me making nsDOMCSSDeclaration objects 
"publicly accessable" from layout via XPCOM factory method? I'd like to 
construct one of these and then wrap it to provide an 
nsDOMCSSAttributeDeclaration object for XUL elements.

Comment 18

[John Morrison]

Changing summary from "XUL - Unable to set box.style from JavaScript' to the 
general 'XUL - nsXULElement::GetStyle() not implemented'

Comment 19

[Pierre Saslawsky]

waterson (in response to your question from 2000-04-02): the thing that bothers 
me is that nsDOMCSSDeclaration comes with all the nsIDOMCSS2Properties while the 
interface you need (I think) is just the nsIDOMCSSStyleDeclaration.

Comment 20

[Chris Waterson]

Ok, so what do you suggest?

Comment 21

[Johnny Stenback (:jst)]

Pierre, CSS2Properties currently inherits from CSSStyleDeclaration in mozilla
but it *doesn't* do that in the current DOM Level 2 CSS spec, I was wondering
about why it does that and I think we should change that to comply with the DOM2
spec, unless there's a really good reason to have it like it is today.

Does anyone know of a good reason for having CSS2Properties inherit from
CSSStyleDeclaration, or would we gain something from *removing* the inheritance
between the interfaces? (not that I know if it's at all related to this bug tho)

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What the DOM2 spec does say is:

  A compliant implementation is not required to implement the CSS2Properties
  interface. If an implementation does implement this interface, the expectation
  is that language-specific methods can be used to cast from an instance of the
  CSSStyleDeclaration interface to the CSS2Properties interface.

which I think explains the current implementation in Mozilla.  See
http://www.w3.org/TR/DOM-Level-2/css.html#CSS-CSS2Properties

Comment 23

[Johnny Stenback (:jst)]

David, FYI:

... the expectation is that language-specific methods can be used to cast from
an instance of the CSSStyleDeclaration interface to the CSS2Properties
interface.

Means that you're supposed to get at the CSS2Properties interface by doing
QueryInreface in C++ or some automatic typecasting in JS, on the object that
implements CSSStyleDeclaration if it's supported. It does not mean that
CSS2Properties needs to, nor should, *inherit* from the CSSStyleDeclaration
interface.

We can, and IMO should, follow the DOM spec on this by changing the interface
declaration, the only downside I can see is that we'll end up with one more
vtable entry in nsDOMCSSDeclaration (ie 4 bytes more per instance).

Comment 24

[Chris Waterson]

not nsbeta2, move to M17

Comment 25

[johng]

This bug might be a blocker for alphanumerica's tool.  cc'ing johng and hangas

Comment 26

[Pete Collins (MDG)]

Actually, i've come to the conclusion that this bug might not be exactly what i
need. I need to set pseudo style rules on xul elements. I'm not worried about
doing gets right now because rules and settings for the app are stored in class
arrays. 

I need to set hover, active etc . .  . Some way.

I also talked to Waterson about this and i'm not sure there is anything around
that can do this yet.

I am going to file a new bug since i beleive it is a different problem from this
one.

pete

Comment 27

[Johnny Stenback (:jst)]

Pete, FYI: it sounds like the DOM Level 2 DocumentCSS method getOverrideStyle()
would do what you're looking for, it's unfortunately not implemented in mozilla
right now but implementing it shouldn't be too hard...

Comment 28

[pete collins]

getOverrideStyle() ?

Is :hover, :active considered override styles?

I need to "set" more than i need "get".

If so then yes that is what i am looking for.

thanks

pete

Comment 29

[pete collins]

I filed this bug:

http://bugzilla.mozilla.org/show_bug.cgi?id=45424


pete

Comment 30

[Jason Kersey]

I'm trying to use display = none to make a fullscreen mode in mozilla, and this
is keeping me from doing it.  I want to avoid using hidden = true as that would
mean caching previous values, and i don't want to have to implement that if
possible.  Any chance of movement on this?

Comment 31

[David Hyatt]

XUL 1.0 requirement.

Comment 32

[Fabian Guisset]

*** Bug 109759 has been marked as a duplicate of this bug. ***

Comment 33

[John Morrison]

*** Bug 120762 has been marked as a duplicate of this bug. ***

Comment 34

[Peter Trudelle]

Adding keyword nsbeta1 to several bugs.

Comment 35

[Peter Trudelle]

nsbeta1- per Nav triage team

Comment 36

[Joe Hewitt (gone)]

Moving non nsbeta1+ XUL 1.0 bugs to mozilla1.2

Comment 37

[Boris Zbarsky [:bzbarsky]]

*** Bug 145927 has been marked as a duplicate of this bug. ***

Comment 38

[David Hyatt]

--> default owner

Comment 39

[Greg K.]

I suggest this bug be assigned to XP Toolkit/Widgets: XUL, and also set All/All.

Comment 40

[Mark Kaplun]

Can anyone please explain to me why implementing the style attribute for XUL is
different then implementing it for HTML and SVG?

It is probably needless to say that it makes dynamic changes to XUL element
appearence almost impossible.

Comment 41

[Mark Kaplun]

A limited workaround - use the insertRule and deleteRule method of a style sheet
for example document.styleSheets[1].insertRule('label {background-color:red}',0)

Comment 42

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

This is almost all of what's necessary.  The one problem I know about is that
nsXULElement::SetInlineStyleAttribute doesn't do all the stuff that
SetAttribute does -- so XBL inheriting the style attribute, or things like
that, won't work.  It could also use a little more than the cursory testing
I've done so far.

Comment 43

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oh, also, nsXULElement stores the string for the style attribute, and this
doesn't keep that up-to-date.  I could fix both problems at once by just calling
|SetAttr|, but that would waste work re-parsing the string, and might confuse
nsDOMCSSAttrDeclaration too.

Comment 44

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Taking.

Comment 45

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Comment 46

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

I discussed this with hyatt in person, and he seemed happy.

There's a bit of changing types nsIStyleRule->nsICSSStyleRule and
nsIHTMLContent->nsIStyledContent.

Beyond that, the main interesting changes are in nsXULElement.cpp.  The
nsXULAttributes struct, for a fully-faulted nsXULElement, is increased by a
word.  We fully fault XUL elements whenever the .style property is accessed.

XUL, unlike HTML, keeps the string version and the style rule version of the
attribute, so we have to keep them in sync when it's set via element.style. 
Thus some of SetAttr() is duplicated, and some is refactored.  Probably some of
the stuff I do in nsXULElement::SetInlineStyleRule could be conditioned on
|aNotify| or on the presence of mutation events, but I suspect the performance
costs of the things I do there are negligible enough that it won't matter.

Comment 47

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oh.  I forgot to turn the two "XXX" comments in nsXULElement::SetInlineStyleRule
into non-"XXX" comments.

Comment 48

[Boris Zbarsky [:bzbarsky]]

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

>Index: xul/content/src/nsXULElement.cpp

>+    // XXX Need to fix the copy stored as a string too!

Why is this an XXX comment?  Do we want to stop doing that at some point?  Or
just come up with a better way to do it?  Maybe clarify?

>+        nsCOMPtr<nsINodeInfo> ni;
>+        nimgr->GetNodeInfo(nsXULAtoms::style, nsnull, kNameSpaceID_None,
>+                           getter_AddRefs(ni));

This call could fail....

With those nits picked, r+sr=bzbarsky.	It's a good thing we have immutable
style rules, eh?  What with that prototype stuff... ;)

Comment 49

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to trunk, 2003-10-29 17:40 -0800.

Note that it's not advisable for XUL authors concerned about performance /
memory use to use this extensively since it causes the XUL element to become
fully heavyweight (which is the same memory cost as setting an attribute through
the DOM).

Comment 50

[[e-mail]]

No DOM Inspector changes required, right?