Bug 739556 (maction-selection)

maction@selection should only be taken into account when actiontype="toggle"

RESOLVED FIXED in mozilla15

Status

()

Core
MathML
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: fredw, Assigned: Andrii Zui)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

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

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

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

Comment 1

5 years ago
Created attachment 613714 [details] [diff] [review]
selection attribute is taken into account only with actiontype="toggle"
Attachment #613714 - Flags: review?(karlt)
(Assignee)

Comment 2

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

Comment 3

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

Comment 4

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

Comment 5

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

Comment 6

5 years ago
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).
Attachment #613714 - Attachment is obsolete: true
Attachment #613714 - Flags: review?(karlt)
Attachment #614026 - Flags: review?(karlt)
(Assignee)

Comment 7

5 years ago
Created attachment 614027 [details] [diff] [review]
static reftests V2
Attachment #613716 - Attachment is obsolete: true
Attachment #613716 - Flags: review?(karlt)
Attachment #614027 - Flags: review?(karlt)
(Reporter)

Updated

5 years ago
Assignee: nobody → PraZuBeR
Status: NEW → ASSIGNED
(Reporter)

Comment 8

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

Comment 9

5 years ago
Created attachment 615200 [details] [diff] [review]
selection attribute is taken into account only with actiontype="toggle" V3

Typo fixed, to follow Mozilla Coding Style.
Attachment #614026 - Attachment is obsolete: true
Attachment #614026 - Flags: review?(karlt)
Attachment #615200 - Flags: review?(karlt)
(Assignee)

Comment 10

5 years ago
Created attachment 615201 [details] [diff] [review]
static reftests V3
Attachment #614027 - Attachment is obsolete: true
Attachment #614027 - Flags: review?(karlt)
Attachment #615201 - Flags: review?(karlt)
(Assignee)

Comment 11

5 years ago
Created attachment 615286 [details] [diff] [review]
static reftests V4

Changed names of test files and forgot to update reftest.list... my apologies.
Attachment #615201 - Attachment is obsolete: true
Attachment #615201 - Flags: review?(karlt)
Attachment #615286 - Flags: review?(karlt)
Attachment #615286 - Flags: review?(karlt) → review+
Attachment #615200 - Flags: review?(karlt) → review+
(Assignee)

Updated

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

Updated

5 years ago
Whiteboard: [autoland-try: -b do -p all -u all -t all] → [autoland-try:-b do -p all -u all -t all]

Updated

5 years ago
Whiteboard: [autoland-try:-b do -p all -u all -t all] → [autoland-in-queue]

Comment 12

5 years ago
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

5 years ago
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

Updated

5 years ago
Whiteboard: [autoland-in-queue]
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/80e220f9541e
https://hg.mozilla.org/integration/mozilla-inbound/rev/2df28cac3884
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/80e220f9541e
https://hg.mozilla.org/mozilla-central/rev/2df28cac3884
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Keywords: dev-doc-needed
Depends on: 749044
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.