Tapestry
  1. Tapestry
  2. TAPESTRY-1102

AlertDialog doesn't trap tabs or set focus to the button

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 4.1.1
    • Fix Version/s: 4.1.1
    • Component/s: JavaScript
    • Labels:
      None

      Description

      The AlertDialog widget used by the validation framework has a couple of navigation issues:

      1) When it comes up, the "OK" button doesn't have the focus, but it appears to the user to be the only active page element.

      2) Users can tab and shift-tab away from the dialog and enter data in form fields behind the bakground iframe. This is non-intuitive, and almost always undesired behavior.

      It appears from the parent class, DoJo's Dialog.js that giving the button a tabIndex, setting it as this.tabStart, and setting initial focus to it should fix these.

      Hard-core would be to trap all keystrokes and stop any key event that wasn't space or enter, but that's probably more of a hammer than is needed.

        Activity

        Greg Woolsey created issue -
        Hide
        Greg Woolsey added a comment -

        Also, the ESC key should close the dialog. If I can figure out how to override the widget without exploding the whole jar file, I'll take a crack at a fix. I know our QA won't let us use 4.1.1 unless little things like this can be fixed, so I want to figure it out before I show them

        I've done both of these with the current dialog DHTML/JS we use in our app, but I want to remove that and use DoJo to reduce the client footprint if I can. I'll see if I can adapt any existing working key event code, but DoJo appears to have it's own framework used by the dojoAttachPoint template attributes, so that may be more a matter of finding their code to do the same thing and using it in AlertDialog.

        (Same with overriding the button look and feel - I need to make it fit our color scheme).

        Show
        Greg Woolsey added a comment - Also, the ESC key should close the dialog. If I can figure out how to override the widget without exploding the whole jar file, I'll take a crack at a fix. I know our QA won't let us use 4.1.1 unless little things like this can be fixed, so I want to figure it out before I show them I've done both of these with the current dialog DHTML/JS we use in our app, but I want to remove that and use DoJo to reduce the client footprint if I can. I'll see if I can adapt any existing working key event code, but DoJo appears to have it's own framework used by the dojoAttachPoint template attributes, so that may be more a matter of finding their code to do the same thing and using it in AlertDialog. (Same with overriding the button look and feel - I need to make it fit our color scheme).
        Hide
        Jesse Kuhnert added a comment -

        Thanks for the input, never had any one care as much about these things. Sounds reasonable to me..

        I've done the first one already (tabStart). The escape key handling I'd love to see implemented as well. I know the editor widget in dojo handles some of these common keys, so perhaps looking at (don't think it's going to be fun exactly ....Some of the keywords to look for are execCommand ...stuff like that):
        http://trac.dojotoolkit.org/browser/trunk/src/widget/RichText.js

        If it doesn't become immediately obvious for you let me know and I'll peg the dojo guy who wrote their event system for some help. The biggest hint will be that RichText (maybe Editor2 instead? ) widget...I know how to do it easily enough on "existing" events - but your uses are more advanced.

        We'd probably love to see more of this type of key handling generalized in dojo as well if it's not already, so feel free to ping me jkuhnert @ gmail. com if you feel like you've found something dojo isn't handling..

        To answer your original question about styling though, just take a look at the current definition for "summarizeErrors" in the tapestry javascript package...You can override that function , or you can override the "AlertDialog" in the tapestry javascript namespace..(or extend and override ) ...I know I know...dojo has namespace concepts as well - and not the javascript kind strictly - they are more like tapestry namespaces...how funny is that?
        http://svn.apache.org/viewvc/tapestry/tapestry4/trunk/tapestry-framework/src/js/tapestry/form/validation.js?view=markup

        I'll deploy the changes I just made in a few more minutes if all goes well.. (This will be the latest pre 0.4 code available from dojo as of an hour or so ago.....It will probably continue to get updated fairly frequently as long as major bug fixing keeps happening - and especially when something in particular gets dropped in that my friend is working on..)

        Show
        Jesse Kuhnert added a comment - Thanks for the input, never had any one care as much about these things. Sounds reasonable to me.. I've done the first one already (tabStart). The escape key handling I'd love to see implemented as well. I know the editor widget in dojo handles some of these common keys, so perhaps looking at (don't think it's going to be fun exactly ....Some of the keywords to look for are execCommand ...stuff like that): http://trac.dojotoolkit.org/browser/trunk/src/widget/RichText.js If it doesn't become immediately obvious for you let me know and I'll peg the dojo guy who wrote their event system for some help. The biggest hint will be that RichText (maybe Editor2 instead? ) widget...I know how to do it easily enough on "existing" events - but your uses are more advanced. We'd probably love to see more of this type of key handling generalized in dojo as well if it's not already, so feel free to ping me jkuhnert @ gmail. com if you feel like you've found something dojo isn't handling.. To answer your original question about styling though, just take a look at the current definition for "summarizeErrors" in the tapestry javascript package...You can override that function , or you can override the "AlertDialog" in the tapestry javascript namespace..(or extend and override ) ...I know I know...dojo has namespace concepts as well - and not the javascript kind strictly - they are more like tapestry namespaces...how funny is that? http://svn.apache.org/viewvc/tapestry/tapestry4/trunk/tapestry-framework/src/js/tapestry/form/validation.js?view=markup I'll deploy the changes I just made in a few more minutes if all goes well.. (This will be the latest pre 0.4 code available from dojo as of an hour or so ago.....It will probably continue to get updated fairly frequently as long as major bug fixing keeps happening - and especially when something in particular gets dropped in that my friend is working on..)
        Hide
        Jesse Kuhnert added a comment -

        Fixed. Releasing in a few more minutes.

        Show
        Jesse Kuhnert added a comment - Fixed. Releasing in a few more minutes.
        Jesse Kuhnert made changes -
        Field Original Value New Value
        Assignee Jesse Kuhnert [ jkuhnert ]
        Fix Version/s 4.1.1 [ 12312021 ]
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Greg Woolsey added a comment -

        Thanks for the quick response. However, when I install the new snapshot you built and run with it's version of DoJo, I get a ton of errors and it can't find the AlertDialog widget any more.

        First, when my page (simple login page with one Tapestry Form component containing name, pwd, and a button) loads, I get these errors:

        DEBUG: DEPRECATED: dojo.lang replaced by dojo.lang.common – will be removed in version: 0.5
        DEBUG: DEPRECATED: dojo.style replaced by dojo.html.style – will be removed in version: 0.5
        DEBUG: DEPRECATED: addParseTreeHandler . ParseTreeHandlers are now reserved for components. Any unfiltered DojoML tag without a ParseTreeHandler is assumed to be a widget – will be removed in version: 0.5
        DEBUG: DEPRECATED: dojo.graphics.color.Color is now dojo.gfx.color.Color. 0.5
        DEBUG: DEPRECATED: dojo.html replaced by dojo.html.* – will be removed in version: 0.5
        DEBUG: DEPRECATED: addParseTreeHandler . ParseTreeHandlers are now reserved for components. Any unfiltered DojoML tag without a ParseTreeHandler is assumed to be a widget – will be removed in version: 0.5

        Which appear to come just from loading the core dojo.js file and initializing logging.

        Then, when I try to submit the form with missing required fields, I get these errors instead of the validation dialog:

        DEBUG: DEPRECATED: dojo.widget.Manager.getImplementationName Could not locate widget implementation for "alertdialog" in "dojo.widget,dojo.widget.validate" registered to namespace "dojo". Developers must specify correct namespaces for all non-Dojo widgets – will be removed in version: 0.5
        ERROR: 10:05:18 AM: Error validating Error : Unable to load https://localhost:443/hedgemon/assets/static/dojo/src/widget/templates/HtmlDialog.html status:404

        I think the 404 is spurious - that template does not exist, but I think dojo went looking because it couldn't find the specified widget.

        Am I missing something?

        It also fails against the latest dojo 0.3.1 with different errors:
        FATAL: Could not load 'dojo.html.style'; last tried '_package_.js'
        FATAL: Could not load 'tapestry.namespace'; last tried '_package_.js'
        FATAL: Could not load 'tapestry.form'; last tried '_package_.js'

        I can be reached at greg . woolsey at gmail if you want. I'll probably be asking more questions and suggesting more things, as we are committed at this point to moving forward with 4.1.1.

        Show
        Greg Woolsey added a comment - Thanks for the quick response. However, when I install the new snapshot you built and run with it's version of DoJo, I get a ton of errors and it can't find the AlertDialog widget any more. First, when my page (simple login page with one Tapestry Form component containing name, pwd, and a button) loads, I get these errors: DEBUG: DEPRECATED: dojo.lang replaced by dojo.lang.common – will be removed in version: 0.5 DEBUG: DEPRECATED: dojo.style replaced by dojo.html.style – will be removed in version: 0.5 DEBUG: DEPRECATED: addParseTreeHandler . ParseTreeHandlers are now reserved for components. Any unfiltered DojoML tag without a ParseTreeHandler is assumed to be a widget – will be removed in version: 0.5 DEBUG: DEPRECATED: dojo.graphics.color.Color is now dojo.gfx.color.Color. 0.5 DEBUG: DEPRECATED: dojo.html replaced by dojo.html.* – will be removed in version: 0.5 DEBUG: DEPRECATED: addParseTreeHandler . ParseTreeHandlers are now reserved for components. Any unfiltered DojoML tag without a ParseTreeHandler is assumed to be a widget – will be removed in version: 0.5 Which appear to come just from loading the core dojo.js file and initializing logging. Then, when I try to submit the form with missing required fields, I get these errors instead of the validation dialog: DEBUG: DEPRECATED: dojo.widget.Manager.getImplementationName Could not locate widget implementation for "alertdialog" in "dojo.widget,dojo.widget.validate" registered to namespace "dojo". Developers must specify correct namespaces for all non-Dojo widgets – will be removed in version: 0.5 ERROR: 10:05:18 AM: Error validating Error : Unable to load https://localhost:443/hedgemon/assets/static/dojo/src/widget/templates/HtmlDialog.html status:404 I think the 404 is spurious - that template does not exist, but I think dojo went looking because it couldn't find the specified widget. Am I missing something? It also fails against the latest dojo 0.3.1 with different errors: FATAL: Could not load 'dojo.html.style'; last tried '_ package _.js' FATAL: Could not load 'tapestry.namespace'; last tried '_ package _.js' FATAL: Could not load 'tapestry.form'; last tried '_ package _.js' I can be reached at greg . woolsey at gmail if you want. I'll probably be asking more questions and suggesting more things, as we are committed at this point to moving forward with 4.1.1.
        Hide
        Jesse Kuhnert added a comment -

        Ouch....Sounds like I've f-ed something up in the release. Definitely wasn't happening this way for me running the apps locally. I'll figure out what the problem is in a couple hours and fix it. Sorry

        Show
        Jesse Kuhnert added a comment - Ouch....Sounds like I've f-ed something up in the release. Definitely wasn't happening this way for me running the apps locally. I'll figure out what the problem is in a couple hours and fix it. Sorry
        Hide
        Greg Woolsey added a comment -

        My bad - I think I had an IE browser cache issue. Cleaned out the world and started over, now it loads clean. Alert button now can be tabbed to, but it still needs the initial focus with

        this.okButton.domNode.focus()

        once I tab to it, I can't tab to other fields. But if I shift-tab first, then the other fields get focus first, and I can use them.

        Also, I can click on the body of the alert dialog to remove focus from the button, and then shift-tab to masked fields. Our QA has me well trained to think like both a dumb user and a hacker

        and tab trapping isn't working - I can still tab to masked fields and manipulate them. I'll try overriding the widget and playing with the code too. Hadn't thought about trying that - loading my own code to override it.

        Show
        Greg Woolsey added a comment - My bad - I think I had an IE browser cache issue. Cleaned out the world and started over, now it loads clean. Alert button now can be tabbed to, but it still needs the initial focus with this.okButton.domNode.focus() once I tab to it, I can't tab to other fields. But if I shift-tab first, then the other fields get focus first, and I can use them. Also, I can click on the body of the alert dialog to remove focus from the button, and then shift-tab to masked fields. Our QA has me well trained to think like both a dumb user and a hacker and tab trapping isn't working - I can still tab to masked fields and manipulate them. I'll try overriding the widget and playing with the code too. Hadn't thought about trying that - loading my own code to override it.
        Hide
        Jesse Kuhnert added a comment -

        Hmmm......I can't re-produce these errors locally. How are you running it against dojo 0.3.1? Have you overridden the default dojo build provided somehow?

        The best example I have of what I know is definitely working are the TimeTracker and Workbench demo applications (http://svn.apache.org/viewvc/tapestry/tapestry4/trunk/tapestry-examples/)

        I definitely want to figure out what is happening either way though, so as to avoid having this happen to more people. (Sorry you ended up being the guinea pig)

        Show
        Jesse Kuhnert added a comment - Hmmm......I can't re-produce these errors locally. How are you running it against dojo 0.3.1? Have you overridden the default dojo build provided somehow? The best example I have of what I know is definitely working are the TimeTracker and Workbench demo applications ( http://svn.apache.org/viewvc/tapestry/tapestry4/trunk/tapestry-examples/ ) I definitely want to figure out what is happening either way though, so as to avoid having this happen to more people. (Sorry you ended up being the guinea pig)
        Hide
        Jesse Kuhnert added a comment -

        Ahh I see now...

        Yeah, I may need to remove that particular button altogether. It's not actually a native browser button so it doesn't even have a "focus" function to call. I don't have time to do a thorough job of implementing this change right this second but will re-open this ticket as a marker so I know to come back and do it..

        P.S. If you do decide to create your own Dialog please feel free to send in a patch (if that's feasible, you may want to do more things specific to your app which is fine..) .

        Show
        Jesse Kuhnert added a comment - Ahh I see now... Yeah, I may need to remove that particular button altogether. It's not actually a native browser button so it doesn't even have a "focus" function to call. I don't have time to do a thorough job of implementing this change right this second but will re-open this ticket as a marker so I know to come back and do it.. P.S. If you do decide to create your own Dialog please feel free to send in a patch (if that's feasible, you may want to do more things specific to your app which is fine..) .
        Jesse Kuhnert made changes -
        Resolution Fixed [ 1 ]
        Status Resolved [ 5 ] Reopened [ 4 ]
        Hide
        Greg Woolsey added a comment -

        This version of an AlertDialog works for me, trapping keystrokes properly. Stupid typos took me a while to track down. I suggest using the body of this example as a new version of the Tapestry widget. For that matter, the keystroke event handlers could be moved to the base Dojo Dialog widget to fix it's general tab navigation problem.

        I had to override the whole summarizeErrors() method to implement a different dialog widget. Would be easier if the widget name string was a field in tapestry.form.validation instead.

        dojo.provide("fireapps.widget.AlertDialog");

        dojo.require("dojo.widget.*");
        dojo.require("dojo.widget.Dialog");
        dojo.require("dojo.widget.Button");
        dojo.require("dojo.event.common");
        dojo.require("dojo.html.common");

        dojo.widget.defineWidget(
        "fireapps.widget.AlertDialog",
        dojo.widget.Dialog,
        {
        bgColor: "white",
        bgOpacity: 0.5,
        okButton:null,
        messageNode:null,
        message:"",

        postCreate: function(args, frag, parentComp)

        { dojo.widget.Dialog.prototype.postCreate.call(this, args, frag, parentComp); var content=document.createElement("div"); this.containerNode.appendChild(content); dojo.html.addClass(this.containerNode, "alertDialog"); this.messageNode=document.createElement("div"); this.messageNode.innerHTML=this.message; content.appendChild(this.messageNode); var buttNode=document.createElement("button"); buttNode.innerHTML = "OK"; content.appendChild(buttNode); this.okButton=buttNode; this.tabStart=this.okButton; this.tabEnd=this.okButton; this.show(); dojo.event.connect(this.okButton, "onclick", this, "hideDialog"); this.okButton.focus(); dojo.event.connect(this.wrapper, 'onkeyup', this, 'dialogKeys'); dojo.event.connect(document.body, 'onkeyup', this, 'bodyKeys'); }

        ,

        dialogKeys:function(e) {
        if (e.keyCode == e.KEY_ESCAPE)

        { this.hideDialog(e); }
        // allow default behavior, but don't let the event keep bubbling/propagating
        if (e.stopPropagation) e.stopPropagation();
        else e.cancelBubble = true;
        },

        bodyKeys:function(e) {
        if (e.keyCode == e.KEY_ESCAPE) { this.hideDialog(e); }

        else if ( ! dojo.dom.isDescendantOf(e.target, this.wrapper, true) )

        { dojo.event.browser.stopEvent(e); this.tabStart.focus(); }

        },

        setMessage:function(str)

        { this.messageNode.innerHTML=str; }

        ,

        hideDialog:function(e)

        { dojo.event.disconnect(this.wrapper, 'onkeyup', this, 'dialogKeys'); dojo.event.disconnect(document.body, 'onkeyup', this, 'bodyKeys'); this.hideModalDialog(); dojo.dom.removeNode(this.okButton); tapestry.widget.AlertDialog.prototype.destroy.call(this); dojo.dom.removeNode(this.bg); }

        },
        "html"
        );

        Show
        Greg Woolsey added a comment - This version of an AlertDialog works for me, trapping keystrokes properly. Stupid typos took me a while to track down. I suggest using the body of this example as a new version of the Tapestry widget. For that matter, the keystroke event handlers could be moved to the base Dojo Dialog widget to fix it's general tab navigation problem. I had to override the whole summarizeErrors() method to implement a different dialog widget. Would be easier if the widget name string was a field in tapestry.form.validation instead. dojo.provide("fireapps.widget.AlertDialog"); dojo.require("dojo.widget.*"); dojo.require("dojo.widget.Dialog"); dojo.require("dojo.widget.Button"); dojo.require("dojo.event.common"); dojo.require("dojo.html.common"); dojo.widget.defineWidget( "fireapps.widget.AlertDialog", dojo.widget.Dialog, { bgColor: "white", bgOpacity: 0.5, okButton:null, messageNode:null, message:"", postCreate: function(args, frag, parentComp) { dojo.widget.Dialog.prototype.postCreate.call(this, args, frag, parentComp); var content=document.createElement("div"); this.containerNode.appendChild(content); dojo.html.addClass(this.containerNode, "alertDialog"); this.messageNode=document.createElement("div"); this.messageNode.innerHTML=this.message; content.appendChild(this.messageNode); var buttNode=document.createElement("button"); buttNode.innerHTML = "OK"; content.appendChild(buttNode); this.okButton=buttNode; this.tabStart=this.okButton; this.tabEnd=this.okButton; this.show(); dojo.event.connect(this.okButton, "onclick", this, "hideDialog"); this.okButton.focus(); dojo.event.connect(this.wrapper, 'onkeyup', this, 'dialogKeys'); dojo.event.connect(document.body, 'onkeyup', this, 'bodyKeys'); } , dialogKeys:function(e) { if (e.keyCode == e.KEY_ESCAPE) { this.hideDialog(e); } // allow default behavior, but don't let the event keep bubbling/propagating if (e.stopPropagation) e.stopPropagation(); else e.cancelBubble = true; }, bodyKeys:function(e) { if (e.keyCode == e.KEY_ESCAPE) { this.hideDialog(e); } else if ( ! dojo.dom.isDescendantOf(e.target, this.wrapper, true) ) { dojo.event.browser.stopEvent(e); this.tabStart.focus(); } }, setMessage:function(str) { this.messageNode.innerHTML=str; } , hideDialog:function(e) { dojo.event.disconnect(this.wrapper, 'onkeyup', this, 'dialogKeys'); dojo.event.disconnect(document.body, 'onkeyup', this, 'bodyKeys'); this.hideModalDialog(); dojo.dom.removeNode(this.okButton); tapestry.widget.AlertDialog.prototype.destroy.call(this); dojo.dom.removeNode(this.bg); } }, "html" );
        Hide
        Greg Woolsey added a comment -

        Also, we should use a class name on the button for CSS formatting.

        Show
        Greg Woolsey added a comment - Also, we should use a class name on the button for CSS formatting.
        Hide
        Jesse Kuhnert added a comment -

        I'm applying your changes right now

        Show
        Jesse Kuhnert added a comment - I'm applying your changes right now
        Hide
        Jesse Kuhnert added a comment -

        Thanks, applied (with small tweaks). Will be deploying in the next few minutes.

        Show
        Jesse Kuhnert added a comment - Thanks, applied (with small tweaks). Will be deploying in the next few minutes.
        Jesse Kuhnert made changes -
        Resolution Fixed [ 1 ]
        Status Reopened [ 4 ] Resolved [ 5 ]
        Mark Thomas made changes -
        Workflow jira [ 12385813 ] Default workflow, editable Closed status [ 12567895 ]
        Mark Thomas made changes -
        Workflow Default workflow, editable Closed status [ 12567895 ] jira [ 12591022 ]
        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Resolved Resolved
        37m 34s 1 Jesse Kuhnert 29/Sep/06 06:14
        Resolved Resolved Reopened Reopened
        12h 30m 1 Jesse Kuhnert 29/Sep/06 18:45
        Reopened Reopened Resolved Resolved
        10h 16m 1 Jesse Kuhnert 30/Sep/06 05:01

          People

          • Assignee:
            Jesse Kuhnert
            Reporter:
            Greg Woolsey
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development