Closed Bug 729924 (maction-statusline) Opened 13 years ago Closed 13 years ago

maction: statusline actiontype should use the second child as a message

Categories

(Core :: MathML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: fredw, Assigned: PraZuBeR)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [good second bug])

Attachments

(2 files, 6 obsolete files)

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>
Attached file testcase
Attached file testcase (MathJax rendering) (obsolete) —
This displays something in the statusline for Opera, but does not currently seem to work for Firefox.
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.
Keywords: dev-doc-needed
Whiteboard: [good second bug]
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.
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).
(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.
Attached patch bugfix (obsolete) — Splinter Review
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); > } > } >}
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.
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.
Attached patch bug-729924-fix (obsolete) — Splinter Review
Attachment #609360 - Attachment is obsolete: true
Attachment #609459 - Flags: review?(karlt)
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.
Attachment #609459 - Flags: review?(karlt) → review-
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?
No need to convert to string. See use of GetNameSpaceID() and Tag() in nsMathMLContainerFrame.cpp.
Attached patch bug-729924-fix corrected (obsolete) — Splinter Review
Patch corrected.
Attachment #609459 - Attachment is obsolete: true
Attachment #609662 - Flags: review?(karlt)
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.
Alias: maction-statusline
Summary: maction: statusline actiontype should display use the second child as a message → maction: statusline actiontype should use the second child as a message
(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 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.
Attachment #609662 - Flags: review?(karlt) → review-
Attached patch bug-729924-fix corrected (obsolete) — Splinter Review
Corrected.
Attachment #609662 - Attachment is obsolete: true
Attachment #609903 - Flags: review?(karlt)
Attached patch bug-729924-fix corrected (obsolete) — Splinter Review
Now namespace is checked first, as in all if's in nsMathMLContainerFrame.cpp.
Attachment #609903 - Attachment is obsolete: true
Attachment #609906 - Flags: review?(karlt)
Attachment #609903 - Flags: review?(karlt)
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.
Attachment #609906 - Attachment is obsolete: true
Attachment #609919 - Flags: review?(karlt)
Attachment #609906 - Flags: review?(karlt)
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.
Attachment #609919 - Flags: review?(karlt) → review+
Attachment #600013 - Attachment is obsolete: true
Assignee: nobody → PraZuBeR
Status: NEW → ASSIGNED
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
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → mozilla14
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.
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
(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>
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: