Closed Bug 269428 Opened 20 years ago Closed 20 years ago

implement <xf:message>

Categories

(Core Graveyard :: XForms, defect)

Product:
Core Graveyard
Component:
XForms
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: smaug, Assigned: smaug)

References

Details

Attachments

(3 files, 12 obsolete files)

Showcase of inherited styles
1.66 KB, text/xml
Details
+ comments
32.61 KB, patch
Details | Diff | Splinter Review
xf:message for trunk, handleAction method splitted
45.09 KB, patch
bryner
: superreview+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a4) Gecko/20040927
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a4) Gecko/20040927

tracking bug for message element

Reproducible: Always
Steps to Reproduce:
1.
2.
3.
Bug 269258 might be useful for modal and modeless message
Depends on: 269258
Attached patch v 0.1 (obsolete) — — Splinter Review 
Ephemeral messages are now some sort of tooltips. They may contain any
elements.
If ephemeral message is activated using a mouse event, the place of the
message in the screen depends on the coordinates of the event, otherwise
the coordinates come from the event target.
To enable ephemeral messages also inside action element, I had to
implement action element as an nsIXTFVisual.

Modal and modeless messages are opened in a new window. 
Name of the window comes from the level attribute of the message element.
This patch utilizes the patch in Bug 269258.

Comments, please :)
Attached patch v 0.1.1 (obsolete) — — Splinter Review 

        Attachment #165895 -
        Attachment is obsolete: true

    
  
Assignee: aaronr → smaug
Status: UNCONFIRMED → NEW
Ever confirmed: true

    
    

    
  
Finally got around to it ... I think it looks good, with a few comments:
* I would expect 'modal' messages to inherit the stylesheets from the parent
document.

* I do not really know about the timer on 'ephemeral' messages. If I show a
help-like message, I would expect the message to stay on screen for as long as
I'm f.x. inside an input box, or hovering over an element.

    
    

    
  
Could you tell me how the style inheritance should work? ;)
I already implemented something, which basically copied computed style
from parent document to modal/modeless documents' <body> element 
(also for other elements could be possible), but
that is probably quite slow way to do it.
Also after that selectors wouldn't work, how to inherit pseudo-classes etc..


    
    

    
  
Functionality-wise? Yes :)

    
    

    
  


  
I would expect the modal window to have the same style as the ephemeral, not
size-wise, etc. but the div's should be styled.

        Attachment #166154 -
        Flags: review?(bryner)
Status: NEW → ASSIGNED

        Attachment #166154 -
        Attachment is obsolete: true

        Attachment #166154 -
        Flags: review?(bryner)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v2 (obsolete)
        — 
          — Splinter Review
      

    


  
This uses data: scheme to create modal and modeless messages.
No longer depends on: 269258

    
    

    
  
Comment on attachment 168510 [details] [diff] [review]
v2

This patch has still the timer for the ephemeral messages. Should we have it?


"I would expect the message to stay on screen for as long as
I'm f.x. inside an input box"
This case we should register to listen two events, for example mouseover and
mouseout. Not a good solution.

        Attachment #168510 -
        Flags: review?(allan)

    
    

    
  
Comment on attachment 168510 [details] [diff] [review]
v2

The instance data does not work on the attached test case.

        Attachment #168510 -
        Flags: review?(allan) → review-

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v3 (obsolete)
        — 
          — Splinter Review
      

    


  
oops, sorry. <output> elements inside messages weren't handled properly.
Added CloneNode() to nsXFormsMessage.

        Attachment #168510 -
        Attachment is obsolete: true

        Attachment #168559 -
        Flags: review?(allan)

    
    

    
  
(In reply to comment #9)
> (From update of attachment 168510 [details] [diff] [review] [edit])
> This patch has still the timer for the ephemeral messages. Should we have it?
> 
> 
> "I would expect the message to stay on screen for as long as
> I'm f.x. inside an input box"
> This case we should register to listen two events, for example mouseover and
> mouseout. Not a good solution.

As we talked about yesterday, I'm still in favor of a (ephemeral) message to
stay on screen until I somehow tell it otherwise. As in: Do not do stuff I have
not asked for.

(The timer is till needed to support <hint/>, which IMHO should popup and
disappear automatically, but where the timer should be is all up to you :) )

    
    

    
  
