As I read the spec, it sounds like some of the changes in bug 739556 were a mistake. (In reply to Frédéric Wang from bug 739556 comment #0) > The MathML spec seems to only define the meaning of the selection attribute > when the actiontype attribute is "toggle". I can see that selection shouldn't apply to statusline and tooltip, but http://www.w3.org/TR/MathML3/chapter3.html#id.3.7.1.1 says "By default, MathML applications that do not recognize the specified actiontype should render the selected sub-expression as defined below" and then goes on to describe the actiontype attribute and the effect of the selection attribute. I assume this should also include the case when there is no actiontype attribute. In our code, if mActionType == NS_MATHML_ACTION_TYPE_NONE, the selection attribute should have effect (if there is more than one child).

(In reply to Karl Tomlinson (:karlt) from comment #0) > I assume this should also include the case when there is no actiontype > attribute. I see now that actiontype is a required attribute, so perhaps there isn't a defined behavior when there is no actiontype attribute. However, the default behavior for unrecognized actiontype attributes is clear even though "Conforming renderers may ignore any value they do not handle".

I guess an maction without actiontype should return a ReflowError so that an "invalid-markup" message is displayed. We should really use NS_MATHML_ACTION_TYPE_UNKNOWN or a similar name instead of NS_MATHML_ACTION_TYPE_NONE when there is a non-standard actiontype. The "Conforming renderers may ignore any value they do not handle" made me think selection attribute could be ignored by default, but now I'm thinking that it could be useful to take it into account to help authors implementing non-standard actiontype, such as the "menu" of the MathML1. I'm going to open a bug to say more. @Andrii: maybe you should fix this bug before trying to implement the "tooltip".

In this patch 2 issues were fixed: 1. Now unknown actiontype takes selection attribute into account. 2. When actiontype is not specified (is empty) or when selection attribute is invalid, mark this as a MathML error.

Just three remarks: - We don't have an official answer from the Math WG. But I think it is fine to do this change now. - Maybe finally it would be best to keep NS_MATHML_ACTION_TYPE_NONE instead of NS_MATHML_ACTION_TYPE_ERROR, use a boolean member mSelectionOutOfRange and call ReflowError when mActionType == NS_MATHML_ACTION_TYPE_ERROR or mSelectionOutOfRange (of course mSelectionOutOfRange can only be true when actiontype=toggle or unknown). First, mActionType is not really a correct name when the selection is out of range. Next, we will more clearly understand how to update these values when doing dynamic changes. - Can you please write more reftests to verify what happen when actiontype is not given or unknown with different selections (maybe compare with an mrow or an element giving a reflow error)? Or when the selection is out of range (with various actiontypes). Also, dynamic tests to see what happen when the selection is changed to become out of range or changed to become valid? Idem for actiontype.

Comment on attachment 629335 [details] [diff] [review] dynamic reftests m4 seems to be marked invalid because actiontype=""? Is there a requirement that the actiontype attribute be non-empty? A missing attribute can be distinguished from an empty value by checking the return value from nsIContent::GetAttr(). Please add a comment to indicate that <msup><mi>x</mi></msup> is to generate an invalid markup error, and that the test makes the assumption that different invalid <math> elements will look the same. I doubt different invalid markups should necessarily all look the same, but I guess we can remove those parts of the test if and when they look different. The name, "maction-dynamic-selection" may be a bit misleading given many of the tests test the actiontype attribute more than the selection attribute. "maction-dynamic-3" may be better.

Comment on attachment 629333 [details] [diff] [review] patch v2 > nsMathMLmactionFrame::Place(nsRenderingContext& aRenderingContext, > bool aPlaceOrigin, > nsHTMLReflowMetrics& aDesiredSize) > { >+ // It is an error when there is no action type. >+ if (mRenderingError || mActionType == NS_MATHML_ACTION_TYPE_NONE) { >+ return ReflowError(aRenderingContext, aDesiredSize); >+ } >+ > aDesiredSize.width = aDesiredSize.height = 0; > aDesiredSize.ascent = 0; > mBoundingMetrics = nsBoundingMetrics(); > nsIFrame* childFrame = GetSelectedFrame(); mRenderingError is used before GetSelectedFrame() is called, and so may not be set correctly. This may happen when finding the intrinsic width of the frame (for a table, for example). It is awkward having to remember to call GetSelectedFrame() at the right time. I suspect the right thing to do would be to update these variables when things change (i.e. in AttributeChanged, ChildListChanged - there may be others), but making that change may be major and shouldn't be part of this patch. Just change the order for this patch. >+ // report an error if something wrong was found in this frame >+ // we can't call nsDisplayMathMLError from here >+ // so ask nsMathMLContainerFrame to do the work for us Start sentences with Capital letters and end with a full stop. >+ // We have two groups of action types. >+ // selection attribute is applied to one group and >+ // not applied to another. Therefore, we need to initiate >+ // a reflow when switching from one group to another. >+ // Note: we should always initiate a reflow when >+ // changing from/to NS_MATHML_ACTION_TYPE_NONE >+ if ((oldActionType & NS_MATHML_ACTION_TYPE_BITMASK_IGNORE_SELECTION) != >+ (mActionType & NS_MATHML_ACTION_TYPE_BITMASK_IGNORE_SELECTION) || >+ (oldActionType == NS_MATHML_ACTION_TYPE_NONE) != // != is xor here >+ (mActionType == NS_MATHML_ACTION_TYPE_NONE)) { > // When the selection attribute is changed we have to initiate a reflow >+ // only when actiontype belongs to the first group (none, unknown or toggle). >+ if (!(mActionType & NS_MATHML_ACTION_TYPE_BITMASK_IGNORE_SELECTION)) { ACTION_TYPE_NONE is not affected by selection. I suspect it would be simpler to have three classes instead of the single bit BITMASK_IGNORE_SELECTION: ACTION_TYPE_CLASS_ERROR, ACTION_TYPE_CLASS_USE_SELECTION, ACTION_TYPE_CLASS_USE_FIRST. Then also define ACTION_TYPE_CLASS_MASK, and define STATUSLINE, TOGGLE, UNKNOWN, and TOOLTIP using these classes. Then reflow is only necessary when changing class. > nsString mRestyle; >+ bool mRenderingError; Can you be more specific with naming mRenderingError, please? Perhaps mHaveSelectionError. Would you mind removing mRestyle while you are here please? That is unused.

(In reply to Frédéric Wang (:fredw) from comment #4) > use a boolean member mSelectionOutOfRange mSelectionIsOutOfRange would be a good name too. However, there is also something odd in GetSelectedFrame because TransmitAutomaticData is called when selection differs from mSelection, but it is not necessarily called when the selection goes out of range (because mSelection may stay the same). It may be best to remove mRenderingError and use a special value for mSelection (e.g. -1) to mean out of range.

(In reply to Karl Tomlinson (:karlt) from comment #7) > m4 seems to be marked invalid because actiontype=""? > Is there a requirement that the actiontype attribute be non-empty? > A missing attribute can be distinguished from an empty value by checking the > return value from nsIContent::GetAttr(). No, that's a mistake. The test should really call removeAttribute instead of setting it to "".

Comment on attachment 630795 [details] [diff] [review] patch v3 This is looking good, thanks. >+ if ((mActionType & NS_MATHML_ACTION_TYPE_CLASS_BITMASK) == >+ NS_MATHML_ACTION_TYPE_CLASS_ERROR) { >+ // Mark mSelection as an error. >+ mSelection = -1; >+ mSelectedFrame = mFrames.FirstChild(); It would make more sense to set mSelectedFrame to NULL here. The code can already handle the situation where there are no children, so it must be OK with a null here. >+ if (mSelection == -1) { >+ return ReflowError(aRenderingContext, aDesiredSize); >+ } >+ > aDesiredSize.width = aDesiredSize.height = 0; > aDesiredSize.ascent = 0; > mBoundingMetrics = nsBoundingMetrics(); > nsIFrame* childFrame = GetSelectedFrame(); Still need to call GetSelectedFrame to set mSelection before using it. Just move the "nsIFrame* childFrame = GetSelectedFrame();" line to before the mSelection line.

Fixed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bdb634caef71 https://hg.mozilla.org/integration/mozilla-inbound/rev/4c627221444a

And backed out due to Android reftest failures. Note that the Try push showed the same failures. https://hg.mozilla.org/integration/mozilla-inbound/rev/9373ecc2dd0d https://tbpl.mozilla.org/php/getParsedLog.php?id=12521176&tree=Mozilla-Inbound The reftest analyzer shows it as a slight pixel variation in one spot.

I suspect part of the antialiasing at the left of __1__ has not been erased properly. See whether the text-rendering: optimizeLegibility CSS property helps. If not, try replacing '_' with a character that won't overlap the edges of its advance box.

Test results with "_" replaced by "-": https://tbpl.mozilla.org/?tree=Try&rev=d284a6e82f88 Android reftests seem to fail, but the summary is empty...

Changed "_" characters to "-".

https://hg.mozilla.org/integration/mozilla-inbound/rev/129be8143484 https://hg.mozilla.org/integration/mozilla-inbound/rev/5e7422731ea0

https://hg.mozilla.org/mozilla-central/rev/129be8143484 https://hg.mozilla.org/mozilla-central/rev/5e7422731ea0