Last Comment Bug 560100 - Map MathML attributes lquote/rquote to style
: Map MathML attributes lquote/rquote to style
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: MathML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla18
Assigned To: Frédéric Wang (:fredw)
:
Mentors:
Depends on: CVE-2012-4180
Blocks: 787215 69409 656391
  Show dependency treegraph
 
Reported: 2010-04-18 05:26 PDT by Frédéric Wang (:fredw)
Modified: 2013-09-22 02:41 PDT (History)
5 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch V1 (8.53 KB, patch)
2010-04-18 05:26 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Map lquote and rquote to style (8.65 KB, patch)
2010-04-20 15:07 PDT, Frédéric Wang (:fredw)
roc: review+
Details | Diff | Splinter Review
Patch V2 (10.90 KB, patch)
2010-04-21 03:17 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Patch V3 - part 1 (11.21 KB, patch)
2012-01-13 08:33 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Patch - part 1 (11.20 KB, patch)
2012-07-08 11:20 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Patch - part 2 (9.03 KB, patch)
2012-07-08 11:21 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Patch - part 3 (4.33 KB, patch)
2012-07-08 11:21 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Update reftests for the <ms> element (5.47 KB, patch)
2012-08-30 14:31 PDT, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review
Use rules from the MathML CSS Profile (5.52 KB, patch)
2012-08-30 14:38 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Use rules from the MathML CSS Profile - V2 (5.54 KB, patch)
2012-08-31 12:52 PDT, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review

Description Frédéric Wang (:fredw) 2010-04-18 05:26:04 PDT
Created attachment 439764 [details] [diff] [review]
Patch V1

Experimental patch. I'm not sure what I do in nsMathMLElement::MapMathMLAttributesInto is correct.
It seems that aData->mContentData->mQuotes is always null.
Comment 1 Boris Zbarsky [:bz] (TPAC) 2010-04-20 09:50:46 PDT
> 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.
Comment 2 Frédéric Wang (:fredw) 2010-04-20 15:05:08 PDT
(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!
Comment 3 Frédéric Wang (:fredw) 2010-04-20 15:07:45 PDT
Created attachment 440352 [details] [diff] [review]
Map lquote and rquote to style

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
Comment 4 Boris Zbarsky [:bz] (TPAC) 2010-04-20 17:53:12 PDT
> +      const nsString quote(NS_LITERAL_STRING("\""));

NS_NAMED_LITERAL_STRING(quote, "\"");
Comment 5 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-04-20 21:18:06 PDT
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
Comment 6 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-04-20 21:57:25 PDT
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.
Comment 7 Frédéric Wang (:fredw) 2010-04-21 03:17:45 PDT
Created attachment 440448 [details] [diff] [review]
Patch V2
Comment 8 Frédéric Wang (:fredw) 2010-04-21 03:22:36 PDT
(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 "
Comment 9 Boris Zbarsky [:bz] (TPAC) 2010-04-21 06:27:04 PDT
> 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?
Comment 10 Boris Zbarsky [:bz] (TPAC) 2010-04-21 06:32:26 PDT
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...
Comment 11 Frédéric Wang (:fredw) 2010-04-21 07:02:47 PDT
<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 12 Karl Tomlinson (:karlt) 2010-04-21 17:16:59 PDT
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.)
Comment 13 Frédéric Wang (:fredw) 2010-04-22 02:09:01 PDT
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)).
Comment 14 Frédéric Wang (:fredw) 2012-01-13 08:33:48 PST
Created attachment 588431 [details] [diff] [review]
Patch V3 - part 1

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...
Comment 15 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-01-24 15:21:38 PST
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.
Comment 16 Frédéric Wang (:fredw) 2012-01-25 02:58:03 PST
(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;
}
Comment 17 Frédéric Wang (:fredw) 2012-05-13 10:53:26 PDT
(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
Comment 18 Frédéric Wang (:fredw) 2012-05-14 06:09:13 PDT
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.
Comment 19 Frédéric Wang (:fredw) 2012-07-08 11:20:57 PDT
Created attachment 640079 [details] [diff] [review]
Patch - part 1
Comment 20 Frédéric Wang (:fredw) 2012-07-08 11:21:13 PDT
Created attachment 640080 [details] [diff] [review]
Patch - part 2
Comment 21 Frédéric Wang (:fredw) 2012-07-08 11:21:39 PDT
Created attachment 640081 [details] [diff] [review]
Patch - part 3
Comment 22 Frédéric Wang (:fredw) 2012-07-08 11:23:12 PDT
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.
Comment 23 Frédéric Wang (:fredw) 2012-08-30 14:31:50 PDT
Created attachment 657039 [details] [diff] [review]
Update reftests for the <ms> element

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).
Comment 24 Frédéric Wang (:fredw) 2012-08-30 14:38:52 PDT
Created attachment 657043 [details] [diff] [review]
Use rules from the MathML CSS Profile
Comment 25 Frédéric Wang (:fredw) 2012-08-31 12:52:52 PDT
Created attachment 657415 [details] [diff] [review]
Use rules from the MathML CSS Profile - V2

Small formatting changes to mathml.css
Comment 26 Frédéric Wang (:fredw) 2012-08-31 13:01:47 PDT
https://tbpl.mozilla.org/?tree=Try&rev=df32e9987538
Comment 27 Karl Tomlinson (:karlt) 2012-09-02 23:46:48 PDT
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.
Comment 28 Frédéric Wang (:fredw) 2012-09-03 00:43:33 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.