Last Comment Bug 569125 - (mstyle) Complete/Improve implementation of mstyle
(mstyle)
: Complete/Improve implementation of mstyle
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: MathML (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla6
Assigned To: Frédéric Wang (:fredw)
:
: Anthony Jones (:kentuckyfriedtakahe, :k17e)
Mentors:
Depends on:
Blocks: mathml-2 449396 569124
  Show dependency treegraph
 
Reported: 2010-05-30 05:50 PDT by Frédéric Wang (:fredw)
Modified: 2014-01-07 03:13 PST (History)
6 users (show)
karlt: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add reftests for mstyle (28.11 KB, patch)
2010-06-10 04:25 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Add reftests for mstyle (V2) (30.30 KB, patch)
2011-01-02 07:56 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Patch - part 1 (992 bytes, patch)
2011-01-02 07:57 PST, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review
Patch - part 2 (4.80 KB, patch)
2011-01-02 07:58 PST, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review
Patch - part 3 (5.01 KB, patch)
2011-01-02 07:58 PST, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review
Patch - part 4 (881 bytes, patch)
2011-01-02 07:58 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Patch - part 5 (1023 bytes, patch)
2011-01-02 07:59 PST, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review
Patch - part 6 (6.34 KB, patch)
2011-01-02 07:59 PST, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review
Patch - part 7 (3.27 KB, patch)
2011-01-03 07:58 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Add reftests for mstyle (V3) (31.06 KB, patch)
2011-01-03 07:59 PST, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review
Patch - part 4 ("lquote" and "rquote") (1.66 KB, patch)
2011-04-12 00:46 PDT, Frédéric Wang (:fredw)
karlt: review-
Details | Diff | Splinter Review
Patch - part 7 (mathvariant) (3.11 KB, patch)
2011-04-12 00:47 PDT, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review
Patch - part 1 ("selection") (989 bytes, patch)
2011-05-11 05:14 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
testcase lquote rquote (1.01 KB, application/xhtml+xml)
2011-05-11 05:17 PDT, Frédéric Wang (:fredw)
no flags Details

Description Frédéric Wang (:fredw) 2010-05-30 05:50:57 PDT
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) 2010-06-10 04:25:39 PDT
Created attachment 450334 [details] [diff] [review]
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) 2010-07-28 21:02:24 PDT
(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) 2010-07-28 23:29:40 PDT
> 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) 2011-01-02 07:56:26 PST
Created attachment 500681 [details] [diff] [review]
Add reftests for mstyle (V2)
Comment 5 Frédéric Wang (:fredw) 2011-01-02 07:57:38 PST
Created attachment 500682 [details] [diff] [review]
Patch - part 1
Comment 6 Frédéric Wang (:fredw) 2011-01-02 07:58:08 PST
Created attachment 500683 [details] [diff] [review]
Patch - part 2
Comment 7 Frédéric Wang (:fredw) 2011-01-02 07:58:32 PST
Created attachment 500684 [details] [diff] [review]
Patch - part 3
Comment 8 Frédéric Wang (:fredw) 2011-01-02 07:58:54 PST
Created attachment 500685 [details] [diff] [review]
Patch - part 4
Comment 9 Frédéric Wang (:fredw) 2011-01-02 07:59:16 PST
Created attachment 500686 [details] [diff] [review]
Patch - part 5
Comment 10 Frédéric Wang (:fredw) 2011-01-02 07:59:48 PST
Created attachment 500687 [details] [diff] [review]
Patch - part 6
Comment 11 Frédéric Wang (:fredw) 2011-01-03 07:58:40 PST
Created attachment 500814 [details] [diff] [review]
Patch - part 7
Comment 12 Frédéric Wang (:fredw) 2011-01-03 07:59:12 PST
Created attachment 500815 [details] [diff] [review]
Add reftests for mstyle (V3)
Comment 13 Frédéric Wang (:fredw) 2011-04-12 00:46:44 PDT
Created attachment 525326 [details] [diff] [review]
Patch - part 4 ("lquote" and "rquote")
Comment 14 Frédéric Wang (:fredw) 2011-04-12 00:47:51 PDT
Created attachment 525327 [details] [diff] [review]
Patch - part 7 (mathvariant)
Comment 15 Karl Tomlinson (:karlt) 2011-05-10 22:20:03 PDT
Comment on attachment 500682 [details] [diff] [review]
Patch - part 1

I assume the extra () is unintentional?
Comment 16 Karl Tomlinson (:karlt) 2011-05-10 22:51:52 PDT
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) 2011-05-10 23:12:00 PDT
(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) 2011-05-10 23:21:54 PDT
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.
Comment 19 Frédéric Wang (:fredw) 2011-05-10 23:39:58 PDT
> 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) 2011-05-10 23:44:28 PDT
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) 2011-05-10 23:53:02 PDT
> 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) 2011-05-11 00:08:35 PDT
Comment on attachment 500815 [details] [diff] [review]
Add reftests for mstyle (V3)

Nice and comprehensive, thanks!
Comment 23 Karl Tomlinson (:karlt) 2011-05-11 00:16:00 PDT
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) 2011-05-11 05:14:59 PDT
Created attachment 531589 [details] [diff] [review]
Patch - part 1 ("selection")
Comment 25 Frédéric Wang (:fredw) 2011-05-11 05:17:09 PDT
Created attachment 531590 [details]
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) 2011-05-11 05:20:31 PDT
(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) 2011-05-11 11:41:06 PDT
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) 2011-05-11 16:11:31 PDT
(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) 2011-05-12 04:24:01 PDT
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) 2011-05-12 05:54:41 PDT
> 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"?

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