Persona is no longer an option for authentication on BMO. For more details see Persona Deprecated.
Bug 297467 - menclose is shown wrong (not implemented)
 Summary: menclose is shown wrong (not implemented)
 Status: RESOLVED FIXED Core Components MathML (show other bugs) Trunk x86 Linux -- normal with 2 votes (vote) mozilla1.9.2b1 Frédéric Wang (:fredw) Hixie (not reading bugmail) Anthony Jones (:kentuckyfriedtakahe, :k17e) (view as bug list) Show dependency tree / graph

Reported: 2005-06-12 01:28 PDT by Martin Mokrejs
Modified: 2009-11-03 04:33 PST (History)
7 users (show)
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Attachments
parse "notation" attribute + display the radical notation (24.48 KB, patch)
2008-10-29 08:21 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
testcase (1014 bytes, text/xml)
2008-10-29 08:22 PDT, Frédéric Wang (:fredw)
no flags Details
screenshot of the testcase (11.41 KB, image/png)
2008-10-29 08:25 PDT, Frédéric Wang (:fredw)
no flags Details
display notations (27.55 KB, patch)
2008-10-30 04:19 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
screenshot of my test page (13.90 KB, image/png)
2008-10-30 04:20 PDT, Frédéric Wang (:fredw)
no flags Details
Current version of the patch. (28.01 KB, patch)
2008-10-31 09:41 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
2008-11-08 04:13 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Screenshot - MathML testsuite (42.19 KB, image/png)
2008-11-08 04:16 PST, Frédéric Wang (:fredw)
no flags Details
screenshot - MathML testsuite (35.26 KB, image/png)
2008-11-10 13:26 PST, Frédéric Wang (:fredw)
no flags Details
Show the remaining issues (5.15 KB, image/png)
2008-11-11 07:00 PST, Frédéric Wang (:fredw)
no flags Details
Patch (45.31 KB, patch)
2008-11-11 07:01 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
all mathml 2 notations +approximation of madruwb (41.80 KB, patch)
2008-11-20 16:05 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
screenshot of the mathml testsuite (35.38 KB, image/png)
2008-11-20 16:06 PST, Frédéric Wang (:fredw)
no flags Details
written arabic text with lam letters (193.98 KB, image/png)
2008-11-20 16:07 PST, Frédéric Wang (:fredw)
no flags Details
smaller size for the "circle notation" (3.39 KB, text/xml)
2008-12-01 13:09 PST, Frédéric Wang (:fredw)
no flags Details
fix a compiling error + other trivial fixes (41.80 KB, patch)
2008-12-21 07:02 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
several fixes (37.82 KB, patch)
2008-12-21 09:12 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
all mathml 2 notations (35.30 KB, patch)
2008-12-23 13:56 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
nsTArray<nsMathMLChar> changes (3.14 KB, patch)
2009-01-08 20:43 PST, Karl Tomlinson (:karlt)
no flags Details | Diff | Splinter Review
Testcase: circle notation (4.19 KB, text/xml)
2009-01-09 13:52 PST, Frédéric Wang (:fredw)
no flags Details
Merge the 2 previous patches + make msqrt inherit from menclose (47.19 KB, patch)
2009-01-09 13:57 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Improve circle notation + dynamic allocation of rects (50.08 KB, patch)
2009-01-10 04:42 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Updated patch (49.78 KB, patch)
2009-01-21 10:24 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Updated patch (49.78 KB, patch)
2009-01-21 12:43 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Circle notation: alignment of the center of the ellipse with the baseline (905 bytes, application/xhtml+xml)
2009-01-29 10:08 PST, Frédéric Wang (:fredw)
no flags Details
circle notation alignement and other fixes (48.94 KB, patch)
2009-01-29 10:10 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Add a shared Measure function (45.92 KB, patch)
2009-02-24 10:20 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Fix conflicts with current trunk. (45.98 KB, patch)
2009-03-10 13:51 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
make the patch compile with current trunk and other fixes according to Karl's comments (45.56 KB, patch)
2009-04-19 03:08 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
GetIntrinsicWidth (6.94 KB, text/xml)
2009-04-19 03:09 PDT, Frédéric Wang (:fredw)
no flags Details
replacement of MeasureChildFrames (47.26 KB, patch)
2009-06-21 05:55 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Testcase for vertical ink extent (1.71 KB, text/xml)
2009-07-04 12:20 PDT, Frédéric Wang (:fredw)
no flags Details
Add a minimal value for height (47.89 KB, patch)
2009-07-04 12:29 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Test dynamic change of notation attribute (1.22 KB, text/xml)
2009-07-19 07:16 PDT, Frédéric Wang (:fredw)
no flags Details
Patch (49.66 KB, patch)
2009-07-19 07:18 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Show why dx_right is necessary (593 bytes, text/xml)
2009-07-20 10:58 PDT, Frédéric Wang (:fredw)
no flags Details
tract -> track (1.66 KB, patch)
2009-07-21 11:06 PDT, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review
Position of radical/longdiv chars + use of dx_left & dx_right + center content inside ellipse + update boundingmetrics ascent & descent (50.97 KB, patch)
2009-07-21 13:18 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
patch menclose (51.03 KB, patch)
2009-07-22 12:29 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
changes according to previous comments (51.27 KB, patch)
2009-08-04 12:50 PDT, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review
Final menclose patch (51.28 KB, patch)
2009-08-05 10:15 PDT, Frédéric Wang (:fredw)
roc: superreview+
Details | Diff | Splinter Review

 Martin Mokrejs 2005-06-12 01:28:07 PDT User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b2) Gecko/20050612 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b2) Gecko/20050612 All these tests are shown wrong: http://www.w3.org/Math/testsuite/testsuite/Presentation/GeneralLayout/menclose/rec-enclose1.xml http://www.w3.org/Math/testsuite/testsuite/Presentation/GeneralLayout/menclose/rec-enclose2.xml http://www.w3.org/Math/testsuite/testsuite/Presentation/GeneralLayout/menclose/rec-enclose3.xml MOZ_DEBUG_FLAGS="-g3 -O0 -ggdb" CFLAGS="-g3 -O0 -ggdb" CXXFLAGS="-g3 -O0 -ggdb" ./configure --enable-debug --disable-optimize --enable-debug-modules=all --enable-debugger-info-modules --enable-detect-webshell-leaks --enable-svg --enable-svg-renderer-libart --enable-image-decoders=all --with-qtdir=/usr/qt/3 --enable-application=suite --disable-freetype2 --enable-jprof --enable-default-toolkit=gtk2 --enable-xft Reproducible: Always rbs 2005-06-12 21:57:55 PDT menclose is not yet implemented. Gervase Markham [:gerv] 2005-09-27 02:04:30 PDT This is an automated message, with ID "auto-resolve01". This bug has had no comments for a long time. Statistically, we have found that bug reports that have not been confirmed by a second user after three months are highly unlikely to be the source of a fix to the code. While your input is very important to us, our resources are limited and so we are asking for your help in focussing our efforts. If you can still reproduce this problem in the latest version of the product (see below for how to obtain a copy) or, for feature requests, if it's not present in the latest version and you still believe we should implement it, please visit the URL of this bug (given at the top of this mail) and add a comment to that effect, giving more reproduction information if you have it. If it is not a problem any longer, you need take no action. If this bug is not changed in any way in the next two weeks, it will be automatically resolved. Thank you for your help in this matter. The latest beta releases can be obtained from: Firefox: http://www.mozilla.org/projects/firefox/ Thunderbird: http://www.mozilla.org/products/thunderbird/releases/1.5beta1.html Seamonkey: http://www.mozilla.org/projects/seamonkey/  Gervase Markham [:gerv] 2005-10-13 10:38:24 PDT This bug has been automatically resolved after a period of inactivity (see above comment). If anyone thinks this is incorrect, they should feel free to reopen it. Martin Mokrejs 2005-11-22 15:24:33 PST Still problem with: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20051122 SeaMonkey/1.5a ./configure --disable-optimize --enable-debug='-g3 -O0' --enable-debug-modules=all --enable-debugger-info-modules --enable-detect-webshell-leaks --enable-svg --enable-svg-renderer-libart --enable-image-decoders=all --with-qtdir=/usr/qt/3 --enable-application=suite --disable-freetype2 --enable-default-toolkit=gtk2 --enable-xft --disable-gssapi Heather L Bailey 2006-01-03 13:39:44 PST Problem continues with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5. is not rendering properly. rbs 2006-01-03 13:56:02 PST  is not yet implemented. Martin Mokrejs 2006-10-13 14:03:49 PDT Still an issue with current trunk build. Martin Mokrejs 2007-01-26 02:03:02 PST Still an issue with current cvs HEAD build: ./configure --disable-optimize --enable-debug='-g3 -O0 -ggdb' --enable-debug-modules=all --enable-debugger-info-modules --enable-detect-webshell-leaks --disable-svg --enable-image-decoders=all --with-qtdir=/usr/qt/3 --enable-application=suite --disable-freetype2 --enable-jprof --enable-default-toolkit=gtk2 --enable-xft --disable-gssapi Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a2pre) Gecko/20070126 SeaMonkey/1.5a distler 2007-05-20 22:14:48 PDT Of the 14 @notation values listed in the MathML2 Specification[1], half are trivially implemented in CSS[2]. Implementing the others would be some work, but would have the advantage of preventing people from coming up with foolish workarounds[3] for the lack of support in Gecko. [1] http://www.w3.org/TR/2003/REC-MathML2-20031021/chapter3.html#presm.menclose [2] http://www.w3.org/TR/2007/WD-mathml-for-css-20070427/#menclose [3] http://golem.ph.utexas.edu/~distler/blog/archives/001264.html Frédéric Wang (:fredw) 2008-05-04 04:32:49 PDT Eliot Kimber recently submitted a related bug where he asks for "strike" attributs in menclose : https://bugzilla.mozilla.org/show_bug.cgi?id=430019 ( note that he also sent a mail to the MathML mailing list entitled "Representing Cancelations of Terms in Fractions" : http://lists.w3.org/Archives/Public/www-math/2008Apr/0195.html ) If someone wants to implement menclose, here is a test page that could help : http://www.maths-informatique-jeux.com/international/mathml_test/menclose.xml Martin Mokrejs 2008-08-27 10:25:13 PDT The display of the original testcase http://www.w3.org/Math/testsuite/testsuite/Presentation/GeneralLayout/menclose/rec-enclose1.xml is badly broken as of current TRUNK. Frédéric Wang (:fredw) 2008-10-29 08:21:54 PDT Created attachment 345289 [details] [diff] [review] parse "notation" attribute + display the radical notation I wrote an experimental patch to prepare the support of the element. For the moment it parses the "notation" attribute to check the enclosing symbols that need to be displayed. I used the code of the element, so the "radical" notation is supported. Frédéric Wang (:fredw) 2008-10-29 08:22:40 PDT Created attachment 345290 [details] testcase Frédéric Wang (:fredw) 2008-10-29 08:25:06 PDT Created attachment 345291 [details] screenshot of the testcase left hand side: the page edited in Amaya. right hand side: the page in Firefox+patch. Frédéric Wang (:fredw) 2008-10-30 04:19:22 PDT Created attachment 345475 [details] [diff] [review] display notations Here is a new version of my patch that allows to display enclosing symbols for 15 notation values (14 from MathML2 + madruwb). Some of them are currently approximated: - diagonal strikes are drawn as horizontal strikes. - circle and roundedbox are drawn as "box". - madruwb is drawn as "bottom+left" Also, padding certainly needs to be increased. Frédéric Wang (:fredw) 2008-10-30 04:20:58 PDT Created attachment 345476 [details] screenshot of my test page http://www.maths-informatique-jeux.com/international/mathml_test/menclose.xml Frédéric Wang (:fredw) 2008-10-31 09:41:10 PDT Created attachment 345730 [details] [diff] [review] Current version of the patch. Waiting for more info from the Math WG about how to deal with several notation values... http://lists.w3.org/Archives/Public/www-math/2008Oct/0002.html Frédéric Wang (:fredw) 2008-11-08 04:13:10 PST Created attachment 347049 [details] [diff] [review] code clean up + add padding Frédéric Wang (:fredw) 2008-11-08 04:16:55 PST Created attachment 347050 [details] Screenshot - MathML testsuite left hand side: current rendering right hand side: sample rendering Frédéric Wang (:fredw) 2008-11-09 15:40:29 PST In my local changes, I added support for new notations: longdiv, roundedbox and madruwb. Hence the remaining notations are now: - circle - updiagonal & downdiagonal strikes Does anyone have an idea about how to implement them ? Karl Tomlinson (:karlt) 2008-11-09 16:36:34 PST nsIRenderingContext has DrawLine() and DrawEllipse() http://mxr.mozilla.org/mozilla-central/source/gfx/public/nsIRenderingContext.h Or you can get the gfxContext from the nsIRenderingContext::ThebesContext() and have a bit more functionality: http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/public/gfxContext.h Frédéric Wang (:fredw) 2008-11-10 13:26:11 PST Created attachment 347352 [details] screenshot - MathML testsuite OK, thanks Karl! I follow your proposal to implement the remaining notations (see screenshot). I've still some improvements to do, but I guess I could propose a patch and ask for a review soon. Frédéric Wang (:fredw) 2008-11-11 07:00:04 PST Created attachment 347532 [details] Show the remaining issues There are 2 minor issues: 1) For madruwb, the character 'ARABIC LETTER LAM INITIAL FORM' does not cover the whole enclosing content (third line of the screenshot). Do I need to add something in the source to make the symbol stretch ? 2) stroke-width of diagonal strikes, roundedbox and ellipse is not the same as the one of the other bars when you zoom-in the page. I know it is possible to use gfxContext to set stroke-width, but I wonder whether there exists something more straightforward. I also have an additional question (not related to menclose): why does RoundedRectangle approximate the rounded angles with Bézier curves? Wouldn't it be better to use elliptical arcs? http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/src/gfxContext.cpp#803 Frédéric Wang (:fredw) 2008-11-11 07:01:30 PST Created attachment 347533 [details] [diff] [review] Patch Karl Tomlinson (:karlt) 2008-11-11 20:04:38 PST (In reply to comment #23) > 1) For madruwb, the character 'ARABIC LETTER LAM INITIAL FORM' does not cover > the whole enclosing content (third line of the screenshot). Do I need to add > something in the source to make the symbol stretch ? This will be difficult. If you have a font called MyFont with support for stretchy LAM, then mathfontMyFont.properties is the file that needs to be set up to describe how the stretched character can be produced. But existing stretched characters either stretch vertically or horizontally but not both. If a font provides parts of the madruwb as separate characters so that the horizontal and vertical components can be considered separately, then perhaps two nsMathMLChars can be used for the two components. If you don't have a font with support for stretchy LAM, then simply scaling a character (which nsMathMLChar does not yet do - bug 414277) may be the best option, but estimating the space to allow for the vertical parts of the stroke is going to involve some heuristics. Have you seen this implemented in other software? What does it do? Also, don't you want U+FEDD ARABIC LETTER LAM ISOLATED FORM, or simply U+0644 ARABIC LETTER LAM here? > 2) stroke-width of diagonal strikes, roundedbox and ellipse is not the same > as the one of the other bars when you zoom-in the page. I know it is > possible to use gfxContext to set stroke-width, but I wonder whether there > exists something more straightforward. I can't think of anything simpler using nsIRenderingContext. Also, I think nsIRenderingContext will eventually be replaced by gfxContext, so it would be better to use gfxContext directly than add to nsIRenderingContext. > I also have an additional question (not related to menclose): why does > RoundedRectangle approximate the rounded angles with Bézier curves? I don't know. Maybe a performance optimization. Frédéric Wang (:fredw) 2008-11-12 10:04:59 PST (In reply to comment #25) > Have you seen this implemented in other software? What does it do? > > Also, don't you want U+FEDD ARABIC LETTER LAM ISOLATED FORM, or simply U+0644 > ARABIC LETTER LAM here? > Actually, I don't know much about arabic notations. I read a pdf file in which it is proposed to represent madruwb by underlining the content, putting a "ARABIC LETTER LAM INITIAL FORM" character on its left and stretching this character vertically. That's what I try to do, but apparently stretching is likely to be really complex. What I understood is that the "madruwb" notation was proposed by moroccan researchers in a W3C note. They made a fork called "Dadzilla" where they work on arabic mathematical notations. I think I should questionned them directly. > I can't think of anything simpler using nsIRenderingContext. I guess you mean gfxContext context here? > > Also, I think nsIRenderingContext will eventually be replaced by gfxContext, > so it would be better to use gfxContext directly than add to > nsIRenderingContext. OK, I'll try to use gfxContext. Frédéric Wang (:fredw) 2008-11-20 16:05:05 PST Created attachment 349302 [details] [diff] [review] all mathml 2 notations +approximation of madruwb Frédéric Wang (:fredw) 2008-11-20 16:06:26 PST Created attachment 349303 [details] screenshot of the mathml testsuite Frédéric Wang (:fredw) 2008-11-20 16:07:52 PST Created attachment 349304 [details] written arabic text with lam letters Frédéric Wang (:fredw) 2008-11-20 16:10:41 PST  Apparently Azzeddine Lazrek & Mustapha Eddahibi don't have examples of an implementation of the madruwb symbol with a curvilinear underline ( http://www.w3.org/TR/arabic-math/#N10AEE ). Eddahibi thinks it should be better to follow TeX directives i.e. drawing the lam symbol with a combination of streched characters. However in my patch, the Lam symbol is currently drawn with curves and lines because it is easier to implement. I submitted screenshot to Azzeddine who asked a calligrapher for advice. Then he sent me some screenshots of text with Lam symbols. Here is a translation of the description he gave: "Lam has a slight curvature, no curvature at the beginning, a slight tail, see attachments". It is not exactly how I draw it. Currently, I haven't a lot of time to improve the madruwb notation. I think the patch can be commited despite its lack of support for the madruwb notation, because it integrates all the notations of MathML 2 that some people asked for. Karl Tomlinson (:karlt) 2008-11-20 17:38:41 PST (In reply to comment #30) > Eddahibi thinks it should be better to follow TeX directives > i.e. drawing the lam symbol with a combination of streched characters. Using curves to connect a set of partial characters for the horizontal component of the Lam would be difficult. Similarly, the slope on the vertical component of the Lam makes constructing that from parts difficult. > However in my patch, the Lam symbol is currently drawn > with curves and lines because it is easier to implement. Yes, that seems the sensible solution. > Currently, I haven't a lot of time to improve the madruwb notation. I think the > patch can be commited despite its lack of support for the madruwb notation, > because it integrates all the notations of MathML 2 that some people asked for. Yes, it's not necessary to add madruwb support at the same time as the other notations. However, what you have for madruwb looks close (to my untrained eye). Karl Tomlinson (:karlt) 2008-11-30 20:16:26 PST Comment on attachment 349302 [details] [diff] [review] all mathml 2 notations +approximation of madruwb I think the precise "right" way to draw madruwb is sufficiently unclear that I don't want the rest of this patch to be held up by spending time on madruwb, so I'd like to consider that separately, after the notations that are listed in MathML2. (I also wonder whether adding a single Arabic notation is going to be useful, with Mozilla's current level of support for other Arabic notations?) This frame type covers drawing of many different notations, but usually only a small subset are actually used. I'd like to reduce the amount of data stored on each of these frames. + nsRect mMenclose; + nsRect mRadicalBar; + nsRect mLeftBar; + nsRect mRightBar; + nsRect mTopBar; + nsRect mBottomBar; + nsRect mVerticalStrike; + nsRect mHorizontalStrike; Can most of these rects be replaced with a single nsRect mEnclosedRect that encloses the contents, and other specific rects be calculated during BuildDisplayLists? +#define NB_CHAR 2 + nsMathMLChar mChar[NB_CHAR]; +#define mLongdivChar mChar[0] +#define mRadicalChar mChar[1] Similarly the nsMathMLChars are usually not used, but they are moderate sized objects. I think an nsTArray that is built up with only the members that are used could work here. In order to locate the particular nsMathMLChars, it will probably be necessary to have mLongDivCharIndex and mRadicalCharIndex or similar. +#define NB_NOTATIONS 15 + PRBool draw_notation[NB_NOTATIONS]; +#define DRAW_LONGDIV draw_notation[0] +#define DRAW_ACTUARIAL draw_notation[1] +#define DRAW_RADICAL draw_notation[2] +#define DRAW_BOX draw_notation[3] +#define DRAW_ROUNDEDBOX draw_notation[4] +... MACROs can often hide what is really happening. A bit map in a single PRUint32 would work well here. The bit-masks for each notation could be defined in the same enum as used for nsDisplayNotation (below). + #define NS_FRAME_ELLIPSE 0 + #define NS_FRAME_ROUNDEDRECTANGLE 1 + #define NS_FRAME_MADRUWB 2 An enum would be better here. Adding entries for eNotationUpDiagonalStrike and eNotationDownDiagonalStrike and unifying nsDisplayMathMLStrike and nsDisplayMathMLFrame into nsDisplayNotation would reduce the number of lines of code. And nsDisplayNotation can be moved to nsMathMLmencloseFrame, as it is only used by that file/class. is equivalent to and so should share implementation. The best way to do this might be to derive nsMathMLmsqrtFrame from nsMathMLmencloseFrame. I think "circle" (ellipse) needs to be large enough to completely enclose the contents. i.e. I think the sample rendering here is wrong: http://www.w3.org/Math/testsuite/build/main/Presentation/GeneralLayout/menclose/menclose1-full.xhtml Consider a matrix (fenced mtable) in a circle. (A circle shouldn't require any padding though.) In nsMathMLmencloseFrame.cpp and nsMathMLmencloseFrame.h, I don't think all those Contributors should be listed. Only list contributors who have written code that you have copied. + virtual PRIntn GetSkipSides() const { return 0; } This need not be provided, because the base class nsMathMLContainerFrame provides an equivalent definition. + void CheckNotation(const nsAString& aNotation); AddNotation would be a clearer name, I think. + static const nsString notationsVals[] = + { NS_LITERAL_STRING("longdiv"), + NS_LITERAL_STRING("actuarial"), + NS_LITERAL_STRING("radical"), + NS_LITERAL_STRING("box"), + NS_LITERAL_STRING("roundedbox"), + NS_LITERAL_STRING("circle"), + NS_LITERAL_STRING("left"), + NS_LITERAL_STRING("right"), + NS_LITERAL_STRING("top"), + NS_LITERAL_STRING("bottom"), + NS_LITERAL_STRING("updiagonalstrike"), + NS_LITERAL_STRING("downdiagonalstrike"), + NS_LITERAL_STRING("verticalstrike"), + NS_LITERAL_STRING("horizontalstrike"), + NS_LITERAL_STRING("madruwb") + }; This is effectively storing each string twice, once for the NS_LITERAL_STRINGs on the right hand side and another (when notationsVals is initialized) in the nsString array on the left hand side. Also, I think static nsStrings will confuse Mozilla's memory leak testing tools. I considered suggesting NS_MULTILINE_LITERAL_STRING(l) with l = NS_LL("..") here but these are not used much and the logic is not very intuitive. I think the easiest thing would be to just use nsAString::EqualsASCII() or EqualsLiteral(). The strings could be in a const char* notations[]. However, instead having a set of conditionals with EqualsLiteral() would make the mapping from the string to the bit mask clearer, and would enable the actuarial and box and nsMathMLChar logic in nsMathMLmencloseFrame::Init() to be moved into CheckNotation(). + // longdiv: + PRUnichar(')'), Can you add this comment please?: "Unicode 5.1 assigns U+27CC to LONG DIVISION, but a right parenthesis renders better with current font support." + mMenclose(), + mRadicalBar(), + mLeftBar(), + mRightBar(), + mTopBar(), + mBottomBar(), + mVerticalStrike(), + mHorizontalStrike() It's not necessary to explicitly initialize fields that have explicitly defined constructors. + // No need to tract the style context given to our MathML chars. tract -> track + gfxCtx->RoundedRectangle( gfxRect( rect.x/u, + rect.y/u, + rect.width/u, + rect.height/u), nsPresContext* presContext = mFrame->PresContext() gfxCtx->RoundedRectangle(presContext->AppUnitsToGfxUnits(rect), + gfxCtx->SetLineWidth(mThickness / GetAppUnitsPerDevPixel(aCtx)); gfxCtx->SetLineWidth(presContext->AppUnitsPerDevPixel(mThickness)); There is also presContext->AppUnitsPerDevPixel() if you need it, so GetAppUnitsPerDevPixel should not be needed. (It should be safe to assume that AppUnitsPerDevPixel is non-zero. We'll have plenty of other problems if that happens.) + aCtx->SetColor(mFrame->GetStyleColor()->mColor); + case NS_FRAME_ELLIPSE: + gfxCtx->SetLineWidth(e); + aCtx->DrawEllipse(rect); + gfxCtx->SetLineWidth(mThickness / GetAppUnitsPerDevPixel(aCtx)); + + if(mDownDiagonal) + aCtx->DrawLine(aRect.x, aRect.y, aRect.XMost(), aRect.YMost()); + else + aCtx->DrawLine(aRect.x, aRect.YMost(), aRect.XMost(), aRect.y); I see that using nsIRenderingContext methods saves some lines of code by performing the NewPath() and Stroke(), but it is best not to mix nsIRenderingContext and gfxContext use, as that would be making assumptions about their interaction. nsIRenderingContext::DrawLine assumes that the line width is always 1: http://hg.mozilla.org/mozilla-central/annotate/7c17efd1d883/gfx/src/thebes/nsThebesRenderingContext.cpp#l434 Can a single SetLineWidth() be used for all mTypes? + default: + gfxCtx->SetLineWidth(e); + aCtx->DrawRect(rect); + break; This is not expected to be executed IIUC. If so, an NS_NOTREACHED would be better here. aTag == nsGkAtoms::mo_ || + aTag == nsGkAtoms::menclose_ || aTag == nsGkAtoms::mfrac_ || else if (aTag == nsGkAtoms::mrow_ || aTag == nsGkAtoms::merror_) newFrame = NS_NewMathMLmrowFrame(mPresShell, aStyleContext); + else if (aTag == nsGkAtoms::menclose_) + newFrame = NS_NewMathMLmencloseFrame(mPresShell, aStyleContext); else if (aTag == nsGkAtoms::math) { // root [itex] element I can't see an obvious order to these sequences of tags, but the two sequences are reasonably consistent in order, and I would like them to remain as consistent as practical. Frédéric Wang (:fredw) 2008-12-01 12:18:12 PST (In reply to comment #32) >I think the precise "right" way to draw madruwb is sufficiently unclear that I >don't want the rest of this patch to be held up by spending time on madruwb, >so I'd like to consider that separately, after the notations that are >listed in MathML2. >(I also wonder whether adding a single Arabic notation is going to be useful, >with Mozilla's current level of support for other Arabic notations?) I agree with your suggestion. Actually, even the authors of the W3C note don't seem to know exactly how to deal with the case of madruwb. Moreover I understood that they are several way to write arabic mathematical notation and that madruwb is supposed to be used in a context where maths are written from right to left, thing that Mozilla does not support. So let's deal with it separately. By the way, I will be too busy in the next weeks in order to modify my patch. I hope I will be able to do it before Firefox 3.1 is released though. > I think "circle" (ellipse) needs to be large enough to completely enclose the > contents. i.e. I think the sample rendering here is wrong: > http://www.w3.org/Math/testsuite/build/main/Presentation/GeneralLayout/menclose/menclose1-full.xhtml > Consider a matrix (fenced mtable) in a circle. > (A circle shouldn't require any padding though.) I can increase the size of the ellipse but be aware that if we want to enclose the whole content, then its size needs to be at least sqrt(2) greater than the box of the content (see attachement below). This means that in the example "nested circle" the outer circle will be twice greater than the bounding box of "abcdefg". This can appear a bit too much and that's why I tried to have a result similar to the one of the sample rendering. >In nsMathMLmencloseFrame.cpp and nsMathMLmencloseFrame.h, I don't think all >those Contributors should be listed. Only list contributors who have written >code that you have copied. I used nsMathMLmsqrt* files and didn't remove the names of contributors. I don't know who made what. >I see that using nsIRenderingContext methods saves some lines of code by >performing the NewPath() and Stroke(), but it is best not to mix >nsIRenderingContext and gfxContext use, as that would be making assumptions >about their interaction Yes, I wrote this before you indicated me that I can use gfxContext... I'll will rewrite this. Frédéric Wang (:fredw) 2008-12-01 13:09:55 PST Created attachment 350820 [details] smaller size for the "circle notation" oops, I forgot to send this. Karl Tomlinson (:karlt) 2008-12-01 13:35:43 PST (In reply to comment #33) > By the way, I will be too busy in the next weeks in order to modify my > patch. I hope I will be able to do it before Firefox 3.1 is released though. Firefox 3.1 has mostly reached feature freeze, sorry, so drivers are unlikely to accept this for 3.1. (Focus is currently on getting bugs ironed out in existing features so that they can be made available to users.) But development has already started on the next release which won't be too far away, so please don't let this stop you. > > I think "circle" (ellipse) needs to be large enough to completely enclose the > > contents. i.e. I think the sample rendering here is wrong: > > http://www.w3.org/Math/testsuite/build/main/Presentation/GeneralLayout/menclose/menclose1-full.xhtml > > Consider a matrix (fenced mtable) in a circle. > > (A circle shouldn't require any padding though.) > > I can increase the size of the ellipse but be aware that if we want to > enclose the whole content, then its size needs to be at least sqrt(2) greater > than the box of the content (see attachement below). This means that in the > example "nested circle" the outer circle will be twice greater than the > bounding box of "abcdefg". This can appear a bit too much and that's why I > tried to have a result similar to the one of the sample rendering. Yes, the issues here stem from the fact that we only have a rectangle describing the content, but I expect erring on the side of too large will avoid the worst situations. Removing the padding will help a little here, and perhaps the size can be reduced by (half?) the line-thickness too, but watch out for ċ. A minimum size might be the best for this. How often do you expect nested circles to be used? If this is a common use case, then perhaps it can be detected and treated specially, but I think that would be best left for a follow-up patch. Frédéric Wang (:fredw) 2008-12-02 06:28:53 PST (In reply to comment #35) > Yes, the issues here stem from the fact that we only have a rectangle > describing the content, but I expect erring on the side of too large will > avoid the worst situations. > > Removing the padding will help a little here, and perhaps the size can be > reduced by (half?) the line-thickness too, but watch out for > ċ. A minimum size might be the > best for this. > > How often do you expect nested circles to be used? > > If this is a common use case, then perhaps it can be detected and treated > specially, but I think that would be best left for a follow-up patch. Actually I don't really see when nested circles are likely to be used, I just took the example of the testsuite to illustrate that the size of nested circles increases faster. For nested boxes, we simply add a padding to the size (arithmetic growth) while in the is case of circle we need to scale the box by a factor k (geometric growth). I'll remove the padding and reduce the line-thickness but this will not change a lot how fast the size of nested circles grows. However, we can imagine that one will not use a lot of circles in the same sequence of nested menclose. So I think we can take k = sqrt(2) (or maybe reduce "by half", that I understand as taking the geometric mean between 1 and sqrt(2), i.e. k = 2^(1/4)). Frédéric Wang (:fredw) 2008-12-21 07:02:15 PST Created attachment 354047 [details] [diff] [review] fix a compiling error + other trivial fixes Frédéric Wang (:fredw) 2008-12-21 09:12:52 PST Created attachment 354056 [details] [diff] [review] several fixes oops, I sent the wrong patch. Frédéric Wang (:fredw) 2008-12-23 13:56:29 PST Created attachment 354331 [details] [diff] [review] all mathml 2 notations This patch addresses almost all the issues, except sharing of implementation between menclose/msqrt and the dynamic allocation of Rects and nsMathMLChars. I tried to use nsTArray as you suggested, but when I do this the characters no longer stretch and I don't understand why. Karl Tomlinson (:karlt) 2009-01-08 20:43:26 PST Created attachment 356126 [details] [diff] [review] nsTArray changes (In reply to comment #39) > I tried to use nsTArray as you suggested, but when > I do this the characters no longer stretch and I don't understand why. This patch applies on top of attachment 354331 [details] [diff] [review]. radical and longdiv stretch around an mfrac for example (but I'm not sure I did anything differently from you to make this work). Frédéric Wang (:fredw) 2009-01-09 13:52:36 PST Created attachment 356243 [details] Testcase: circle notation Frédéric Wang (:fredw) 2009-01-09 13:57:58 PST Created attachment 356245 [details] [diff] [review] Merge the 2 previous patches + make msqrt inherit from menclose OK, thanks it works now. Here is a new patch that makes msqrt inherit from menclose. The remaining issues are: - allocation of rects - add a minimal size for the circle notation Karl Tomlinson (:karlt) 2009-01-09 14:21:29 PST (In reply to comment #42) > - allocation of rects I wasn't actually suggesting an nsTArray on the frame (though maybe that's an option). Can creation of the specific individual rects be delayed until BuildDisplayLists so they don't need to be stored on the frame? Frédéric Wang (:fredw) 2009-01-10 04:42:31 PST Created attachment 356319 [details] [diff] [review] Improve circle notation + dynamic allocation of rects > Can creation of the specific individual rects be delayed > until BuildDisplayLists so they don't need to be stored on the frame? I'm not sure to understand what you mean here. Rects are not only used in BuildDisplayLists but also in FixInterFrameSpacing, so I think they must be stored in nsMathMLmencloseFrame. In this last patch I use nsTArray so that only the rects needed are created. In addition, I improve the "circle" notation (see attachment 356243 [details]). Frédéric Wang (:fredw) 2009-01-21 10:24:43 PST Created attachment 357983 [details] [diff] [review] Updated patch Previous patch was broken due to changes in CSSFrameConstructor.cpp Frédéric Wang (:fredw) 2009-01-21 12:43:49 PST Created attachment 358010 [details] [diff] [review] Updated patch Karl Tomlinson (:karlt) 2009-01-21 21:07:36 PST Comment on attachment 358010 [details] [diff] [review] Updated patch Thanks for making the requested changes, Frédéric. The overall structure is looking good. In the www-math thread, we're still not describing precisely the same interpretation of the spec for the position and size of notations when there are more than one, but I'll follow-up there http://lists.w3.org/Archives/Public/www-math/2009Jan/0015.html + void allocateMathMLChar(PRUint32 mask); + void allocateMathMLRect(PRUint32 mask); + PRBool isToDraw(PRUint32 mask); Member functions in Mozilla should normally start with a capital letter (AllocateMathMLChar, IsToDraw). + PRUint32 notationsToDraw; + nscoord ruleThickness; Member variables should start with "m" (mNotationsToDraw). + if(mask == NOTATION_LONGDIV) + { Mozilla style is a space after the "if" and the block's opening brace on the same line as the condition. See section "Control Structures" at https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide +static const PRUnichar kRadicalChar = PRUnichar(0x221A); The PRUnichar() cast shouldn't be necessary. + (mMathMLChar[i]).SetData(presContext, Char); The first set of parentheses is not necessary. + UpdatePresentationDataFromChildAt(0, -1, + NS_MATHML_COMPRESSED, + NS_MATHML_COMPRESSED); - // 1. The REC says: - // The element leaves both attributes [displaystyle and scriptlevel] - // unchanged within all its arguments. - // 2. The TeXBook (Ch 17. p.141) says that \sqrt is cramped I'd prefer to retain at least comment 2 in nsMathMLmencloseFrame::TransmitAutomaticData so as to give a partial explanation for the NS_MATHML_COMPRESSED arguments. (I can't see comment 1 in the MATHML 3 spec so I'm happy for that to be left out.) + if (!NS_MATHML_HAS_ERROR(mPresentationData.flags)) + { + if(isToDraw(NOTATION_RADICAL)) An early return statement when NS_MATHML_HAS_ERROR would save having to indent the rest of the function. + if(isToDraw(NOTATION_RADICAL) || isToDraw(NOTATION_LONGDIV)) + { + } + else if(isToDraw(NOTATION_CIRCLE)) + { + } + else + { Each of these blocks adjusts aDesiredSize, but they each only consider a subset of the notations and only one of these blocks will be executed. What we need is to consider all notations that will be drawn and set aDesiredSize to the maximum dimensions. + // Increase height + if(bmBase.ascent * ratio < bmBase.ascent + psi || + bmBase.descent * ratio < bmBase.descent + psi) Why do both the ascent and descent need to be greater than the threshold? Isn't it only the height (ascent + descent) that is important? + { + mBoundingMetrics.ascent = bmBase.ascent + psi; + mBoundingMetrics.descent = bmBase.descent + psi; + } + else + { + mBoundingMetrics.ascent = bmBase.ascent * ratio; + mBoundingMetrics.descent = bmBase.descent * ratio; This else block looks like it will skew the position of an ellipse according to whether ascent or descent is larger. The centre of the ellipse should be midway between the top and bottom of the child content. + } + contentDy = mBoundingMetrics.ascent; + aDesiredSize.height = mBoundingMetrics.ascent + mBoundingMetrics.descent; + mBoundingMetrics.ascent = aDesiredSize.height/2; + mBoundingMetrics.descent = aDesiredSize.height/2; + aDesiredSize.ascent = mBoundingMetrics.ascent; This looks like it is moving the baseline to the centre of the ellipse. The baseline of the menclose element should be the same as the baseline of the child content. + mBoundingMetrics.width = bmBase.width * ratio; + mBoundingMetrics.leftBearing = bmBase.leftBearing * ratio; + mBoundingMetrics.rightBearing = bmBase.rightBearing * ratio; + contentDx = (ratio - 1)*bmBase.width/2; + mMenclose.SetRect(0, 0, + aDesiredSize.width, aDesiredSize.height); + There is an inconsistency here. The height of the ellipse depends on the ink extents of the child content while the width depends on the logical (not ink) width. Also multiplying both leftBearing and rightBearing by ratio is not what you want. Think about the extents of the ellipse centred on the child content. + else + { + psi = 3*ruleThickness; + nscoord delta = psi % onePixel; + if (delta) + psi += onePixel - delta; // round up + + if (ruleThickness < onePixel)ruleThickness = onePixel; + + // add a padding + mBoundingMetrics.ascent = bmBase.ascent + psi; + mBoundingMetrics.descent = bmBase.descent + psi; + mBoundingMetrics.width = bmBase.width + 2*psi; If there is only a "top" notation, then space should not be added to the sides nor the bottom. + mBoundingMetrics.leftBearing = bmBase.leftBearing; + mBoundingMetrics.rightBearing = bmBase.rightBearing; The left and right bearing should include the ink of the notations. + contentDy = mBoundingMetrics.ascent; + contentDy = mBoundingMetrics.ascent; + contentDy = aDesiredSize.ascent; + PositionRowChildFrames(contentDx, contentDy); I can't think of a good reason why contentDy should ever not be the position of the baseline (ascent), so this variable should not be needed. +/* virtual */ nscoord +nsMathMLmencloseFrame::GetIntrinsicWidth(nsIRenderingContext* aRenderingContext) +{ + // The child frames form an mrow + nscoord width = nsMathMLContainerFrame::GetIntrinsicWidth(aRenderingContext); + + if(isToDraw(NOTATION_RADICAL)) + { + // Add the width of the radical symbol + width += mMathMLChar[mRadicalCharIndex].GetMaxWidth(PresContext(), + *aRenderingContext); + } + + if(isToDraw(NOTATION_LONGDIV)) + { + // Add the width of the longdiv symbol + width += mMathMLChar[mLongDivCharIndex].GetMaxWidth(PresContext(), + *aRenderingContext); + } + + return width; +} + The width needs to depend on other notations also. This is needed when the menclose is within a mtable. There is a bit of computation involved in finding the maximum width, which would be similar to what Place is measuring, so it would be good to share this code. The main reason why we can't currently use nsMathMLmencloseFrame::Place for measuring the width through nsMathMLContainerFrame::GetIntrinsicWidth is that Place currently uses nsMathMLChar::Stretch, whereas for GetIntrinsicWidth we want to use nsMathMLChar::GetMaxWidth (because the height is not known). I think this can be handled by moving most of the Place measuring (but not the placement) to a method called Measure, which takes a parameter aWidthOnly, which will be TRUE when called from GetIntrinsicWidth. The Measure function would then use Stretch or GetMaxWidth according to aWidthOnly. nsMathMLContainerFrame::GetIntrinsicWidth could then be used for nsMathMLmencloseFrame and could call the Measure virtual method instead of MeasureChildFrames, and nsMathMLContainerFrame::MeasureChildFrames could be renamed to Measure (with the extra argument). (I'm wasn't fond of the situation that we currently have with MeasureChildFrames anyway.) (This would all be simpler if there were a nsMathMLContainerFrame for the implied child mrow around child frames, but I suspect adding an anonymous nsMathMLContainerFrame would be more complex.) (In reply to comment #44) > > Can creation of the specific individual rects be delayed > > until BuildDisplayLists so they don't need to be stored on the frame? > > I'm not sure to understand what you mean here. Rects are not only used in > BuildDisplayLists but also in FixInterFrameSpacing, so I think they must be > stored in nsMathMLmencloseFrame. +nscoord +nsMathMLmencloseFrame::FixInterFrameSpacing(nsHTMLReflowMetrics& aDesiredSize) +{ + nscoord gap = nsMathMLContainerFrame::FixInterFrameSpacing(aDesiredSize); + if (!gap) return 0; + + nsRect rect; + + if(isToDraw(NOTATION_ROUNDEDBOX) || + isToDraw(NOTATION_CIRCLE) || + isToDraw(NOTATION_UPDIAGONALSTRIKE) || + isToDraw(NOTATION_DOWNDIAGONALSTRIKE)) + mMenclose.MoveBy(gap, 0); I think that the only rectangle that needs to be stored on the frame object is a rectangle bounding the child content. All of the other rectangles only need to be moved in FixInterFrameSpacing because they are calculated in Place. If they are instead calculated in BuildDisplayList, then they will not need to be moved. (The position of the radical bar can be determined from mMathMLChar[mRadicalCharIndex] rect). + + if(isToDraw(NOTATION_LONGDIV)) + { + mMathMLChar[mLongDivCharIndex].GetRect(rect); + rect.MoveBy(gap, 0); + mMathMLChar[mLongDivCharIndex].SetRect(rect); + } + + if(isToDraw(NOTATION_RADICAL)) + { + mMathMLChar[mRadicalCharIndex].GetRect(rect); + rect.MoveBy(gap, 0); + mMathMLChar[mRadicalCharIndex].SetRect(rect); I would do this as a loop over each mMathMLChar[i]. +class nsDisplayMathMLFrame : public nsDisplayItem + nsresult DisplayFrame(nsDisplayListBuilder* aBuilder, + nsIFrame* aFrame, const nsRect& aRect, + const nsDisplayListSet& aLists, + nscoord aThickness, PRUint32 aType); I'd prefer to call this nsDisplayNotation and DisplayNotation or similar, because "frame" tends to mean something quite different in Mozilla. +enum { + NS_FRAME_DOWNDIAGONALSTRIKE, + NS_FRAME_ELLIPSE, + NS_FRAME_ROUNDEDRECTANGLE, + NS_FRAME_UPDIAGONALSTRIKE +}; I don't think this needs to be separate from the nsMathMLmencloseFrame::NOTATION_* enum. The identifiers from that enum can also be used in DisplayFrame/DisplayNotation. Karl Tomlinson (:karlt) 2009-01-22 19:37:10 PST (In reply to comment #47) > In the www-math thread, we're still not describing precisely > the same interpretation of the spec for the position and size of notations > when there are more than one, but I'll follow-up there I think I'm starting to see the merits of modifying individual notations so that they look good with the other notations in the same menclose. I'll get my thoughts together and followup to www-math. Karl Tomlinson (:karlt) 2009-01-23 15:45:04 PST (In reply to comment #47) > If there is only a "top" notation, then space should not be added to the sides > nor the bottom. Actually, adding space to the sides may be fine if you think it looks better to have an overhang. It would be nice to have a minimum length for the "top" so that it doesn't ever look like a dot, and always having an overhang is one way of enforcing a minimum. Frédéric Wang (:fredw) 2009-01-29 10:08:34 PST Created attachment 359546 [details] Circle notation: alignment of the center of the ellipse with the baseline Frédéric Wang (:fredw) 2009-01-29 10:10:27 PST Created attachment 359547 [details] [diff] [review] circle notation alignement and other fixes Thanks for the review. Here is a new patch that fixes trivial issues and prevents the circle notation to change the baseline. Now I have to move the determination of the maximum size to an auxiliary function and to improve GetIntrinsicWidth & FixInterFrameSpacing. > What we need is to consider all notations that will be drawn and set > aDesiredSize to the maximum dimensions. You're right. If I want to be consistent with the algorithm I proposed, I should not mix the setting of aDesiredSize and computation of the maximum dimensions. Anyway when I remove this computation from Place the two steps will be really separated. >Why do both the ascent and descent need to be greater than the threshold? >Isn't it only the height (ascent + descent) that is important? >This else block looks like it will skew the position of an ellipse according >to whether ascent or descent is larger. The centre of the ellipse should be >midway between the top and bottom of the child content. >This looks like it is moving the baseline to the centre of the ellipse. The >baseline of the menclose element should be the same as the baseline of the >child content. >There is an inconsistency here. The height of the ellipse depends on the ink >extents of the child content while the width depends on the logical (not ink) >width. Also multiplying both leftBearing and rightBearing by ratio is not >what you want. Think about the extents of the ellipse centred on the child >content. >I can't think of a good reason why contentDy should ever not be the position >of the baseline (ascent), so this variable should not be needed. I don't know why I chosed to move the baseline rather than the ellipse but the last patch should fix all these problems. However, I think another issue can appear when the other notations increase the size: the center moves and the ellipse may overlap with its content (under the assumption that I draw the ellipse inscribed exactly in the new bounding box). > If there is only a "top" notation, then space should not be added to the sides > nor the bottom. > Actually, adding space to the sides may be fine if you think it looks better to > have an overhang. It would be nice to have a minimum length for the "top" so > that it doesn't ever look like a dot, and always having an overhang is one way > of enforcing a minimum. OK, I now add a space only when it's necessary (especially so they are not needed in the case of strikes). Frédéric Wang (:fredw) 2009-01-30 11:54:45 PST Among the rects stored in the frame, I removed all but mMenclose. Now, I wonder whether it would be more straightforward to use mMenclose.width for GetIntrinsicWidth() rather than introducing the function Measure. Why do you think about this? (By the way, I changed the rendering of notation=circle again because it was incorrect in the last patch) Karl Tomlinson (:karlt) 2009-02-05 15:59:04 PST (In reply to comment #52) > Among the rects stored in the frame, I removed all but mMenclose. Now, I wonder > whether it would be more straightforward to use mMenclose.width for > GetIntrinsicWidth() rather than introducing the function Measure. Why do you > think about this? I'm not sure that I understand your proposal fully, but I don't think this deals with the nsMathMLChar::Stretch/GetMaxWidth issue. GetIntrinsicWidth() will be called before Place, when the height of child content (and accurate widths) is not yet known, so it is not possible to set mMenclose accurately. (It doesn't really matter if it does get set, because it will be overwritten with better values, but positions only need to be set/recorded when Place is called with aPlaceOrigin TRUE.) Also, think about whether mMenclose is even needed. I was suggesting keeping one rectangle when I was imagining positioning notations based on child content. If you are positioning notations based on the boundary of the entire menclose element, and aDesiredSize gets set to the rectangle that you need, then this rectangle will be stored for you and available through nsIFrame::GetRect(). Frédéric Wang (:fredw) 2009-02-06 02:47:01 PST > I'm not sure that I understand your proposal fully, but I don't think this > deals with the nsMathMLChar::Stretch/GetMaxWidth issue. > > GetIntrinsicWidth() will be called before Place, when the height of child > content (and accurate widths) is not yet known, so it is not possible to set > mMenclose accurately. (It doesn't really matter if it does get set, because it > will be overwritten with better values, but positions only need to be > set/recorded when Place is called with aPlaceOrigin TRUE.) OK, I was not sure whether "Place" was called before "GetIntrinsicWidth" or not. So I believe I will use a shared "Measure" method as you suggested. > Also, think about whether mMenclose is even needed. I was suggesting keeping > one rectangle when I was imagining positioning notations based on child > content. > If you are positioning notations based on the boundary of the entire menclose > element, and aDesiredSize gets set to the rectangle that you need, then this > rectangle will be stored for you and available through nsIFrame::GetRect(). In this case I think mMenclose is no longer needed so I'll remove it. Frédéric Wang (:fredw) 2009-02-24 10:20:15 PST Created attachment 363908 [details] [diff] [review] Add a shared Measure function Hi Karl, Today I have worked on the patch again. I've implemented the shared function Measure. I don't know how to test the behavior of GetIntrinsicWith, but I hope it is correct now. The code to get the maximum size is closer to the algorithm I proposed on the MathML mailing list. I ignore leftBearing/rightBearing when computing the maximum size and set them to 0 and mBoundingMetrics.width respectively. This should be the only difference to the original code. It does not seem to affect the rendering. Finally, this new version of the patch improves rendering of "circle" notation and no longer uses Rects stored in the frame. Frédéric Wang (:fredw) 2009-03-10 13:51:44 PDT Created attachment 366651 [details] [diff] [review] Fix conflicts with current trunk. Karl Tomlinson (:karlt) 2009-03-10 23:27:41 PDT Comment on attachment 366651 [details] [diff] [review] Fix conflicts with current trunk. Sorry, for the delay on this. I was thinking over a couple of issues. (In reply to comment #47) > I think this can be handled by moving most of the Place measuring (but not > the placement) to a method called Measure, which takes a parameter > aWidthOnly, which will be TRUE when called from GetIntrinsicWidth. What I was trying to achieve here was to do all the measuring in one function. But I overlooked the need to return contentDx for placement, sorry. I now think it will be tidier to perform measuring and placement in the same function. That function could be a non-virtual: PlaceInternal(nsIRenderingContext& aRenderingContext, PRBool aPlaceOrigin, nsHTMLReflowMetrics& aDesiredSize, PRBool aWidthOnly) Currently for mrow we have: nsMathMLContainerFrame::GetIntrinsicWidth -> nsMathMLContainerFrame::MeasureChildFrames -> nsMathMLContainerFrame::Place and for msqrt: nsMathMLmsqrtFrame::GetIntrinsicWidth -> nsMathMLContainerFrame::GetIntrinsicWidth (for measuring child frames only) -> nsMathMLmsqrtFrame::MeasureChildFrames -> nsMathMLContainerFrame::Place These could change, so that for mrow: nsMathMLContainerFrame::GetIntrinsicWidth -> nsMathMLContainerFrame::MeasureForWidth -> nsMathMLContainerFrame::Place and for menclose: (no nsMathMLmencloseFrame::GetIntrinsicWidth) nsMathMLContainerFrame::GetIntrinsicWidth -> nsMathMLmencloseFrame::MeasureForWidth (measure whole frame) -> nsMathMLmencloseFrame::PlaceInternal -> nsMathMLContainerFrame::Place (for measuring child frames only) and also for menclose: nsMathMLmencloseFrame::Place -> nsMathMLmencloseFrame::PlaceInternal -> nsMathMLContainerFrame::Place (for measuring child frames only) (In reply to comment #55) > I don't know how to test the behavior of GetIntrinsicWidth, > but I hope it is correct now. Test with something like: | | abcdefg | | The distance between the two "|"s on each side gives an indication of the difference between the widths returned from Place and GetIntrinsicWidth. If there are stretchy characters involved, then there will be extra space between the two "|"s on the right, from GetIntrinsicWidth (to accommodate larger versions of the stretchy characters), but other notations should have consistent spacing between the "|"s. It looks like space for notations is added twice with this patch as it is now. > The code to get the maximum size is closer to the algorithm I proposed on > the MathML mailing list. I ignore leftBearing/rightBearing when computing the > maximum size and set them to 0 and mBoundingMetrics.width respectively. This > should be the only difference to the original code. It does not seem > to affect the rendering. The nsBoundingMetrics ascent, descent, leftBearing, and rightBearing (should) represent tight ink bounds. That is the extents of the glyphs that are drawn. nsHTMLReflowMetrics and nsBoundingMetrics::width represent logical bounds derived from global font metrics and character advances. These are an indication of how much space should be occupied. But note that nsHTMLReflowMetrics::ascent/height are based on global font metrics, and so tend to be poor metrics for positioning. When positioning adjacent items, sometimes the MathML code uses the ink bounds, sometimes the logical bounds, and sometimes a function of both. When horizontal distances only are involved, often the maximum of the two type of metrics works well. The metrics returned for menclose (including the notations) should keep these distinct meanings. None of these sets of metrics should get smaller when notations are added, though both are often likely to get larger. (Currently it looks like this patch ignores ascent/height of baseSize.) Currently this patch uses ink extents for vertical distances and logical extents for horizontal distances. Consider, for example: Because logical extents are used for the width, the stroke extends beyond the minus horizontally. But ink extents are used for the height, so the stroke is almost horizontal. I think ink extents are probably best used in the vertical sizing calculations (especially because logical vertical metrics are often poor), but either a minimum size or some overlap will be needed. Consider also ·. With ink extents, a box or circle will be close to the dot, which I /think/ would be preferred. With logical extents the notation would enclose a character cell. Currently the box is close vertically but wide horizontally. I think similar metrics should usually be used in each direction if the situation is similar. e.g. all sides of a box should use similar metrics. However different notations may use different metrics. msqrt even uses ink metrics, with a minimum size of a "1" (which is really a logical metric) for the position of the bar, but uses logical metrics for horizontal distances. What do you think is best for strikes? I guess ink extents would work if used with either be a minimum length or some overlap beyond the extents of the glyph. Or should the strike extend across the whole character cell, which I think is normal for CSS line-through/overline/underline text-decorations? If the strike should extend across the whole character cell, then the maximum of ink metrics and an em ascent/height or the size of a 1 (similar to msqrt) could be a suitable vertical metric, given the problems with the logical vertical metrics. > Finally, this new version of the patch improves rendering of "circle" > notation and no longer uses Rects stored in the frame. Great, thanks. + PRUint8 mLongDivCharIndex , mRadicalCharIndex; + // Is the char already allocated? + if (mask & mNotationsToDraw) + return; + Initializing mLongDiv/mRadicalCharIndex in the constructor and testing those variables instead of mNotationsToDraw would make AllocateMathMLChar more self-contained rather than needing to rely on mNotationsToDraw being set elsewhere. + &(mMathMLChar[i]), The parentheses are unnecessary here. + else if (aNotation.EqualsLiteral("circle")) + mNotationsToDraw |= NOTATION_CIRCLE; + else if (aNotation.EqualsLiteral("left")) { + mNotationsToDraw |= NOTATION_LEFT; + } Some of these conditional substatements have braces and some do not. If any substatement has braces, I find it least confusing to have every alternative substatement also use braces. + else + { Put the opening brace beside the "else". + // The TeXBook (Ch 17. p.141) says that \sqrt is cramped + UpdatePresentationDataFromChildAt(0, -1, + NS_MATHML_COMPRESSED, + NS_MATHML_COMPRESSED); I'm not really clear what setting NS_MATHML_COMPRESSED does, but it feels weird changing the rendering because there is a notation. (Strikes, for example, shouldn't change how things are rendered.) Let's do this only when mNotationsToDraw == NOTATION_RADICAL. + mMathMLChar[mRadicalCharIndex].GetRect(rect); + nscoord dx = rect.width; + nscoord dy = nscoord(0.2f * mEmHeight); + rect.SetRect(dx, dy, + mencloseRect.width - dx, mRuleThickness); Use dy = rect.y so that mEmHeight is not needed. + if (IsToDraw(NOTATION_VERTICALSTRIKE)) { + rect.SetRect(0, mencloseRect.height/2, + mencloseRect.width, mRuleThickness); + + rv = DisplayBar(aBuilder, this, rect, aLists); + NS_ENSURE_SUCCESS(rv, rv); + } + + if (IsToDraw(NOTATION_HORIZONTALSTRIKE)) { + rect.SetRect(mencloseRect.width/2, 0, + mRuleThickness, mencloseRect.height); + + rv = DisplayBar(aBuilder, this, rect, aLists); + NS_ENSURE_SUCCESS(rv, rv); + } Should vertical/horizontal be swapped here? Also, consider mRuleThickness/2 when positioning to center the bar. + nscoord psi = 3*mRuleThickness; Mozilla style is to have spaces around binary operators. + if (mRuleThickness < onePixel)mRuleThickness = onePixel; Add a space after ")" too. + if(IsToDraw(NOTATION_ROUNDEDBOX) || + IsToDraw(NOTATION_TOP) || + IsToDraw(NOTATION_LEFT) || + IsToDraw(NOTATION_RIGHT) || + IsToDraw(NOTATION_BOTTOM)) + mBoundingMetrics.width += psi; I don't think we want NOTATION_LEFT here. + // Set vertical parameters + if(!aWidthOnly) Don't bother testing aWidthOnly for simple vertical metric calculations. It doesn't matter if the vertical metrics get set. Added the tests for aWidthOnly adds more code (and may even make it slower). (aWidthOnly does need to be tested when dealing with mMathMLChar though.) + for (PRInt32 i = 0; i < (PRInt32)mMathMLChar.Length(); i++) Use PRUint32 i, so that the cast is not necessary. +class nsDisplayMathMLFrame : public nsDisplayItem class nsDisplayNotation + private: Align "private" consistently with "public". +nsresult +nsMathMLmencloseFrame::nsDisplayNotation(nsDisplayListBuilder* aBuilder, nsMathMLmencloseFrame::DisplayNotation + gfxPoint(rect.XMost(), rect.Y()) + ); Closing parentheses in Mozilla are usually at the end of the line, rather than isolated like this. Karl Tomlinson (:karlt) 2009-03-12 19:42:42 PDT *** Bug 430019 has been marked as a duplicate of this bug. *** Karl Tomlinson (:karlt) 2009-03-13 17:46:45 PDT (In reply to comment #57) > + // The TeXBook (Ch 17. p.141) says that \sqrt is cramped > + UpdatePresentationDataFromChildAt(0, -1, > + NS_MATHML_COMPRESSED, > + NS_MATHML_COMPRESSED); > > I'm not really clear what setting NS_MATHML_COMPRESSED does, but it feels > weird changing the rendering because there is a notation. (Strikes, for > example, shouldn't change how things are rendered.) Let's do this only when > mNotationsToDraw == NOTATION_RADICAL. Actually, I guess when IsToDraw(NOTATION_RADICAL) would be better than when mNotationsToDraw == NOTATION_RADICAL, so that the contents of "horizontalstrike radical" are rendered the same as "radical". Frédéric Wang (:fredw) 2009-04-19 03:08:04 PDT Created attachment 373557 [details] [diff] [review] make the patch compile with current trunk and other fixes according to Karl's comments Finally I found time to work on this patch. The next step is to reorganize the measurement functions. I hope I'll be able to do it next week. I guess fixing bounds computation will require a more careful study, so I can't say when it will be ready. I attach a testcase for GetIntrinsicWidth. Frédéric Wang (:fredw) 2009-04-19 03:09:23 PDT Created attachment 373558 [details] GetIntrinsicWidth Frédéric Wang (:fredw) 2009-06-21 05:55:12 PDT Created attachment 384286 [details] [diff] [review] replacement of MeasureChildFrames Hi Karl, I've just made the requested change to fix the MeasureChildFrames issue and... this change actually removed the unexpected spaces in get_intrinsic_width test! Now the radical symbol is displayed in the same way as before the introduction of menclose. Do you really think it is so important to modify the way ink/logical measures are used? Frédéric Wang (:fredw) 2009-07-04 12:20:58 PDT Created attachment 386838 [details] Testcase for vertical ink extent Frédéric Wang (:fredw) 2009-07-04 12:29:20 PDT Created attachment 386840 [details] [diff] [review] Add a minimal value for height I made two slight modifications to the previous patch. Before the computation of the bounding box of menclose, I check that the base height is not below the one of a "1" (for boxes, circle & strikes). I think this change addresses the issues you mentioned. Also, I replace the IsToDraw function by a macro. Karl Tomlinson (:karlt) 2009-07-15 22:54:07 PDT Comment on attachment 386840 [details] [diff] [review] Add a minimal value for height (In reply to comment #62) > replacement of MeasureChildFrames (In reply to comment #64) > Add a minimal value for height This looks much better, thanks. A mechanism is needed to deal with dynamic changes to the notation attribute. It looks like the AttributeChanged() method is for dealing with this. It's probably easiest to reset the appropriate member variables and recalculate them. Watch out that the notation attribute should not affect msqrt frames. >diff --git a/layout/mathml/nsMathMLContainerFrame.h b/layout/mathml/nsMathMLContainerFrame.h >- // if in an , and so frames implement MeasureChildFrames to >+ // if in an , and so their frames implement MeasureChildFrames to Missed a MeasureChildFrames -> MeasureForWidth >diff --git a/layout/mathml/nsMathMLmencloseFrame.cpp b/layout/mathml/nsMathMLmencloseFrame.cpp >+#define IS_TO_DRAW(mask) (mask & mNotationsToDraw) I preferred the method that you had previously over the macro. If you want to make sure that it is considered for inlining you can move the definition to the class body. Macros have disadvantages. Although many of the disadvantages don't apply in this situation, the general rule should be to avoid macros unless there is a good reason to use them. http://www.research.att.com/~bs/bs_faq2.html#macro http://en.wikipedia.org/wiki/Inline_function#Comparison_to_macros http://www.parashift.com/c++-faq-lite/inline-functions.html#faq-9.5 >+ mMathMLChar.AppendElement(); >+ mMathMLChar[i].SetData(presContext, Char); Should check for out of memory during AppendElement() (unfortunately). >+ nsRect mencloseRect = nsIFrame::GetRect(), rect; >+ mencloseRect.x = mencloseRect.y = 0; rect is better declared separately. >+ if (IS_TO_DRAW(NOTATION_RADICAL)) { >+ rv = mMathMLChar[mRadicalCharIndex].Display(aBuilder, this, aLists); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ mMathMLChar[mRadicalCharIndex].GetRect(rect); >+ nscoord dx = rect.width; >+ nscoord dy = rect.y; >+ rect.SetRect(dx, dy, >+ mencloseRect.width - dx, mRuleThickness); An nsRect declaration with the default constructor is only needed for this block, so move the declaration or rect to here. >+ if (IS_TO_DRAW(NOTATION_BOTTOM)) { >+ rect.SetRect(0, mencloseRect.height - mRuleThickness, >+ mencloseRect.width, mRuleThickness); These other uses can have their own declaration using the appropriate constructor instead of SetRect(). Shouldn't radical and longdiv be handled similarly? Currently the bar for the radical is drawn at the top of the char but the bar for the longdiv is drawn at the top of the menclose. >+ // Set vertical parameters >+ if (((IS_TO_DRAW(NOTATION_TOP) && IS_TO_DRAW(NOTATION_RIGHT) && >+ IS_TO_DRAW(NOTATION_BOTTOM) && IS_TO_DRAW(NOTATION_LEFT)) || >+ IS_TO_DRAW(NOTATION_UPDIAGONALSTRIKE) || >+ IS_TO_DRAW(NOTATION_DOWNDIAGONALSTRIKE) || >+ IS_TO_DRAW(NOTATION_VERTICALSTRIKE) || >+ IS_TO_DRAW(NOTATION_CIRCLE) || >+ IS_TO_DRAW(NOTATION_ROUNDEDBOX)) && >+ No blank line here. >+ bmBase.ascent + bmBase.descent < bmOne.ascent + bmOne.descent Is this condition necessary? It creates a discontinuity when the test depends on the sum but the values being set are ascent and descent individually. >+ ) Closing parentheses in Mozilla are usually at the end of the line, rather than isolated like this. >+ bmBase.descent = PR_MAX(bmOne.descent, bmBase.descent); I wonder whether "PR_MAX(0, bmBase.descent)" would work fine here. Could this block then also apply to radical and longdiv? There is already similar code for radical below, and I can't think why longdiv should be different to radical. It looks like perhaps the existing implementation of msqrt could potentially result in the radical symbol not reaching the baseline, but that seems like a bug. >+ // radical notation: >+ if (IS_TO_DRAW(NOTATION_RADICAL)) { >+ // built-in: adjust clearance psi to emulate \mathstrut using '1' >+ // (TexBook, p.131) >+ if (bmOne.ascent > bmBase.ascent) >+ psi += bmOne.ascent - bmBase.ascent; It looks like this can be removed by adding the difference to bmBase.ascent above. >+ >+ if (mRuleThickness < onePixel) >+ mRuleThickness = onePixel; This can be removed as it has already been done above. >+ psi += onePixel - delta; // round up >+ >+ >+ >+ // Stretch the radical symbol to the appropriate height if it is not One blank line is enough. >+ /////////////// >+ // The maximum size is now computed: set the remaining parameters >+ mBoundingMetrics.leftBearing = 0; >+ mBoundingMetrics.rightBearing = mBoundingMetrics.width; These should also consider the corresponding values in bmBase. >+ >+ if (IS_TO_DRAW(NOTATION_RADICAL)) { >+ // get the leading to be left at the top of the resulting frame >+ // this seems more reliable than using fm->GetLeading() on suspicious >+ // fonts >+ leading = nscoord(0.2f * mEmHeight); >+ >+ aDesiredSize.ascent = mBoundingMetrics.ascent + leading; The leading should be added to the top of the radical bar "bmBase.ascent + psi + mRuleThickness", and aDesiredSize.ascent can be the maximum of the result and mBoundingMetrics.ascent. >+ aDesiredSize.height = aDesiredSize.ascent + >+ PR_MAX(bmBase.descent, >+ mBoundingMetrics.descent + mRuleThickness); Similarly, mRuleThickness should be added to the ink descent for the radical not the entire menclose. Probably worth recording the value calculated for radical above. >+ } >+ else { >+ aDesiredSize.ascent = mBoundingMetrics.ascent; >+ aDesiredSize.height = mBoundingMetrics.ascent + mBoundingMetrics.descent; These should also consider the corresponding values in baseSize. >+ ////////////////// >+ // Set position and size >+ if (IS_TO_DRAW(NOTATION_RADICAL)) >+ mMathMLChar[mRadicalCharIndex].SetRect(nsRect(0, leading, bmRadical.width, >+ bmRadical.ascent + >+ bmRadical.descent)); >+ >+ if (IS_TO_DRAW(NOTATION_LONGDIV)) >+ mMathMLChar[mLongDivCharIndex].SetRect(nsRect(0, 0, bmLongdiv.width, >+ bmLongdiv.ascent + >+ bmLongdiv.descent)); These characters are stretched considering only the content (which makes sense to me) and so should be placed relative to the content rather than relative to the menclose frame. Setting the rects can move to within the "if (aPlaceOrigin) {" block. >+ >+ mReference.x = 0; >+ mReference.y = aDesiredSize.ascent; >+ >+ if (aPlaceOrigin) { >+ ////////////////// >+ // Finish reflowing child frames >+ PositionRowChildFrames(contentDx, aDesiredSize.ascent); >+ } >+ >+ return NS_OK; >+} >+ NS_DISPLAY_DECL_NAME("MathMLFrame") Can you use a more precise name here? >+ private: Align "private" consistently with "public". >+ nscoord mThickness; >+ PRUint32 mType; Alignment. >+void nsDisplayNotation::Paint(nsDisplayListBuilder* aBuilder, >+ nsIRenderingContext* aCtx, >+ const nsRect& aDirtyRect) >+{ >+ // get the gfxRect >+ nsPresContext* presContext = mFrame->PresContext(); >+ gfxRect rect = presContext-> >+ AppUnitsToGfxUnits(mRect + aBuilder->ToReferenceFrame(mFrame)); Does cairo stroke lines inside the path or centered on the path? I suspect this rect needs to be Inset() appropriately. >+ nsIDeviceContext *aContext; >+ aCtx->GetDeviceContext(aContext); >+ gfxFloat e = mThickness / aContext->AppUnitsPerDevPixel(); gfxFloat e = presContext->AppUnitsToGfxUnits(mThickness); >+ gfxCtx->SetLineWidth(e); >+ >+ Only one blank line. >+ gfxCtx->Ellipse(rect.pos + rect.size / 2, rect.size); "/ 2.0" to make it clear that floating point arithmetic is being used. >+ case NOTATION_UPDIAGONALSTRIKE: >+ gfxCtx->Line(gfxPoint(rect.X(), rect.YMost()), >+ gfxPoint(rect.XMost(), rect.Y())); >+ break; >+ >+ case NOTATION_DOWNDIAGONALSTRIKE: >+ gfxCtx->Line(rect.pos, rect.pos + rect.size); >+ break; gfxRect has TopLeft() TopRight() BottomLeft() and BottomRight(), which would makes things clear here. >diff --git a/layout/mathml/nsMathMLmencloseFrame.h b/layout/mathml/nsMathMLmencloseFrame.h >new file mode 100644 >--- /dev/null >+++ b/layout/mathml/nsMathMLmencloseFrame.h >+ nscoord mRuleThickness; Unnecessary spaces. >+ PRInt8 mLongDivCharIndex , mRadicalCharIndex; Remove the space before the ",". >diff --git a/layout/mathml/nsMathMLmsqrtFrame.cpp b/layout/mathml/nsMathMLmsqrtFrame.cpp Can some of the header includes be removed from nsMathMLmsqrtFrame.cpp? >+NS_IMETHODIMP >+nsMathMLmsqrtFrame::Init(nsIContent* aContent, > nsMathMLmsqrtFrame::~nsMathMLmsqrtFrame() > { > } >- >-NS_IMETHODIMP >-nsMathMLmsqrtFrame::Init(nsIContent* aContent, Can the definition of Init remain after the destructor, please? That seems more consistent with other MathML frame implementations, and I don't see any reason for it to move. Frédéric Wang (:fredw) 2009-07-16 14:40:46 PDT > Shouldn't radical and longdiv be handled similarly? Consider this example in the MathML testsuite: http://www.w3.org/Math/testsuite/build/main/Presentation/GeneralLayout/menclose/rec-enclose1-plain.xhtml I think the longdiv notation should match the whole cell of the mtable. It is not the case if we draw longdiv like msqrt. Imagine that we replace 131 by 35. Then the top bar of the longdiv would be below the top of this new number, while in my opinion we want them to be aligned. Karl Tomlinson (:karlt) 2009-07-16 15:55:56 PDT (In reply to comment #66) If I understand your example correctly, for the top bar of the longdiv to be aligned with the top of the divisor in this example, the menclose would need to stretch to fill the height of the mtd. It would be kind of like treating the long division sign as a stretchy operator and menclose as an embellishment of the operator. The spec doesn't say anything about menclose stretching, and many of the other notations I would not expect to stretch. Having the longdiv notation expand to the bounds of the menclose rather than bound the content may be OK, but would require stretching the longdiv char to the height of the menclose. It seems simpler to just stretch according to the height of the content. Frédéric Wang (:fredw) 2009-07-17 09:26:09 PDT Your are right, streching the longdiv to the height of menclose is not enough. In this case, let's treat longdiv like radical. Frédéric Wang (:fredw) 2009-07-19 07:16:58 PDT Created attachment 389368 [details] Test dynamic change of notation attribute Frédéric Wang (:fredw) 2009-07-19 07:18:04 PDT Created attachment 389369 [details] [diff] [review] Patch I think this patch fixes all the problems you mentioned, except for one remark that I do not really understand: > These characters are stretched considering only the content (which makes sense > to me) and so should be placed relative to the content rather than relative to > the menclose frame. Karl Tomlinson (:karlt) 2009-07-19 16:19:26 PDT (In reply to comment #70) > except for one remark that I do not really understand: > > > These characters are stretched considering only the content (which makes > > sense to me) and so should be placed relative to the content rather than > > relative to the menclose frame. The height used for nsMathMLChar::Stretch() is based on the height of the children. However, the position passed to nsMathMLChar::SetRect() is relative to the top of the menclose frame (which may include other notations). It is better to think of positioning the nsMathMLChars relative to the baseline. A vertical position (relative to the top of the menclose) for SetRect() of "aDesiredSize.ascent - bmRadicalChar.ascent" should do the right thing, I think. I'm less fussy about the horizontal position, but I think "contentDx - bmRadicalChar.width" would be better. Karl Tomlinson (:karlt) 2009-07-19 16:23:16 PDT (In reply to comment #71) > "aDesiredSize.ascent - bmRadicalChar.ascent" Actually, aDesiredSize.ascent - radicalAscent. Karl Tomlinson (:karlt) 2009-07-19 16:33:19 PDT + // Update horizontal parameters + contentDx = PR_MAX(contentDx, bmRadicalChar.width); + mBoundingMetrics.width = PR_MAX(mBoundingMetrics.width, + bmRadicalChar.width + bmBase.width); There may be a few places in the patch that are involved in calculation of maximum content offset and maximum width. What I think we really want is the maximum for the contentDx (= distance to content origin) and the maximum distance from content origin to frame right. Then the frame width is the sum of these two values. It may be easier and clearer to have another variable for the distance from content origin to frame right, instead of trying to work with the total width. Frédéric Wang (:fredw) 2009-07-20 09:52:16 PDT (In reply to comment #32) > + // No need to tract the style context given to our MathML chars. > > tract -> track > Some files still have this typo. Do you want I include fixes for them in the menclose patch? fred@localhost:~/mozilla/src/layout/mathml\$ grep "No need to tract" *.{h,cpp} nsMathMLmfencedFrame.cpp: // No need to tract the style contexts given to our MathML chars. nsMathMLmoFrame.cpp:// No need to tract the style context given to our MathML char. nsMathMLmrootFrame.cpp: // No need to tract the style context given to our MathML char. (In reply to comment #71) > It is better to think of positioning the nsMathMLChars relative to the > baseline. A vertical position (relative to the top of the menclose) for > SetRect() of "aDesiredSize.ascent - bmRadicalChar.ascent" should do the right > thing, I think. > > I'm less fussy about the horizontal position, but I think > "contentDx - bmRadicalChar.width" would be better. OK, I'll do that. Thanks. (In reply to comment #73) > + // Update horizontal parameters > + contentDx = PR_MAX(contentDx, bmRadicalChar.width); > + mBoundingMetrics.width = PR_MAX(mBoundingMetrics.width, > + bmRadicalChar.width + bmBase.width); > > There may be a few places in the patch that are involved in calculation of > maximum content offset and maximum width. > > What I think we really want is the maximum for the contentDx (= distance to > content origin) and the maximum distance from content origin to frame right. > Then the frame width is the sum of these two values. > > It may be easier and clearer to have another variable for the distance from > content origin to frame right, instead of trying to work with the total width. Actually, I think we should even use two variables dx_left and dx_right, and try to maximize them. Then contentDx = dx_left and the width is dx_left + bmBase.width + dx_right. dx_right is generally computed in the same way as dx_left (and is 0 for radical & longdiv). Moreover, I think this will fix another issue of the "circle" notation. Currently, when you use for instance "circle radical", the content is not centered horizontally inside the ellipse (compare with "circle" alone). An easy way to fix that is to make dx_right and dx_left equal (by taking the maximum) when the "circle" notation is drawn. For vertical positioning, I think ascents/descents are sufficient. Currently the same problem exists for the "circle" notation (content is not always centered vertically inside the ellipse) but I think we can also fix it as described above. Frédéric Wang (:fredw) 2009-07-20 10:58:09 PDT Created attachment 389511 [details] Show why dx_right is necessary - radical+box: there is no space between content and left bar - radical+circle: some part of the content go out of the ellipse Karl Tomlinson (:karlt) 2009-07-20 13:47:12 PDT (In reply to comment #74) > > tract -> track > > > > Some files still have this typo. Do you want I include fixes for them in the > menclose patch? Yes, would be good to catch those before they spread further, thanks. Either in this patch or a separate patch (which you can attach to this bug). > Actually, I think we should even use two variables dx_left and dx_right, > and try to maximize them. Then contentDx = dx_left and the width is dx_left + > bmBase.width + dx_right. That sounds good. Frédéric Wang (:fredw) 2009-07-21 11:06:06 PDT Created attachment 389721 [details] [diff] [review] tract -> track Frédéric Wang (:fredw) 2009-07-21 13:18:22 PDT Created attachment 389749 [details] [diff] [review] Position of radical/longdiv chars + use of dx_left & dx_right + center content inside ellipse + update boundingmetrics ascent & descent Here are the announced changes. Also, I update mBoundingMetrics ascent & descent after computing aDesiredSize vertical extent for some notations. > rect.SetRect(dx, rect.y, > mencloseRect.width - dx, mRuleThickness); Currently for longdiv & radical, the overbar stops at the left of the mencloseRect (see attachment 389511 [details] ). I wonder whether it should rather cover only the enclosed content. I suppose this would require to store bmBase.width in the menclose frame, in order to replace "mencloseRect.width - dx". Karl Tomlinson (:karlt) 2009-07-21 14:20:20 PDT Comment on attachment 389721 [details] [diff] [review] tract -> track Thank you Karl Tomlinson (:karlt) 2009-07-21 14:27:02 PDT (In reply to comment #78) > Currently for longdiv & radical, the overbar stops at the left of the > mencloseRect (see attachment 389511 [details] ). I wonder whether it should rather cover > only the enclosed content. Probably. > I suppose this would require to store bmBase.width > in the menclose frame, in order to replace "mencloseRect.width - dx". Yes. I think storing that on the frame would be a good solution. I think the shorter bar would be best, but I think I could tolerate a long bar in these unusual situations too. Frédéric Wang (:fredw) 2009-07-22 12:29:40 PDT Created attachment 390040 [details] [diff] [review] patch menclose Frédéric Wang (:fredw) 2009-08-01 05:01:44 PDT + // Set horizontal parameters + if (IsToDraw(NOTATION_ROUNDEDBOX) || + IsToDraw(NOTATION_TOP) || + IsToDraw(NOTATION_LEFT) || + IsToDraw(NOTATION_BOTTOM)) + dx_left = padding; + + if (IsToDraw(NOTATION_ROUNDEDBOX) || + IsToDraw(NOTATION_TOP) || + IsToDraw(NOTATION_RIGHT) || + IsToDraw(NOTATION_BOTTOM)) + dx_right = padding; + Actually, we can apply the instructions above to NOTATION_CIRCLE and always set padding2 to (ratio - 1) * bmBase.width / 2 below (and similarly for vertical parameters). + // Update horizontal parameters + padding2 = padding; + if (bmBase.width * ratio > bmBase.width + 2 * padding2) + padding2 = (ratio - 1) * bmBase.width / 2; + + dx_left = PR_MAX(dx_left, padding2); + dx_right = PR_MAX(dx_right, padding2); Karl Tomlinson (:karlt) 2009-08-02 22:26:08 PDT Comment on attachment 390040 [details] [diff] [review] patch menclose >+nsresult nsMathMLmencloseFrame::AllocateMathMLChar(PRUint32 mask) >+{ >+ // Is the char already allocated? >+ if ((mask == NOTATION_LONGDIV && mLongDivCharIndex >= 0) || >+ (mask == NOTATION_RADICAL && mRadicalCharIndex >= 0)) >+ return NS_OK; >+ >+ // No need to track the style context given to our MathML chars. >+ // The Style System will use Get/SetAdditionalStyleContext() to keep it >+ // up-to-date if dynamic changes arise. >+ PRUint32 i = mMathMLChar.Length(); >+ nsAutoString Char; >+ >+ if (mask == NOTATION_LONGDIV) { >+ Char.Assign(kLongDivChar); >+ mLongDivCharIndex = i; >+ } else if (mask == NOTATION_RADICAL) { >+ Char.Assign(kRadicalChar); >+ mRadicalCharIndex = i; >+ } >+ >+ if (!mMathMLChar.AppendElement()) >+ return NS_ERROR_OUT_OF_MEMORY; >+ >+ if (aNotation.EqualsLiteral("longdiv")) { >+ rv = AllocateMathMLChar(NOTATION_LONGDIV); >+ NS_ENSURE_SUCCESS(rv, rv); >+ mNotationsToDraw |= NOTATION_LONGDIV; If "longdiv" is specified twice but the allocation fails in the first call to AllocateMathMLChar(), then NOTATION_LONGDIV would be (incorrectly) added to mNotationsToDraw after the second AllocateMathMLChar. Check AppendElement() before setting mLongDivCharIndex, I think. >+ nscoord delta = padding % onePixel; >+ if (delta) >+ padding += onePixel - delta; Add "// round up" to the last line here, please. >+ if ((IsToDraw(NOTATION_TOP) && IsToDraw(NOTATION_RIGHT) && >+ IsToDraw(NOTATION_BOTTOM) && IsToDraw(NOTATION_LEFT)) || >+ // set a minimal value for the base height Is there a reason why this shouldn't be done when only NOTATION_LEFT or(/and) NOTATION_RIGHT is present? >+ mBoundingMetrics.leftBearing = bmBase.leftBearing; >+ mBoundingMetrics.rightBearing = bmBase.leftBearing + mBoundingMetrics.width; What is needed here is the maximum ink extents of the notations and the base. I think the following should be close enough: mBoundingMetrics.leftBearing = PR_MIN(0, dx_left + bmBase.leftBearing); mBoundingMetrics.rightBearing = PR_MAX(mBoundingMetrics.width, dx_left + bmBase.rightBearing); >+ aDesiredSize.height = PR_MAX(mBoundingMetrics.ascent + >+ mBoundingMetrics.descent, baseSize.height); Use dimensions relative to the baseline. This would be better: aDesiredSize.height = aDesiredSize.ascent + PR_MAX(mBoundingMetrics.descent, baseSize.height - baseSize.ascent); but >+ aDesiredSize.ascent = PR_MAX(aDesiredSize.ascent, >+ longdivAscent + leading); >+ aDesiredSize.height = PR_MAX(aDesiredSize.height, >+ aDesiredSize.ascent + >+ PR_MAX(bmBase.descent, longdivDescent + >+ mRuleThickness)); this doesn't consider the changes needed in height when ascent becomes longdivAscent + leading. I think it might be best to use another variable for descent and then calculate the maximum ascent and descent rather than maximum ascent and height. >+ // center the content (vertically) I think a better description would be "center the menclose around the content", as it is really the menclose being changed here, not the position of the content (as laid out in my mind, at least). >+ nsDisplayNotation(nsIFrame* aFrame, const nsRect& aRect, >+ nscoord aThickness, PRUint32 aType) >+ PRUint32 mType; >+ nsresult DisplayNotation(nsDisplayListBuilder* aBuilder, >+ nsIFrame* aFrame, const nsRect& aRect, >+ const nsDisplayListSet& aLists, >+ nscoord aThickness, PRUint32 aType); By giving the type of the NOTATION_* enum a name, all these PRUint32 types can be replaced with the specific enum type (which provides better compile-time type checking). (In reply to comment #82) > Actually, we can apply the instructions above to NOTATION_CIRCLE and always > set padding2 to (ratio - 1) * bmBase.width / 2 below (and similarly for > vertical parameters). Yes, please. That would simply things. The rest looks good, thank you. Frédéric Wang (:fredw) 2009-08-04 12:50:45 PDT Created attachment 392551 [details] [diff] [review] changes according to previous comments Karl Tomlinson (:karlt) 2009-08-04 14:56:10 PDT > - // round up psi > delta = psi % onePixel; > if (delta) > - psi += onePixel - delta; > + psi += onePixel - delta; // round up This change is ok, but I was actually asking for a comment for the rounding up of padding (not psi). Thanks for the other changes. I'll see if I can try this out today. Karl Tomlinson (:karlt) 2009-08-04 21:54:00 PDT Comment on attachment 392551 [details] [diff] [review] changes according to previous comments Looks nice! r=me with "// round up" here: >+ nscoord delta = padding % onePixel; >+ if (delta) >+ padding += onePixel - delta; Frédéric Wang (:fredw) 2009-08-05 10:11:04 PDT (In reply to comment #85) > This change is ok, but I was actually asking for a comment for the rounding up > of > padding (not psi). > Oops, sorry :-) I'll fix it. (In reply to comment #86) > (From update of attachment 392551 [details] [diff] [review]) > Looks nice! > r=me with "// round up" here: Thank you for the review! Frédéric Wang (:fredw) 2009-08-05 10:15:43 PDT Created attachment 392745 [details] [diff] [review] Final menclose patch Robert O'Callahan (:roc) (email my personal email if necessary) 2009-08-06 16:19:07 PDT Comment on attachment 392745 [details] [diff] [review] Final menclose patch rubber-stamp=me --- this doesn't really need sr Karl Tomlinson (:karlt) 2009-08-11 18:28:21 PDT Checked in, thanks: http://hg.mozilla.org/mozilla-central/rev/dc8ef04fc953 Needed a small change http://hg.mozilla.org/mozilla-central/rev/7326f167a78b for e:/builds/moz2_slave/mozilla-central-win32-debug/build/layout/mathml/nsMathMLmencloseFrame.cpp(464) : error C2668: 'sqrt' : ambiguous call to overloaded function D:\msvs8\VC\INCLUDE\math.h(581): could be 'long double sqrt(long double)' D:\msvs8\VC\INCLUDE\math.h(533): or 'float sqrt(float)' D:\msvs8\VC\INCLUDE\math.h(128): or 'double sqrt(double)' while trying to match the argument list '(int)' Apparently MSVS has some extra sqrt functions in its math.h, but usually its best to use double constants in double arithmetic anyway. Frédéric Wang (:fredw) 2009-08-11 23:03:05 PDT Thank you. What about attachment 389721 [details] [diff] [review]? Karl Tomlinson (:karlt) 2009-08-12 15:51:12 PDT (In reply to comment #91) > What about attachment 389721 [details] [diff] [review]? Sorry, that slipped my mind. I've made a note to myself to check that in, but it may be several days before I do. Karl Tomlinson (:karlt) 2009-08-16 15:24:42 PDT Comment on attachment 389721 [details] [diff] [review] tract -> track http://hg.mozilla.org/mozilla-central/rev/7cebc1512da6 Eliot Kimber 2009-10-30 13:53:13 PDT Thanks for resolving this issue. I'm trying to determine when I can expect the fixes to be released into Firefox? Quick testing with 3.5.4 on OSX indicates that most, if not all, of menclose is not implemented, so I guess this fix hasn't made its way to the Firefox base. Cheers, Eliot Frédéric Wang (:fredw) 2009-10-31 06:22:38 PDT (In reply to comment #94) > Thanks for resolving this issue. I'm trying to determine when I can expect the > fixes to be released into Firefox? Quick testing with 3.5.4 on OSX indicates > that most, if not all, of menclose is not implemented, so I guess this fix > hasn't made its way to the Firefox base. > > Cheers, > > Eliot Apparently, the target milestone is mozilla 1.9.2 so I guess I will be in Firefox 3.6 Eliot Kimber 2009-11-02 07:31:47 PST Thanks--I wasn't sure how to map the Mozilla releases to Firefox releases. Cheers, Eliot Eliot Kimber 2009-11-03 04:33:35 PST Got the 3.6 beta and the menclose test looks great. Thanks again for getting all this implemented. Cheers, Eliot

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