Closed Bug 475518 Opened 11 years ago Closed 10 years ago

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

Categories

(Core :: MathML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a3

People

(Reporter: laurent, Assigned: fredw)

Details

Attachments

(1 file, 3 obsolete files)

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".
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.
Assignee: nobody → fred.wang
Laurent, I guess the serializer should not generate these private MathML attributes. How does the it behave with the "_moz*" attributes?
"_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..).
(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?
>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 :-)
OK, thanks for the information. Here is an updated patch.
Attachment #410948 - Attachment is obsolete: true
Attachment #411215 - Flags: review?(mozbugz)
Flags: wanted1.9.2?
Flags: wanted1.9.2? → wanted1.9.2-
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.
Attachment #411215 - Flags: superreview?(bzbarsky)
Attachment #411215 - Flags: review?(karlt)
Attachment #411215 - Flags: review+
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 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.
Attachment #411215 - Flags: superreview?(bzbarsky) → superreview+
Attached patch Final Patch (obsolete) — Splinter Review
Attachment #411215 - Attachment is obsolete: true
Thanks for the review. I've updated the patch to fix conflicts.
Keywords: checkin-needed
@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 ?
Attached patch Final PatchSplinter Review
Yes, sorry I don't know where this change comes from.
Attachment #431348 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/93d38d3a15b9
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a3
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.