475518 - MathML layout code should use _moz* attributes instead of -moz-* attributes

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

[Laurent Jouanneau]

The code of MathML creates some "private" attributes (see atoms MOZcolumnalign, MOZcolumnline etc). Their names begin with a "-". This is not good because:

- it is not valid in XML.
- it doesn't follow the implicit usage in Gecko of the "_moz" prefix in "private" attribute names.
- it introduced a hack in the XML serializer. See nsXMLContentSerializer.cpp line 735:

     // XXX Hack to get around the fact that MathML can add
     //     attributes starting with '-', which makes them
     //     invalid XML.
     if (!nameStr.IsEmpty() && nameStr.First() == '-')
       continue;

The serializer should have only one hack on "_moz" attributes (and this will be fixed in bug 422403), and MathML attributes should follow this "normalization".

Comment 1

[Frédéric Wang (:fredw)]

Attachment: Use _moz for private MathML attributes

Definition of XML attribute names. "-" is allowed inside the name but not as the first character:

http://www.w3.org/TR/REC-xml/#NT-AttDef
http://www.w3.org/TR/REC-xml/#NT-Name
http://www.w3.org/TR/REC-xml/#NT-NameStartChar
http://www.w3.org/TR/REC-xml/#NT-NameChar

-moz-math-font-size does not seem to be used, and apparently is likely to be implemented later when bug 69409 is fixed. Hence this patch removes this GkAtom.

This patch also removes the hack in the XML serializer.

Comment 2

[Frédéric Wang (:fredw)]

Laurent, I guess the serializer should not generate these private MathML attributes. How does the it behave with the "_moz*" attributes?

Comment 3

[Laurent Jouanneau]

"_moz*" attributes are removed, according to the test on sharedName, few lines above the code you removed. In fact, I think the code you removed was already useless because of this previous test.

The hack should also be removed from the XML serializer, but here we have to add a test on sharedName similar to the XHTML serializer one, so all _moz* attributes will be also removed when using the XML serializer (ex: when serializing SVG+MathML for ex..).

Comment 4

[Frédéric Wang (:fredw)]

(In reply to comment #3)
> "_moz*" attributes are removed, according to the test on sharedName, few lines
> above the code you removed. In fact, I think the code you removed was already
> useless because of this previous test.
> 
> The hack should also be removed from the XML serializer, but here we have to
> add a test on sharedName similar to the XHTML serializer one, so all _moz*
> attributes will be also removed when using the XML serializer (ex: when
> serializing SVG+MathML for ex..).

OK, actually I had not really looked carefully to the code of the serializers and did not see the two versions...

I think we should also remove the test ('-' == *sharedName) but apparently there are still some "-moz*" attributes in nsGkAtomList.h so it's probably better not to do it now. Hence I guess what I have to do for this bug is removing the two hacks and adding a test on sharedName in the XML serializer. Do you agree?

Comment 5

[Laurent Jouanneau]

>we should also remove the test ('-' == *sharedName) but apparently
there are still some "-moz*" attributes in nsGkAtomList.h so it's probably
better not to do it now

Even if I think those other names are CSS names only, I think we shouldn't remove this test on '-' because perhaps all "-moz" attributes are not stored in an Atom, and then some of them are hardcoded somewhere. We have to verify it, but can be hard..

>Do you agree?

Yes, except if there is a very old reason I don't know, to not unserialize these attributes (except MathML attributes). Do it, and we will see if a super reviewer is not agree with us :-)

Comment 6

[Frédéric Wang (:fredw)]

Attachment: More changes in the XML/XHTML serializers

OK, thanks for the information. Here is an updated patch.

Comment 7

[Karl Tomlinson (:karlt)]

Comment on attachment 411215 [details] [diff] [review]
More changes in the XML/XHTML serializers

>@@ -848,38 +850,40 @@ nsXMLContentSerializer::SerializeAttribu
>         continue;
>     }
> 
>     const nsAttrName* name = aContent->GetAttrNameAt(index);
>     PRInt32 namespaceID = name->NamespaceID();
>     nsIAtom* attrName = name->LocalName();
>     nsIAtom* attrPrefix = name->GetPrefix();
> 
>+    // Filter out any attribute starting with [-|_]moz
>+    const char* sharedName;
>+    attrName->GetUTF8String(&sharedName);
>+    if ((('_' == *sharedName) || ('-' == *sharedName)) &&
>+        !nsCRT::strncmp(sharedName+1, kMozStr, PRUint32(sizeof(kMozStr)-1))) {
>+      continue;
>+    }
>+

If we do this, it should now be updated in line with the recent change to nsXHTMLContentSerializer:  http://hg.mozilla.org/mozilla-central/diff/cb17f4d92942/content/base/src/nsXHTMLContentSerializer.cpp
Sorry I'm slow here.

Requesting sr from bz to confirm that filtering [-|_]moz attributes is the right thing to do.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 411215 [details] [diff] [review]
More changes in the XML/XHTML serializers

The filtering is fine, but why do we no longer need it in the XHTML serializer?

Comment 9

[Karl Tomlinson (:karlt)]

The filtering is already done in the XHTML serializer here:
http://hg.mozilla.org/mozilla-central/annotate/1008f5c2e057/content/base/src/nsXHTMLContentSerializer.cpp#l348

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 411215 [details] [diff] [review]
More changes in the XML/XHTML serializers

Perfect, thanks!  sr=me if this is updated to the new atom api.

Comment 11

[Frédéric Wang (:fredw)]

Attachment: Final Patch

Comment 12

[Frédéric Wang (:fredw)]

Thanks for the review. I've updated the patch to fix conflicts.

Comment 13

[Laurent Jouanneau]

@Frédéric : it seems you included wrong removal of files (toolkit/crashreporter/google-breakpad/src/common/dwarf/* ) in your last patch, isn't it ?

Comment 14

[Frédéric Wang (:fredw)]

Attachment: Final Patch

Yes, sorry I don't know where this change comes from.

Comment 15

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/93d38d3a15b9