A future version of MathML3 will add "phasorangle" to the list of suggested notations: http://www.w3.org/Math/draft-spec/chapter3-d.html#id.3.3.9.3 Comment from Neil Soiffer: "The key part (at least for me to implement it in MathPlayer) was to see that it uses a slope of 2 (angle = tan^-1(2)) for the angled line and that value seems to work reasonably well. It was relatively simple to implement in MathPlayer. I and the rest of the MathML WG hope that it will be easy to add to MathJax, FireFox, and Webkit." For people willing to work on this: the code to modify is http://dxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLmencloseFrame.cpp probably the bottom line will be drawn with the same line as NOTATION_BOTTOM while the other line will need a specific drawing in nsDisplayNotation::Paint. This will be very similar to bug 875294, so attachment 757830 [details] [diff] [review] should help to find the places to modify.

I'd like to take it up. I don't have knowledge of the layout. So, it take time to understand. Please pardon me developing slow.

proposed patch

Thanks, I haven't tested but the patch looks good. I'm not familiar with this phasorangle notation, but it seems to me that with your code the slanted line may be drawn over the content. Is that what we want? If not, I think you must update dx_left to leave some space to draw that slanted line. Also, can you please create a reftest: https://developer.mozilla.org/en-US/docs/Creating_reftest-based_unit_tests For example a != reftest <menclose> vs <menclose notation="phasorangle"> You can also do something more complicated to get a == reftest. First, draw your notation in a phasorangle.html page with <div style="position: absolute; top: 20px; left; 20px"> <math> <mspace height="100px" depth="100px"/> <menclose notation="phasorangle"><mspace width="100px" height="50px" mathbackground="red"/></menclose> </math> </div> then on the same page, use <div style="position: absolute; top: 20px; left; 20px"> <math> <mspace height="100px" depth="100px"/> ??? </math> </div> where ??? are <mspace> rectangles (with a mathbackground) positioned with <mpadded> at the places you expect to see your phasorangle lines (i.e. on the left and at the bottom). So your rectangles should hide the phasorangle lines. The <mspace height="100px" depth="100px"/> ensures that the two <math> elements have the same height. This means that if you copy this in a phasorangle-ref.html page and just modify the color of the phasorangle lines (e.g. by using <menclose notation="phasorangle" mathcolor="red">) then the two pages should still be the same, unless the phasorangle lines are outside the area you delimited with your rectangles. You can do something even more accurate using SVG and Javascript to hide the phasorangle lines. See the testcase for MathJax: https://github.com/mathjax/MathJax-test/tree/master/testsuite/MathMLToDisplay/Presentation/GeneralLayout/menclose Actually, I don't think we have such tests for the other menclose notations, so that would be great if you can write tests for these notations too or import those from MathJax.

Branko, any update on this?

Hi Frédéric, Thank you for your explanation the other day! I hope this patch is fine now. I will create the reftest. Cheers, Branko

Comment on attachment 8371124 [details] testcase No need to ask review for a testcase.

