The default bug view has changed. See this FAQ.

selection attribute on maction should be considered by default

RESOLVED FIXED in mozilla16

Status

()

Core
MathML
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: karlt, Assigned: Andrii Zui)

Tracking

(Blocks: 3 bugs, {dev-doc-complete, regression})

Trunk
mozilla16
dev-doc-complete, regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

5 years ago
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).
(Reporter)

Updated

5 years ago
Blocks: 739556
(Reporter)

Comment 1

5 years ago
(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".
Blocks: 749103
Assignee: nobody → PraZuBeR
Status: NEW → ASSIGNED
(Assignee)

Comment 3

5 years ago
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.
Attachment #628530 - Flags: review?(karlt)
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.
(Assignee)

Updated

5 years ago
Attachment #628530 - Flags: review?(karlt)
(Assignee)

Comment 5

5 years ago
Created attachment 629333 [details] [diff] [review]
patch v2
Attachment #628530 - Attachment is obsolete: true
Attachment #629333 - Flags: review?(karlt)
(Assignee)

Comment 6

5 years ago
Created attachment 629335 [details] [diff] [review]
dynamic reftests
Attachment #629335 - Flags: review?(karlt)
Blocks: 544036
(Reporter)

Comment 7

5 years ago
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.
Attachment #629335 - Flags: review?(karlt) → review-
(Reporter)

Comment 8

5 years ago
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.
Attachment #629333 - Flags: review?(karlt) → review-
(Reporter)

Comment 9

5 years ago
(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 "".
(Reporter)

Updated

5 years ago
Blocks: 761832
(Assignee)

Comment 11

5 years ago
Created attachment 630795 [details] [diff] [review]
patch v3
Attachment #629333 - Attachment is obsolete: true
Attachment #629335 - Attachment is obsolete: true
Attachment #630795 - Flags: review?(karlt)
(Assignee)

Comment 12

5 years ago
Created attachment 630796 [details] [diff] [review]
dynamic reftests
Attachment #630796 - Flags: review?(karlt)
(Reporter)

Updated

5 years ago
Attachment #630796 - Flags: review?(karlt) → review+
(Reporter)

Comment 13

5 years ago
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.
Attachment #630795 - Flags: review?(karlt) → review-
(Assignee)

Comment 14

5 years ago
Created attachment 630892 [details] [diff] [review]
patch v4

Fixed.
Attachment #630795 - Attachment is obsolete: true
Attachment #630892 - Flags: review?(karlt)
(Assignee)

Comment 15

5 years ago
Created attachment 631118 [details] [diff] [review]
patch v5
Attachment #630892 - Attachment is obsolete: true
Attachment #630892 - Flags: review?(karlt)
Attachment #631118 - Flags: review?(karlt)
(Reporter)

Updated

5 years ago
Attachment #631118 - Flags: review?(karlt) → review+
(Assignee)

Updated

5 years ago
Whiteboard: [autoland-try:631118,630796:-b do -p all -u all -t all]
(Assignee)

Updated

5 years ago
Whiteboard: [autoland-try:631118,630796:-b do -p all -u all -t all] → [autoland-try:-b do -p all -u all -t all]
Whiteboard: [autoland-try:-b do -p all -u all -t all]
https://tbpl.mozilla.org/?tree=Try&rev=f44488c2f88c
Keywords: checkin-needed, dev-doc-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdb634caef71
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c627221444a
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla16
Version: Other Branch → Trunk
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.
Flags: in-testsuite+
Target Milestone: mozilla16 → ---
(Reporter)

Comment 19

5 years ago
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...
https://tbpl.mozilla.org/?tree=Try&rev=c9fee5105886
(Assignee)

Comment 22

5 years ago
Created attachment 632408 [details] [diff] [review]
dynamic reftests

Changed "_" characters to "-".
Attachment #630796 - Attachment is obsolete: true
Attachment #632408 - Flags: review?(karlt)
(Reporter)

Updated

5 years ago
Attachment #632408 - Flags: review?(karlt) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/129be8143484
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e7422731ea0
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/129be8143484
https://hg.mozilla.org/mozilla-central/rev/5e7422731ea0
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
https://developer.mozilla.org/en-US/docs/MathML/Element/maction#Gecko-specific_notes
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.