Map MathML attributes lquote/rquote to style

RESOLVED FIXED in mozilla18

Status

()

Core
MathML
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: fredw, Assigned: fredw)

Tracking

(Blocks: 1 bug)

Trunk
mozilla18
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 8 obsolete attachments)

(Assignee)

Description

7 years ago
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.
> 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.
(Assignee)

Comment 2

7 years ago
(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!
(Assignee)

Comment 3

7 years ago
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
Assignee: nobody → fred.wang
Attachment #439764 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #440352 - Flags: review?(roc)
> +      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
Attachment #440352 - Flags: review?(roc)
Attachment #440352 - Flags: review?(dbaron)
Attachment #440352 - Flags: review+
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.
(Assignee)

Comment 7

7 years ago
Created attachment 440448 [details] [diff] [review]
Patch V2
Attachment #440352 - Attachment is obsolete: true
Attachment #440352 - Flags: review?(dbaron)
(Assignee)

Comment 8

7 years ago
(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...
(Assignee)

Comment 11

7 years ago
<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.)
(Assignee)

Comment 13

7 years ago
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)).
(Assignee)

Updated

6 years ago
Blocks: 656391
(Assignee)

Updated

6 years ago
Blocks: 569125
(Assignee)

Updated

6 years ago
Assignee: fred.wang → nobody
(Assignee)

Comment 14

6 years ago
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...
Assignee: nobody → fred.wang
Attachment #440448 - Attachment is obsolete: true
Attachment #588431 - Flags: feedback?(dbaron)
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.
(Assignee)

Comment 16

6 years ago
(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;
}
(Assignee)

Comment 17

5 years ago
(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
(Assignee)

Comment 18

5 years ago
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.
(Assignee)

Updated

5 years ago
Assignee: fred.wang → nobody
(Assignee)

Updated

5 years ago
Attachment #588431 - Attachment is obsolete: true
Attachment #588431 - Flags: feedback?(dbaron)
(Assignee)

Comment 19

5 years ago
Created attachment 640079 [details] [diff] [review]
Patch - part 1
(Assignee)

Comment 20

5 years ago
Created attachment 640080 [details] [diff] [review]
Patch - part 2
(Assignee)

Comment 21

5 years ago
Created attachment 640081 [details] [diff] [review]
Patch - part 3
(Assignee)

Comment 22

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 785956
(Assignee)

Updated

5 years ago
Blocks: 787215
(Assignee)

Updated

5 years ago
Depends on: 785720
(Assignee)

Comment 23

5 years ago
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).
Attachment #640079 - Attachment is obsolete: true
Attachment #640080 - Attachment is obsolete: true
Attachment #640081 - Attachment is obsolete: true
Attachment #657039 - Flags: review?(karlt)
(Assignee)

Comment 24

5 years ago
Created attachment 657043 [details] [diff] [review]
Use rules from the MathML CSS Profile
Attachment #657039 - Flags: review?(karlt) → review+
(Assignee)

Comment 25

5 years ago
Created attachment 657415 [details] [diff] [review]
Use rules from the MathML CSS Profile - V2

Small formatting changes to mathml.css
Assignee: nobody → fred.wang
(Assignee)

Updated

5 years ago
Attachment #657043 - Attachment is obsolete: true
(Assignee)

Comment 26

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=df32e9987538
(Assignee)

Updated

5 years ago
Attachment #657415 - Flags: review?(karlt)
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.
Attachment #657415 - Flags: review?(karlt) → review+
(Assignee)

Comment 28

5 years ago
(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.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/2524a701f06d
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0544769406c
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2524a701f06d
https://hg.mozilla.org/mozilla-central/rev/b0544769406c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
(Assignee)

Updated

5 years ago
No longer blocks: 569125
(Assignee)

Updated

4 years ago
No longer blocks: 785956
You need to log in before you can comment on or make changes to this bug.