> 
> As we talked about yesterday, I'm still in favor of a (ephemeral) message to
> stay on screen until I somehow tell it otherwise. As in: Do not do stuff I have
> not asked for.

But how would you then handle the case like this:
<xf:message level="ephemeral" ev:event="blur">foo</xf:message>
When should we hide the message?

And what if the message is covering something important on the page?

> 
> (The timer is till needed to support <hint/>, which IMHO should popup and
> disappear automatically, but where the timer should be is all up to you :) )

But hint is still just an ephemeral message, so why should it be a special case.

Comments from anyone else?

    
    

    
  
Comment on attachment 168559 [details] [diff] [review]
v3

NB. I have only looked at nsXFormsMessageElement...

+++ nsXFormsMessageElement.cpp	2004-12-12 21:05:02.000000000 +0200

Nit: How about having implementation of inherited functions first, and
proprietary functions after them, to keep consistency with rest of xforms code?
(f.x.: Have OnCreated() as the first function, and your .*Ephemeral() just
before factory)

>+class nsXFormsMessageElement : public nsXFormsXMLVisualStub,
>+  // The position of the ephemeral message
>+  PRInt32 mX;
>+  PRInt32 mY;

Nit: Why not "include the comment" in the names? mPosX, mPosY?

>+nsXFormsMessageElement::ShowEphemeral()
...
>+  if (mEphemeralTimer)
>+    mEphemeralTimer->InitWithFuncCallback(sEphemeralCallbackHide, this, 
>+                                          3000, nsITimer::TYPE_ONE_SHOT);

How about using a const or a define for the timeout?

>+  nsXFormsMessageElement * msg =
>+    NS_STATIC_CAST(nsXFormsMessageElement *,
>+                   doc->GetProperty(nsXFormsAtoms::messageProperty));

Nit:
  nsXFormsMessageElement *msg =
    NS_STATIC_CAST(nsXFormsMessageElement*,
		   doc->GetProperty(nsXFormsAtoms::messageProperty));

But, do you need to use Get/SetProperty and messageProperty at all? Can you not
use a static class member instead?

>+nsXFormsMessageElement::WillChangeDocument(nsIDOMDocument *aNewDocument)
...
>+      nsXFormsMessageElement * msg =
>+        NS_STATIC_CAST(nsXFormsMessageElement *,
>+                       doc->GetProperty(nsXFormsAtoms::messageProperty));

Same nit as above.

>+nsXFormsMessageElement::CloneNode(nsIDOMNode* aSrc, nsIDOMNode** aTarget)
...
>+  // Clone the visual content of the <output>.
of eventual <output>s.

>+  // Accoring to the XForms Schema it is enough 
According

>+nsXFormsMessageElement::CloneNode(nsIDOMNode* aSrc, nsIDOMNode** aTarget)

I do not know enough about DOM to review this function, but it seems to work in
action :)

>+nsXFormsMessageElement::HandleAction(nsIDOMEvent* aEvent, 
...
>+  PRBool hasSNB = nsXFormsUtils::GetSingleNodeBindingValue(mElement,

PRBool hasBinding? SNB is not a too well known TLA :)

>+  //XXX How to handle the following:
>+  //XXX <message level="ephemeral" src="http://mozilla.org"/>

Good question :)

>+          // Move the tooltip a bit.

tooltip? message I guess? Same goes for:

>+            // Tooltip 10 pixels from the left-top corner of the target element.

I have not tried this in practice, and I know it's difficult to get it exactly
right, but should the message not keep out of way of the control element? F.x.
10 pixels from the bottom-left corner instead?

