Closed Bug 283366 Opened 20 years ago Closed 20 years ago

It should be possible to define the name of the 'class' attribute in XTF

Categories

(Core Graveyard :: XTF, defect)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: smaug, Assigned: smaug)

References

Details

Attachments

(2 files, 2 obsolete files)

This is needed in XForms:
"A host language is expected to add attributes such as xml:lang as well as an 
attribute, named class, that holds a list of strings that can be matched by CSS 
class selectors."
Attached patch v.1 (obsolete) — Splinter Review
Blocks: 283219
Comment on attachment 175360 [details] [diff] [review]
v.1

Maybe bryner could say something about this.
Attachment #175360 - Flags: review?(bryner)
Status: NEW → ASSIGNED
Comment on attachment 175360 [details] [diff] [review]
v.1

>Index: content/xtf/src/nsXTFVisualWrapper.cpp

>+nsIAtom *
>+nsXTFVisualWrapper::GetClassAttributeName() const
>+{
>+  nsCOMPtr<nsIAtom> clazz;
>+  GetXTFVisual()->GetClassAttributeName(getter_AddRefs(clazz));
>+  return clazz;
>+}

What's keeping clazz from being deallocated the moment this function returns? 
Should the wrapper hold a ref to the atom or something?

>+
>+const nsAttrValue*
>+nsXTFVisualWrapper::GetClasses() const
>+{
>+  const nsAttrValue* val = nsnull;
>+  nsCOMPtr<nsIAtom> clazzAttr = GetClassAttributeName();
>+  if (clazzAttr) {
>+    val = mAttrsAndChildren.GetAttr(clazzAttr);
>+    // This is possibly the first time we need any classes.
>+    if (val && val->Type() == nsAttrValue::eString) {
>+      nsAutoString value;
>+      val->ToString(value);
>+      nsAttrValue newValue;
>+      newValue.ParseAtomArray(value);
>+      NS_CONST_CAST(nsAttrAndChildArray*, &mAttrsAndChildren)->
>+        SetAndTakeAttr(clazzAttr, newValue);
>+    }
>+  }
>+  return val;
>+}
>+
>+NS_IMETHODIMP_(PRBool)
>+nsXTFVisualWrapper::HasClass(nsIAtom* aClass, PRBool /*aCaseSensitive*/) const
>+{
>+  const nsAttrValue* val = GetClasses();
>+  if (val) {
>+    if (val->Type() == nsAttrValue::eAtom) {
>+      return aClass == val->GetAtomValue();
>+    }
>+
>+    if (val->Type() == nsAttrValue::eAtomArray) {
>+      return val->GetAtomArrayValue()->IndexOf(aClass) >= 0;
>+    }
>+  }
>+  return PR_FALSE;
>+}
>+
>Index: content/xtf/src/nsXTFVisualWrapper.h
>===================================================================
>RCS file: /cvsroot/mozilla/content/xtf/src/nsXTFVisualWrapper.h,v
>retrieving revision 1.3
>diff -u -8 -p -r1.3 nsXTFVisualWrapper.h
>--- content/xtf/src/nsXTFVisualWrapper.h	1 Dec 2004 22:26:49 -0000	1.3
>+++ content/xtf/src/nsXTFVisualWrapper.h	23 Feb 2005 21:32:23 -0000
>@@ -58,16 +58,21 @@ public:
> 
>   // nsIXTFVisualWrapperPrivate
>   NS_IMETHOD CreateAnonymousContent(nsPresContext* aPresContext,
>                                     nsISupportsArray& aAnonymousItems);
>   virtual void DidLayout();
>   virtual void GetInsertionPoint(nsIDOMElement** insertionPoint);
>   virtual PRBool ApplyDocumentStyleSheets();
>   
>+  // nsIStyledContent
>+  virtual nsIAtom *GetClassAttributeName() const;
>+  virtual const nsAttrValue* GetClasses() const;
>+  NS_IMETHOD_(PRBool) HasClass(nsIAtom* aClass, PRBool aCaseSensitive) const;
>+
> protected:
>   // to be implemented by subclasses:
>   virtual nsIXTFVisual *GetXTFVisual() const = 0;
>   
>   nsCOMPtr<nsIDOMElement> mVisualContent;
> };
> 
> #endif // __NS_XTFVISUALWRAPPER_H__
>Index: extensions/xforms/nsXFormsAtoms.h
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsAtoms.h,v
>retrieving revision 1.11
>diff -u -8 -p -r1.11 nsXFormsAtoms.h
>--- extensions/xforms/nsXFormsAtoms.h	1 Feb 2005 19:38:00 -0000	1.11
>+++ extensions/xforms/nsXFormsAtoms.h	23 Feb 2005 21:32:27 -0000
>@@ -58,14 +58,15 @@ class nsXFormsAtoms
>   static NS_HIDDEN_(nsIAtom *) messageProperty;
>   static NS_HIDDEN_(nsIAtom *) ref;
>   static NS_HIDDEN_(nsIAtom *) nodeset;
>   static NS_HIDDEN_(nsIAtom *) model;
>   static NS_HIDDEN_(nsIAtom *) selected;
>   static NS_HIDDEN_(nsIAtom *) appearance;
>   static NS_HIDDEN_(nsIAtom *) incremental;
>   static NS_HIDDEN_(nsIAtom *) value;
>+  static NS_HIDDEN_(nsIAtom *) clazz;
> 
>   static NS_HIDDEN_(void) InitAtoms();
> 
>  private:
>   static NS_HIDDEN_(const nsStaticAtom) Atoms_info[];
> };
>Index: extensions/xforms/nsXFormsAtoms.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsAtoms.cpp,v
>retrieving revision 1.10
>diff -u -8 -p -r1.10 nsXFormsAtoms.cpp
>--- extensions/xforms/nsXFormsAtoms.cpp	1 Feb 2005 19:38:00 -0000	1.10
>+++ extensions/xforms/nsXFormsAtoms.cpp	23 Feb 2005 21:32:27 -0000
>@@ -53,16 +53,17 @@ nsIAtom* nsXFormsAtoms::uploadFileProper
> nsIAtom* nsXFormsAtoms::messageProperty;
> nsIAtom *nsXFormsAtoms::ref;
> nsIAtom *nsXFormsAtoms::value;
> nsIAtom *nsXFormsAtoms::nodeset;
> nsIAtom *nsXFormsAtoms::model;
> nsIAtom *nsXFormsAtoms::selected;
> nsIAtom *nsXFormsAtoms::appearance;
> nsIAtom *nsXFormsAtoms::incremental;
>+nsIAtom *nsXFormsAtoms::clazz;
> 
> const nsStaticAtom nsXFormsAtoms::Atoms_info[] = {
>   { "src",                      &nsXFormsAtoms::src },
>   { "bind",                     &nsXFormsAtoms::bind },
>   { "type",                     &nsXFormsAtoms::type },
>   { "readonly",                 &nsXFormsAtoms::readonly },
>   { "required",                 &nsXFormsAtoms::required },
>   { "relevant",                 &nsXFormsAtoms::relevant },
>@@ -73,16 +74,17 @@ const nsStaticAtom nsXFormsAtoms::Atoms_
>   { "UploadFileProperty",       &nsXFormsAtoms::uploadFileProperty },
>   { "messageProperty",          &nsXFormsAtoms::messageProperty },
>   { "ref",                      &nsXFormsAtoms::ref },
>   { "value",                    &nsXFormsAtoms::value },
>   { "nodeset",                  &nsXFormsAtoms::nodeset },
>   { "model",                    &nsXFormsAtoms::model },
>   { "selected",                 &nsXFormsAtoms::selected },
>   { "appearance",               &nsXFormsAtoms::appearance },
>-  { "incremental",              &nsXFormsAtoms::incremental }
>+  { "incremental",              &nsXFormsAtoms::incremental },
>+  { "class",                    &nsXFormsAtoms::clazz }
> };
> 
> void
> nsXFormsAtoms::InitAtoms()
> {
>   NS_RegisterStaticAtoms(Atoms_info, NS_ARRAY_LENGTH(Atoms_info));
> }
>Index: extensions/xforms/nsXFormsStubElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsStubElement.cpp,v
>retrieving revision 1.7
>diff -u -8 -p -r1.7 nsXFormsStubElement.cpp
>--- extensions/xforms/nsXFormsStubElement.cpp	28 Jan 2005 03:49:44 -0000	1.7
>+++ extensions/xforms/nsXFormsStubElement.cpp	23 Feb 2005 21:32:28 -0000
>@@ -37,16 +37,17 @@
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsXFormsStubElement.h"
> #include "nsIDOMElement.h"
> #include "nsIDOMEventTarget.h"
> #include "nsIDOM3Node.h"
> #include "nsMemory.h"
> #include "nsXFormsUtils.h"
>+#include "nsXFormsAtoms.h"
> 
> static const nsIID sScriptingIIDs[] = {
>   NS_IDOMELEMENT_IID,
>   NS_IDOMEVENTTARGET_IID,
>   NS_IDOM3NODE_IID
> };
> 
> // nsXFormsStubElement implementation
>@@ -369,16 +370,23 @@ nsXFormsXMLVisualStub::GetApplyDocumentS
> 
> NS_IMETHODIMP
> nsXFormsXMLVisualStub::DidLayout()
> {
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
>+nsXFormsXMLVisualStub::GetClassAttributeName(nsIAtom** aName)
>+{
>+  *aName = nsXFormsAtoms::clazz;
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
> nsXFormsXMLVisualStub::OnCreated(nsIXTFXMLVisualWrapper *aWrapper)
> {
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
> nsXFormsXMLVisualStub::CloneState(nsIDOMElement *aElement)
> {
Attachment #175360 - Flags: review?(bryner)
Attached patch v1.1 (obsolete) — Splinter Review
This holds a ref to the atom.
Attachment #175360 - Attachment is obsolete: true
Attached patch v1.2Splinter Review
this doesn't break so many things.
Attachment #175562 - Attachment is obsolete: true
Attachment #175569 - Flags: review?(bryner)
Attachment #175569 - Flags: superreview?(bzbarsky)
Comment on attachment 175569 [details] [diff] [review]
v1.2

>Index: content/xtf/src/nsXTFVisualWrapper.cpp
>+nsXTFVisualWrapper::Init()
>+{
>+  nsXTFVisualWrapperBase::Init();

What if this fails?  We need to propagate that error.  Also, does the
superclass init _have_ to be called before GetClassAttributeName() here?  If
so, document that.

>+nsXTFVisualWrapper::GetClassAttributeName() const
>+{

Add some debug code here that assets that whatever
GetXTFVisual()->GetClassAttributeName() is returning right now is the same as
mClassAttributeName.

sr=bzbarsky with those fixed.
Attachment #175569 - Flags: superreview?(bzbarsky) → superreview+
There is one small issue with your patch:

XTF has 2 models for storing attributes, depending on
nsIXTFElement::isAttributeHandler. 
If nsIXTFElement::isAttributeHandler is 'false', then all attributes are handled
by the wrapper (i.e. they are stored in mAttrsAndChildren), and your patch works
perfectly. 
If, however, nsIXTFElement::isAttributeHandler is 'true', the xtf element
signifies that it wants to handle some of its attributes directly via the
nsIXTFAttributeHandler interface. 
This is e.g. useful to map some attributes directly onto the xtf element's
visual content. E.g. you could have an xtf element 'mywidget' which contains a
<xul:box> as visual content. When you set <mywidget style="..."/>, you might
want the style attribute to be handled directly by the <xul:box>.
The problem with your patch is that nsXTFVisualWrapper::GetClasses() assumes
that the class attribute is stored by the wrapper. To make the code correct for
the case where the class attribute is handled by the xtf element itself, you
could change GetClasses() to something like this:
  if (HandledByInner(clazzAttr)) {
    nsAutoString str;
    nsresult rv = mAttributeHandler->GetAttribute(aName, str);
    nsAttrValue retval;
    retval.ParseAtomArray(value);
    return retval;
  }
  else {
    ... your code ...
  }

Unfortunately this is not very efficient as str needs to be parsed every time,
but at least it makes your code work for all cases. 

Also, how about lazily initializing mClassAttributeName in
GetClassAttributeName() instead of setting it in Init()?
(In reply to comment #7)
> There is one small issue with your patch:
> 
> XTF has 2 models for storing attributes, depending on
> nsIXTFElement::isAttributeHandler. 

Of course, thanks for reminding me. I'll update the patch.
>    nsAttrValue retval;
>    retval.ParseAtomArray(value);
>    return retval;

This doesn't work.  It returns the wrong data type.
(In reply to comment #9)
> >    nsAttrValue retval;
> >    retval.ParseAtomArray(value);
> >    return retval;
> 
> This doesn't work.  It returns the wrong data type.
> 

Damn. Very right. Looks like the next best option is to keep GetClasses as it is
and just add a big comment in the idl to the effect that class attributes cannot
be served through nsIXTFAttributeHandler.
Attached patch v1.3Splinter Review
>Also, how about lazily initializing mClassAttributeName in
>GetClassAttributeName() instead of setting it in Init()?
What if the XTFVisual returns always null as a class attribute name?
I rather keep the initialization in init().

>We need to propagate that error.
Done.

>does the superclass init _have_ to be called 
>before GetClassAttributeName() here
As far as I see no.

>Add some debug code here...
Done.

>add a big comment in the idl to the effect...
Done.
Attachment #175834 - Flags: review?(bryner)
Attachment #175569 - Flags: review?(bryner)
Attachment #175834 - Flags: review?(bryner) → review+
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: