Bug 569125 (mstyle)

Complete/Improve implementation of mstyle

RESOLVED FIXED in mozilla6

Status

()

Core
MathML
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: fredw, Assigned: fredw)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla6
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments, 5 obsolete attachments)

4.80 KB, patch
karlt
: review+
Details | Diff | Splinter Review
5.01 KB, patch
karlt
: review+
Details | Diff | Splinter Review
1023 bytes, patch
karlt
: review+
Details | Diff | Splinter Review
6.34 KB, patch
karlt
: review+
Details | Diff | Splinter Review
31.06 KB, patch
karlt
: review+
Details | Diff | Splinter Review
1.66 KB, patch
karlt
: review-
Details | Diff | Splinter Review
3.11 KB, patch
karlt
: review+
Details | Diff | Splinter Review
989 bytes, patch
Details | Diff | Splinter Review
1.01 KB, application/xhtml+xml
Details
(Assignee)

Description

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

Updated

7 years ago
Alias: mstyle
Blocks: 525772
(Assignee)

Comment 1

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

Comment 3

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

Comment 4

7 years ago
Created attachment 500681 [details] [diff] [review]
Add reftests for mstyle (V2)
Attachment #450334 - Attachment is obsolete: true
(Assignee)

Comment 5

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

Comment 6

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

Comment 7

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

Comment 8

7 years ago
Created attachment 500685 [details] [diff] [review]
Patch - part 4
(Assignee)

Comment 9

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

Comment 10

7 years ago
Created attachment 500687 [details] [diff] [review]
Patch - part 6
(Assignee)

Updated

7 years ago
Keywords: dev-doc-needed
(Assignee)

Comment 11

7 years ago
Created attachment 500814 [details] [diff] [review]
Patch - part 7
(Assignee)

Comment 12

7 years ago
Created attachment 500815 [details] [diff] [review]
Add reftests for mstyle (V3)
Attachment #500681 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Blocks: 449396
No longer depends on: 449396
(Assignee)

Comment 13

6 years ago
Created attachment 525326 [details] [diff] [review]
Patch - part 4 ("lquote" and "rquote")
Attachment #500685 - Attachment is obsolete: true
(Assignee)

Comment 14

6 years ago
Created attachment 525327 [details] [diff] [review]
Patch - part 7 (mathvariant)
Attachment #500814 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #500682 - Flags: review?(karlt)
(Assignee)

Updated

6 years ago
Attachment #500683 - Flags: review?(karlt)
(Assignee)

Updated

6 years ago
Attachment #500684 - Flags: review?(karlt)
(Assignee)

Updated

6 years ago
Attachment #525326 - Flags: review?(karlt)
(Assignee)

Updated

6 years ago
Attachment #500686 - Flags: review?(karlt)
(Assignee)

Updated

6 years ago
Attachment #500687 - Flags: review?(karlt)
(Assignee)

Updated

6 years ago
Attachment #525327 - Flags: review?(karlt)
(Assignee)

Updated

6 years ago
Attachment #500815 - Flags: review?(karlt)
Comment on attachment 500682 [details] [diff] [review]
Patch - part 1

I assume the extra () is unintentional?
Attachment #500682 - Flags: review?(karlt) → review+
Attachment #500683 - Flags: review?(karlt) → review+
Comment on attachment 500684 [details] [diff] [review]
Patch - part 3

Yes, InheritAutomaticData seems a better place to check for attribute changes than Place/Reflow.
Attachment #500684 - Flags: review?(karlt) → review+
(Assignee)

Comment 17

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

Comment 19

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

Comment 21

6 years ago
> 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!
Attachment #500815 - Flags: review?(karlt) → review+
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.
Attachment #525327 - Flags: review?(karlt) → review+
(Assignee)

Comment 24

6 years ago
Created attachment 531589 [details] [diff] [review]
Patch - part 1 ("selection")
Attachment #500682 - Attachment is obsolete: true
(Assignee)

Comment 25

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

Comment 27

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

Comment 30

6 years ago
> 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"?
Depends on: 560100
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
Assignee: nobody → fred.wang
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
Flags: in-testsuite+
Docs are updated:
https://developer.mozilla.org/en/MathML/Element/mstyle#Gecko-specific_notes
https://developer.mozilla.org/en/Firefox_6_for_developers#MathML
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Updated

5 years ago
Depends on: 787215
No longer depends on: 560100
(Assignee)

Updated

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