The default bug view has changed. See this FAQ.

dynamic change of the maction actiontype attribute doesn't work

RESOLVED FIXED in mozilla15

Status

()

Core
MathML
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Andrii Zui, Assigned: Andrii Zui)

Tracking

(Blocks: 1 bug)

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 615141 [details]
testcase

Dynamic change of maction actiontype attribute has no effect. Possibly it is because mActionType is initialized in nsMathMLmactionFrame::Init and never changed afterwards. 

Possible solution is to override nsMathMLContainerFrame::AttributeChanged in nsMathMLmactionFrame and update mActionType there.

The testcase demonstrates the issue.
(Assignee)

Comment 1

5 years ago
Created attachment 615142 [details]
testcase

Sorry, first testcase was uploaded improperly.
Attachment #615141 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #615142 - Attachment mime type: text/plain → text/html
(Assignee)

Updated

5 years ago
Blocks: 744783
Assignee: nobody → PraZuBeR
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Version: unspecified → Trunk
(Assignee)

Comment 2

5 years ago
Created attachment 615865 [details] [diff] [review]
override nsMathMLContainerFrame::AttributeChanged

In here, nsMathMLContainerFrame::AttributeChanged was overridden in nsMathMLmactionFrame to update mActionType, also I made some optimization by checking whether it's needed to do a reflow or not. nsMathMLmactionFrame::Init was cleaned up a bit as well.

There is still one problem. Although it seemed that AttributeChanged should work only with kNameSpaceID_MathML, it doesn't seem to work (see #if 0 section). Also there is a question about namespace used in all mContent->GetAttr or mContent->SetAttr calls in nsMathMLmactionFrame. It's kNameSpaceID_None there, but shouldn't it be kNameSpaceID_MathML?
Attachment #615865 - Flags: feedback?(karlt)
> There is still one problem. Although it seemed that AttributeChanged should
> work only with kNameSpaceID_MathML, it doesn't seem to work (see #if 0
> section). Also there is a question about namespace used in all
> mContent->GetAttr or mContent->SetAttr calls in nsMathMLmactionFrame. It's
> kNameSpaceID_None there, but shouldn't it be kNameSpaceID_MathML?