Comment on attachment 8371137 [details] [diff] [review] patch 2 Review of attachment 8371137 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Branko. I have left some comments. Since you use the BOTTOM notation to draw the horizontal line, I now think this will be a bit different than LONGDIV and RADICAL. What you want is - adding kPhasorangleWidth * mRuleThickness to dx_left to leave room for the notation (what you did) - set the bottom metrics (ink and logical). This will be automatically be done by the BOTTOM notation. - update the top metrics if necessary. You can only compute it once you have the final logical bottom. Then it is obtained by moving up from the bottom by the angled line height (2 * kPhasorangleWidth * mRuleThickness). ::: layout/mathml/nsMathMLmencloseFrame.cpp @@ +289,5 @@ > + if (IsToDraw(NOTATION_PHASORANGLE)) { > + DisplayNotation(aBuilder, this, mencloseRect, aLists, > + mRuleThickness, NOTATION_PHASORANGLE); > + } > + The empty lines should not contain whitespace. Please verify the indentation level of content of the if statement. See https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style @@ +447,5 @@ > + phasorangleAscent); > + mBoundingMetrics.descent = std::max(mBoundingMetrics.descent, > + phasorangleDescent); > + } > + } The computation of the vertical metrics might be a bit different (see my initial comment). I suspect this should be adapted and moved below. @@ +644,1 @@ > mBoundingMetrics.ascent = aDesiredSize.TopAscent(); I don't think that's appropriate to do that for NOTATION_PHASORANGLE since we don't have "top" notation. I think you will need to compute the vertical metrics differently (see my initial comment). @@ +651,5 @@ > IsToDraw(NOTATION_DOWNDIAGONALSTRIKE) || > IsToDraw(NOTATION_VERTICALSTRIKE) || > IsToDraw(NOTATION_CIRCLE) || > + IsToDraw(NOTATION_ROUNDEDBOX) || > + IsToDraw(NOTATION_PHASORANGLE)) This is redundant since you set NOTATION_BOTTOM above. @@ +850,5 @@ > + // that uses a slope of 2 (angle = tan^-1(2)). > + // H = w * tan(angle) = w * 2 > + > + gfxFloat H = rect.Height(); > + gfxFloat w = .5*H; there should be space around the * operator. It's possible that the height of the frame is no longer the one you originally computed so you won't get the desired slope (try notation="phasorangle top circle"). I think what you want is w = gfxFloat(kPhasorangleWidth) * e; and H = 2 * w then the line is now from rect.BottomLeft() to rect.BottomLeft() - gfxPoint(w, H) (note that you actually didn't use H in your code) @@ +859,5 @@ > + gfxCtx->Stroke(); > + > + } > + break; > + Please remove the whitespace at the end of lines (https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Whitespace) and put the break inside the braces (https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Control_Structures)

Note: alternatively, if you don't want to use the BOTTOM line and do as radical/longdiv, you will need to call DisplayBar to draw the bottom line. And the coordinates of the angled line should be computed from the position of that bar rather than from the bottom of the frame.

Thanks for the comments, Frédéric. I have added another patch.

Comment on attachment 8376743 [details] [diff] [review] menclose-phasorangle-v3.patch Review of attachment 8376743 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Branko. That's almost ready. For the reftests, cf bug 963453 and those already available in http://mxr.mozilla.org/mozilla-central/source/layout/reftests/mathml/ ::: layout/mathml/nsMathMLmencloseFrame.cpp @@ +431,5 @@ > + > + // Update horizontal parameters > + dx_left = std::max(dx_left, phasorangleWidth); > + } > + No need for this empty line before the "Update horizontal parameters" comment. Also please check the alignment and remove extra trailing space at the end of lines (in other places too). @@ +638,5 @@ > > + // phasorangle notation: > + // move up from the bottom by the angled line height > + if (IsToDraw(NOTATION_PHASORANGLE) && !aWidthOnly) > + mBoundingMetrics.ascent = 2 * kPhasorangleWidth * mRuleThickness - mBoundingMetrics.descent; So I think this should be mBoundingMetrics.ascent = std::max(mBoundingMetrics.ascent, 2 * kPhasorangleWidth * mRuleThickness - mBoundingMetrics.descent) to take into account other notations with larger ascent. Also, don't bother testing aWidthOnly. If we are only measuring the width, mBoundingMetrics.ascent will be set with irrelevant value but won't be used anyway.

I'm cc'ing Anuj since he worked on menclose tests.

Thanks, Frédéric. Updated patch.

Oooups, I have just noticed the misalignment in the patch v4. It was aligned properly on my system. I had a tab char there which was differently interpreted. OK, I have fixed it now. Patch v5.

Comment on attachment 8378674 [details] [diff] [review] patch v5 Review of attachment 8378674 [details] [diff] [review]: ----------------------------------------------------------------- OK, that looks good, thanks. Would you mind waiting that bug 963453 is fixed (hopefully soon) and update your patch again. I think it would be best to combine the != test menclose-1r with a == test menclose-2-phasorangle (you can already start to write such a reftest). I'll then submit your patch to try server.

