Experimental patch. I'm not sure what I do in nsMathMLElement::MapMathMLAttributesInto is correct. It seems that aData->mContentData->mQuotes is always null.

> It seems that aData->mContentData->mQuotes is always null. If it's null, that means it's not set yet and you should set it. If it's non-null you must not touch it, because some higher-weight rule has already set it.

(In reply to comment #1) > > It seems that aData->mContentData->mQuotes is always null. > > If it's null, that means it's not set yet and you should set it. If it's > non-null you must not touch it, because some higher-weight rule has already set > it. Thanks for the info!

This patch works on a simple testcase. Apparently, the reftest quotes-1 does not use the correct default values, so I had to modify it in order to pass it. http://www.w3.org/TR/MathML3/chapter3.html#id.3.2.8.2

> + const nsString quote(NS_LITERAL_STRING("\"")); NS_NAMED_LITERAL_STRING(quote, "\"");

Comment on attachment 440352 [details] [diff] [review] Map lquote and rquote to style Looks good to me but I'm not an expert on mapped attributes, so also seeking review from dbaron

Does this change the handling of what happens when you have an lquote attribute but not an rquote, or vice-versa? It seems like it does, particularly in cases of inheritance. Are these attributes ever expected to be inherited to descendants? The 'quotes' property *is* inherited.

(In reply to comment #4) > > + const nsString quote(NS_LITERAL_STRING("\"")); > > NS_NAMED_LITERAL_STRING(quote, "\""); I don't know why, but nsCSSValue::SetStringValue does not seem to like this change. (In reply to comment #6) > Does this change the handling of what happens when you have an lquote attribute > but not an rquote, or vice-versa? No, it seems to work correctly. The only problem I've found is when there is no attribute at all (the curly quotes are used instead of the straight ones). I've modified my patch to add more tests in the reftest as well as a rule in mathml.css. > It seems like it does, particularly in cases > of inheritance. Are these attributes ever expected to be inherited to > descendants? The 'quotes' property *is* inherited. lquote/rquote are not inherited. The token element <ms></ms> defines a literal string, and these two attributes allow to specify the quotes to use. By default, it's a straight quote "

> nsCSSValue::SetStringValue does not seem to like this change. Hmm... Probably because it takes an nsString, not an nsAString. <sigh>. > lquote/rquote are not inherited. The token element <ms></ms> defines a literal > string, So what should happen when <ms lquote="f">a<ms>b</ms>c</ms> happens?

Or similar things where the kids are, say, HTML spans that have before open-quote styling on them. I guess maybe it doesn't matter too much what happens in those cases...

<ms/> is a token element, so it can only contain the empty elements <mglyph/>, <malignmark/> or some text. For such kind of non-valid XML fragments, I don't think there is a "right" behavior.

Comment on attachment 440448 [details] [diff] [review] Patch V2 Does this change the behavior of lquote and rquote on an mstyle? nsMathMLTokenFrame::SetQuotes was using GetAttribute. (I'm currently really not fond of mstyle.)

Yes, I've forgotten this <mstyle/> element again :-( lquote/rquote are on token elements <ms/>, so all will be like inheritance when they are on <mstyle/>. However, as pointed by David in comment 6, this won't work when only one of the two attributes is set: we want to be able to inherit the left and right values independently but the "quotes" property does not allow that. Some proposals I currently have: 1) attach private attributes _moz-lquote and _moz-rquote on <ms/> using GetAttribute and map them to properties content after and content before (but we don't want that). 2) allow the internal quotes structure to inherit quote values independently. 3) add CSS properties -moz-ms-lquote, -moz-ms-rquote. 4) ignore XXX comment http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLTokenFrame.cpp#384 For the moment, I'm going to choose option 4) but if the bug should really be fixed I think 3) is best. However, I can still extract from the patch the fix to set the correct default quote values and add the corresponding reftests. PS: Strangely, lquote/rquote attributes have no effect for my stable Mozilla's version (Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.9.1.9) Gecko/20100414 Iceweasel/3.5.9 (like Firefox/3.5.9)).

So I prepared three new patches for this bug. For the moment I need feedback on the implementation of -moz-ms-lquote/-moz-ms-rquote. It's the first time I modify layout/style/ and I'm not familiar at all with this code...

Could you explain in English what it is you're trying to do? That would help me figure out both (a) if that's what you've actually done and (b) whether it's the right thing to do.

(In reply to David Baron [:dbaron] from comment #15) > Could you explain in English what it is you're trying to do? That would > help me figure out both (a) if that's what you've actually done and (b) > whether it's the right thing to do. The purpose of the bug is to rewrite MathML lquote/rquote attributes: <math><ms lquote="(" rquote="]">mytext</ms></math> should render (mytext] However, we also want to take into account the values given by parent <mstyle>'s, so <math> <mstyle lquote="("> <mstyle rquote="]"> <ms>mytext</ms> </mstyle> </mstyle> </math> should also render the same way. Moreover, lquote and rquote should be understood as leading and trailing quotes. For example, a third way to get the previous rendering is in RTL context: <math dir="rtl"><ms lquote="[" rquote=")">mytext</ms></math>. Note that the default value for lquote/rquote is &quot;. So my idea is to implement private MsLquote and MsRquote properties that are inherited by default and initialized to &quot;. They will only be modified in content/mathml/content/src/nsMathMLElement.cpp by mapping the values of attributes lquote/rquote of <mstyle> or <ms> elements (see part 2: https://raw.github.com/fred-wang/MozillaCentralPatches/master/ms-quotes-2.diff). Then, I add new values -moz-ms-lquote and -moz-ms-rquote in the "content" property, in order to use MsLquote/MsRquote values. Finally, layout/mathml/mathml.css will have the following rules to draw the quotes: ms:before { content: -moz-ms-lquote; } ms:after { content: -moz-ms-rquote; }

(In reply to Frédéric Wang from comment #14) > Created attachment 588431 [details] [diff] [review] > Patch V3 - part 1 > Bill Gianopoulos told me that this patch has bit-rotted. He has a version that should apply to trunk here: http://www.wg9s.com/mozilla/firefox/patches/ms-quotes-1.diff

I don't think I mentioned it, but in my personal notes I indicate that the current patch does not work with dir=rtl and when the quotes are set with setAttribute. Also, this bug is blocking bug 656391 i.e. the quotes are not updated when one uses removeAttribute (the patch fixes that). So maybe that's something Andrii can work on if it remains some time at the end of his google summer of code project.

I don't plan to work on this in the short term and don't want to continue to unbitrot the patches. I uploaded the latest versions from my patch queue. Feel free to take the bug and finish the work on it.

This applies on top of bug 785720. It adds a test for <ms> in dir=RTL and more dynamic tests for <ms>. Currently, with these updated tests, we have the following failures: - dir-9.html - mstyle-5.xhtml - whitespace-trim-4.html - quotes-1-ref.xhtml I have a patch that replaces the current <ms> implementation by CSS rules in mathml.css (those of the MathML CSS Profile). It makes quotes-1-ref.xhtml pass (and should fix bug 656391) but does not improve the situation for the other issues (I opened bug 787215 for these issues).

Small formatting changes to mathml.css

Comment on attachment 657415 [details] [diff] [review] Use rules from the MathML CSS Profile - V2 I'm happy with this because it is much simpler, if you are happy that this is removing mstyle support for these attributes.

(In reply to Karl Tomlinson (:karlt) from comment #27) > Comment on attachment 657415 [details] [diff] [review] > Use rules from the MathML CSS Profile - V2 > > I'm happy with this because it is much simpler, if you are happy that this > is removing mstyle support for these attributes. It seems that mstyle for these attributes does not work with the old code, anyway.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2524a701f06d https://hg.mozilla.org/integration/mozilla-inbound/rev/b0544769406c

https://hg.mozilla.org/mozilla-central/rev/2524a701f06d https://hg.mozilla.org/mozilla-central/rev/b0544769406c