Last Comment Bug 739556 - (maction-selection) maction@selection should only be taken into account when actiontype="toggle"
(maction-selection)
: maction@selection should only be taken into account when actiontype="toggle"
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: MathML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Andrii Zui
:
Mentors:
http://www.w3.org/TR/MathML3/chapter3...
Depends on: 749044
Blocks: maction
  Show dependency treegraph
 
Reported: 2012-03-27 03:13 PDT by Frédéric Wang (:fredw)
Modified: 2012-11-29 09:42 PST (History)
5 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
selection attribute is taken into account only with actiontype="toggle" (1.11 KB, patch)
2012-04-10 12:27 PDT, Andrii Zui
no flags Details | Diff | Review
static reftests (3.88 KB, patch)
2012-04-10 12:28 PDT, Andrii Zui
no flags Details | Diff | Review
selection attribute is taken into account only with actiontype="toggle" V2 (1.10 KB, patch)
2012-04-11 09:18 PDT, Andrii Zui
no flags Details | Diff | Review
static reftests V2 (3.32 KB, patch)
2012-04-11 09:19 PDT, Andrii Zui
no flags Details | Diff | Review
selection attribute is taken into account only with actiontype="toggle" V3 (1.31 KB, patch)
2012-04-15 15:37 PDT, Andrii Zui
karlt: review+
Details | Diff | Review
static reftests V3 (2.95 KB, patch)
2012-04-15 15:38 PDT, Andrii Zui
no flags Details | Diff | Review
static reftests V4 (2.94 KB, patch)
2012-04-16 03:00 PDT, Andrii Zui
karlt: review+
Details | Diff | Review

Description Frédéric Wang (:fredw) 2012-03-27 03:13:21 PDT
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 2012-04-10 12:27:49 PDT
Created attachment 613714 [details] [diff] [review]
selection attribute is taken into account only with actiontype="toggle"
Comment 2 Andrii Zui 2012-04-10 12:28:56 PDT
Created attachment 613716 [details] [diff] [review]
static reftests
Comment 3 Frédéric Wang (:fredw) 2012-04-10 13:32:38 PDT
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) 2012-04-10 13:35:18 PDT
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) 2012-04-10 13:41:09 PDT
(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 2012-04-11 09:18:07 PDT
Created attachment 614026 [details] [diff] [review]
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 2012-04-11 09:19:23 PDT
Created attachment 614027 [details] [diff] [review]
static reftests V2
Comment 8 Frédéric Wang (:fredw) 2012-04-12 07:20:07 PDT
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 2012-04-15 15:37:41 PDT
Created attachment 615200 [details] [diff] [review]
selection attribute is taken into account only with actiontype="toggle" V3

Typo fixed, to follow Mozilla Coding Style.
Comment 10 Andrii Zui 2012-04-15 15:38:28 PDT
Created attachment 615201 [details] [diff] [review]
static reftests V3
Comment 11 Andrii Zui 2012-04-16 03:00:18 PDT
Created attachment 615286 [details] [diff] [review]
static reftests V4

Changed names of test files and forgot to update reftest.list... my apologies.
Comment 12 Mozilla RelEng Bot 2012-04-19 08:15:35 PDT
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 2012-04-19 16:00:27 PDT
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/autolanduser@mozilla.com-54edafa8155b

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