@Branko: now that bug 963453 is fixed, can you try to write menclose2-* menclose-5, menclose-6 tests for phasorangle?

@fredw: Any updates here?

This is the patch for menclose 5 and menclose 6 tests. Please review.

Comment on attachment 8400513 [details] [diff] [review] phasorangle.patch Review of attachment 8400513 [details] [diff] [review]: ----------------------------------------------------------------- The tests look good. However, your patch is weird: the menclose-5-phasorangle.html and menclose-6-phasorangle.html are marked as deletion and reftest.list is marked as a new file. Could you try to use to get a proper format: https://developer.mozilla.org/en-US/docs/Mercurial_Queues You could import Branko's patch and either directly do the modifications or create a separate patch on top of it. Also, could you try to write a menclose-2-phasorangle test (see what Anuj did for the other notations).

Submitting revised patch with added menclose-2 tests. Please review. Thank you. Aniket.

Comment on attachment 8409780 [details] [diff] [review] Revised patch Review of attachment 8409780 [details] [diff] [review]: ----------------------------------------------------------------- The patch seems to delete menclose-6-ref.html... I think you don't need to add menclose-6-phasorangle-ref.html, but instead just reuse the existing menclose-6-ref.html. ::: layout/reftests/mathml/menclose-2-phasorangle.html @@ +8,5 @@ > + { > + var box = document.getElementById("box").getBoundingClientRect(); > + document.getElementById("path").setAttribute("d", > + "M" + (box.left + "," + (box.top + box.bottom)/2 ) + " " + > + "l" + (box.width + "," + (-box.height))); I'm not sure I understand that one. This is supposed to cover the drawing of the phasorangle implemented by Branko's patch. I only see a diagonal line here. ::: layout/reftests/mathml/reftest.list @@ +260,5 @@ > != rowlines-3-1.html rowlines-3-1-ref.html > == rowlines-3-2.html rowlines-3-2-ref.html > +== menclose-6-phasorangle.html menclose-6-phasorangle-ref.html > +== menclose-5-phasorangle.html menclose-5-phasorangle-ref.html > +== menclose-2-phasorangle.html menclose-2-phasorangle-ref.html perhaps you can add these lines at a logical place (near the existing menclose-* tests and preserving the order)

Okay! Will make the said changes and will submit the patch soon! Thank you!

Comment on attachment 8419555 [details] [diff] [review] added menclose notation phasorangle with reftest Review of attachment 8419555 [details] [diff] [review]: ----------------------------------------------------------------- thanks

@Anuj: it seems that there are a few test failures for the phasorangle test. As usual, we probably want to increase a bit the length and thickness of the bar, as necessary.

(In reply to Frédéric Wang (:fredw) from comment #27) > @Anuj: it seems that there are a few test failures for the phasorangle test. > As usual, we probably want to increase a bit the length and thickness of the > bar, as necessary. Tests are failing on Android Build and B2G ICS Emulator Opt, I think we should mark them fails-if.

(In reply to Anuj Agarwal [:anujagarwal464] from comment #28) > Tests are failing on Android Build and B2G ICS Emulator Opt, I think we > should mark them fails-if. Did you try to use the reftest-analyser? I checked a few of them and I only saw small pixel diffs due to antialiasing. So increasing a bit the length/thickness should work in these cases.

B2G ICS Emulator Opt > max difference: 255, number of differing pixels: 81 Increased the stroke width and length of sloped line.

By mistake uploaded old patch.

https://tbpl.mozilla.org/?tree=Try&rev=b8cd25bbe94f For this and bug 848725, see https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities#Checkin_comment for commit message format.

Still seems to fail on B2G ICS Emulator Opt. The problem seems to be that the rulethicness is > 1 so the length of the sloped line is larger. Perhaps you should do w = 8 * 2 to be less strict.

Trying with stroke-width=5px: https://tbpl.mozilla.org/?tree=Try&rev=dae2a679d2aa

stroke-width: 5px

https://developer.mozilla.org/en-US/Firefox/Releases/32 https://developer.mozilla.org/en-US/docs/Web/MathML/Element/menclose