Last Comment Bug 669713 - In <munderover accent=true> the scriptlevel of the over child is not incremented, even when rendered as a supscript
: In <munderover accent=true> the scriptlevel of the over child is not incremen...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: MathML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: Jonathan Hage
:
Mentors:
Depends on: 668204
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-06 12:11 PDT by Frédéric Wang (:fredw)
Modified: 2011-08-14 05:19 PDT (History)
3 users (show)
Ms2ger: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (446 bytes, text/html)
2011-07-06 12:11 PDT, Frédéric Wang (:fredw)
no flags Details
Code (2.09 KB, patch)
2011-07-11 05:03 PDT, Jonathan Hage
no flags Details | Diff | Splinter Review
Reftest (2.05 KB, patch)
2011-07-11 05:03 PDT, Jonathan Hage
no flags Details | Diff | Splinter Review
Code V2 (2.83 KB, patch)
2011-07-18 00:56 PDT, Jonathan Hage
no flags Details | Diff | Splinter Review
Code V3 (2.83 KB, patch)
2011-07-18 05:12 PDT, Jonathan Hage
no flags Details | Diff | Splinter Review
Code V4 (2.73 KB, patch)
2011-07-18 05:54 PDT, Jonathan Hage
no flags Details | Diff | Splinter Review
Reftests v2 (2.39 KB, patch)
2011-08-03 01:25 PDT, Jonathan Hage
no flags Details | Diff | Splinter Review
Reftests v2 (2.46 KB, patch)
2011-08-03 02:11 PDT, Jonathan Hage
karlt: review+
Details | Diff | Splinter Review
Code v4 (2.96 KB, patch)
2011-08-03 02:21 PDT, Jonathan Hage
karlt: review-
Details | Diff | Splinter Review
Code v5 (3.34 KB, patch)
2011-08-05 02:10 PDT, Jonathan Hage
karlt: review+
Details | Diff | Splinter Review

Description Frédéric Wang (:fredw) 2011-07-06 12:11:39 PDT
Created attachment 544304 [details]
testcase

The MathML3 spec says for munder (and similarly for mover, munderover):

"It always sets displaystyle to "false" within underscript and overscript, but increments scriptlevel by 1 only when accentunder or accent, respectively, are "false". Within base, it always leaves both attributes unchanged. (see Section 3.1.6 Displaystyle and Scriptlevel). "

"If base is an operator with movablelimits="true" (or an embellished operator whose mo element core has movablelimits="true"), and displaystyle="false", then underscript is drawn in a subscript position. In this case, the accentunder attribute is ignored. This is often used for limits on symbols such as &sum;. "

Currently, when we are in the case of an operator with movablelimits="true" and displaystyle="false", the effect of accentunder is taken into account, for example the scriptlevel is not incremented. See the testcase.

A possible fix would be to modify the calls to SetIncrementScriptLevel in TransmitAutomaticData, to force incrementing scriptlevel when movablelimits="true" and displaystyle="false".
Comment 1 Frédéric Wang (:fredw) 2011-07-06 12:40:19 PDT
The elements in the testcase should be rendered as their msup/msub/msubsup counterparts, so one can write a reftest for this bug.
Comment 2 Jonathan Hage 2011-07-11 05:03:17 PDT
Created attachment 545150 [details] [diff] [review]
Code
Comment 3 Jonathan Hage 2011-07-11 05:03:54 PDT
Created attachment 545151 [details] [diff] [review]
Reftest
Comment 4 Frédéric Wang (:fredw) 2011-07-16 05:18:26 PDT
> PRBool subsupDisplay =
>		NS_MATHML_EMBELLISH_IS_MOVABLELIMITS(mEmbellishData.flags)
>		&& !NS_MATHML_IS_DISPLAYSTYLE(mPresentationData.flags);

The operator && should be placed at the end of a line, not the beginning.
subsupDisplay could be defined just before the if statement above and used in that statement.
Comment 5 Frédéric Wang (:fredw) 2011-07-16 05:20:08 PDT
I guess this patch can not solve the problem for munder/mover without the fix for bug 668204.
Comment 6 Jonathan Hage 2011-07-18 00:56:00 PDT
Created attachment 546470 [details] [diff] [review]
Code V2
Comment 7 Jonathan Hage 2011-07-18 05:12:13 PDT
Created attachment 546499 [details] [diff] [review]
Code V3
Comment 8 Jonathan Hage 2011-07-18 05:54:18 PDT
Created attachment 546509 [details] [diff] [review]
Code V4
Comment 9 Frédéric Wang (:fredw) 2011-07-18 22:00:13 PDT
(In reply to comment #8)
> Created attachment 546509 [details] [diff] [review] [review]
> Code V4

FYI, this patch misses a mercurial header with commit message.
Comment 10 Karl Tomlinson (:karlt) 2011-07-28 22:29:32 PDT
Comment on attachment 545151 [details] [diff] [review]
Reftest

I wonder whether these <math> elements should have an explicit
displaystyle="false" attribute as the test depends on this and "When the
display attribute is missing, a rendering agent is free to initialize as
appropriate to the context."

Hopefully we'll add further scriplevel tests, so the test file name should
include a number.  It would also be helpful to indicate that this is a
moveablelimits test, so perhaps "scriptlevel-moveablelimits-1.html".
Comment 11 Jonathan Hage 2011-08-03 01:25:31 PDT
Created attachment 550320 [details] [diff] [review]
Reftests v2
Comment 12 Jonathan Hage 2011-08-03 02:11:30 PDT
Created attachment 550322 [details] [diff] [review]
Reftests v2
Comment 13 Jonathan Hage 2011-08-03 02:21:16 PDT
Created attachment 550324 [details] [diff] [review]
Code v4
Comment 14 Karl Tomlinson (:karlt) 2011-08-03 21:57:52 PDT
Comment on attachment 550324 [details] [diff] [review]
Code v4

>   /* The REC says:
>      Within underscript, <munderover> always sets displaystyle to "false",
>      but increments scriptlevel by 1 only when accentunder is "false". 
>@@ -283,28 +287,28 @@ XXX The winner is the outermost setting 
>      that math accents and \overline change uncramped styles to their
>      cramped counterparts.
>   */
>   if (tag == nsGkAtoms::mover_ ||
>       tag == nsGkAtoms::munderover_) {
>     PRUint32 compress = NS_MATHML_EMBELLISH_IS_ACCENTOVER(mEmbellishData.flags)
>       ? NS_MATHML_COMPRESSED : 0;
>     SetIncrementScriptLevel(tag == nsGkAtoms::mover_ ? 1 : 2,
>-                            !NS_MATHML_EMBELLISH_IS_ACCENTOVER(mEmbellishData.flags));
>+                            !NS_MATHML_EMBELLISH_IS_ACCENTOVER(mEmbellishData.flags || subsupDisplay));

The logic here is not right.  subsupDisplay is boolean while flags is a bitmask.
It would probably be tidiest to add an "increment" boolean variable so the logic doesn't need to be duplicated.

Can you update the "REC says" comment to describe what the new logic supports, please?
Comment 15 Jonathan Hage 2011-08-05 02:10:33 PDT
Created attachment 550979 [details] [diff] [review]
Code v5

The logic is not duplicated, we have one hand accentover and the other hand accentunder.    
I updated the "REC says" but I'm not sure if it's what you want.
Comment 16 :Ms2ger 2011-08-14 03:21:06 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/7e6ca29cfa7c

In the future, could you please provide a commit message that include bug number, description of what the patch does, and reviewer?
Comment 17 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-08-14 05:19:29 PDT
http://hg.mozilla.org/mozilla-central/rev/7e6ca29cfa7c

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