739556 - (maction-selection) maction@selection should only be taken into account when actiontype="toggle"

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

[Frédéric Wang (:fredw)]

The MathML spec seems to only define the meaning of the selection attribute when the actiontype attribute is "toggle". So it seems that nsMathMLmactionFrame::GetSelectedFrame() should always returns the first child in the other cases.

Comment 1

[Andrii Zui]

Attachment: selection attribute is taken into account only with actiontype="toggle"

Comment 2

[Andrii Zui]

Attachment: static reftests

Comment 3

[Frédéric Wang (:fredw)]

Comment on attachment 613716 [details] [diff] [review]
static reftests

>+  <maction actiontype="tooltip" selection=2>

For consistency, we should maybe always use quotes around attribute values.

Comment 4

[Frédéric Wang (:fredw)]

Comment on attachment 613714 [details] [diff] [review]
selection attribute is taken into account only with actiontype="toggle"


>@@ -176,16 +176,20 @@ nsMathMLmactionFrame::GetSelectedFrame()
>   if (!value.IsEmpty()) {
>     PRInt32 errorCode;
>     selection = value.ToInteger(&errorCode);
>     if (NS_FAILED(errorCode)) 
>       selection = 1;
>   }
>   else selection = 1; // default is first frame
> 
>+  // selection is applied only to toggle, return first child otherwise
>+  if (NS_MATHML_ACTION_TYPE_TOGGLE != mActionType)
>+      selection = 1;
>+

Perhaps you can optimize a bit and immediately set selection = 1 when NS_MATHML_ACTION_TYPE_TOGGLE != mActionType, without retrieving the selection attribute and doing further operations on it.

Comment 5

[Frédéric Wang (:fredw)]

(In reply to Andrii Zui from comment #2)
> Created attachment 613716 [details] [diff] [review]
> static reftests

These tests look good. When possible, I find it better to have == tests (with an != test a totally unrelated difference may make it pass). Maybe you can convert maction-statusline-2-ref.html to an == test by playing on the order of children of the maction element.

Comment 6

[Andrii Zui]

Attachment: selection attribute is taken into account only with actiontype="toggle" V2

Now if ActionType != toggle function immediately returns first child (if exists).

Comment 7

[Andrii Zui]

Attachment: static reftests V2

Comment 8

[Frédéric Wang (:fredw)]

Currently attribute changes for maction are handled in

nsMathMLContainerFrame::AttributeChanged

which simply reflows the whole frame. So I expect dynamic reftests to pass without problems (except in cases involving embellish op data, like in bug 657279).

Such reftests will be useful if we try to optimize a bit more how attribute changes are handled in another bug (cf XXXldb comment in nsMathMLContainerFrame::AttributeChanged). For example in the case of maction, switching between actiontypes statuline and tooltip shouldn't necessitate a reflow. Similarly the selection attribute should only reflow when changing the selected frame when actiontype=toggle.

Comment 9

[Andrii Zui]

Attachment: selection attribute is taken into account only with actiontype="toggle" V3

Typo fixed, to follow Mozilla Coding Style.

Comment 10

[Andrii Zui]

Attachment: static reftests V3

Comment 11

[Andrii Zui]

Attachment: static reftests V4

Changed names of test files and forgot to update reftest.list... my apologies.

Comment 12

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 615200, 615286
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=54edafa8155b
Try run started, revision 54edafa8155b. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=54edafa8155b

Comment 13

[Mozilla RelEng Bot]

Try run for 54edafa8155b is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=54edafa8155b
Results (out of 316 total builds):
    exception: 3
    success: 279
    warnings: 32
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-54edafa8155b

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/80e220f9541e
https://hg.mozilla.org/integration/mozilla-inbound/rev/2df28cac3884

Comment 15

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/80e220f9541e
https://hg.mozilla.org/mozilla-central/rev/2df28cac3884

Comment 16

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/MathML/Element/maction#Gecko-specific_notes
https://developer.mozilla.org/en-US/docs/Firefox_15_for_developers#MathML