You are using both kNameSpaceID_MathML (in #if 0) and kNameSpaceID_None, so that can not work. I think for the attributes, kNameSpaceID_None should. For example in the SVG code:

http://mxr.mozilla.org/mozilla-central/source/layout/svg/base/src/nsSVGImageFrame.cpp#216
(Assignee)

Comment 4

5 years ago
> You are using both kNameSpaceID_MathML (in #if 0) and kNameSpaceID_None, so
> that can not work.

I tried using only kNameSpaceID_MathML everywhere in maction, but it didn't help.
(In reply to Andrii Zui from comment #4)
> > You are using both kNameSpaceID_MathML (in #if 0) and kNameSpaceID_None, so
> > that can not work.
> 
> I tried using only kNameSpaceID_MathML everywhere in maction, but it didn't
> help.

Yes, since apparently the correct syntax for attributes is without namespace. Otherwise, I guess you also have to specify the MathML namespace in a SetAttributeNS of your tests.
Comment on attachment 615865 [details] [diff] [review]
override nsMathMLContainerFrame::AttributeChanged

>+PRInt32
>+nsMathMLmactionFrame::ParseActionType(const nsAutoString& value)

This method doesn't need access to the nsMathMLmactionFrame object or class so
make this a plain old static linkage function.

If you pass in the nsIContent* instead of the value then the GetAttr code can
be in this method instead of in both of the callers.

>+    if ((mActionType == NS_MATHML_ACTION_TYPE_STATUSLINE || 
>+         mActionType == NS_MATHML_ACTION_TYPE_TOOLTIP) && 
>+        (oldActionType == NS_MATHML_ACTION_TYPE_STATUSLINE ||
>+         oldActionType == NS_MATHML_ACTION_TYPE_TOOLTIP)) {
>+      needsReflow = false;

Do we only need to reflow when either the old or new action type is toggle?

The optimizations would look more worthwhile if needsReflow defaulted to false
and was set to true only in a few cases.

>+  } else if (aAttribute == nsGkAtoms::selection_) {
>+    // When the selection attribute is changed we have to initiate a reflow
>+    // only when actiontype is toggle.
>+    if (NS_MATHML_ACTION_TYPE_TOGGLE != mActionType)
>+      needsReflow = false;
>+  } else {

Please use braces around the NS_MATHML_ACTION_TYPE_TOGGLE != mActionType
block (to make it clear that the nested if/else blocks are as intended).
Attachment #615865 - Flags: feedback?(karlt) → feedback+
(Assignee)

Comment 7

5 years ago
Created attachment 617255 [details] [diff] [review]
patch v2: optimization of AttributeChanged function

I updated the patch with Karl's comments taken into account.

I believe that in nsMathMLmactionFrame::AttributeChanged it's enough to call FrameNeedsReflow with eTreeChange instead of eStyleChange. But I'm not much familiar with it, so I can be mistaken.

Also, I merged the GetAttr call into GetActionType function (renamed from ParseActionType in the previous version of the patch). It will take actiontype attribute from kNameSpaceID_None, I hope it's ok since the call of GetAttr in nsMathMLmactionFrame::Init was referring to kNameSpaceID_None as well.
Attachment #615865 - Attachment is obsolete: true
Comment on attachment 617255 [details] [diff] [review]
patch v2: optimization of AttributeChanged function

>+PRInt32
>+GetActionType(nsIContent* aContent)

Include the static keyword here for internal linkage.
Otherwise, this looks good, thanks.
(Assignee)

Comment 9

5 years ago
Created attachment 617455 [details] [diff] [review]
patch v3: optimization of AttributeChanged function
Attachment #617255 - Attachment is obsolete: true
Attachment #617455 - Flags: review?(karlt)
Comment on attachment 617455 [details] [diff] [review]
patch v3: optimization of AttributeChanged function

Can you modify the checkin comment a bit, please?
The key change is that this now handles dynamic changes to the actiontype attribute, rather than optimization of an existing functionality.
The bug number should also be included.
Perhaps "b=617455 Implement AttributeChanged on nsMathMLmactionFrame r=karlt".
Attachment #617455 - Flags: review?(karlt) → review+
(Assignee)

Comment 11

5 years ago
Created attachment 618011 [details] [diff] [review]
patch v4: Implement AttributeChanged on nsMathMLmactionFrame

Fixed. Haven't thought about the description much, sorry about that.
Attachment #617455 - Attachment is obsolete: true
Attachment #618011 - Flags: review?(karlt)
Blocks: 544036
Attachment #618011 - Flags: review?(karlt) → review+
Andrii, can you please also attach to this bug the dynamic reftests you wrote last week (and ask review)? That will allow to test your patch (if I remember well) when you send your patch to the try server.
(Assignee)

Comment 13

5 years ago
Created attachment 622097 [details] [diff] [review]
dynamic reftests
Attachment #622097 - Flags: review?(karlt)
Attachment #622097 - Flags: review?(karlt) → review+
(Assignee)

Updated

5 years ago
Whiteboard: [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 14

5 years ago
Autoland Patchset:
	Patches: 618011, 622097
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=87e2cdb9d86a
Try run started, revision 87e2cdb9d86a. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=87e2cdb9d86a
(In reply to Mozilla RelEng Bot from comment #14)
> Autoland Patchset:
> 	Patches: 618011, 622097
> 	Branch: mozilla-central => try
> 	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=87e2cdb9d86a
> Try run started, revision 87e2cdb9d86a. To cancel or monitor the job, see:
> https://tbpl.mozilla.org/?tree=Try&rev=87e2cdb9d86a

The results look good. I don't know why the bot takes so much time to post a message indicating the end...
Whiteboard: [autoland-in-queue]
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/05a02c6ec1b6
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3a321bf58ce
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/05a02c6ec1b6
https://hg.mozilla.org/mozilla-central/rev/c3a321bf58ce
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.