Closed Bug 745535 Opened 12 years ago Closed 12 years ago

dynamic change of the maction actiontype attribute doesn't work

Categories

(Core :: MathML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: PraZuBeR, Assigned: PraZuBeR)

References

Details

Attachments

(3 files, 4 obsolete files)

Attached file testcase (obsolete) —
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.
Attached file testcase
Sorry, first testcase was uploaded improperly.
Attachment #615141 - Attachment is obsolete: true
Attachment #615142 - Attachment mime type: text/plain → text/html
Blocks: 744783
Assignee: nobody → PraZuBeR
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Version: unspecified → Trunk
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
> 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+
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.
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+
Fixed. Haven't thought about the description much, sorry about that.
Attachment #617455 - Attachment is obsolete: true
Attachment #618011 - Flags: review?(karlt)
Blocks: maction
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.
Attached patch dynamic reftestsSplinter Review
Attachment #622097 - Flags: review?(karlt)
Attachment #622097 - Flags: review?(karlt) → review+
Whiteboard: [autoland-try:-b do -p all -u all -t all]
Whiteboard: [autoland-try:-b do -p all -u all -t all] → [autoland-in-queue]
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]
You need to log in before you can comment on or make changes to this bug.