569125 - (mstyle) Complete/Improve implementation of mstyle

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

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

MathML3 says:

"The mstyle element is used to make style changes that affect the rendering of its contents. It can be given any attribute accepted by any other presentation element, except for the attributes described below. In addition, the mstyle element can be given certain special attributes listed in the next subsection."

The exceptions are:

- attributes height, depth or width on mstyle apply to mspace, not to mglyph, mpadded, or mtable.
- attributes rowalign, columnalign, or groupalign on mstyle apply to mtable, not to mtr, mlabeledtr, mtd, and maligngroup.
- attribute lspace on mstyle applies to mo, not to mpadded.
- attribute voffset on mstyle does not apply to mpadded.
- attribute fontfamily on mstyle does not apply to mglyph.
- attribute index cannot be set on mstyle.
- attribute align on mstyle applies to the munder, mover, and munderover, not mtable and mstack.
- attributes src, alt on mglyph and actiontype on maction cannot be set on mstyle.

The additional attributes are:

- scriptlevel
- displaystyle
- scriptsizemultiplier
- scriptminsize
- infixlinebreakstyle
- decimal point

It is quite complicated and apparently our implementation does not always follow these rules. I suggest to add a comment each time we use a MathML attribute to explicitly say whether the attribute may be taken from a mstyle or if we are in the case of one of the exception above. This will prevent interrogation such that

http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLmtableFrame.cpp#587

Also, we should add reftests for mstyle (I already have a patch with some tests for that). Apparently, there are currently problems with

* mfenced: open, close, separators 
* mfrac: bevelled
* menclose: notation
* mover/munder/munderover: accent, accentunder

and probably other attributes defined in mathml.css such that those of tabular elements...

Comment 1

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

Attachment: Add reftests for mstyle

I attach a patch adding reftests for attributes that are currently accessed by GetAttribute, FindAttrValueIn or GetAttr (not all of nsMathMLmtableFrame and nsMathMLTokenFrame, though).

Comment 2

[Karl Tomlinson (:karlt)]

(In reply to comment #1)
> Created attachment 450334 [details] [diff] [review]
> Add reftests for mstyle

These tests look good, thanks.
Did you mean them for landing now?
I sent them to try server but got some failures.
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1280365345.1280367211.25331.gz

(Load layout/tools/reftest/reftest-analyzer.xhtml in the browser and paste the REFTEST output of the failures.)

Comment 3

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

> Did you mean them for landing now?

I think my intention was to fix the mstyle's bugs before. But maybe we can already send a patch with the failing tests disabled.

> I sent them to try server but got some failures.
> http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1280365345.1280367211.25331.gz

Yes, that's what I say here:

> Apparently, there are currently problems with
> 
> * mfenced: open, close, separators 
> * mfrac: bevelled
> * menclose: notation
> * mover/munder/munderover: accent, accentunder

and apparently, there are also problems with maction and the lquote/rquote of ms.

Sometimes we obviously don't use GetAttribute (for example to catch the notation of menclose) and sometimes it is not placed correctly (for mfrac, it seems that mIsBevelled should be updated in nsMathMLmfracFrame::PlaceInternal, just like mLineThickness).

Comment 4

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

Attachment: Add reftests for mstyle (V2)

Comment 5

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

Attachment: Patch - part 1

Comment 6

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

Attachment: Patch - part 2

Comment 7

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

Attachment: Patch - part 3

Comment 8

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

Attachment: Patch - part 4

Comment 9

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

Attachment: Patch - part 5

Comment 10

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

Attachment: Patch - part 6

Comment 11

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

Attachment: Patch - part 7

Comment 12

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

Attachment: Add reftests for mstyle (V3)

Comment 13

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

Attachment: Patch - part 4 ("lquote" and "rquote")

Comment 14

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

Attachment: Patch - part 7 (mathvariant)

Comment 15

[Karl Tomlinson (:karlt)]

Comment on attachment 500682 [details] [diff] [review]
Patch - part 1

I assume the extra () is unintentional?

Comment 16

[Karl Tomlinson (:karlt)]

Comment on attachment 500684 [details] [diff] [review]
Patch - part 3

Yes, InheritAutomaticData seems a better place to check for attribute changes than Place/Reflow.

Comment 17

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

(In reply to comment #15)
> Comment on attachment 500682 [details] [diff] [review] [review]
> Patch - part 1
> 
> I assume the extra () is unintentional?

Yes, it seems I copy it from a if statement.

Comment 18

[Karl Tomlinson (:karlt)]

Comment on attachment 525326 [details] [diff] [review]
Patch - part 4 ("lquote" and "rquote")

># HG changeset patch
># Parent bb6d5c8592dd5061392e23d8f8d082355b4b3ba7
># User Frédéric Wang <[e-mail]>
>Support for attribute "lquote" and "rquote" on mstyle (bug 569125). r=karlt
>+nsMathMLTokenFrame::InheritAutomaticData(nsIFrame* aParent)
>+{
>+  // let the base class get the default from our parent
>+  nsMathMLContainerFrame::InheritAutomaticData(aParent);
>+
>+  SetQuotes(PR_TRUE);

Is it necessary to have aNotify == PR_TRUE here?

nsGenericDOMDataNode uses RunDOMEventWhenSafe() so I guess that's OK if necessary.
I don't understand why it is necessary in AttributeChanged() either and I didn't find an explanation in bug 476547.

Comment 19

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

> Is it necessary to have aNotify == PR_TRUE here?
> 
> nsGenericDOMDataNode uses RunDOMEventWhenSafe() so I guess that's OK if
> necessary.

I think I tried with aNotify == PR_FALSE but it didn't work (not 100% sure, just a memory that I suspected aNotify == PR_TRUE could be less efficient)

> I don't understand why it is necessary in AttributeChanged() either and I
> didn't find an explanation in bug 476547.

Robert may know.

Comment 20

[Karl Tomlinson (:karlt)]

Comment on attachment 525327 [details] [diff] [review]
Patch - part 7 (mathvariant)

Looks like the mathvariant change is correct.
I'm not sure whether or not we should do the same with fontstyle.
Did mstyle affect defaults on all presentation element attributes prior to the deprecation of fontstyle.

Comment 21

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

> I'm not sure whether or not we should do the same with fontstyle.
> Did mstyle affect defaults on all presentation element attributes prior to
> the deprecation of fontstyle.

I think so, but I guess we don't need to bother about deprecated attributes.
We didn't in the content part:

http://hg.mozilla.org/mozilla-central/file/a45b5cf1d625/content/mathml/content/src/nsMathMLElement.cpp#l123

Comment 22

[Karl Tomlinson (:karlt)]

Comment on attachment 500815 [details] [diff] [review]
Add reftests for mstyle (V3)

Nice and comprehensive, thanks!

Comment 23

[Karl Tomlinson (:karlt)]

Comment on attachment 525327 [details] [diff] [review]
Patch - part 7 (mathvariant)

(In reply to comment #21)
> I think so, but I guess we don't need to bother about deprecated attributes.
> We didn't in the content part:

I'm fine with including fontstyle, especially since you already have the patch.
"fontfamily" is specifically mentioned, which implies that fontstyle should have the default behavior.

Comment 24

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

Attachment: Patch - part 1 ("selection")

Comment 25

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

Attachment: testcase lquote rquote

It seems to work without aNotify = PR_TRUE in both functions.
However, I think nsMathMLTokenFrame::SetQuotes should also set the quote to the default value " when an attribute is removed.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #18)
> Is it necessary to have aNotify == PR_TRUE here?
> 
> nsGenericDOMDataNode uses RunDOMEventWhenSafe() so I guess that's OK if
> necessary.
> I don't understand why it is necessary in AttributeChanged() either and I
> didn't find an explanation in bug 476547.

We have to make sure nsGenericDOMDataNode::SetTextInternal fires its nsNodeUtils::CharacterDataWillChange and nsNodeUtils::CharacterDataChanged notifications, otherwise the nsTextFrames for the quotes will get out of sync with the DOM text, which can cause bad crashes, such as in bug 476547.

Comment 27

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

OK, so I guess aNotify == PR_TRUE should also be used in InheritAutomaticData.
I've opened bug 656391 for the issue indicated in comment 25.

Comment 28

[Karl Tomlinson (:karlt)]

(In reply to comment #26)
> We have to make sure nsGenericDOMDataNode::SetTextInternal fires its
> nsNodeUtils::CharacterDataWillChange and nsNodeUtils::CharacterDataChanged
> notifications, otherwise the nsTextFrames for the quotes will get out of
> sync with the DOM text, which can cause bad crashes, such as in bug 476547.

Thanks.  Is it safe to have CharacterDataWillChange/CharacterDataChanged run during AppendFrames/InsertFrames/RemoveFrame?

Comment 29

[Karl Tomlinson (:karlt)]

Comment on attachment 525326 [details] [diff] [review]
Patch - part 4 ("lquote" and "rquote")

Apparently it is not safe during SetInitialChildList anyway.
This is while loading mstyle-1.xhtml:

###!!! ASSERTION: recurring into frame construction: 'mPresContext->mLayoutPhaseCount[eLayoutPhase_FrameC] == 0', file ../../../layout/base/nsPresContext.h, line 1407
nsAutoLayoutPhase::Enter [nsPresContext.h:1408]
nsAutoLayoutPhase::nsAutoLayoutPhase [nsPresContext.h:1374]
nsCSSFrameConstructor::CharacterDataChanged [nsCSSFrameConstructor.cpp:7870]
PresShell::CharacterDataChanged [nsPresShell.cpp:4943]
nsNodeUtils::CharacterDataChanged [nsNodeUtils.cpp:111]
nsGenericDOMDataNode::SetTextInternal [nsGenericDOMDataNode.cpp:394]
nsGenericDOMDataNode::SetText [nsGenericDOMDataNode.cpp:1048]
nsIContent::SetText [nsIContent.h:548]
SetQuote [nsMathMLTokenFrame.cpp:416]
nsMathMLTokenFrame::SetQuotes [nsMathMLTokenFrame.cpp:433]
nsMathMLTokenFrame::InheritAutomaticData [nsMathMLTokenFrame.cpp:68]
nsMathMLContainerFrame::RebuildAutomaticDataForChildren [nsMathMLContainerFrame.cpp:702]
nsMathMLContainerFrame::RebuildAutomaticDataForChildren [nsMathMLContainerFrame.cpp:703]
nsMathMLmathInlineFrame::SetInitialChildList [nsMathMLContainerFrame.h:486]
nsCSSFrameConstructor::ConstructFrameFromItemInternal [nsCSSFrameConstructor.cpp:3780]
nsCSSFrameConstructor::ConstructFramesFromItem [nsCSSFrameConstructor.cpp:5476]
nsCSSFrameConstructor::ConstructFramesFromItemList [nsCSSFrameConstructor.cpp:9486]
nsCSSFrameConstructor::ProcessChildren [nsCSSFrameConstructor.cpp:9626]
nsCSSFrameConstructor::ConstructTableCell [nsCSSFrameConstructor.cpp:2145]
nsCSSFrameConstructor::ConstructFrameFromItemInternal [nsCSSFrameConstructor.cpp:3708]
nsCSSFrameConstructor::ConstructFramesFromItem [nsCSSFrameConstructor.cpp:5476]
nsCSSFrameConstructor::ConstructFramesFromItemList [nsCSSFrameConstructor.cpp:9486]
nsCSSFrameConstructor::ProcessChildren [nsCSSFrameConstructor.cpp:9626]
nsCSSFrameConstructor::ConstructTableRow [nsCSSFrameConstructor.cpp:2006]
nsCSSFrameConstructor::ConstructFrameFromItemInternal [nsCSSFrameConstructor.cpp:3708]
nsCSSFrameConstructor::ConstructFramesFromItem [nsCSSFrameConstructor.cpp:5476]
nsCSSFrameConstructor::ConstructFramesFromItemList [nsCSSFrameConstructor.cpp:9486]
nsCSSFrameConstructor::ProcessChildren [nsCSSFrameConstructor.cpp:9626]
nsCSSFrameConstructor::ConstructFrameFromItemInternal [nsCSSFrameConstructor.cpp:3797]
nsCSSFrameConstructor::ConstructFramesFromItem [nsCSSFrameConstructor.cpp:5476]
nsCSSFrameConstructor::ConstructFramesFromItemList [nsCSSFrameConstructor.cpp:9486]
nsCSSFrameConstructor::ProcessChildren [nsCSSFrameConstructor.cpp:9626]
nsCSSFrameConstructor::ConstructTable [nsCSSFrameConstructor.cpp:1952]
nsCSSFrameConstructor::ConstructFrameFromItemInternal [nsCSSFrameConstructor.cpp:3708]
nsCSSFrameConstructor::ConstructFramesFromItem [nsCSSFrameConstructor.cpp:5476]
nsCSSFrameConstructor::ConstructFramesFromItemList [nsCSSFrameConstructor.cpp:9486]
nsCSSFrameConstructor::ContentAppended [nsCSSFrameConstructor.cpp:6675]
nsCSSFrameConstructor::CreateNeededFrames [nsCSSFrameConstructor.cpp:6332]
nsCSSFrameConstructor::CreateNeededFrames [nsCSSFrameConstructor.cpp:6335]
nsCSSFrameConstructor::CreateNeededFrames [nsCSSFrameConstructor.cpp:6354]
PresShell::FlushPendingNotifications [nsPresShell.cpp:4829]

I think we should land the rest of the changes.  quotes seem more difficult.

Comment 30

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

> I think we should land the rest of the changes.  quotes seem more difficult.

That sounds good. Probably, the best way to deal with the case of quotes is bug 560100. Should mstyle-1 reftest be marked as "fails"?

Comment 31

[Karl Tomlinson (:karlt)]

I separated the quotes test into mstyle-5.html

All of part 4 except the SetQuotes() ended up being merged into part 7.

http://hg.mozilla.org/mozilla-central/rev/2e5e73691621
http://hg.mozilla.org/mozilla-central/rev/012256e31122
http://hg.mozilla.org/mozilla-central/rev/e74ee4a0ee99
http://hg.mozilla.org/mozilla-central/rev/8ce8b8015ff3
http://hg.mozilla.org/mozilla-central/rev/ec74f6ddd354
http://hg.mozilla.org/mozilla-central/rev/b01c9a16df7b
http://hg.mozilla.org/mozilla-central/rev/a692da152ecb

Comment 32

[Florian Scholz (Open Web Docs)]

Docs are updated:
https://developer.mozilla.org/en/MathML/Element/mstyle#Gecko-specific_notes
https://developer.mozilla.org/en/Firefox_6_for_developers#MathML