>+        }
>+        else {

Nit. Keep on same line. Same goes next two |else|s in same function.

>+          nsCOMPtr<nsIDOM3Node> e3(do_QueryInterface(mVisualElement));

e3? At least n3, then. But I would prefer visualNode

>+    if (!hasSrc) {
>+
>+    }

?

>+        nsCOMPtr<nsIDOMCSSValue> w;

This, and the later h, are not too descriptive. cssWidth?

>+        nsCOMPtr<nsIDOMCSSPrimitiveValue> pvalueW(do_QueryInterface(w));

pValueW(idth) or cssWidthValue? Same goes for pvalueH.

>+        computedWidth = NS_STATIC_CAST(PRInt32, width);
>+        computedHeight = NS_STATIC_CAST(PRInt32, height);

Is this the Mozilla-way of doing float-to-int conversion?

>+      rv = ddoc->CreateElementNS(NS_LITERAL_STRING(NS_NAMESPACE_XHTML),
>+                                 NS_LITERAL_STRING("head"),
>+                                 getter_AddRefs(headEl));
>+      NS_ENSURE_SUCCESS(rv, rv);
>+      htmlEl->AppendChild(headEl, getter_AddRefs(tmp));

How about setting the title to something? "XForms Message"? Dunno.

>+        nsCOMPtr<nsIDOM3Node> b3(do_QueryInterface(bodyEl));

A bit short variable name.

>+      // We need some args to set the window modal.
>+      args =
>+        do_QueryInterface(NS_STATIC_CAST(nsIXFormsActionModuleElement*,this));

Include XXX/todo in the comment, and (nit) insert a space before this.

        Attachment #168559 -
        Flags: review?(allan) → review-

    
    

    
  
I still have to test some things in action, but that'll be tomorrow.

    
    

    
  
(In reply to comment #13)
> > 
> > As we talked about yesterday, I'm still in favor of a (ephemeral) message to
> > stay on screen until I somehow tell it otherwise. As in: Do not do stuff I have
> > not asked for.
> 
> But how would you then handle the case like this:
> <xf:message level="ephemeral" ev:event="blur">foo</xf:message>
> When should we hide the message?
> 
> And what if the message is covering something important on the page?
> 
> > 
> > (The timer is till needed to support <hint/>, which IMHO should popup and
> > disappear automatically, but where the timer should be is all up to you :) )
> 
> But hint is still just an ephemeral message, so why should it be a special case.
> 
> Comments from anyone else?

One reason to make it special is just so that hint IS different from ephemeral,
thereby giving the author yet another way to do a similar, but not exactly the
same, thing.  One other way to maybe do ephemeral is keep the message on the
screen until the mouse is moved again.  Then make the user move the mouse away
and come back before displaying it again.

    
    

    
  
(In reply to comment #14)
> 
> But, do you need to use Get/SetProperty and messageProperty at all? Can you not
> use a static class member instead?

No, I don't want that one XForms document modifies another one.

> 
> How about setting the title to something? "XForms Message"? Dunno.


Ah, I must have accidentally removed the title part. It was definately
there in some revision :)



    
    

    
  

      
      
      
      

        Attached patch
          
          
        v4 (obsolete)
        — 
          — Splinter Review
      

    


  
- Review comments
- Fixed a crash when using modal messages, but now the styling is apparently a
bit
  different.

        Attachment #168559 -
        Attachment is obsolete: true

        Attachment #168656 -
        Flags: review?(allan)

    
    

    
  
(In reply to comment #13)
> > 
> > As we talked about yesterday, I'm still in favor of a (ephemeral) message to
> > stay on screen until I somehow tell it otherwise. As in: Do not do stuff I have
> > not asked for.
> 
> But how would you then handle the case like this:
> <xf:message level="ephemeral" ev:event="blur">foo</xf:message>
> When should we hide the message?
> 
> And what if the message is covering something important on the page?

Ok, I guess my point is "political" (and only covers <message>, not <hint>): How
much should be left to the XForms designer? My point is that the page designer
should decide when to remove the ephemeral message again, we should not do stuff
he/she is not asking for. Whereas removing it automatically, might help some,
but annoy others.

> > 
> > (The timer is till needed to support <hint/>, which IMHO should popup and
> > disappear automatically, but where the timer should be is all up to you :) )
> 
> But hint is still just an ephemeral message, so why should it be a special case.

hint is implemented as an ephemeral message, but it still has a special purpose.

    
    

    
  
(In reply to comment #17)
> (In reply to comment #14)
> > 
> > But, do you need to use Get/SetProperty and messageProperty at all? Can you not
> > use a static class member instead?
> 
> No, I don't want that one XForms document modifies another one.

doh, of course. Fair enough then :)

    
    

    
  
(In reply to comment #14)
> Nit: How about having implementation of inherited functions first, and
> proprietary functions after them, to keep consistency with rest of xforms code?
> (f.x.: Have OnCreated() as the first function, and your .*Ephemeral() just
> before factory)

Do have a comment for not doing it?
 
> >+  nsXFormsMessageElement * msg =
> >+    NS_STATIC_CAST(nsXFormsMessageElement *,
> >+                   doc->GetProperty(nsXFormsAtoms::messageProperty));
> Nit:
>   nsXFormsMessageElement *msg =
>     NS_STATIC_CAST(nsXFormsMessageElement*,
> 		   doc->GetProperty(nsXFormsAtoms::messageProperty));

Still there.
 
> >+nsXFormsMessageElement::WillChangeDocument(nsIDOMDocument *aNewDocument)
> ...
> >+      nsXFormsMessageElement * msg =
> >+        NS_STATIC_CAST(nsXFormsMessageElement *,
> >+                       doc->GetProperty(nsXFormsAtoms::messageProperty));
> 
> Same nit as above.

do.
 
> >+nsXFormsMessageElement::CloneNode(nsIDOMNode* aSrc, nsIDOMNode** aTarget)
> ...

> >+        }
> >+        else {
> 
> Nit. Keep on same line. Same goes next two |else|s in same function.

All still there.
 
> >+        computedWidth = NS_STATIC_CAST(PRInt32, width);
> >+        computedHeight = NS_STATIC_CAST(PRInt32, height);
> 
> Is this the Mozilla-way of doing float-to-int conversion?

I guess it is then?

    
    

    
  
Comment on attachment 168656 [details] [diff] [review]
v4

#include "above comments".

Model has the correct size now, cute. With the above answered/changed, I'm the
happy camper.

We still have one issue though, that is styling. But let's do a seperate bug
for that?

r=me

    
    

    
  
Comment on attachment 168656 [details] [diff] [review]
v4

#include "above comments".

Model has the correct size now, cute. With the above answered/changed, I'm the
happy camper.

We still have one issue though, that is styling. But let's do a seperate bug
for that?

r=me

        Attachment #168656 -
        Flags: review?(allan) → review+

    
    

    
  
Hmm, one comment was probably enough :)

(In reply to comment #23)
> Model has the correct size now, cute. With the above answered/changed, I'm the
> happy camper.

"Modal" that is ...

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v4 + Allan's comments (obsolete)
        — 
          — Splinter Review
      

    


  
Darin could you look at this, especially the DOM serialization and
data: scheme creation.

        Attachment #168784 -
        Flags: superreview?(darin)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        opps this is the v4 + comments (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #168784 -
        Attachment is obsolete: true

        Attachment #168785 -
        Flags: superreview?(darin)

        Attachment #168784 -
        Flags: superreview?(darin)

    
    

    
  
When I use this sample from the XForms spec:

<model>
  <message level="modal" ev:event="xforms-ready">This is not a drill!</message>
  ...
</model>

I don't get anything close to the suggested UI:
http://www.w3.org/TR/2003/REC-xforms-20031014/images/ui-input-with-alert.png

Is there a reason why you are not just using window.alert to implement this?

    
    

    
  
OK, I sort of answered my own question.  If there is not an instance data
binding on the <message> tag, then we are supposed to render the explicit
content of the <message> element.  OK, fine... but perhaps we should be loading
a XUL document and then modifying its DOM directly instead of doing the
serialization to data: URL.  nsPromptService.cpp loads
chrome://global/content/commonDialog.xul, so we might want to consider hacking
that dialog at runtime.

http://lxr.mozilla.org/mozilla/source/toolkit/content/commonDialog.xul

Take note of the element with ID info.header and info.box.  It might be better
to insert content into one of those XUL box elements (bryner suggested info.header).

    
    

    
  
(In reply to comment #28)
> a XUL document and then modifying its DOM directly instead of doing the
> serialization to data: URL.  nsPromptService.cpp loads
> chrome://global/content/commonDialog.xul, so we might want to consider hacking
> that dialog at runtime.
> 

Hmm, what should then happen to <script> elements? If <message> has one as a
child, it will be (re)executed in chrome. And that would be dangerous.



    
    

    
  
Yup, that sounds like it'd be a problem.  I hadn't considered that :-(  Hmm...

    
    

    
  
Still, it seems like the standard modal message should have an "OK" button, no?
 I also noticed a bug in a seamonkey build, where the modal message dialog was
the same size as the browser window (observed under WinXP).
Blocks: 277318

    
    

    
  

      
      
      
      

        Attached patch
          
          
        OK button (obsolete)
        — 
          — Splinter Review
      

    


  
This adds an OK button (value is from chrome://global/locale/dialog.properties
)
and the modal/modeless windows should have better size now.
Tested on Linux.

I may still investigate whether it would be possible to use commonDialog.xul as
a template for messages. (There are some problem to use it with modal
messages).
But messages are useable even without that and look and feel is pretty good
too.

        Attachment #168656 -
        Attachment is obsolete: true

        Attachment #168785 -
        Flags: superreview?(darin)

        Attachment #170664 -
        Flags: review?(allan)

    
    

    
  
xforms.css should probably contain something for
the ephemeral messages. Perhaps

message[level="ephemeral"], hint {
  background: #ffffcc;
  border: 1px black solid;
  visibility: hidden !important;
  position: absolute !important;
}

    
    

    
  

      
      
      
      

        Attached patch
          
          
        + help, hint, alert (obsolete)
        — 
          — Splinter Review
      

    


  
Decided to put also hint, help and alert to this one.

        Attachment #170664 -
        Attachment is obsolete: true

        Attachment #170692 -
        Flags: review?(allan)

    
    

    
  
Comment on attachment 170664 [details] [diff] [review]
OK button

It's Monday and I am no XUL-guy, so I am not the correct guy to get a code
review from, but the OK button looks nice :)

r=me

        Attachment #170664 -
        Flags: review?(allan) → review+

    
    

    
  
Comment on attachment 170692 [details] [diff] [review]
+ help, hint, alert

Here's my "Monday-review" :)

1) Code-wise I only have one thing. That's a comment for this one:
> --- nsXFormsMessageElement.cpp	6 Jan 2005 22:43:23 -0000	1.1.2.1
>+            mPosY = mPosY + height;
>+ 
>+            if (height > 20)
>+              mPosY -= height > 30 ? 10 : 10 - (30 - height);

The for the "user experience":

2) I would personally like to see the hint appear if I TAB into a control, I
try not to use my mouse if I can help it.

3) How about letting the OK-button have focus initially, in a modal message
dialog?

4) @src on ephemeral does not work, but that's a different story. Will you
create a seperate bug as a reminder, and include a link to it in the code?

Otherwise, everything seems to work, but we should get somebody more QA-like to
look at it though, it's still a bit "quirky". This probably needs an
explanation, let's chat about that one.

With the above four issues handled, it's 10-4 from here.

r=me

        Attachment #170692 -
        Flags: review?(allan) → review+

    
    

    
  
+  aURL = NS_ConvertUTF8toUTF16(b64String);

  CopyUTF8toUTF16(b64String, aURL);
is a bit more efficient...

    
    

    
  


  
Added few comments and tried to reduce the quirkyness of the <hint>s.
I'd like to fix the xforms-hint/help event creation bug after Bug 265467
is checked in.

        Attachment #168785 -
        Attachment is obsolete: true

        Attachment #170692 -
        Attachment is obsolete: true

        Attachment #171184 -
        Flags: superreview?(darin)

    
    

    
  
Comment on attachment 171184 [details] [diff] [review]
+ help, hint, alert, + allan's comments

The MDG patch was landed, so I'll do still a new patch using that.
I should be able to fix the  xforms-hint/help event creation bug now.

        Attachment #171184 -
        Flags: superreview?(darin)

    
    

    
  


  
This is for the branch.

        Attachment #171184 -
        Attachment is obsolete: true

        Attachment #171655 -
        Flags: superreview?(darin)

    
    

    
  
Comment on attachment 171655 [details] [diff] [review]
This should create xforms-hint and xforms-help properly.

>Index: nsXFormsControlStub.cpp

>+  if (mEventListener) {
>+    targ->RemoveEventListener(NS_LITERAL_STRING("mouseover"), mEventListener, PR_TRUE);
>+    targ->RemoveEventListener(NS_LITERAL_STRING("focus"), mEventListener, PR_TRUE);
>+    targ->RemoveEventListener(NS_LITERAL_STRING("keypress"), mEventListener, PR_TRUE);
>+    mEventListener = nsnull;
>+  }
>+  
>+  if (aInitialize) {
>+    mEventListener = new nsXFormsHintHelpListener();
>+    if (!mEventListener)
>+      return;
>+    
>+    targ->AddEventListener(NS_LITERAL_STRING("mouseover"), mEventListener, PR_TRUE);
>+    targ->AddEventListener(NS_LITERAL_STRING("focus"), mEventListener, PR_TRUE);
>+    targ->AddEventListener(NS_LITERAL_STRING("keypress"), mEventListener, PR_TRUE);
>+  }
>+}

