Last Comment Bug 729924 - (maction-statusline) maction: statusline actiontype should use the second child as a message
(maction-statusline)
: maction: statusline actiontype should use the second child as a message
Status: RESOLVED FIXED
[good second bug]
: dev-doc-complete
Product: Core
Classification: Components
Component: MathML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Andrii Zui
:
Mentors:
Depends on:
Blocks: maction
  Show dependency treegraph
 
Reported: 2012-02-23 06:51 PST by Frédéric Wang (:fredw)
Modified: 2014-01-28 04:02 PST (History)
7 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (679 bytes, text/html)
2012-02-23 07:27 PST, Frédéric Wang (:fredw)
no flags Details
testcase (MathJax rendering) (808 bytes, text/html)
2012-02-23 07:28 PST, Frédéric Wang (:fredw)
no flags Details
bugfix (13.35 KB, patch)
2012-03-26 10:07 PDT, Andrii Zui
no flags Details | Diff | Review
bug-729924-fix (2.00 KB, patch)
2012-03-26 13:32 PDT, Andrii Zui
karlt: review-
Details | Diff | Review
bug-729924-fix corrected (2.21 KB, patch)
2012-03-27 02:17 PDT, Andrii Zui
karlt: review-
Details | Diff | Review
bug-729924-fix corrected (2.26 KB, patch)
2012-03-27 15:02 PDT, Andrii Zui
no flags Details | Diff | Review
bug-729924-fix corrected (2.26 KB, patch)
2012-03-27 15:10 PDT, Andrii Zui
no flags Details | Diff | Review
bug-729924-fix final (2.28 KB, patch)
2012-03-27 15:32 PDT, Andrii Zui
karlt: review+
Details | Diff | Review

Description Frédéric Wang (:fredw) 2012-02-23 06:51:20 PST
The REC describes actiontype like this:

"<maction actiontype="statusline"> (expression) (message) </maction>

    The renderer displays the first child. When a reader clicks on the expression or moves the pointer over it, the renderer sends a rendering of the message to the browser statusline. Because most browsers in the foreseeable future are likely to be limited to displaying text on their statusline, the second child should be an mtext element in most circumstances. For non-mtext messages, renderers might provide a natural language translation of the markup, but this is not required. "

Currently, our implementation uses the syntax

"<maction actiontype="statusline#(message)">
(expression)
</maction>
Comment 1 Frédéric Wang (:fredw) 2012-02-23 07:27:03 PST
Created attachment 600011 [details]
testcase
Comment 2 Frédéric Wang (:fredw) 2012-02-23 07:28:28 PST
Created attachment 600013 [details]
testcase (MathJax rendering)

This displays something in the statusline for Opera, but does not currently seem to work for Firefox.
Comment 3 Frédéric Wang (:fredw) 2012-02-23 07:31:00 PST
For non-textual message, MathJax seems to convert the message into a string representation of the tree "mrow(msup((mi(x)...)". I don't think it is really useful to implement this case.
Comment 4 Karl Tomlinson (ni?:karlt) 2012-02-23 15:54:27 PST
Is there any other situation where web content can control what is written in the status line?

This feature would need a security review, with consideration of risks and benefits if adding new powers to web content.

window.status is disabled by default
https://developer.mozilla.org/en/DOM/window.status

But now that Firefox status messages are displayed over content, they can be easily spoofed by other means, so perhaps this is not a concern.
Comment 5 Frédéric Wang (:fredw) 2012-02-24 00:30:04 PST
I don't know, but note that maction@actiontype=statusline is already implemented (see attachment 600011 [details]) so people can already modify the statusline via maction...

BTW, I reported to MathJax folks that attachment 600013 [details] no longer works with Chrome/Firefox (probably because window.status is disabled) so they are going to use their own message area (div positioned at the bottom of the page).
Comment 6 Karl Tomlinson (ni?:karlt) 2012-02-26 14:30:33 PST
(In reply to Frédéric Wang from comment #5)
> I don't know, but note that maction@actiontype=statusline is already
> implemented (see attachment 600011 [details]) so people can already modify
> the statusline via maction...

Ah, thanks.  Sorry, I overlooked that.
Modifying the existing actiontype=statusline to match the spec sounds good.

Interestingly window.status is only disabled in Firefox, not in Core.

http://hg.mozilla.org/mozilla-central/annotate/cd4853b0b94a/modules/libpref/src/init/all.js#l590
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/app/profile/firefox.js&rev=1.25&mark=192#190

I haven't read through all of bug 22183, but now that status messages are displayed over content they shouldn't be assumed to be chrome anyway.
Comment 7 Andrii Zui 2012-03-26 10:07:21 PDT
Created attachment 609360 [details] [diff] [review]
bugfix
Comment 8 Andrii Zui 2012-03-26 10:09:41 PDT
Comment on attachment 609360 [details] [diff] [review]
bugfix

>/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>/* ***** BEGIN LICENSE BLOCK *****
> * Version: MPL 1.1/GPL 2.0/LGPL 2.1
> *
> * The contents of this file are subject to the Mozilla Public License Version
> * 1.1 (the "License"); you may not use this file except in compliance with
> * the License. You may obtain a copy of the License at
> * http://www.mozilla.org/MPL/
> *
> * Software distributed under the License is distributed on an "AS IS" basis,
> * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
> * for the specific language governing rights and limitations under the
> * License.
> *
> * The Original Code is Mozilla MathML Project.
> *
> * The Initial Developer of the Original Code is
> * The University Of Queensland.
> * Portions created by the Initial Developer are Copyright (C) 1999
> * the Initial Developer. All Rights Reserved.
> *
> * Contributor(s):
> *   Roger B. Sidje <rbs@maths.uq.edu.au>
> *
> * Alternatively, the contents of this file may be used under the terms of
> * either of the GNU General Public License Version 2 or later (the "GPL"),
> * or the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
> * in which case the provisions of the GPL or the LGPL are applicable instead
> * of those above. If you wish to allow use of your version of this file only
> * under the terms of either the GPL or the LGPL, and not to allow others to
> * use your version of this file under the terms of the MPL, indicate your
> * decision by deleting the provisions above and replace them with the notice
> * and other provisions required by the GPL or the LGPL. If you do not delete
> * the provisions above, a recipient may use your version of this file under
> * the terms of any one of the MPL, the GPL or the LGPL.
> *
> * ***** END LICENSE BLOCK ***** */
>
>#include "nsCOMPtr.h"
>#include "nsFrame.h"
>#include "nsPresContext.h"
>#include "nsStyleContext.h"
>#include "nsStyleConsts.h"
>#include "nsINameSpaceManager.h"
>
>#include "nsCSSRendering.h"
>#include "prprf.h"         // For PR_snprintf()
>
>#include "nsIDocShellTreeItem.h"
>#include "nsIDocShellTreeOwner.h"
>#include "nsIWebBrowserChrome.h"
>#include "nsIInterfaceRequestor.h"
>#include "nsIInterfaceRequestorUtils.h"
>#include "nsIDOMElement.h"
>
>#include "nsIDOMEventTarget.h"
>
>#include "nsMathMLmactionFrame.h"
>#include "nsAutoPtr.h"
>#include "nsStyleSet.h"
>#include "nsDisplayList.h"
>#include "nsContentUtils.h"
>
>//
>// <maction> -- bind actions to a subexpression - implementation
>//
>
>#define NS_MATHML_ACTION_TYPE_NONE         0
>#define NS_MATHML_ACTION_TYPE_TOGGLE       1
>#define NS_MATHML_ACTION_TYPE_STATUSLINE   2
>#define NS_MATHML_ACTION_TYPE_TOOLTIP      3 // unsupported
>
>
>nsIFrame*
>NS_NewMathMLmactionFrame(nsIPresShell* aPresShell, nsStyleContext* aContext)
>{
>  return new (aPresShell) nsMathMLmactionFrame(aContext);
>}
>
>NS_IMPL_FRAMEARENA_HELPERS(nsMathMLmactionFrame)
>
>nsMathMLmactionFrame::~nsMathMLmactionFrame()
>{
>  // unregister us as a mouse event listener ...
>  //  printf("maction:%p unregistering as mouse event listener ...\n", this);
>  if (mListener) {
>    mContent->RemoveSystemEventListener(NS_LITERAL_STRING("click"), mListener,
>                                        false);
>    mContent->RemoveSystemEventListener(NS_LITERAL_STRING("mouseover"), mListener,
>                                        false);
>    mContent->RemoveSystemEventListener(NS_LITERAL_STRING("mouseout"), mListener,
>                                        false);
>  }
>}
>
>NS_IMETHODIMP
>nsMathMLmactionFrame::Init(nsIContent*      aContent,
>                           nsIFrame*        aParent,
>                           nsIFrame*        aPrevInFlow)
>{
>  nsAutoString value, prefix;
>
>  // Init our local attributes
>
>  mChildCount = -1; // these will be updated in GetSelectedFrame()
>  mSelection = 0;
>  mSelectedFrame = nsnull;
>  nsRefPtr<nsStyleContext> newStyleContext;
>
>  mActionType = NS_MATHML_ACTION_TYPE_NONE;
>  aContent->GetAttr(kNameSpaceID_None, nsGkAtoms::actiontype_, value);
>  if (!value.IsEmpty()) {
>    if (value.EqualsLiteral("toggle"))
>      mActionType = NS_MATHML_ACTION_TYPE_TOGGLE;
>
>    // XXX use goto to jump out of these if?
>
>    if (NS_MATHML_ACTION_TYPE_NONE == mActionType) {
>      // expected tooltip prefix (8ch)...
>      if (8 < value.Length() && 0 == value.Find("tooltip#"))
>        mActionType = NS_MATHML_ACTION_TYPE_TOOLTIP;
>    }
>
>    if (NS_MATHML_ACTION_TYPE_NONE == mActionType)
>      if (value.EqualsLiteral("statusline"))
>        mActionType = NS_MATHML_ACTION_TYPE_STATUSLINE;
>  }
>
>  // Let the base class do the rest
>  return nsMathMLContainerFrame::Init(aContent, aParent, aPrevInFlow);
>}
>
>NS_IMETHODIMP
>nsMathMLmactionFrame::TransmitAutomaticData() {
>  // The REC defines the following element to be space-like:
>  // * an maction element whose selected sub-expression exists and is
>  //   space-like;
>  nsIMathMLFrame* mathMLFrame = do_QueryFrame(mSelectedFrame);
>  if (mathMLFrame && mathMLFrame->IsSpaceLike()) {
>    mPresentationData.flags |= NS_MATHML_SPACE_LIKE;
>  } else {
>    mPresentationData.flags &= ~NS_MATHML_SPACE_LIKE;
>  }
>
>  // The REC defines the following element to be an embellished operator:
>  // * an maction element whose selected sub-expression exists and is an
>  //   embellished operator;
>  mPresentationData.baseFrame = mSelectedFrame;
>  GetEmbellishDataFrom(mSelectedFrame, mEmbellishData);
>
>  return NS_OK;
>}
>
>nsresult
>nsMathMLmactionFrame::ChildListChanged(PRInt32 aModType)
>{
>  // update cached values
>  mChildCount = -1;
>  mSelection = 0;
>  mSelectedFrame = nsnull;
>  GetSelectedFrame();
>
>  return nsMathMLContainerFrame::ChildListChanged(aModType);
>}
>
>// return the frame whose number is given by the attribute selection="number"
>nsIFrame* 
>nsMathMLmactionFrame::GetSelectedFrame()
>{
>  nsAutoString value;
>  PRInt32 selection; 
>
>  GetAttribute(mContent, mPresentationData.mstyle, nsGkAtoms::selection_,
>               value);
>  if (!value.IsEmpty()) {
>    PRInt32 errorCode;
>    selection = value.ToInteger(&errorCode);
>    if (NS_FAILED(errorCode)) 
>      selection = 1;
>  }
>  else selection = 1; // default is first frame
>
>  if (-1 != mChildCount) { // we have been in this function before...
>    // cater for invalid user-supplied selection
>    if (selection > mChildCount || selection < 1) 
>      selection = 1;
>    // quick return if it is identical with our cache
>    if (selection == mSelection) 
>      return mSelectedFrame;
>  }
>
>  // get the selected child and cache new values...
>  PRInt32 count = 0;
>  nsIFrame* childFrame = mFrames.FirstChild();
>  while (childFrame) {
>    if (!mSelectedFrame) 
>      mSelectedFrame = childFrame; // default is first child
>    if (++count == selection) 
>      mSelectedFrame = childFrame;
>
>    childFrame = childFrame->GetNextSibling();
>  }
>  // cater for invalid user-supplied selection
>  if (selection > count || selection < 1) 
>    selection = 1;
>
>  mChildCount = count;
>  mSelection = selection;
>
>  return mSelectedFrame;
>}
>
>NS_IMETHODIMP
>nsMathMLmactionFrame::SetInitialChildList(ChildListID     aListID,
>                                          nsFrameList&    aChildList)
>{
>  nsresult rv = nsMathMLContainerFrame::SetInitialChildList(aListID, aChildList);
>
>  // This very first call to GetSelectedFrame() will cause us to be marked as an
>  // embellished operator if the selected child is an embellished operator
>  if (!GetSelectedFrame()) {
>    mActionType = NS_MATHML_ACTION_TYPE_NONE;
>  }
>  else {
>    // create mouse event listener and register it
>    mListener = new nsMathMLmactionFrame::MouseListener(this);
>    // printf("maction:%p registering as mouse event listener ...\n", this);
>    mContent->AddSystemEventListener(NS_LITERAL_STRING("click"), mListener,
>                                     false, false);
>    mContent->AddSystemEventListener(NS_LITERAL_STRING("mouseover"), mListener,
>                                     false, false);
>    mContent->AddSystemEventListener(NS_LITERAL_STRING("mouseout"), mListener,
>                                     false, false);
>  }
>  return rv;
>}
>
>//  Only paint the selected child...
>NS_IMETHODIMP
>nsMathMLmactionFrame::BuildDisplayList(nsDisplayListBuilder*   aBuilder,
>                                       const nsRect&           aDirtyRect,
>                                       const nsDisplayListSet& aLists)
>{
>  nsresult rv = DisplayBorderBackgroundOutline(aBuilder, aLists);
>  NS_ENSURE_SUCCESS(rv, rv);
>
>  nsIFrame* childFrame = GetSelectedFrame();
>  if (childFrame) {
>    // Put the child's background directly onto the content list
>    nsDisplayListSet set(aLists, aLists.Content());
>    // The children should be in content order
>    rv = BuildDisplayListForChild(aBuilder, childFrame, aDirtyRect, set);
>    NS_ENSURE_SUCCESS(rv, rv);
>  }
>
>#if defined(NS_DEBUG) && defined(SHOW_BOUNDING_BOX)
>  // visual debug
>  rv = DisplayBoundingMetrics(aBuilder, this, mReference, mBoundingMetrics, aLists);
>#endif
>  return rv;
>}
>
>// Only reflow the selected child ...
>NS_IMETHODIMP
>nsMathMLmactionFrame::Reflow(nsPresContext*          aPresContext,
>                             nsHTMLReflowMetrics&     aDesiredSize,
>                             const nsHTMLReflowState& aReflowState,
>                             nsReflowStatus&          aStatus)
>{
>  nsresult rv = NS_OK;
>  aStatus = NS_FRAME_COMPLETE;
>  aDesiredSize.width = aDesiredSize.height = 0;
>  aDesiredSize.ascent = 0;
>  mBoundingMetrics = nsBoundingMetrics();
>  nsIFrame* childFrame = GetSelectedFrame();
>  if (childFrame) {
>    nsSize availSize(aReflowState.ComputedWidth(), NS_UNCONSTRAINEDSIZE);
>    nsHTMLReflowState childReflowState(aPresContext, aReflowState,
>                                       childFrame, availSize);
>    rv = ReflowChild(childFrame, aPresContext, aDesiredSize,
>                     childReflowState, aStatus);
>    SaveReflowAndBoundingMetricsFor(childFrame, aDesiredSize,
>                                    aDesiredSize.mBoundingMetrics);
>    mBoundingMetrics = aDesiredSize.mBoundingMetrics;
>  }
>  FinalizeReflow(*aReflowState.rendContext, aDesiredSize);
>  NS_FRAME_SET_TRUNCATION(aStatus, aReflowState, aDesiredSize);
>  return rv;
>}
>
>// Only place the selected child ...
>/* virtual */ nsresult
>nsMathMLmactionFrame::Place(nsRenderingContext& aRenderingContext,
>                            bool                 aPlaceOrigin,
>                            nsHTMLReflowMetrics& aDesiredSize)
>{
>  aDesiredSize.width = aDesiredSize.height = 0;
>  aDesiredSize.ascent = 0;
>  mBoundingMetrics = nsBoundingMetrics();
>  nsIFrame* childFrame = GetSelectedFrame();
>  if (childFrame) {
>    GetReflowAndBoundingMetricsFor(childFrame, aDesiredSize, mBoundingMetrics);
>    if (aPlaceOrigin) {
>      FinishReflowChild(childFrame, PresContext(), nsnull, aDesiredSize, 0, 0, 0);
>    }
>    mReference.x = 0;
>    mReference.y = aDesiredSize.ascent;
>  }
>  aDesiredSize.mBoundingMetrics = mBoundingMetrics;
>  return NS_OK;
>}
>
>// ################################################################
>// Event handlers 
>// ################################################################
>
>NS_IMPL_ISUPPORTS1(nsMathMLmactionFrame::MouseListener,
>                   nsIDOMEventListener)
>
>
>// helper to show a msg on the status bar
>// curled from nsObjectFrame.cpp ...
>void
>ShowStatus(nsPresContext* aPresContext, nsString& aStatusMsg)
>{
>  nsCOMPtr<nsISupports> cont = aPresContext->GetContainer();
>  if (cont) {
>    nsCOMPtr<nsIDocShellTreeItem> docShellItem(do_QueryInterface(cont));
>    if (docShellItem) {
>      nsCOMPtr<nsIDocShellTreeOwner> treeOwner;
>      docShellItem->GetTreeOwner(getter_AddRefs(treeOwner));
>      if (treeOwner) {
>        nsCOMPtr<nsIWebBrowserChrome> browserChrome(do_GetInterface(treeOwner));
>        if (browserChrome) {
>          browserChrome->SetStatus(nsIWebBrowserChrome::STATUS_LINK, aStatusMsg.get());
>        }
>      }
>    }
>  }
>}
>
>NS_IMETHODIMP
>nsMathMLmactionFrame::MouseListener::HandleEvent(nsIDOMEvent* aEvent)
>{
>  nsAutoString eventType;
>  aEvent->GetType(eventType);
>  if (eventType.EqualsLiteral("mouseover")) {
>    mOwner->MouseOver();
>  }
>  else if (eventType.EqualsLiteral("click")) {
>    mOwner->MouseClick();
>  }
>  else if (eventType.EqualsLiteral("mouseout")) {
>    mOwner->MouseOut();
>  }
>  else {
>    NS_ABORT();
>  }
>
>  return NS_OK;
>}
>
>void
>nsMathMLmactionFrame::MouseOver()
>{
>  // see if we should display a status message
>  if (NS_MATHML_ACTION_TYPE_STATUSLINE == mActionType) {
>    nsString value;
>    // retrieve content from a second child and check whether it's mtext or not
>    nsIContent *content = mFrames.FirstChild()->GetNextSibling()->GetContent();
>    if (content->LocalName().EqualsLiteral("mtext"))
>    {
>      content->GetFirstChild()->GetText()->AppendTo(value);
>      ShowStatus(PresContext(), value);
>    }
>  }
>}
>
>void
>nsMathMLmactionFrame::MouseOut()
>{
>  // see if we should remove the status message
>  if (NS_MATHML_ACTION_TYPE_STATUSLINE == mActionType) {
>    nsAutoString value;
>    value.SetLength(0);
>    ShowStatus(PresContext(), value);
>  }
>}
>
>void
>nsMathMLmactionFrame::MouseClick()
>{
>  if (NS_MATHML_ACTION_TYPE_TOGGLE == mActionType) {
>    if (mChildCount > 1) {
>      PRInt32 selection = (mSelection == mChildCount)? 1 : mSelection + 1;
>      nsAutoString value;
>      char cbuf[10];
>      PR_snprintf(cbuf, sizeof(cbuf), "%d", selection);
>      value.AssignASCII(cbuf);
>      bool notify = false; // don't yet notify the document
>      mContent->SetAttr(kNameSpaceID_None, nsGkAtoms::selection_, value, notify);
>
>      // Now trigger a content-changed reflow...
>      PresContext()->PresShell()->
>        FrameNeedsReflow(mSelectedFrame, nsIPresShell::eTreeChange,
>                         NS_FRAME_IS_DIRTY);
>    }
>  }
>}
Comment 9 Andrii Zui 2012-03-26 10:18:08 PDT
Sorry for this spam, tried to fix a typo (line 369: whether, not wheter) in my patch, but that's not critical. Sorry for this spam.
Comment 10 Frédéric Wang (:fredw) 2012-03-26 10:26:41 PDT
Thanks for working on this, Andriy. In general, people submit their changes as one or more "patches" so that they can easily be understood by others and integrated in the trunk. The idea is that only the necessary changes should appear in a patch.

Please have a look at:
https://developer.mozilla.org/En/Developer_Guide/How_to_Submit_a_Patch

And also:
https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide

When you think your patch is ready, you may ask Karl to review it.
Comment 11 Andrii Zui 2012-03-26 13:32:59 PDT
Created attachment 609459 [details] [diff] [review]
bug-729924-fix
Comment 12 Karl Tomlinson (ni?:karlt) 2012-03-26 20:18:43 PDT
Comment on attachment 609459 [details] [diff] [review]
bug-729924-fix

>+    if (content->LocalName().EqualsLiteral("mtext")) {

I'm not familiar with LocalName().  Would Tag() and GetNameSpaceID() be more
appropriate?  (That is what nsCSSFrameConstructor uses, and there are similar
uses in nsMathMLContainerFrame.)

>+      nsAutoString value;
>+      textFrg->AppendTo(value);
>       ShowStatus(PresContext(), value);

It's probably more appropriate to name the string something like "text" now
that it is not an attribute value.

http://www.w3.org/TR/MathML3/chapter3.html#presm.mtext
and http://www.w3.org/TR/MathML3/chapter2.html#fund.collapse
indicate that whitespace should be collapsed.

nsMathMLTokenFrame uses text.CompressWhitespace(), which I expect should do
the right thing.
Comment 13 Andrii Zui 2012-03-27 00:50:50 PDT
Karl, thanks for the review, I've already fixed the issues you mentioned. Still, I'm not sure which extraction is more appropriate: I could use NodeName instead of LocalName, or I could write 

>   content->Tag()->ToString(buf);

and then use EqualsLiteral. The output is the same. Which one should I write?
Comment 14 Karl Tomlinson (ni?:karlt) 2012-03-27 01:44:41 PDT
No need to convert to string.
See use of GetNameSpaceID() and Tag() in nsMathMLContainerFrame.cpp.
Comment 15 Andrii Zui 2012-03-27 02:17:41 PDT
Created attachment 609662 [details] [diff] [review]
bug-729924-fix corrected

Patch corrected.
Comment 16 Frédéric Wang (:fredw) 2012-03-27 02:58:11 PDT
Comment on attachment 609662 [details] [diff] [review]
bug-729924-fix corrected

>+    // retrieve content from a second child
>+    nsIFrame* child = mFrames.FirstChild()->GetNextSibling();
>+    if (!child) return;
>+

I think you also have to verify that the first child exists.
Comment 17 Andrii Zui 2012-03-27 03:19:26 PDT
(In reply to Frédéric Wang from comment #16)
> Comment on attachment 609662 [details] [diff] [review]
> bug-729924-fix corrected
> 
> >+    // retrieve content from a second child
> >+    nsIFrame* child = mFrames.FirstChild()->GetNextSibling();
> >+    if (!child) return;
> >+
> 
> I think you also have to verify that the first child exists.

IIUC, getters that never fail and never return null are named Foo(), while all other getters use GetFoo() - quote from the Mozilla Coding Style Guide. Also, if there are no children at all, you simply won't be able to get into MouseOver event.
Comment 18 Karl Tomlinson (ni?:karlt) 2012-03-27 14:37:51 PDT
Comment on attachment 609662 [details] [diff] [review]
bug-729924-fix corrected

(In reply to Frédéric Wang from comment #16)
> I think you also have to verify that the first child exists.

Yes, I think you are right.

(In reply to Andriy Zui from comment #17)
> IIUC, getters that never fail and never return null are named Foo(), while
> all other getters use GetFoo() - quote from the Mozilla Coding Style Guide.

Yes, that is meant to be the convention, but not all frames are going to have
children, so apparently the convention wasn't followed here.

> Also, if there are no children at all, you simply won't be able to get into
> MouseOver event.

Perhaps we might get into such a situation during event propagation if one of
the event handlers removes the children.

I suggest using mFrames.FrameAt(1), so that we need only check one nsIFrame.
(The other check is done in FrameAt.)

>+    if (content->Tag() == nsGkAtoms::mtext_) {

That part looks good.  What about GetNameSpaceID()?  I assume that should also
be checked.
Comment 19 Andrii Zui 2012-03-27 15:02:19 PDT
Created attachment 609903 [details] [diff] [review]
bug-729924-fix corrected

Corrected.
Comment 20 Andrii Zui 2012-03-27 15:10:22 PDT
Created attachment 609906 [details] [diff] [review]
bug-729924-fix corrected

Now namespace is checked first, as in all if's in nsMathMLContainerFrame.cpp.
Comment 21 Andrii Zui 2012-03-27 15:32:15 PDT
Created attachment 609919 [details] [diff] [review]
bug-729924-fix final

Renamed child to childFrame and did a minor code style fix which I haven't noticed before. Karl, I'm very sorry for this spam, I'll read my code more carefully and multiple times in the future to avoid this.
Comment 22 Karl Tomlinson (ni?:karlt) 2012-03-27 15:37:54 PDT
Comment on attachment 609919 [details] [diff] [review]
bug-729924-fix final

Yes, we're not very consistent on '*'s and spaces, but this looks good, thanks.
Comment 23 Frédéric Wang (:fredw) 2012-03-28 05:28:06 PDT
Patch sent to try server:
https://tbpl.mozilla.org/?tree=Try&rev=6afee260953e
Comment 24 Ryan VanderMeulen [:RyanVM] 2012-03-28 18:01:26 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/095fd525afa7

Also, to make life easier for those checking in patches for you, please follow
the directions below for any future patches you submit. Thanks!
https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Comment 25 Phil Ringnalda (:philor) 2012-03-28 22:53:40 PDT
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/bb679e5939b9 - it probably was something else in the same push causing multiple test failures, but you got caught in the sweep.
Comment 27 Matt Brubeck (:mbrubeck) 2012-03-29 08:39:36 PDT
https://hg.mozilla.org/mozilla-central/rev/d3cc19901240
Comment 29 Frédéric Wang (:fredw) 2012-04-13 02:28:33 PDT
I also updated the status page:
https://developer.mozilla.org/en/Mozilla_MathML_Project/Status#section_8
Comment 30 Frédéric Wang (:fredw) 2014-01-28 04:02:52 PST
(In reply to Karl Tomlinson (back Jan 28 :karlt) from comment #6)
> Ah, thanks.  Sorry, I overlooked that.
> Modifying the existing actiontype=statusline to match the spec sounds good.
> 
> Interestingly window.status is only disabled in Firefox, not in Core.
> 

I just read this page http://html5sec.org/ where the statusline is combined with href to hide a javascript link:

<math>
  <maction actiontype="statusline" href="javascript:alert(3)">
    <mtext>CLICKME</mtext>
    <mtext>http://http://google.com</mtext>
   </maction>
</math>

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