Last Comment Bug 749044 - selection attribute on maction should be considered by default
: selection attribute on maction should be considered by default
Status: RESOLVED FIXED
: dev-doc-complete, regression
Product: Core
Classification: Components
Component: MathML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Andrii Zui
:
Mentors:
Depends on:
Blocks: maction 749103 761832 maction-selection
  Show dependency treegraph
 
Reported: 2012-04-25 18:17 PDT by Karl Tomlinson (ni?:karlt)
Modified: 2012-11-29 09:44 PST (History)
5 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (7.12 KB, patch)
2012-05-30 16:15 PDT, Andrii Zui
no flags Details | Diff | Review
patch v2 (8.99 KB, patch)
2012-06-01 14:15 PDT, Andrii Zui
karlt: review-
Details | Diff | Review
dynamic reftests (4.91 KB, patch)
2012-06-01 14:16 PDT, Andrii Zui
karlt: review-
Details | Diff | Review
patch v3 (8.36 KB, patch)
2012-06-06 18:01 PDT, Andrii Zui
karlt: review-
Details | Diff | Review
dynamic reftests (4.98 KB, patch)
2012-06-06 18:03 PDT, Andrii Zui
karlt: review+
Details | Diff | Review
patch v4 (8.46 KB, patch)
2012-06-07 01:58 PDT, Andrii Zui
no flags Details | Diff | Review
patch v5 (8.47 KB, patch)
2012-06-07 13:54 PDT, Andrii Zui
karlt: review+
Details | Diff | Review
dynamic reftests (4.98 KB, patch)
2012-06-12 14:14 PDT, Andrii Zui
karlt: review+
Details | Diff | Review

Description Karl Tomlinson (ni?:karlt) 2012-04-25 18:17:44 PDT
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).
Comment 1 Karl Tomlinson (ni?:karlt) 2012-04-25 18:22:50 PDT
(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".
Comment 2 Frédéric Wang (:fredw) 2012-04-26 00:55:54 PDT
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".
Comment 3 Andrii Zui 2012-05-30 16:15:10 PDT
Created attachment 628530 [details] [diff] [review]
patch v1

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.
Comment 4 Frédéric Wang (:fredw) 2012-05-31 00:39:25 PDT
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 5 Andrii Zui 2012-06-01 14:15:46 PDT
Created attachment 629333 [details] [diff] [review]
patch v2
Comment 6 Andrii Zui 2012-06-01 14:16:19 PDT
Created attachment 629335 [details] [diff] [review]
dynamic reftests
Comment 7 Karl Tomlinson (ni?:karlt) 2012-06-05 02:10:27 PDT
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 8 Karl Tomlinson (ni?:karlt) 2012-06-05 02:12:19 PDT
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.
Comment 9 Karl Tomlinson (ni?:karlt) 2012-06-05 02:29:42 PDT
(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.
Comment 10 Frédéric Wang (:fredw) 2012-06-05 02:45:04 PDT
(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 11 Andrii Zui 2012-06-06 18:01:50 PDT
Created attachment 630795 [details] [diff] [review]
patch v3
Comment 12 Andrii Zui 2012-06-06 18:03:02 PDT
Created attachment 630796 [details] [diff] [review]
dynamic reftests
Comment 13 Karl Tomlinson (ni?:karlt) 2012-06-06 23:13:00 PDT
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.
Comment 14 Andrii Zui 2012-06-07 01:58:01 PDT
Created attachment 630892 [details] [diff] [review]
patch v4

Fixed.
Comment 15 Andrii Zui 2012-06-07 13:54:55 PDT
Created attachment 631118 [details] [diff] [review]
patch v5
Comment 16 Frédéric Wang (:fredw) 2012-06-08 00:37:21 PDT
https://tbpl.mozilla.org/?tree=Try&rev=f44488c2f88c
Comment 18 Ryan VanderMeulen [:RyanVM] 2012-06-09 13:05:25 PDT
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.
Comment 19 Karl Tomlinson (ni?:karlt) 2012-06-10 22:47:07 PDT
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.
Comment 20 Frédéric Wang (:fredw) 2012-06-12 02:02:12 PDT
Test results with "_" replaced by "-":

https://tbpl.mozilla.org/?tree=Try&rev=d284a6e82f88

Android reftests seem to fail, but the summary is empty...
Comment 21 Frédéric Wang (:fredw) 2012-06-12 13:54:02 PDT
https://tbpl.mozilla.org/?tree=Try&rev=c9fee5105886
Comment 22 Andrii Zui 2012-06-12 14:14:23 PDT
Created attachment 632408 [details] [diff] [review]
dynamic reftests

Changed "_" characters to "-".

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