nit: It might be good to use NS_NAMED_LITERAL_STRING here for
those three strings as an optimization so you don't have to 
construct the objects twice.


>Index: nsXFormsControlStub.h

>+  /** This event listener is used to create xforms-hint and xforms-help events.*/
>+  nsCOMPtr<nsXFormsHintHelpListener> mEventListener;

nit: Since nsXFormsHintHelpListener doesn't add any methods, I
think it would be better to change this to:

  nsCOMPtr<nsIDOMEventListener> mEventListener;

It is preferred to use nsCOMPtr only with XPCOM interface
pointers (though people do otherwise somewhat frequently).


>+nsXFormsMessageElement::ConstructMessageWindowURL(nsAString& aData,
...
>+    if (bundle) {
>+      
>+      bundle->GetStringFromName(NS_LITERAL_STRING("button-accept").get(),

nit: maybe kill that newline


sr=darin

        Attachment #171655 -
        Flags: superreview?(darin) → superreview+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        + comments
        — 
          — Splinter Review
      

    


  

        Attachment #171655 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        xf:message for trunk (obsolete)
        — 
          — Splinter Review
      

    


  

    
    

    
  
Comment on attachment 171781 [details] [diff] [review]
xf:message for trunk

I'm not sure if this needs to be re-reviewed for the trunk.
If not, could you just mark
r+ / sr+.

        Attachment #171781 -
        Flags: superreview?(darin)

        Attachment #171781 -
        Flags: review?(allan)

    
  

        Attachment #171781 -
        Flags: review?(allan) → review+

    
    

    
  
Comment on attachment 171781 [details] [diff] [review]
xf:message for trunk

bryner might have time for this. I hope.

        Attachment #171781 -
        Flags: superreview?(darin) → superreview?(bryner)

    
    

    
  
Comment on attachment 171781 [details] [diff] [review]
xf:message for trunk

>--- nsXFormsActionElement.cpp	5 Nov 2004 02:15:00 -0000	1.1
>+++ nsXFormsActionElement.cpp	19 Jan 2005 20:11:48 -0000
>@@ -37,31 +37,88 @@
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsXFormsActionElement.h"
> #include "nsIXFormsModelElement.h"
> #include "nsIDOMNodeList.h"
> #include "nsIDOMDocument.h"
> #include "nsIDOMEvent.h"
> #include "nsIDOMElement.h"
>+#include "nsIXTFXMLVisualWrapper.h"
>+
>+#define ACTION_STYLE_HIDDEN \
>+  "position:absolute;z-index:2147483647;visibility:hidden;"
> 
> #define DEFERRED_REBUILD     0x01
> #define DEFERRED_RECALCULATE 0x02
> #define DEFERRED_REVALIDATE  0x04
> #define DEFERRED_REFRESH     0x08
> 
> nsXFormsActionElement::nsXFormsActionElement()
> {
> }
> 
>-NS_IMPL_ISUPPORTS_INHERITED3(nsXFormsActionElement,
>-                             nsXFormsActionModuleBase,
>-                             nsIXTFElement,
>-                             nsIXTFGenericElement,
>-                             nsIXFormsActionElement)
>+NS_IMPL_ADDREF_INHERITED(nsXFormsActionElement, nsXFormsXMLVisualStub)
>+NS_IMPL_RELEASE_INHERITED(nsXFormsActionElement, nsXFormsXMLVisualStub)
>+
>+NS_INTERFACE_MAP_BEGIN(nsXFormsActionElement)
>+  NS_INTERFACE_MAP_ENTRY(nsIXFormsActionModuleElement)
>+  NS_INTERFACE_MAP_ENTRY(nsIXFormsActionElement)
>+  NS_INTERFACE_MAP_ENTRY(nsIDOMEventListener)
>+  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIXTFXMLVisual)

You don't need this nsISupports line; that is handled by nsXFormsXMLVisualStub.

It seems unfortunate that all action elements must create divs, when they are
only needed if there is a message child.  However, I don't have any better
ideas at the moment.  I'll think it over a bit.

Why did you make it create a new object to listen for the hint/help events
rather than just use the element class as the listener?

>--- /dev/null	2005-01-19 18:33:46.771559496 +0200
>+++ nsXFormsMessageElement.cpp	2005-01-19 20:18:55.000000000 +0200
>+NS_IMETHODIMP
>+nsXFormsMessageElement::HandleAction(nsIDOMEvent* aEvent, 
>+                                     nsIXFormsActionElement *aParentAction)
>+{

This method could use some comments, both for me and for anyone else who may be
reading it...

...

>+  // ephemeral
>+  /// @bug How to handle the following:
>+  ///      <message level="ephemeral" src="http://mozilla.org"/>
>+  if (level.EqualsLiteral("ephemeral")) {
>+    nsCOMPtr<nsIDOMEventTarget> target;
>+    aEvent->GetTarget(getter_AddRefs(target));
>+    nsCOMPtr<nsIDOMElement> targetEl(do_QueryInterface(target));
>+    if (targetEl) {
>+      nsCOMPtr<nsIDOMNSDocument> nsDoc(do_QueryInterface(doc));
>+      if (nsDoc) {
>+        nsCOMPtr<nsIDOMNSUIEvent> uie(do_QueryInterface(aEvent));
>+        PRInt32 oldX = mPosX;
>+        PRInt32 oldY = mPosY;
>+        if (uie) {
>+          uie->GetPageX(&mPosX);
>+          uie->GetPageY(&mPosY);
>+          // Move the message a bit.
>+          mPosY += 10;
>+        } else {

Do we really need to handle these two cases differently?  I'm a little worried
about inconsistency.

Also, it might be worthwhile to factor out the ephemeral and modal/modeless
cases into separate functions... just makes it clearer what we're doing.

Looks ok otherwise, though I wish it wasn't this complicated.  I'd like to see
a revised patch.

        Attachment #171781 -
        Flags: superreview?(bryner) → superreview-

    
    

    
  
> It seems unfortunate that all action elements must create divs, when they are
> only needed if there is a message child.  However, I don't have any better
> ideas at the moment.  I'll think it over a bit.

Yes, I don't like this either, but someone may use scripts to add 
a message child etc.

> 
> Why did you make it create a new object to listen for the hint/help events
> rather than just use the element class as the listener?

Because for example nsXFormsInputElement is also eventlistener so that
I would have to handle ambiguousness in many places and also the event handling
would be split to many places (for example to nsXFormsInputElement::Focus()).

> This method could use some comments, both for me and for anyone else who may 
> be reading it...

Added something. ;)


> 
> Do we really need to handle these two cases differently?  I'm a little worried
> about inconsistency.

I removed the UIEvent case.

> 
> Also, it might be worthwhile to factor out the ephemeral and modal/modeless
> cases into separate functions... just makes it clearer what we're doing.

Done.

Patch coming soon after few other patches have been checked in.

    
    

    
  


  

        Attachment #171781 -
        Attachment is obsolete: true

        Attachment #172592 -
        Flags: superreview?(bryner)

        Attachment #172592 -
        Flags: superreview?(bryner) → superreview+

    
    

    
  
Checked in on the trunk.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: