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...

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

(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.)

> 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 on attachment 500682 [details] [diff] [review] Patch - part 1 I assume the extra () is unintentional?

Comment on attachment 500684 [details] [diff] [review] Patch - part 3 Yes, InheritAutomaticData seems a better place to check for attribute changes than Place/Reflow.

(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 on attachment 525326 [details] [diff] [review] Patch - part 4 ("lquote" and "rquote") ># HG changeset patch ># Parent bb6d5c8592dd5061392e23d8f8d082355b4b3ba7 ># User Frédéric Wang <fred.wang@free.fr> >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.

> 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 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.

> 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 on attachment 500815 [details] [diff] [review] Add reftests for mstyle (V3) Nice and comprehensive, thanks!

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.

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.

(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.

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.

(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 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.

> 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"?

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

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