September 2005 - Development - lists.drupal.org

[drupal-devel] [feature] admin/access hover help - little patch, big help
by jjeff 15 Sep '05

15 Sep '05

Issue status update for http://drupal.org/node/28301 Post a follow up: http://drupal.org/project/comments/add/28301 Project: Drupal Version: cvs Component: user.module Category: feature requests Priority: normal Assigned to: jjeff Reported by: jjeff Updated by: jjeff Status: patch (ready to be committed) Attachment: http://drupal.org/files/issues/admin.access.hover.help.patch (799 bytes) Here's a patch that adds a 'title' tag to all of the checkboxes in the admin/access grid. When you roll over the checkboxes in most browsers a tool-tip box will pop up with information about what each checkbox is. Examples: "anonymous user : edit own blog" "authenticated user : post comments" "admin : administer filters" On a site with many roles and many modules, it becomes very easy to scroll away from the table headers, so this is essential for figuring out what you're clicking. It doesn't solve all of the admin/access page usability problems, but it does make the page a lot more usable. Little patch. Big help. -Jeff jjeff

5 8

[drupal-devel] [task] Improvments to Filter UI
by m3avrck 15 Sep '05

15 Sep '05

Issue status update for http://drupal.org/node/27364 Post a follow up: http://drupal.org/project/comments/add/27364 Project: Drupal Version: cvs Component: filter.module Category: tasks Priority: normal Assigned to: Bèr Kessels Reported by: Bèr Kessels Updated by: m3avrck Status: patch (ready to be committed) Attachment: http://drupal.org/files/issues/filter.module_12.patch (20.31 KB) Updated patch with Dries fixes in. m3avrck Previous comments: ------------------------------------------------------------------------ Sun, 24 Jul 2005 09:30:54 +0000 : Bèr Kessels This is a mockup for the Filter UI. Currently it is soo complex, that I dare to call it broken. Please comment on this, for I will not ake any patches, when the general idea is disliked. The idea is t split filters and input formats better. Filters are filters, defined in modules. Input formats are bundles of filters. This is how we have it ATM, but the interface fails to communicate that. I tried to develop a consistent (with the rest of Drupal) interfce that makes it clearer what is what. The menu is as follows: * administer ** .... ** settings *** input formats *** filters ** ... ------------------------------------------------------------------------ Sun, 24 Jul 2005 09:37:10 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/input-formats_list1.png (16.34 KB) * administer ** .... ** settings *** input formats >> see attachement 1 *** filters ** ------------------------------------------------------------------------ Sun, 24 Jul 2005 09:39:02 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/input-formats_add.png (23.42 KB) * administer ** .... ** settings *** input formats >> see attachement 2, tab 2 *** filters ** Note: The default checkbox lmay seem a bit odd. But checking it will remove the "default status" from the current "default" one and make this one "default". The help text, and a drupal_set_message should explain this. ------------------------------------------------------------------------ Sun, 24 Jul 2005 09:40:08 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/input-formats_edit_format.png (22.17 KB) * administer ** .... ** settings *** input formats >> clicking a 'configure' link in the table of attachement 1 *** filters ** ------------------------------------------------------------------------ Sun, 24 Jul 2005 09:40:55 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/input-filters_list.png (36.24 KB) * administer ** .... ** settings *** input formats *** filters >> see attachement 4 ** ------------------------------------------------------------------------ Sun, 24 Jul 2005 09:41:40 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/input-formats_edit.png (22.63 KB) * administer ** .... ** settings *** input formats *** filters >> clicking a 'configure' link in the table of attachement 4 ** ------------------------------------------------------------------------ Mon, 25 Jul 2005 14:46:27 +0000 : Bèr Kessels IMO this makes the interface easier. But other disagree. And because it also removes one feature: (being able to use E.g. HTML in different input formats, with different settings) I'll simply wont fix this :( ------------------------------------------------------------------------ Thu, 28 Jul 2005 13:36:12 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy.patch (10.28 KB) I decided to go for it anyway. Be it in a slightly different way then my initial mockups. The difference is that I left the filters where they are, hidden beneath the formats. So, here is the patch that makes the filter UI more consistent with screens like blocs, menus, vocabularies et al also nice to note is that: 93 lines are added, 104 are removed. So netto we have less code :) (cvs diff filter.module | grep ^+ | wc -l) ------------------------------------------------------------------------ Thu, 28 Jul 2005 13:37:02 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/input-formats_list_html.png (44.64 KB) Here is how the listing now looks. ------------------------------------------------------------------------ Thu, 28 Jul 2005 13:37:57 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/input-formats_edit_format_html.png (113.71 KB) And the "configure" screen. The 'add' screen looks exactly the same, only with the default values pre-filled. ------------------------------------------------------------------------ Thu, 28 Jul 2005 13:39:19 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/input-formats_edit_format_too_long.png (121.82 KB) Oh, And here is a really nice example of why the current screen is no good. And here I "only" have four roles. I run a site with 12 roles, imagine this screen then! ------------------------------------------------------------------------ Thu, 28 Jul 2005 13:48:35 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy_HEAD.patch (10.24 KB) And here is a pathc for HEAD (I forgot that I developed this on a 4.6.2 codebase. Sorry) ------------------------------------------------------------------------ Thu, 28 Jul 2005 13:55:25 +0000 : stefan nagtegaal I like this very, very much! This is - indeed - much better than the current UI. This always fits, and makes the life of people who use a fixed width theme much nicer.. Ber, FYI (you probably know this too), 'admin/access' does also break fixed width themes due to the fact that it expands the width of the table when more roles are being added.. Good catch! I like it much more this way.. +1 from me... ------------------------------------------------------------------------ Fri, 29 Jul 2005 07:27:34 +0000 : Dries The code style needs work (spaces, tabs, 'as' -> 'AS'), the code comments need work, and debug statements should be removed. Screenshots look good to me. db_query('UPDATE {filter_formats} SET cache = %d, name = \'%s\', roles = \'%s\' WHERE format = %d', (int)$cache, $name, $roles, $format); can be written better/shorter quote-wise. ------------------------------------------------------------------------ Fri, 29 Jul 2005 09:04:16 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy_HEAD_0.patch (10.37 KB) Dries, thanks a lot for the review. OI fixed your comments. Or so I hope. code-style.pl was of no use (can that *please* be removed from core?) because it chokes on the help texts. It also pointed out a lot of old formatting isues, whic, when fixed wuold flood this patch with unrelated fixes. I tried to narow my style-searching to the areas I touched. And I made the comments more verbous. The query is fixed. it saves no characters, but looks better now. Thanks for the tip. The print_r..... blush. thats what I get for not running my default grep -r print_r on my code after finishing up... I could not find a lot of spaces and tabs. So it lmight be that i missed one. I triple checked it though. ------------------------------------------------------------------------ Fri, 29 Jul 2005 09:32:21 +0000 : Steven Moving the role settings inward does fix the wide-table problem, but it makes the filter configuration much more opaque. We could perhaps fix this by showing the roles as a comma-separate list in the input formats table. Such a list can wrap and will not stretch the table. Other than that: * Having the instructions for the Roles and Filters form_group() at the bottom is a bit weird. The distance is too big. * The table that was used for toggling filters on/off was much more compact before, and IMO a lot clearer. Why change to a flat list with the description staggered below each item? ------------------------------------------------------------------------ Fri, 29 Jul 2005 10:31:30 +0000 : Bèr Kessels consistancy, consistancy and more consistancy. There really is nothing worse that having a different 'concept' of UI for every thing in Drupal. We are going in the right direction, with the blocks being editable objects, menus acting like that, taxonomy, users, etc.. I wanted to make this act similar. I am not at all happy with the fact tah the list is still a form. But we need this, for setting the default. A big -1 for a comma-separate list. That is extremely hackish, even worse useabilitywise and just silly * we should avoid typing when not needed. * we should avoid errors on input, when they can be avoided (a typo is made very easy, I know all about it) Form set errors won't fix that usability -wise. I agree that you sort-of loose the overview. But IMO that is a minor loss comared to what we gain, usability-wise. The wtoo-wide was onlywhat triggered me. But do not think that I am noly making this patch to fix that issue. This patch is there to improve usability, through consistancy. o make drupal behave the same in most places in admin. If you know one screen, you know most of them. We really should move away from custom-hacks for every page, because that is useability rule #1: consistancy. Stadardisation, even if sometimes a custom hack might make things easier in that single case, it will reduce your overall usability so much that nearly ever it is worth that hack. That select-boxes instead of the table issue: consistancy. As well that it completely broke fluid CSS layouts, my solution is more consistant, and uses forms the way they are meant. If we cannot use $descritption in checkboxes they should be removed. but IMO it feels much more consistant. Tables are for tabular data, and this is simply a list of items, with additional data. And i do not really get what you mean with "too far down". Do you mean that you would like to see the order of the groups different? ------------------------------------------------------------------------ Fri, 29 Jul 2005 13:31:24 +0000 : Dries I haven't tested Ber's patch yet but looking at the screenshots, it like what I see. I remember being confused by the current filter UI, and often, I still am. ------------------------------------------------------------------------ Fri, 29 Jul 2005 19:53:26 +0000 : moshe weitzman I dislike the recent movement away from overview pages. I think we can still provide and overview while allowing editing to happen in a dedicated form. An example in the wrong direction is 'content types' admin. It is impossible to get an overview, and the listing page is worthless. I agree with Steven on this one. It is true that consistency is desirable. But consistent crap is not. ------------------------------------------------------------------------ Fri, 29 Jul 2005 20:02:39 +0000 : killes(a)www.drop.org I agree with Moshe. Getting a quick overview about what is set how is essential for an admin. ------------------------------------------------------------------------ Sat, 30 Jul 2005 00:27:15 +0000 : Robrecht Jacques Overall I like the screenshots. Did not test the patch. Bèr, what I think Steven (#15) meant by "showing the roles as a comma-separate list in the input formats table" is that as an overview page it would be better if you added a list (not an input box!) to screenshot "input-formats_list1.png" (comment #1). Add a column that reads "Roles" (or "Enabled for") and then for each row a comma seperated list of the roles that can use this input format (or maybe even better a HTML list). You would still need to click "configure" to _change_ the roles. The "Default" text (not a radio button) you already show in the "Options" column is something similar: it immediately shows that this input format is the default without you having to go to the "configure" page. Why not do the same for "roles"? If this is what Steven meant, then I tend to agree with him. And to help moshe and killes: maybe you could even add a column that _lists_ the used input filters. Again, only a list, not a way to change it. That's what the "operations" column is for. A table like that will "fluid" better than what we have now, still it is a _complete_ overview of the settings. I could have misunderstand someone... I have the tendency to do that... ------------------------------------------------------------------------ Mon, 22 Aug 2005 08:21:32 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/filter_interface_improvements.patch (14.52 KB) by popular demand: a comma separated list. ------------------------------------------------------------------------ Mon, 22 Aug 2005 08:24:31 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/format_now_contains_roles.png (22.09 KB) and a screenshot ------------------------------------------------------------------------ Mon, 22 Aug 2005 18:39:44 +0000 : Dries Please check your use of print theme('page', $output). Normally, you just return $output. ------------------------------------------------------------------------ Mon, 29 Aug 2005 18:54:28 +0000 : Bèr Kessels All instances of theme('page',...) in filter.module are now changed to return ... ; furthermore, the patch is the same. ------------------------------------------------------------------------ Mon, 29 Aug 2005 20:04:02 +0000 : Uwe Hermann Bèr, I think you forgot to attach your updated patch. ------------------------------------------------------------------------ Mon, 29 Aug 2005 20:27:39 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/filter_interface_improvements_2.patch (13.52 KB) my email client has this nice warning system for attachements. Maybe drupal should search for the word attachement too, and when no att. found give an error ;). Or maybe i should just learn to pay attention. Guess that is easier. Anyway, here it is. ------------------------------------------------------------------------ Thu, 08 Sep 2005 20:22:54 +0000 : Dries I wanted to apply this patch but: 1. I can't change the default input format. 'Save'-button doesn't work. 2. I got confused by the fact I can't change the roles of the default filter. I think the form group description should explain this. 3. form_group(t(filter)) should be form_group(t('filter'), ...). ------------------------------------------------------------------------ Thu, 08 Sep 2005 20:51:24 +0000 : m3avrck Found a few more issues: If I go into 'Configure' for 'PHP Code' and click the 'Configure' tab at the top and then click 'list filters' link in the text, I get this warning with PHP5 warning: Missing argument 1 for filter_admin_format() in \drupal\modules\filter.module on line 378. Also, it is sort of confusing as to what the 'List' tab implies. For example, if I click on 'input formats' in the admin menu, I get a list of the current filters. When I click 'configure' for one of those, I see the options for that filter. However, it says 'list' in one of the tab, which doesn't really mean much. I thought that was the list of filters, not the options for that filter. That should be cleaned up. Also, there is no way to get back to the full list of filters unless you click on 'input formats' in the admin menu. A setting/link to fix this would be great. Also, where is the link to add filters??? I think the options/layout/tabs should mimic of that of admin > users. So on admin > input formats, it has the tabs 'list', 'add format' ... very clear and consistent. On a configure page for a filter, it should have 'list', 'view', 'configure', 'rearrange' ... this would make it easier to navigate and get back to the original list of filters. Make sense? I think this and Dries' comments put this patch over the top :) ------------------------------------------------------------------------ Fri, 09 Sep 2005 01:48:55 +0000 : tag In my initial battle to figure out this module's (quite nice) functionality, what tripped me up the most was actually the nomenclature. The way I keep it straight in my head now is to translate "input format" to "filter set". If you view those (the current) screens with this in mind, I think it's a lot easier to understand what's what. It sort of describes this in the help text, but the terms "filter" and "input format" don't really convey the relationship between the two -- there's no way to intuit that an "input format" is a collection of "filters". Not to my mind at least... and well, we know nobody reads instructions... Are there any technicalities that make renaming "input format" to "filter set" not accurate? Or does someone have a better term/terms to better show the relationship of these elements? (I'm aware of filters in servlet architectures but I don't think there's much of a collision with that usage). Anyhow, I guess this doesn't quite address the UI controls and/or layout, but would still help a lot. ------------------------------------------------------------------------ Fri, 09 Sep 2005 06:35:34 +0000 : Bèr Kessels I prefer filter set. In fact: I like it a lot. Anyone else ideas on this? ------------------------------------------------------------------------ Fri, 09 Sep 2005 10:18:18 +0000 : Dries I too had problems with the terminology; it took me months to get used to 'input formats' versus 'filters'. Using 'filter sets' might improve the situation -- especially from an administrator's point of view when you have to wrap your head around the UI/difference. I'm not native English, but 'filter set' sounds more explanatory, yet slightly more geeky so I'm not sure if it flies for users who don't need to understand the underlying concepts (eg. under the node and comment submission forms). ------------------------------------------------------------------------ Fri, 09 Sep 2005 11:00:05 +0000 : Bèr Kessels a heads up: I ma redoing the patch. But decided no to change the name yet. That is a too big change. IT should be in a separate issue, IMO. ------------------------------------------------------------------------ Fri, 09 Sep 2005 15:27:43 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy_2.patch (13.57 KB) * Bugs as reported by dries fixed. * The [add] tab not appearing is due to your menu caching, refresh that please. * Fixed an additional bug: Some agents do not send TRUE for disabled checkboxen. Also in current filter admin. and people: we have a problem. Deleting a filter trows errors, due to the revisions changes. that happens in filter module now, but also after this patch. I have too little knowledge of the filter system to fix that, and IMO that is a separate issue. It should be fixed right after this patch is committed. ------------------------------------------------------------------------ Fri, 09 Sep 2005 15:57:18 +0000 : Bèr Kessels FYI: the delete bug is waiting here: http://drupal.org/node/30781 ------------------------------------------------------------------------ Fri, 09 Sep 2005 18:18:17 +0000 : m3avrck Get this warning in PHP5: Warning: Missing argument 1 for filter_admin_format() in \modules\filter.module on line 379 I have traced this error back to line 240. Why are there no callback arguments like above on line 235? Looks like a source of a problem/bug here so needs to be addressed, not sure exactly how to fix it myself (otherwise I would). Should be easy it seems. Also, for a quick and easy usability improvement, on line 239, change t('list') to t('view') ... makes the menus less confusing. After those fixes, looks like this patch is ready to be committed :) ------------------------------------------------------------------------ Fri, 09 Sep 2005 18:23:27 +0000 : m3avrck Also, this dialog is a bit confusing: "Choose which roles may use this filter format You are editing the default format. For the default format, all roles must be enabled. Therefore you cannot change them. " Maybe make it so that first line doesn't appear there on that page? ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:35:50 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy_2_0.patch (13.59 KB) changed that line of help. Can this please still be taken in consideration before the freeze? ------------------------------------------------------------------------ Tue, 13 Sep 2005 18:29:28 +0000 : m3avrck Can't apply patch, getting error: Patch malformed at line 287 ------------------------------------------------------------------------ Tue, 13 Sep 2005 19:10:07 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/filter_interface_improvements_2_0.patch (14.19 KB) strange. the dowloaded one breaks too, yet the one i uploaded stll works. anyway, rerolled ------------------------------------------------------------------------ Tue, 13 Sep 2005 19:25:25 +0000 : Dries IMO, the form function needs more work. You need to group the form construction and the form saving. Right now, when you don't provide a title, you are given an error and an emptied form. This may be a left-over from the original code though but it would be cool if you could tidy this up. The form_group() description still need a trailing point. There is a broken link in the following help text on the 'add input format'-page: /If you notice some filters are causing conflicts in the output, you can [rearrange] them./. ------------------------------------------------------------------------ Tue, 13 Sep 2005 19:30:12 +0000 : Bèr Kessels dont have time anymore. sorry. ------------------------------------------------------------------------ Tue, 13 Sep 2005 20:50:34 +0000 : m3avrck new patch soon! ------------------------------------------------------------------------ Tue, 13 Sep 2005 22:10:25 +0000 : m3avrck Attachment: http://drupal.org/files/issues/filter.module_10.patch (20.27 KB) Ok here's a new patch. Lot's of fixes in this one (all of the ones from above except the one noted below), including support for the new revisions patch. Additionally, 'input formats' has been renamed to 'input filters'. After debate on IRC, and the above comments in this post, it seems that 'input formats' is somewhat misleading and doesn't make sense. 'input filters' clears this up and makes sense to both native and non-native English speakers (as tested in IRC anywho ;)). This also makes more sense since Drupal refers to everything as 'filters' and not 'formats'. However, there is one issue still left unresolved: If you create a new input filter, and don't specify a title, page reloads clearing all filled in fields and prompts the error message. Additionally, a blank filter is created. Wanted to get this patch posted, I'll look at it later today but if someone can get this fixed easily, please do, thanks! ------------------------------------------------------------------------ Tue, 13 Sep 2005 22:38:05 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/filter_interface_improvements_3.patch (21.49 KB) this patch should fix the issue m3 rasies abuot amepty formats being created. Yes, i use a simple drupal_goto, in case of a for error. And no, it is not very easy to get the old posted data back. ------------------------------------------------------------------------ Tue, 13 Sep 2005 23:44:26 +0000 : moshe weitzman the most visible place for this 'input format' term is on the node form. thats where ordinary users see this stuff. in that context, i don't think 'input filters is an improvement over input formats. the question we are asking our users is 'what is the format of your post'?, not what filters do you want applied ... just my .02. sorry i wasn't in IRC when this came up. this need not hold up the whole patch. ------------------------------------------------------------------------ Wed, 14 Sep 2005 06:48:03 +0000 : Bèr Kessels Moshe. I agreed with you, and was against the name change. In this patch at least. However, it slipped in. Stetting code to be committed for it has gone trough so many review cycles that this patch has MUCH better code than the rest of the filter module, which REALLY needs a big overhaul :) ------------------------------------------------------------------------ Wed, 14 Sep 2005 13:37:27 +0000 : Dries Are you saving the data before validating it? If we want to convert this form to the new form API, we have to fix this cleanly. Sorry. I agree with Moshe that 'input filters' is suboptimal. People want to select how their text is going to be /processed/. The word/filter/ sounds rather technical to me. Also, the difference between a single filter and a set of filters becomes rather blurry to the point it could be very confusing. ------------------------------------------------------------------------ Wed, 14 Sep 2005 13:41:42 +0000 : m3avrck Well I was the one that changed 'input filters' to 'input formats' as that seemed to be the best thing we could come up. I guess it got confusing because people wanted this to change but maybe not in this patch. So I will revert that change in a forthecoming patch. Additinally, this patch is *not* ready. I was going through the form logic last night and it is a bit off, hence why errors are displayed along side a an empty form. I'm going to work as hard as I can and as fast to clean this patch up like I mentioned I would yesterday. I'll have some ready soon enough that will be commit worthy. ------------------------------------------------------------------------ Wed, 14 Sep 2005 14:22:41 +0000 : Bèr Kessels according to me its a go or leave. if m3 or anyone else wants to take over fine. I am out. no time. ------------------------------------------------------------------------ Wed, 14 Sep 2005 21:30:45 +0000 : m3avrck Attachment: http://drupal.org/files/issues/filter.module_11.patch (28.03 KB) Ok new patch! Everything is fixed. Code has been reworked and lots of bugs fixed. Code should also be simpler to read (or at least follow in terms of logic now). Ready to go! ------------------------------------------------------------------------ Wed, 14 Sep 2005 22:02:26 +0000 : Souvent22 +1. I honestly have not played around with the filter formats ever. I looked at them, adn they were a mess. Did the patch, and everything was clear as day. Anything that makes drupal easier to use...AND makes it easier to understand and make sense should def. be included.

1 0

[drupal-devel] [feature] Prevent accidentally navigating away from pages where content has changed
by nedjo 15 Sep '05

15 Sep '05

Issue status update for http://drupal.org/node/30220 Post a follow up: http://drupal.org/project/comments/add/30220 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: m3avrck Updated by: nedjo Status: patch (code needs review) Attachment: http://drupal.org/files/issues/formcheck_5.js (2.12 KB) The revised js file. nedjo Previous comments: ------------------------------------------------------------------------ Thu, 01 Sep 2005 22:47:42 +0000 : m3avrck How many times have you been posting a comment or working on a new post when you click on a link in another application and you navigate away from the content you are editing, losing all of your changes? Well I say we introduce a script, much similiar to that of Blogger, that uses the Javascript window.onbeforeunload handler to prevent this from happening. We can set this handler to call a function that compares the contents of all forms when the page loads and then quickly compares that to the current contents just before the user is about to leave the page. If they have changed, we should prompt the user so they don't lose their work. This would save many headaches and degrade nicely for users without Javascript. Additionally, this script should work with the newly introduced JS-based upload feature to prevent navigating away as a file is being uploaded as well. If I have sometime I'll start work on a patch tomorrow, just wanted to get the idea into the queue right now :) ------------------------------------------------------------------------ Sat, 10 Sep 2005 23:48:33 +0000 : StuartDH Sounds like a great feature to me. We regularly get authors and editors telling us that they've lost the last hour of work by accidentally navigating away, and I've even just had one of them email me about it a few minutes ago, so it'd be great if you could put something together for this. Cheers ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:33:58 +0000 : MichelleC Sounds great to me, too. I often have several tabs open doing stuff and it's easy to lose my place and leave a page with unfinished edits. Would this alert you when you're going to close an unsaved tab/window as well? Michelle ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:34:19 +0000 : MrMattles Great for us multitaskers that rush and click the wrong thing ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:36:52 +0000 : chx Very strong -1 as this is IE only. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:38:40 +0000 : praseodymium Konqueror already does this, +1 for the idea in general. ------------------------------------------------------------------------ Mon, 12 Sep 2005 15:07:21 +0000 : webchick Yeah, I think chx is correc that onbeforeunload is an extension developed by MS [1] (it's not in the ECMAScript specification [2]. However, I found out that this is implemented in Mozilla since 1.7 [3] (which corresponds to Firefox 1.0, I think?) and I've tested Blogger.com's version of this functionality in: Firefox 1.0.6 (working) Firefox Deer Park Alpha 1 (working) Opera 8.0.2 (working) Safari 1.3 (does NOT work) Safari 2.0.1 (working) So it seems to be a 'de facto' standard if nothing else. [1] http://msdn.microsoft.com/workshop/author/dhtml/reference/events/onbeforeun… [2] http://www.ecma-international.org/publications/standards/Ecma-262.htm [3] http://www.mozilla.org/status/2004-03-01.html ------------------------------------------------------------------------ Mon, 12 Sep 2005 15:09:15 +0000 : webchick Also, I should clarify... that "does NOT work" in Safari doesn't display any errors or anything, it just doesn't save the form results as expected (so pre-Tiger Safari users are going to be used to this). So I don't see any harm in including this functionality since it seems to degrade gracefully in non-supporting browsers. ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:10:18 +0000 : m3avrck Attachment: http://drupal.org/files/issues/node.module_11.patch (951 bytes) And a patch is ready! Code was based on many example, including this [4] and this [5] along with adding my own thoughts and what not ;) [4] http://www.codestore.org/store.nsf/cmnts/451FA051398A9AF486256EE0000FB7D1?O… [5] http://www.blogger.com/app/scripts/formcheck.js ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:10:45 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck.js (1.54 KB) And here is the JS to throw in misc/. ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:22:56 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_0.js (1.61 KB) Better JS file attached without tabs. Also, tested and working great in FF and IE6. Doesn't work in Opera 8, however no errors are present, I believe this is just because the event handler is not supported. If there is a work around for Opera, please let me know! ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:24:18 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_1.js (1.61 KB) One more try with those tabs. ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:32:48 +0000 : moshe weitzman hmmm. i usually find these prompts annoying. any chance we can attach the behavior only to the node and comment forms? those are the "high risk" areas. what do others think? ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:42:39 +0000 : m3avrck Yes, the JS is only loaded on the node edit/create pages, won't affect any admin/login/etc form. Also, the prompt is unobstrusive and if you goto save a node you don't get a warning that the contents have changed because you are explicity saving the node (hence no reason!). ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:53:01 +0000 : Dries I want some JS-folks to review the code. ------------------------------------------------------------------------ Mon, 12 Sep 2005 22:09:10 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_15.patch (1.94 KB) After talking with Drumm on IRC, this patch makes the integration with Drupal much simpler and it's off by default. Only turned on for the node edit/change page right now but any module/form can easily add this by passing a 'TRUE' parameter. ------------------------------------------------------------------------ Mon, 12 Sep 2005 23:38:35 +0000 : nedjo +1 on idea, js looks generally good, a few suggestions: 1. Instead of writing onsubmit via PHP, use a js addLoadEvent call. 2. Shouldn't the return false in isElementChanged be at the end (outside the switch block)? 3. It's be nice to find a way to make messages like 'You have unsaved changes.' translatable. Pass in global js variables via a t('') call? 4. onbeforeunload event should probably be in a if isJsEnabled test, and should parallel drupal.js's event adding (see addLoadEvent). 5. Comments should be in standard format and in present tense, e.g., /** * Checks to see if a form has been changed after the page loads */ 6. e_ should be just e to match other js files, e.g., autocomplete. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:54:35 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_2.js (1.58 KB) Ok here is an updated JS file. A few notes, I couldn't get the addSubmitEvent() in drupa.js to reliably work so I had to set the isSubmit() true in the PHP creation of the form. If you look at Blogger.com, they do this as well... so either we both missed an obvious way to do this, or that is the most practical. Hopefully some wise JS gurus will chime in with an answer. Same goes for onbeforeunload event, which is completely different then addEvent defined in drupal.js. As for the t('') I agree this would be useful but I'm not sure of the best way to do this. One way would be to put in form() a t('') passed in, and then write this to a var in formcheck.php which returns a JS file type. Thoughts? ------------------------------------------------------------------------ Tue, 13 Sep 2005 14:06:44 +0000 : Junyor FWIW, Opera doesn't support the onbeforeunload event. However, this issue doesn't affect Opera anyway: form contents are retained in history as long as the page isn't closed. IOW, this feature doesn't work in Opera, but it isn't needed either. ------------------------------------------------------------------------ Tue, 13 Sep 2005 14:15:56 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_3.js (1.64 KB) Updated JS styling and JS-killswitch after talking with Unconed on IRC. ------------------------------------------------------------------------ Tue, 13 Sep 2005 14:39:15 +0000 : m3avrck Just to build on Junyor's comment [6] this functionality is built into Opera 8 and it *doesn't* produce any errors. [6] http://drupal.org/#comment-44066 ------------------------------------------------------------------------ Tue, 13 Sep 2005 15:04:37 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_16.patch (2.1 KB) Updated patch to fix possible problem of overwriting onsubmit event handler thanks to Thox on IRC. ------------------------------------------------------------------------ Tue, 13 Sep 2005 15:13:59 +0000 : webchick I tested this and can confirm that it works in *both* versions of Safari that I have access to: 2.0.1 (OS X Tiger) and 1.3 (OS X Panther). So looks like this code goes one better over the Blogger.com stuff, because their stuff doesn't workk in 1.3. Nice job! ;) ------------------------------------------------------------------------ Tue, 13 Sep 2005 15:25:21 +0000 : m3avrck Well I'm gonna set this to commit then. Tested and working on IE, FF, Safari. Doesn't work or break Opera or Konqueror but not needed in these cases. Thox and Unconded have offered thoughts and I've modified code as needed. Doesn't seem like there is anything else left except a commit :) ------------------------------------------------------------------------ Tue, 13 Sep 2005 17:13:48 +0000 : Bèr Kessels In general I dislike the feature. Konqueror somehow dies this, and that is the way it /should/ be. Browsers should take care of this, not the web app. But, since it is only konq. this patch has a good enough additional value :) I would like to see this patch tested with, tinyMCE and HTMLAREA, at least. For I am quite sure this will break these modules so bad that they are near unusable. Which brings me to the next point: using the textarea hook a simple module could take care of this. I would very very much prefer this living in a contrib, or even a core module. Just not "enforced" on me. And the last point: if this is for core, please use that textarea hook too. This is where that hook is for: extending the textareas. ------------------------------------------------------------------------ Tue, 13 Sep 2005 17:32:25 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_18.patch (2.23 KB) Updated patch after talking with Thox and Uncloned to attach this event only to forms with a specific class (hence avoiding the problem of being on an edit page with other admin/search forms). ------------------------------------------------------------------------ Tue, 13 Sep 2005 17:32:55 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_4.js (1.67 KB) Updated JS file. ------------------------------------------------------------------------ Tue, 13 Sep 2005 17:45:21 +0000 : m3avrck Berkes, this patch *does not* break TinyMCE, just tried. However, TinyMCE does not take into account this script. This should be an easy patch to TinyMCE. Also, this doesn't affect just textareas, it affects all fields within a given form (e.g., text fields, check boxes, etc...). Sure the blunt of editing/creating a node is in the textarea, but there are still all of those other changes that can be made (new title, revision status, front page promotion, etc...) so this needs to account for them all which it does. This script is unobtrusive and degrades perfectly well. It is tested and working in FF, IE, Safari and doens't cause problems in Opera or Konqueror which don't need this feature anyways (since they already have it). The usability boost of this script is too enormous *not* to include in core. As a contributed module, it is really too flaky, and along those lines, autocomplete.js, inlineuploads.js and related should be in their own contributed modules :) I do see your points and I hope this clears it up. It doesn't cause any problems and only attaches to specified forms, and is off by default. Any module can easily make use of this script when they use: form(..., TRUE). This is the same behavior as the other JS files included with HEAD as well. Once this is in core I'll work on a patch to TinyMCE to get that up to speed ;-) (and as such, TinyMCE still works fine, no probs/errors/etc, and good reason too if you look at how the code *actually* works ;-)). ------------------------------------------------------------------------ Tue, 13 Sep 2005 17:54:37 +0000 : Bèr Kessels I see. the "why" for not using form textarea is perfectly valid. And I tried to explain that, eventough I feel this belongs in the Agent, it is a usability enhancement for all the others using sillier browsers ;). And you are right about that part of contributions, exp since you cannot use hook_texarea, having this in a contrib cannot be achieved. I hereby retract my hesitations. (though I have no time to reapply the latest patch and test it on konq. The last JS updates broke drupal on konq, which Steven fixed, right away though!) ------------------------------------------------------------------------ Thu, 15 Sep 2005 16:31:11 +0000 : m3avrck Ready to go! ------------------------------------------------------------------------ Thu, 15 Sep 2005 20:50:06 +0000 : nedjo Attachment: http://drupal.org/files/issues/formcheck.patch (1.37 KB) Nice work m3avrck. I've made some tweaks to minimize code additions on the PHP side and improve the js. Mainly, it's now enough to set the 'formcheck' class attribute for a form. function form() will add the js file, and the needed onsubmit event will be added dynamically to forms. This is consistent with other core js files, which don't hard-code event calls but add them dynamically, based on js support. The useful functionality is unchanged.

1 0

[drupal-devel] [feature] Prevent accidentally navigating away from pages where content has changed
by nedjo 15 Sep '05

15 Sep '05

Issue status update for http://drupal.org/node/30220 Post a follow up: http://drupal.org/project/comments/add/30220 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: m3avrck Updated by: nedjo -Status: patch (ready to be committed) +Status: patch (code needs review) Attachment: http://drupal.org/files/issues/formcheck.patch (1.37 KB) Nice work m3avrck. I've made some tweaks to minimize code additions on the PHP side and improve the js. Mainly, it's now enough to set the 'formcheck' class attribute for a form. function form() will add the js file, and the needed onsubmit event will be added dynamically to forms. This is consistent with other core js files, which don't hard-code event calls but add them dynamically, based on js support. The useful functionality is unchanged. nedjo Previous comments: ------------------------------------------------------------------------ Thu, 01 Sep 2005 22:47:42 +0000 : m3avrck How many times have you been posting a comment or working on a new post when you click on a link in another application and you navigate away from the content you are editing, losing all of your changes? Well I say we introduce a script, much similiar to that of Blogger, that uses the Javascript window.onbeforeunload handler to prevent this from happening. We can set this handler to call a function that compares the contents of all forms when the page loads and then quickly compares that to the current contents just before the user is about to leave the page. If they have changed, we should prompt the user so they don't lose their work. This would save many headaches and degrade nicely for users without Javascript. Additionally, this script should work with the newly introduced JS-based upload feature to prevent navigating away as a file is being uploaded as well. If I have sometime I'll start work on a patch tomorrow, just wanted to get the idea into the queue right now :) ------------------------------------------------------------------------ Sat, 10 Sep 2005 23:48:33 +0000 : StuartDH Sounds like a great feature to me. We regularly get authors and editors telling us that they've lost the last hour of work by accidentally navigating away, and I've even just had one of them email me about it a few minutes ago, so it'd be great if you could put something together for this. Cheers ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:33:58 +0000 : MichelleC Sounds great to me, too. I often have several tabs open doing stuff and it's easy to lose my place and leave a page with unfinished edits. Would this alert you when you're going to close an unsaved tab/window as well? Michelle ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:34:19 +0000 : MrMattles Great for us multitaskers that rush and click the wrong thing ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:36:52 +0000 : chx Very strong -1 as this is IE only. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:38:40 +0000 : praseodymium Konqueror already does this, +1 for the idea in general. ------------------------------------------------------------------------ Mon, 12 Sep 2005 15:07:21 +0000 : webchick Yeah, I think chx is correc that onbeforeunload is an extension developed by MS [1] (it's not in the ECMAScript specification [2]. However, I found out that this is implemented in Mozilla since 1.7 [3] (which corresponds to Firefox 1.0, I think?) and I've tested Blogger.com's version of this functionality in: Firefox 1.0.6 (working) Firefox Deer Park Alpha 1 (working) Opera 8.0.2 (working) Safari 1.3 (does NOT work) Safari 2.0.1 (working) So it seems to be a 'de facto' standard if nothing else. [1] http://msdn.microsoft.com/workshop/author/dhtml/reference/events/onbeforeun… [2] http://www.ecma-international.org/publications/standards/Ecma-262.htm [3] http://www.mozilla.org/status/2004-03-01.html ------------------------------------------------------------------------ Mon, 12 Sep 2005 15:09:15 +0000 : webchick Also, I should clarify... that "does NOT work" in Safari doesn't display any errors or anything, it just doesn't save the form results as expected (so pre-Tiger Safari users are going to be used to this). So I don't see any harm in including this functionality since it seems to degrade gracefully in non-supporting browsers. ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:10:18 +0000 : m3avrck Attachment: http://drupal.org/files/issues/node.module_11.patch (951 bytes) And a patch is ready! Code was based on many example, including this [4] and this [5] along with adding my own thoughts and what not ;) [4] http://www.codestore.org/store.nsf/cmnts/451FA051398A9AF486256EE0000FB7D1?O… [5] http://www.blogger.com/app/scripts/formcheck.js ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:10:45 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck.js (1.54 KB) And here is the JS to throw in misc/. ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:22:56 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_0.js (1.61 KB) Better JS file attached without tabs. Also, tested and working great in FF and IE6. Doesn't work in Opera 8, however no errors are present, I believe this is just because the event handler is not supported. If there is a work around for Opera, please let me know! ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:24:18 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_1.js (1.61 KB) One more try with those tabs. ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:32:48 +0000 : moshe weitzman hmmm. i usually find these prompts annoying. any chance we can attach the behavior only to the node and comment forms? those are the "high risk" areas. what do others think? ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:42:39 +0000 : m3avrck Yes, the JS is only loaded on the node edit/create pages, won't affect any admin/login/etc form. Also, the prompt is unobstrusive and if you goto save a node you don't get a warning that the contents have changed because you are explicity saving the node (hence no reason!). ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:53:01 +0000 : Dries I want some JS-folks to review the code. ------------------------------------------------------------------------ Mon, 12 Sep 2005 22:09:10 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_15.patch (1.94 KB) After talking with Drumm on IRC, this patch makes the integration with Drupal much simpler and it's off by default. Only turned on for the node edit/change page right now but any module/form can easily add this by passing a 'TRUE' parameter. ------------------------------------------------------------------------ Mon, 12 Sep 2005 23:38:35 +0000 : nedjo +1 on idea, js looks generally good, a few suggestions: 1. Instead of writing onsubmit via PHP, use a js addLoadEvent call. 2. Shouldn't the return false in isElementChanged be at the end (outside the switch block)? 3. It's be nice to find a way to make messages like 'You have unsaved changes.' translatable. Pass in global js variables via a t('') call? 4. onbeforeunload event should probably be in a if isJsEnabled test, and should parallel drupal.js's event adding (see addLoadEvent). 5. Comments should be in standard format and in present tense, e.g., /** * Checks to see if a form has been changed after the page loads */ 6. e_ should be just e to match other js files, e.g., autocomplete. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:54:35 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_2.js (1.58 KB) Ok here is an updated JS file. A few notes, I couldn't get the addSubmitEvent() in drupa.js to reliably work so I had to set the isSubmit() true in the PHP creation of the form. If you look at Blogger.com, they do this as well... so either we both missed an obvious way to do this, or that is the most practical. Hopefully some wise JS gurus will chime in with an answer. Same goes for onbeforeunload event, which is completely different then addEvent defined in drupal.js. As for the t('') I agree this would be useful but I'm not sure of the best way to do this. One way would be to put in form() a t('') passed in, and then write this to a var in formcheck.php which returns a JS file type. Thoughts? ------------------------------------------------------------------------ Tue, 13 Sep 2005 14:06:44 +0000 : Junyor FWIW, Opera doesn't support the onbeforeunload event. However, this issue doesn't affect Opera anyway: form contents are retained in history as long as the page isn't closed. IOW, this feature doesn't work in Opera, but it isn't needed either. ------------------------------------------------------------------------ Tue, 13 Sep 2005 14:15:56 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_3.js (1.64 KB) Updated JS styling and JS-killswitch after talking with Unconed on IRC. ------------------------------------------------------------------------ Tue, 13 Sep 2005 14:39:15 +0000 : m3avrck Just to build on Junyor's comment [6] this functionality is built into Opera 8 and it *doesn't* produce any errors. [6] http://drupal.org/#comment-44066 ------------------------------------------------------------------------ Tue, 13 Sep 2005 15:04:37 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_16.patch (2.1 KB) Updated patch to fix possible problem of overwriting onsubmit event handler thanks to Thox on IRC. ------------------------------------------------------------------------ Tue, 13 Sep 2005 15:13:59 +0000 : webchick I tested this and can confirm that it works in *both* versions of Safari that I have access to: 2.0.1 (OS X Tiger) and 1.3 (OS X Panther). So looks like this code goes one better over the Blogger.com stuff, because their stuff doesn't workk in 1.3. Nice job! ;) ------------------------------------------------------------------------ Tue, 13 Sep 2005 15:25:21 +0000 : m3avrck Well I'm gonna set this to commit then. Tested and working on IE, FF, Safari. Doesn't work or break Opera or Konqueror but not needed in these cases. Thox and Unconded have offered thoughts and I've modified code as needed. Doesn't seem like there is anything else left except a commit :) ------------------------------------------------------------------------ Tue, 13 Sep 2005 17:13:48 +0000 : Bèr Kessels In general I dislike the feature. Konqueror somehow dies this, and that is the way it /should/ be. Browsers should take care of this, not the web app. But, since it is only konq. this patch has a good enough additional value :) I would like to see this patch tested with, tinyMCE and HTMLAREA, at least. For I am quite sure this will break these modules so bad that they are near unusable. Which brings me to the next point: using the textarea hook a simple module could take care of this. I would very very much prefer this living in a contrib, or even a core module. Just not "enforced" on me. And the last point: if this is for core, please use that textarea hook too. This is where that hook is for: extending the textareas. ------------------------------------------------------------------------ Tue, 13 Sep 2005 17:32:25 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_18.patch (2.23 KB) Updated patch after talking with Thox and Uncloned to attach this event only to forms with a specific class (hence avoiding the problem of being on an edit page with other admin/search forms). ------------------------------------------------------------------------ Tue, 13 Sep 2005 17:32:55 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_4.js (1.67 KB) Updated JS file. ------------------------------------------------------------------------ Tue, 13 Sep 2005 17:45:21 +0000 : m3avrck Berkes, this patch *does not* break TinyMCE, just tried. However, TinyMCE does not take into account this script. This should be an easy patch to TinyMCE. Also, this doesn't affect just textareas, it affects all fields within a given form (e.g., text fields, check boxes, etc...). Sure the blunt of editing/creating a node is in the textarea, but there are still all of those other changes that can be made (new title, revision status, front page promotion, etc...) so this needs to account for them all which it does. This script is unobtrusive and degrades perfectly well. It is tested and working in FF, IE, Safari and doens't cause problems in Opera or Konqueror which don't need this feature anyways (since they already have it). The usability boost of this script is too enormous *not* to include in core. As a contributed module, it is really too flaky, and along those lines, autocomplete.js, inlineuploads.js and related should be in their own contributed modules :) I do see your points and I hope this clears it up. It doesn't cause any problems and only attaches to specified forms, and is off by default. Any module can easily make use of this script when they use: form(..., TRUE). This is the same behavior as the other JS files included with HEAD as well. Once this is in core I'll work on a patch to TinyMCE to get that up to speed ;-) (and as such, TinyMCE still works fine, no probs/errors/etc, and good reason too if you look at how the code *actually* works ;-)). ------------------------------------------------------------------------ Tue, 13 Sep 2005 17:54:37 +0000 : Bèr Kessels I see. the "why" for not using form textarea is perfectly valid. And I tried to explain that, eventough I feel this belongs in the Agent, it is a usability enhancement for all the others using sillier browsers ;). And you are right about that part of contributions, exp since you cannot use hook_texarea, having this in a contrib cannot be achieved. I hereby retract my hesitations. (though I have no time to reapply the latest patch and test it on konq. The last JS updates broke drupal on konq, which Steven fixed, right away though!) ------------------------------------------------------------------------ Thu, 15 Sep 2005 16:31:11 +0000 : m3avrck Ready to go!

1 0

Returned Mail: [drupal-devel] [feature] Prevent accidentally navigating away from pages where content has changed
by drupal-devel＠drupal.org 15 Sep '05

15 Sep '05

The following message from m3avrck <drupal-devel(a)drupal.org> was not authorized for entry in the 01Lists.com forum. Posts can only be made by registered users of the forum. From: m3avrck <drupal-devel(a)drupal.org> To: drupal-devel(a)drupal.org Subject: [drupal-devel] [feature] Prevent accidentally navigating away from pages where content has changed **************************************** Issue status update for http://drupal.org/node/30220 Post a follow up: http://drupal.org/project/comments/add/30220 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: m3avrck Updated by: m3avrck Status: patch (ready to be committed) Attachment: http://drupal.org/files/issues/formcheck_4.js (1.67 KB) Updated JS file. m3avrck Previous comments: ------------------------------------------------------------------------ Thu, 01 Sep 2005 22:47:42 +0000 : m3avrck How many times have you been posting a comment or working on a new post when you click on a link in another application and you navigate away from the content you are editing, losing all of your changes? Well I say we introduce a script, much similiar to that of Blogger, that uses the Javascript window.onbeforeunload handler to prevent this from happening. We can set this handler to call a function that compares the contents of all forms when the page loads and then quickly compares that to the current contents just before the user is about to leave the page. If they have changed, we should prompt the user so they don't lose their work. This would save many headaches and degrade nicely for users without Javascript. Additionally, this script should work with the newly introduced JS-based upload feature to prevent navigating away as a file is being uploaded as well. If I have sometime I'll start work on a patch tomorrow, just wanted to get the idea into the queue right now :) ------------------------------------------------------------------------ Sat, 10 Sep 2005 23:48:33 +0000 : StuartDH Sounds like a great feature to me. We regularly get authors and editors telling us that they've lost the last hour of work by accidentally navigating away, and I've even just had one of them email me about it a few minutes ago, so it'd be great if you could put something together for this. Cheers ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:33:58 +0000 : MichelleC Sounds great to me, too. I often have several tabs open doing stuff and it's easy to lose my place and leave a page with unfinished edits. Would this alert you when you're going to close an unsaved tab/window as well? Michelle ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:34:19 +0000 : MrMattles Great for us multitaskers that rush and click the wrong thing ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:36:52 +0000 : chx Very strong -1 as this is IE only. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:38:40 +0000 : praseodymium Konqueror already does this, +1 for the idea in general. ------------------------------------------------------------------------ Mon, 12 Sep 2005 15:07:21 +0000 : webchick Yeah, I think chx is correc that onbeforeunload is an extension developed by MS [1] (it's not in the ECMAScript specification [2]. However, I found out that this is implemented in Mozilla since 1.7 [3] (which corresponds to Firefox 1.0, I think?) and I've tested Blogger.com's version of this functionality in: Firefox 1.0.6 (working) Firefox Deer Park Alpha 1 (working) Opera 8.0.2 (working) Safari 1.3 (does NOT work) Safari 2.0.1 (working) So it seems to be a 'de facto' standard if nothing else. [1] http://msdn.microsoft.com/workshop/author/dhtml/reference/events/onbeforeun… [2] http://www.ecma-international.org/publications/standards/Ecma-262.htm [3] http://www.mozilla.org/status/2004-03-01.html ------------------------------------------------------------------------ Mon, 12 Sep 2005 15:09:15 +0000 : webchick Also, I should clarify... that "does NOT work" in Safari doesn't display any errors or anything, it just doesn't save the form results as expected (so pre-Tiger Safari users are going to be used to this). So I don't see any harm in including this functionality since it seems to degrade gracefully in non-supporting browsers. ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:10:18 +0000 : m3avrck Attachment: http://drupal.org/files/issues/node.module_11.patch (951 bytes) And a patch is ready! Code was based on many example, including this [4] and this [5] along with adding my own thoughts and what not ;) [4] http://www.codestore.org/store.nsf/cmnts/451FA051398A9AF486256EE0000FB7D1?O… [5] http://www.blogger.com/app/scripts/formcheck.js ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:10:45 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck.js (1.54 KB) And here is the JS to throw in misc/. ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:22:56 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_0.js (1.61 KB) Better JS file attached without tabs. Also, tested and working great in FF and IE6. Doesn't work in Opera 8, however no errors are present, I believe this is just because the event handler is not supported. If there is a work around for Opera, please let me know! ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:24:18 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_1.js (1.61 KB) One more try with those tabs. ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:32:48 +0000 : moshe weitzman hmmm. i usually find these prompts annoying. any chance we can attach the behavior only to the node and comment forms? those are the "high risk" areas. what do others think? ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:42:39 +0000 : m3avrck Yes, the JS is only loaded on the node edit/create pages, won't affect any admin/login/etc form. Also, the prompt is unobstrusive and if you goto save a node you don't get a warning that the contents have changed because you are explicity saving the node (hence no reason!). ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:53:01 +0000 : Dries I want some JS-folks to review the code. ------------------------------------------------------------------------ Mon, 12 Sep 2005 22:09:10 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_15.patch (1.94 KB) After talking with Drumm on IRC, this patch makes the integration with Drupal much simpler and it's off by default. Only turned on for the node edit/change page right now but any module/form can easily add this by passing a 'TRUE' parameter. ------------------------------------------------------------------------ Mon, 12 Sep 2005 23:38:35 +0000 : nedjo +1 on idea, js looks generally good, a few suggestions: 1. Instead of writing onsubmit via PHP, use a js addLoadEvent call. 2. Shouldn't the return false in isElementChanged be at the end (outside the switch block)? 3. It's be nice to find a way to make messages like 'You have unsaved changes.' translatable. Pass in global js variables via a t('') call? 4. onbeforeunload event should probably be in a if isJsEnabled test, and should parallel drupal.js's event adding (see addLoadEvent). 5. Comments should be in standard format and in present tense, e.g., /** * Checks to see if a form has been changed after the page loads */ 6. e_ should be just e to match other js files, e.g., autocomplete. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:54:35 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_2.js (1.58 KB) Ok here is an updated JS file. A few notes, I couldn't get the addSubmitEvent() in drupa.js to reliably work so I had to set the isSubmit() true in the PHP creation of the form. If you look at Blogger.com, they do this as well... so either we both missed an obvious way to do this, or that is the most practical. Hopefully some wise JS gurus will chime in with an answer. Same goes for onbeforeunload event, which is completely different then addEvent defined in drupal.js. As for the t('') I agree this would be useful but I'm not sure of the best way to do this. One way would be to put in form() a t('') passed in, and then write this to a var in formcheck.php which returns a JS file type. Thoughts? ------------------------------------------------------------------------ Tue, 13 Sep 2005 14:06:44 +0000 : Junyor FWIW, Opera doesn't support the onbeforeunload event. However, this issue doesn't affect Opera anyway: form contents are retained in history as long as the page isn't closed. IOW, this feature doesn't work in Opera, but it isn't needed either. ------------------------------------------------------------------------ Tue, 13 Sep 2005 14:15:56 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_3.js (1.64 KB) Updated JS styling and JS-killswitch after talking with Unconed on IRC. ------------------------------------------------------------------------ Tue, 13 Sep 2005 14:39:15 +0000 : m3avrck Just to build on Junyor's comment [6] this functionality is built into Opera 8 and it *doesn't* produce any errors. [6] http://drupal.org/#comment-44066 ------------------------------------------------------------------------ Tue, 13 Sep 2005 15:04:37 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_16.patch (2.1 KB) Updated patch to fix possible problem of overwriting onsubmit event handler thanks to Thox on IRC. ------------------------------------------------------------------------ Tue, 13 Sep 2005 15:13:59 +0000 : webchick I tested this and can confirm that it works in *both* versions of Safari that I have access to: 2.0.1 (OS X Tiger) and 1.3 (OS X Panther). So looks like this code goes one better over the Blogger.com stuff, because their stuff doesn't workk in 1.3. Nice job! ;) ------------------------------------------------------------------------ Tue, 13 Sep 2005 15:25:21 +0000 : m3avrck Well I'm gonna set this to commit then. Tested and working on IE, FF, Safari. Doesn't work or break Opera or Konqueror but not needed in these cases. Thox and Unconded have offered thoughts and I've modified code as needed. Doesn't seem like there is anything else left except a commit :) ------------------------------------------------------------------------ Tue, 13 Sep 2005 17:13:48 +0000 : Bèr Kessels In general I dislike the feature. Konqueror somehow dies this, and that is the way it /should/ be. Browsers should take care of this, not the web app. But, since it is only konq. this patch has a good enough additional value :) I would like to see this patch tested with, tinyMCE and HTMLAREA, at least. For I am quite sure this will break these modules so bad that they are near unusable. Which brings me to the next point: using the textarea hook a simple module could take care of this. I would very very much prefer this living in a contrib, or even a core module. Just not "enforced" on me. And the last point: if this is for core, please use that textarea hook too. This is where that hook is for: extending the textareas. ------------------------------------------------------------------------ Tue, 13 Sep 2005 17:32:25 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_18.patch (2.23 KB) Updated patch after talking with Thox and Uncloned to attach this event only to forms with a specific class (hence avoiding the problem of being on an edit page with other admin/search forms).

1 0

Returned Mail: [drupal-devel] [task] Collapsible settings for all settings pages?
by drupal-devel＠drupal.org 15 Sep '05

15 Sep '05

The following message from m3avrck <drupal-devel(a)drupal.org> was not authorized for entry in the 01Lists.com forum. Posts can only be made by registered users of the forum. From: m3avrck <drupal-devel(a)drupal.org> To: drupal-devel(a)drupal.org Subject: [drupal-devel] [task] Collapsible settings for all settings pages? **************************************** Issue status update for http://drupal.org/node/30038 Post a follow up: http://drupal.org/project/comments/add/30038 Project: Drupal Version: cvs Component: base system Category: tasks Priority: normal Assigned to: Anonymous Reported by: m3avrck Updated by: m3avrck -Status: active +Status: patch (ready to be committed) Attachment: http://drupal.org/files/issues/drupal_17.patch (9.17 KB) Rerolled against HEAD. Since it seems the FORM api is going to go in *after* the freeze, this should go in now, so before any of the forms are updated to the new API (so this change is accounted for later instead of having to do another patch :)) m3avrck Previous comments: ------------------------------------------------------------------------ Tue, 30 Aug 2005 17:37:22 +0000 : m3avrck Is there a reason collapsible fieldsets is used *only* on the main settings page? What about the users setting page? That is really long and would benefit. And the uploads setting page, as you add more roles, that page can get really long too. I think we should add this feature to all standard Drupal settings pages that are long like the settings page... it really helps and would make things look more consistent, instead of "why the heck is this on one page and not them all?". Need to get this into the next release as well to be consistent with the approach of this new feature. ------------------------------------------------------------------------ Tue, 30 Aug 2005 20:38:54 +0000 : m3avrck Attachment: http://drupal.org/files/issues/modules_0.patch (9.17 KB) I've gone through all of the settings pages for the included default Drupal modules and have applied the collapsible form fix. This really does make a *huge* difference... it makes the Settings interface more consistent and greatly improves readability. Let's get this one in! :) ------------------------------------------------------------------------ Tue, 30 Aug 2005 21:32:11 +0000 : Souvent22 Installed, and used the patched. Worked great. Much needed. :). +1 ------------------------------------------------------------------------ Tue, 30 Aug 2005 21:54:42 +0000 : chx -1 and +1 at the same time. +1 but only after the forms API is in -- it'd be better if we won't break the fastly evolving patch. ------------------------------------------------------------------------ Tue, 30 Aug 2005 22:28:25 +0000 : m3avrck Beat me to the post :) I just was reviewing the Forms API patch and realized this one is no longer applicable if that goes through. chx, can you include the fixes in this patch with the forms api? it just makes a few extra menus 'collapsible' which with the new API, requires even less code change, haha!

1 0

Returned Mail: [drupal-devel] [task] Extend db_query()
by drupal-devel＠drupal.org 15 Sep '05

15 Sep '05

The following message from killes <drupal-devel(a)drupal.org> was not authorized for entry in the 01Lists.com forum. Posts can only be made by registered users of the forum. From: killes <drupal-devel(a)drupal.org> To: drupal-devel(a)drupal.org Subject: [drupal-devel] [task] Extend db_query() **************************************** Issue status update for http://drupal.org/node/17656 Post a follow up: http://drupal.org/project/comments/add/17656 Project: Drupal Version: cvs Component: database system Category: tasks Priority: normal Assigned to: killes(a)www.drop.org Reported by: killes(a)www.drop.org Updated by: killes(a)www.drop.org Status: patch (code needs review) Attachment: http://drupal.org/files/issues/db_query_3.patch (2.85 KB) Kjartan prefers this version. I also added some comment. killes(a)www.drop.org Previous comments: ------------------------------------------------------------------------ Mon, 21 Feb 2005 12:48:30 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/db-query.patch (2.16 KB) We should make our database abstraction layer more robust and ensure that module authors can use it without string manipulations inside the query. Several queries use implode() to get their arguments into the query. This is undesirable as we rely on the module author to check the keys and values of such arrays for exploitation attempts. I have created the attached patch which shouldbe able to allow us to not use implode anymore. A minor problem is that all inserted values will be treated as strings. This might be a problem with PostgreSQL at least. However, the same strategy is already used in Drupal core without any complaints I know of. Summary: This patch will alow us to simplify some code in node.module, user.module, taxonomy.module and probably others. ------------------------------------------------------------------------ Mon, 21 Feb 2005 17:03:58 +0000 : killes(a)www.drop.org It's a patch. ------------------------------------------------------------------------ Mon, 21 Feb 2005 17:19:13 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/db-query_0.patch (2.07 KB) Squeezed out two lines of code after consultation with Karoly. Adds only 10 loc (plus some docs). ------------------------------------------------------------------------ Mon, 21 Feb 2005 17:23:08 +0000 : chx Do I need to say +1? ------------------------------------------------------------------------ Thu, 03 Mar 2005 00:15:10 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/db-query_1.patch (2.69 KB) After some discussion with Adrian at Drupal Con we found out that we do not know why node_save currently works with pgsql. It currently assumes that all db columns are strings. It seems to work but we should not rely on it. Here is a patch that checks for the type of field that is inserted. It needs testing. ------------------------------------------------------------------------ Tue, 26 Jul 2005 01:17:04 +0000 : drumm +1 for making this into an API. I've seen too many hacked together query builders in Drupal and Contrib. I have not tested. ------------------------------------------------------------------------ Wed, 27 Jul 2005 11:30:29 +0000 : Bèr Kessels untested. a big +1 for the feature ------------------------------------------------------------------------ Wed, 27 Jul 2005 11:41:52 +0000 : killes(a)www.drop.org the patch still applies. the new patch here updates node_save to use it. Untested. ------------------------------------------------------------------------ Wed, 27 Jul 2005 11:42:24 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/node-%a.patch (926 bytes) the patch still applies. the new patch here updates node_save to use it. Untested. ------------------------------------------------------------------------ Wed, 27 Jul 2005 11:45:17 +0000 : Cvbge "After some discussion with Adrian at Drupal Con we found out that we do not know why node_save currently works with pgsql. " Well, it does not work. Real life exampless of not-working include flexinode (my experience) and forum module (as someone reported). Probably also others. The bug occurs when a normal user (but with necessarily rights) adds a node and he has no controls (the 'moderated', 'sticky', 'published' etc). The, in node_load() $node->sticky, $node->moderated (and maybe others) are set to FALSE (or TRUE, but in this case it works). When doing printf("%s", FALSE) the FALSE is change to empty string. The sticky and moderated db fields are numeric and postgresql do not accept '' (empty string) as a value of integer type. The result is for example such error: warning: pg_query(): Query failed: ERROR: invalid input syntax for integer: "" in ..../includes/database.pgsql.inc on line 45. user error: query: INSERT INTO node (title, uid, type, teaser, status, moderate, promote, sticky, body, comment, created, changed, nid) VALUES('xx', '2', 'flexinode-1', '<div class="flexinode-body flexinode-1"><div class="flexinode-image-3"><div class="form-item"> <label>Zdjęcie:</label><br /> <img alt="xx" src="..../pliki/" /><br />Get original file (28KB) [1] </div> </div></div>', '1', '', '1', '', '<div class="flexinode-body flexinode-1"><div class="flexinode-image-3"><div class="form-item"> <label>Zdjęcie:</label><br /> <img alt="xx" src="..../pliki/" /><br />Get original file (28KB) [2] </div> < in ..../includes/database.pgsql.inc on line 62. [1] http://drupal.org/..../pliki//tmp/male.jpg [2] http://drupal.org/..../pliki//tmp/male.jpg ------------------------------------------------------------------------ Wed, 27 Jul 2005 13:03:14 +0000 : Cvbge Attachment: http://drupal.org/files/issues/node.module_5.diff (500 bytes) Here's a quick fix for 4.6 ------------------------------------------------------------------------ Wed, 27 Jul 2005 13:10:54 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/node-a.patch (1.8 KB) the last patch wasn't good. ------------------------------------------------------------------------ Mon, 08 Aug 2005 08:19:14 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/db_api_taxonomy.patch (3.5 KB) Here is an additional patch for taxonomy.module it removes the two custom functions that are used for inserts and updates. Needs testing. @Cvbge: shoduld we have a better test than just is_numeric in _db_argument_type to fix the pgsql problems? We could also try to pass $value by reference and cast it to int. this function is only used for updates/inserts so we can afford an additional check. ------------------------------------------------------------------------ Mon, 08 Aug 2005 08:37:25 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/db_query.patch (2.74 KB) Here's a revised version fo the original patch. _db_argument_type now gets the $value argument by reference and we check for bools too. numerics and bools are cast to int. ------------------------------------------------------------------------ Mon, 08 Aug 2005 10:34:36 +0000 : Bèr Kessels I cannot say anything about performance for I do not know how to benchmark this. But I tried it, And rewrote some queries in a custom module, to use it, and like it a lot. The code becomes cleaner, and better readable. But above all, i see this idea going into a very interesting and good direction: that of 'more' query builders in core'. a +1! ------------------------------------------------------------------------ Mon, 08 Aug 2005 10:51:14 +0000 : Cvbge I'll be talking about postgres db only. The patch won't work as expected in some cases. Let's say we have a text/char/other character column and we want to insert a text that looks like a number. The _db_argument_type() will recognize the value as a numeric/number etc and will use $key = %d format. Even not considering printf() numeric conversion this can lead to incorrect value inserted into db. Examples of such problematic texts are: 01 (leading 0), 2e2 (scientific notation), probably also text with trailing/leading white spaces. When inserting such data into db without quotes the postgres db will think this is a number and will change 010 to 10, 2e2 to 200 . Here is an example: piotr=> CREATE TABLE t(c CHAR(10)); piotr=> INSERT INTO t VALUES (01); piotr=> INSERT INTO t VALUES (2e2); piotr=> INSERT INTO t VALUES (-01); piotr=> INSERT INTO t VALUES (000); piotr=> SELECT * from t; c ------------ 1 200 -1 0 (4 rows) I think it's safe to use '%s' everywhere. I could not find it writtent directly in postgres documentation (although I remember reading it somewhere some time ago). Postgres will convert the string to correct integer/numeric/bool value. ------------------------------------------------------------------------ Sat, 13 Aug 2005 01:14:41 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/db_query_0.patch (2.69 KB) Ok, the patch now does only distinguish between integerss and the rest. This is needed to decide whether we need quotes. ------------------------------------------------------------------------ Sat, 13 Aug 2005 09:40:59 +0000 : Cvbge I'm sorry, but I see no reason for this integer check. Can you explain why there is a need to differentiate between integers and other types? The only problem I see is with converting bool(FALSE). This value is converted to empty string ('') and when trying to insert it into numeric, float, bool, date (probably any column that is not text type) etc you get error. One solution would be to check for bool(FALSE) and convert it to 0. This would work for numeric-like and bool fields. But it would not work for DATA columns (would produce error). Also, entering '0' into text column might not be what the author wanted. But this is more a problem with the coder - if you have integer/date etc field, insert integer/date/etc data type, not bool! Side note: the integer-version, the one without '', will be used only for integers (i.e. ..., -2, -1, 0, 1, 2, ... see http://php.net/manual/en/language.types.integer.php) ------------------------------------------------------------------------ Sat, 13 Aug 2005 09:43:19 +0000 : Cvbge "DATA" should be "DATE" ------------------------------------------------------------------------ Sun, 14 Aug 2005 00:53:34 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/db_query_1.patch (2.71 KB) Ok, another attempt. There is indeed no real reason to use %d for integers, they work fine as strings and are converted by sprintf to strings independend of %d or %s. But it is the Right Thing(tm), so I kept it. Booleans, however, need the %d formatter in order to be converted to 0 and 1. TRUE to 1 always works, but FALSE to 0 only works with %d. All tests on php 4.3.x. ------------------------------------------------------------------------ Thu, 18 Aug 2005 21:46:04 +0000 : Cvbge The part I was objecting previously looks ok now. I haven't tested the code though. I still think it'd be nice to change php NULL to real SQL NULLs, but I don't have how to do it (at least not with current approach). The best would be if there was (s)printf flag that would just ignore it's argument... ------------------------------------------------------------------------ Sat, 27 Aug 2005 19:36:04 +0000 : Thomas Ilsche Attachment: http://drupal.org/files/issues/node_save.patch (907 bytes) +1, tested Runs nicely but maybe NULL and floats might need special treatment. I have attached an INCOMPLETE patch for node_save using the new feature that also fixes the following issue. ------------------------------------------------------------------------ Tue, 30 Aug 2005 18:13:48 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/node_save_0.patch (3.65 KB) Here is an updated patch that takes a lot of the code I added to node_save out again (approx 20 loc). it also fixes a docs glitch in that function. Needs testing and is to be used with the patch from http://drupal.org/node/17656#comment-39222 ------------------------------------------------------------------------ Sun, 04 Sep 2005 18:13:36 +0000 : Cvbge A remark about NULL values: in this patch they are treated as integers and thus allways changed to '0'. In existing code the '' was INSERTed into text-like columns (%s) if didn't check if the variable was set (or did not care). If they now use %a and still do not check for NULLs the '0' will be inserted which migh or might not create problems (it won't matter for if()s, but maybe for other uses?) ------------------------------------------------------------------------ Sun, 04 Sep 2005 19:50:58 +0000 : killes(a)www.drop.org Cvbge: AFAIK we don't use NULL values inside the DB in Drupal core. ------------------------------------------------------------------------ Sun, 04 Sep 2005 20:56:41 +0000 : Cvbge You're probably right, then 1. not checking if a variable is set is a bug (and I've already filled a bug for one of such bugs, can't find it - sticky, moderated etc. fields were not set at all when not selected when submitting a post) 2. There's already a lot of DEFAULT NULL in the database schema... ------------------------------------------------------------------------ Fri, 09 Sep 2005 15:03:09 +0000 : Cvbge Hello, NULLs again. Previously I wrote that NULLs are treated as integers and passed with %d and converted to 0. This is of course not true [were I sick when writting it?]. They are treated as strings and converted to '' (empty string) This will create similar problems as FALSE. Maybe NULL should be treated as %d (and converted to 0) after all? ------------------------------------------------------------------------ Tue, 13 Sep 2005 16:06:30 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/db_query_2.patch (2.72 KB) Ok, I've included is_null() in the condition. ------------------------------------------------------------------------ Tue, 13 Sep 2005 16:31:55 +0000 : Cvbge ok, last thing (really ;)): maybe add some information to docs saying that null is converted to '0' (not to '') ? :)

1 0

Returned Mail: [drupal-devel] [feature] Prevent accidentally navigating away from pages where content has changed
by drupal-devel＠drupal.org 15 Sep '05

15 Sep '05

The following message from m3avrck <drupal-devel(a)drupal.org> was not authorized for entry in the 01Lists.com forum. Posts can only be made by registered users of the forum. From: m3avrck <drupal-devel(a)drupal.org> To: drupal-devel(a)drupal.org Subject: [drupal-devel] [feature] Prevent accidentally navigating away from pages where content has changed **************************************** Issue status update for http://drupal.org/node/30220 Post a follow up: http://drupal.org/project/comments/add/30220 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: m3avrck Updated by: m3avrck Status: patch (ready to be committed) Attachment: http://drupal.org/files/issues/drupal_18.patch (2.23 KB) Updated patch after talking with Thox and Uncloned to attach this event only to forms with a specific class (hence avoiding the problem of being on an edit page with other admin/search forms). m3avrck Previous comments: ------------------------------------------------------------------------ Thu, 01 Sep 2005 22:47:42 +0000 : m3avrck How many times have you been posting a comment or working on a new post when you click on a link in another application and you navigate away from the content you are editing, losing all of your changes? Well I say we introduce a script, much similiar to that of Blogger, that uses the Javascript window.onbeforeunload handler to prevent this from happening. We can set this handler to call a function that compares the contents of all forms when the page loads and then quickly compares that to the current contents just before the user is about to leave the page. If they have changed, we should prompt the user so they don't lose their work. This would save many headaches and degrade nicely for users without Javascript. Additionally, this script should work with the newly introduced JS-based upload feature to prevent navigating away as a file is being uploaded as well. If I have sometime I'll start work on a patch tomorrow, just wanted to get the idea into the queue right now :) ------------------------------------------------------------------------ Sat, 10 Sep 2005 23:48:33 +0000 : StuartDH Sounds like a great feature to me. We regularly get authors and editors telling us that they've lost the last hour of work by accidentally navigating away, and I've even just had one of them email me about it a few minutes ago, so it'd be great if you could put something together for this. Cheers ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:33:58 +0000 : MichelleC Sounds great to me, too. I often have several tabs open doing stuff and it's easy to lose my place and leave a page with unfinished edits. Would this alert you when you're going to close an unsaved tab/window as well? Michelle ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:34:19 +0000 : MrMattles Great for us multitaskers that rush and click the wrong thing ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:36:52 +0000 : chx Very strong -1 as this is IE only. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:38:40 +0000 : praseodymium Konqueror already does this, +1 for the idea in general. ------------------------------------------------------------------------ Mon, 12 Sep 2005 15:07:21 +0000 : webchick Yeah, I think chx is correc that onbeforeunload is an extension developed by MS [1] (it's not in the ECMAScript specification [2]. However, I found out that this is implemented in Mozilla since 1.7 [3] (which corresponds to Firefox 1.0, I think?) and I've tested Blogger.com's version of this functionality in: Firefox 1.0.6 (working) Firefox Deer Park Alpha 1 (working) Opera 8.0.2 (working) Safari 1.3 (does NOT work) Safari 2.0.1 (working) So it seems to be a 'de facto' standard if nothing else. [1] http://msdn.microsoft.com/workshop/author/dhtml/reference/events/onbeforeun… [2] http://www.ecma-international.org/publications/standards/Ecma-262.htm [3] http://www.mozilla.org/status/2004-03-01.html ------------------------------------------------------------------------ Mon, 12 Sep 2005 15:09:15 +0000 : webchick Also, I should clarify... that "does NOT work" in Safari doesn't display any errors or anything, it just doesn't save the form results as expected (so pre-Tiger Safari users are going to be used to this). So I don't see any harm in including this functionality since it seems to degrade gracefully in non-supporting browsers. ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:10:18 +0000 : m3avrck Attachment: http://drupal.org/files/issues/node.module_11.patch (951 bytes) And a patch is ready! Code was based on many example, including this [4] and this [5] along with adding my own thoughts and what not ;) [4] http://www.codestore.org/store.nsf/cmnts/451FA051398A9AF486256EE0000FB7D1?O… [5] http://www.blogger.com/app/scripts/formcheck.js ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:10:45 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck.js (1.54 KB) And here is the JS to throw in misc/. ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:22:56 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_0.js (1.61 KB) Better JS file attached without tabs. Also, tested and working great in FF and IE6. Doesn't work in Opera 8, however no errors are present, I believe this is just because the event handler is not supported. If there is a work around for Opera, please let me know! ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:24:18 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_1.js (1.61 KB) One more try with those tabs. ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:32:48 +0000 : moshe weitzman hmmm. i usually find these prompts annoying. any chance we can attach the behavior only to the node and comment forms? those are the "high risk" areas. what do others think? ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:42:39 +0000 : m3avrck Yes, the JS is only loaded on the node edit/create pages, won't affect any admin/login/etc form. Also, the prompt is unobstrusive and if you goto save a node you don't get a warning that the contents have changed because you are explicity saving the node (hence no reason!). ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:53:01 +0000 : Dries I want some JS-folks to review the code. ------------------------------------------------------------------------ Mon, 12 Sep 2005 22:09:10 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_15.patch (1.94 KB) After talking with Drumm on IRC, this patch makes the integration with Drupal much simpler and it's off by default. Only turned on for the node edit/change page right now but any module/form can easily add this by passing a 'TRUE' parameter. ------------------------------------------------------------------------ Mon, 12 Sep 2005 23:38:35 +0000 : nedjo +1 on idea, js looks generally good, a few suggestions: 1. Instead of writing onsubmit via PHP, use a js addLoadEvent call. 2. Shouldn't the return false in isElementChanged be at the end (outside the switch block)? 3. It's be nice to find a way to make messages like 'You have unsaved changes.' translatable. Pass in global js variables via a t('') call? 4. onbeforeunload event should probably be in a if isJsEnabled test, and should parallel drupal.js's event adding (see addLoadEvent). 5. Comments should be in standard format and in present tense, e.g., /** * Checks to see if a form has been changed after the page loads */ 6. e_ should be just e to match other js files, e.g., autocomplete. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:54:35 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_2.js (1.58 KB) Ok here is an updated JS file. A few notes, I couldn't get the addSubmitEvent() in drupa.js to reliably work so I had to set the isSubmit() true in the PHP creation of the form. If you look at Blogger.com, they do this as well... so either we both missed an obvious way to do this, or that is the most practical. Hopefully some wise JS gurus will chime in with an answer. Same goes for onbeforeunload event, which is completely different then addEvent defined in drupal.js. As for the t('') I agree this would be useful but I'm not sure of the best way to do this. One way would be to put in form() a t('') passed in, and then write this to a var in formcheck.php which returns a JS file type. Thoughts? ------------------------------------------------------------------------ Tue, 13 Sep 2005 14:06:44 +0000 : Junyor FWIW, Opera doesn't support the onbeforeunload event. However, this issue doesn't affect Opera anyway: form contents are retained in history as long as the page isn't closed. IOW, this feature doesn't work in Opera, but it isn't needed either. ------------------------------------------------------------------------ Tue, 13 Sep 2005 14:15:56 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_3.js (1.64 KB) Updated JS styling and JS-killswitch after talking with Unconed on IRC. ------------------------------------------------------------------------ Tue, 13 Sep 2005 14:39:15 +0000 : m3avrck Just to build on Junyor's comment [6] this functionality is built into Opera 8 and it *doesn't* produce any errors. [6] http://drupal.org/#comment-44066 ------------------------------------------------------------------------ Tue, 13 Sep 2005 15:04:37 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_16.patch (2.1 KB) Updated patch to fix possible problem of overwriting onsubmit event handler thanks to Thox on IRC. ------------------------------------------------------------------------ Tue, 13 Sep 2005 15:13:59 +0000 : webchick I tested this and can confirm that it works in *both* versions of Safari that I have access to: 2.0.1 (OS X Tiger) and 1.3 (OS X Panther). So looks like this code goes one better over the Blogger.com stuff, because their stuff doesn't workk in 1.3. Nice job! ;) ------------------------------------------------------------------------ Tue, 13 Sep 2005 15:25:21 +0000 : m3avrck Well I'm gonna set this to commit then. Tested and working on IE, FF, Safari. Doesn't work or break Opera or Konqueror but not needed in these cases. Thox and Unconded have offered thoughts and I've modified code as needed. Doesn't seem like there is anything else left except a commit :) ------------------------------------------------------------------------ Tue, 13 Sep 2005 17:13:48 +0000 : Bèr Kessels In general I dislike the feature. Konqueror somehow dies this, and that is the way it /should/ be. Browsers should take care of this, not the web app. But, since it is only konq. this patch has a good enough additional value :) I would like to see this patch tested with, tinyMCE and HTMLAREA, at least. For I am quite sure this will break these modules so bad that they are near unusable. Which brings me to the next point: using the textarea hook a simple module could take care of this. I would very very much prefer this living in a contrib, or even a core module. Just not "enforced" on me. And the last point: if this is for core, please use that textarea hook too. This is where that hook is for: extending the textareas.

1 0

Returned Mail: [drupal-devel] [task] New drupal forms api.
by drupal-devel＠drupal.org 15 Sep '05

15 Sep '05

The following message from m3avrck <drupal-devel(a)drupal.org> was not authorized for entry in the 01Lists.com forum. Posts can only be made by registered users of the forum. From: m3avrck <drupal-devel(a)drupal.org> To: drupal-devel(a)drupal.org Subject: [drupal-devel] [task] New drupal forms api. **************************************** Issue status update for http://drupal.org/node/29465 Post a follow up: http://drupal.org/project/comments/add/29465 Project: Drupal Version: cvs Component: base system Category: tasks Priority: critical Assigned to: adrian Reported by: adrian Updated by: m3avrck Status: patch (code needs work) I agree with Ber and Souvent. I think we should all focus on our pet patches for the Drupal 4.7 freeze (which I think should be Friday give us that one day ;). After that, we should have a "sub-freeze" where *only* FORM related API patches go in, set this for 3-4 days after the 4.7 freeze. Then, once that is complete, *then* get back into the regular bug/performance/usability freeze over all. m3avrck Previous comments: ------------------------------------------------------------------------ Tue, 23 Aug 2005 11:34:00 +0000 : adrian Attachment: http://drupal.org/files/issues/form.inc (20.53 KB) This is the first check in of the new forms api code. The system has been designed to co-exist with the current forms api, and is contained in a new include file (includes/form.inc). Forms are now defined in their component arrays, similar to how menu items are defined. example : <?php $form['body'] = array(type => 'textarea', default_value => $node->body, cols => 60, rows => 60); ?> Elements can also be nested, and the $edit follows this definition. For instance : <?php $form['author'] = array(type => 'fieldset', title => t('Authoring information'), collapsible => TRUE, collapsed => TRUE, weight => -1); $form['author']['name'] = array(type => 'textfield', title => t('Authored by'), maxlength => 60, autocomplete_path => 'user/autocomplete', default_value => $node->name, weight => -1); ?> All the properties used are defined as constants, and are documented for each of the elements, and individually. ------------------------------------------------------------------------ Tue, 23 Aug 2005 11:46:19 +0000 : adrian A patch for node.module, blog.module and taxonomy.module that changes them to use the new form format. This patch is very far from complete, but I wanted to get the code out so that i'm not working alone anymore. ------------------------------------------------------------------------ Tue, 23 Aug 2005 12:08:01 +0000 : adrian Attachment: http://drupal.org/files/issues/forms.patch (9.98 KB) The actual patch =) I forgot to mention, this adds a new hook .. namely hook_elements, which allows us to define the defaults for the elements (ie : cols and rows for textareas) meaning they don't have to be defined for each of the elements. ------------------------------------------------------------------------ Tue, 23 Aug 2005 12:09:11 +0000 : chx A few notes from my conversation with adrian. valid => array('integer', 'uid') for this to work you need function valid_integer($element) and valid_uid($element). $extra for form_select is legacy and really needed. ------------------------------------------------------------------------ Tue, 23 Aug 2005 12:39:13 +0000 : fago i really like this approach. further i'd like to see the possibility to define an additional class to a form element, which is currently not working. so we 'd have to bring _form_get_class() and drupal_attributes() together. ------------------------------------------------------------------------ Tue, 23 Aug 2005 12:56:31 +0000 : adrian that works already. <?php $form[attributes]['class'] = 'someclass'; ?> Although I am considering just adding a class property ... ie: <?php $form[class][] = 'someclass'; ?> The fact that this is done via arrays, it means that the developer can add classes as he or she sees fit. ------------------------------------------------------------------------ Tue, 23 Aug 2005 13:29:10 +0000 : fago really? i don't think so. e.g. $checkbox = '<input type="checkbox" class="'. _form_get_class('form-checkbox', $element[required], _form_get_error($element[name])) .'" name="'. $element[name] .'" id="'. $element[id].'" value="'. $element[return_value] .'"'. ($element[value] ? ' checked="checked"' : '') . drupal_attributes($element[attributes]) .' />' so we will end up with two class attributes, which won't work and isn't standard compliance. your css property idea would be ideal imho. ------------------------------------------------------------------------ Tue, 23 Aug 2005 13:40:12 +0000 : nevets Minor point on #5 and #6, when accessing an associated array like $form[class][] = 'someclass'; if the key is a string it should be enclosed in quotes, i.e. $form['class'][] = 'someclass'; (This is from the PHP documentation.) ------------------------------------------------------------------------ Tue, 23 Aug 2005 14:35:29 +0000 : moshe weitzman Does this API affect form validation also? Thats the vague impression I had in my head, but I don't see any validation changed in node or taxonomy modules. perhaps that part is coming next. There are reasons to love this patch. But one thing I don't like is the movement toward arrays and away from functions. Modern editors and IDE's offer function tips and function completion. These are huge time and brain savers. They are great for newbies and for experts. It is so helpful to just type 'form_sel', press tab, and have form_select('title', 'name', 'value', 'options') printed for you, with all the arguments. When you define forms in an array instead of functions, as proposed in this patch, you lose a lot of developer productivity and friendliness for newbies. Developers are also more prone to mistakes this way since the editor can't guide them along. This is the sort of advantage that means nothing to the many people who just use a plain editor, and means everything to IDE users. Maybe someone can think of a way to keep the flexibility without losing IDE productivity. ------------------------------------------------------------------------ Wed, 24 Aug 2005 08:45:53 +0000 : adrian The api has a drupal_validate_form() function, which does the following validation : It steps through each of the elements, and executes any of the valid properties. An example would be valid => 'username'. It then calls valid_username($element), which can check for errors. It then calls $form_id_validate() , which can check for errors between form elements. It then (optionally) calls $callback_validate(), which allows you to have unique form id's , similar to how the example does the node form. You could create a function $type_node_form_validate(), to validate only that form, or a theme_$type_node_form() to theme that form differently. An example of where this would be used is for CCK, where it will have a single callback for all nodes created by it. Errors are flagged using form_error($element). It's different from form_set_error, in that it also sets the error property of the element, which I think is more practical than using the globals. Regarding the IDE discussion, I am on the fence about that, but definitely leaning towards preferring the arrays over the function calls. I think that the menu system has proven itself, and that it's better to be consistent. The plan for 4.7, is to leave the current form api in , so that all contrib modules don't need to be ported, but to switch core over to the new form system anyway, so for the time being .. the old functions will still exist. All this would be a non-issue if php supported named parameters, which is essentially what we are reproducing using arrays. ------------------------------------------------------------------------ Wed, 24 Aug 2005 12:36:08 +0000 : Thomas Ilsche I agree with moshe, and I think for day to day use the current forms api does a good job - however on complicated constructions i consider this to be really useful. The problem i see is to keep the whole forms api consistent and easy to learn, any ideas? I'd be against deprecating the current functions. About the keyword definitions. I think it should be consistent with for hook_menu and all its "named parameter" friends, and to at least not confuse it more define the keywords without the leading underscore. ------------------------------------------------------------------------ Wed, 24 Aug 2005 13:20:13 +0000 : adrian I'd prefer to make the menu system properties be consistent with the form system. actually. I know the conventional logic is that all constants being uppercase, and the first versions of the form code did stick to that, but the end result of the lowercase constants was far more readable code (the underscores however are a necessity to allow for nesting.) What i was thinking was, that we could use the conventional form api as constructors for the form array. ie: <?php $group .= form_textfield(t('File system path'), 'file_directory_path', $directory_path, 60, 255, t('desc here'), null, etc) ?> turns into <?php $form['files']['file_directory_path'] = form_textfield(t('File system path'), $directory_path, 60, 255, t('desc here'), null, etc); $form['files']['file_directory_path'][valid] = 'directory'; // any other properties that aren't in constructors. ?> instead of <?php $form['files']['file_create_path'] = array( type => 'textfield', title => t('File system path'), default_value => $directory_path, maxlength => 255, valid => 'directory', description => t('desc here') ); ?> and form_textfield turns into <?php function form_textfield($title, $value, $size, $maxlength, $description = NULL, $attributes = NULL, $required = FALSE) { return array( title => $title, size => $size, maxlength => $maxlength, description = $description, attributes => $attributes, required => $required); } ?> Benefits : 1) easier to port 2) the ide thingy Drawbacks : 1) more than one way to do something. 2) all forms will need to be upgraded, since the core form api will change. ie: breaks all of contrib. 3) Constructors could become unwieldy trying to tend to most of the parameters that can be set (weight, valid, validation_arguments, etc) ------------------------------------------------------------------------ Wed, 24 Aug 2005 13:47:17 +0000 : Dries There should only be one way to build forms. Simplicity and uniformity is king. For now, I leave it up to Adrian to decide what this "one way" is going to look like. (I like his initial approach. The code is shorter which saves time too.) ------------------------------------------------------------------------ Wed, 24 Aug 2005 14:24:48 +0000 : chx I second Dries. The old form API be gone. The IDE is going to be a problem, yes. A possible approach: the default array have all keys possible set to NULL or so. ------------------------------------------------------------------------ Wed, 24 Aug 2005 14:28:50 +0000 : Bèr Kessels I prefer the One Way too. having more ways to do something normally results in two half-witted ways, instead of one way that works As Best As Possible. I like the arrays approach. I love it, in fact. I have a feeling that the more code in drupal adopts the Array Way [tm], the more power AND uniformity we get. Just look at the success of array based menus: powerfull, yet simple to develop with. But, I have a few hesitations: one is the lowercase CONSTANTS. I know, this is good for readability, so I lean towards the side of: then just use lowercase constants. But still, something does not feel right about it. I think this needs some more though, or comments of others. Another thing I dislike is the way we use parameters to construct, IMO, completely different widgets. We should try to not think in terms of HTML, but in terms of usage and display. In HTML a collapsible form is similar -or nearly- to a noncollapsible. Same goes for a multi-select and none-multiselect select. But where I really think we should have different APIs is for autofills. They are IMHO not textfields, but a complete separate widget. Thus they should get a separate API. And last about the IDEs: allthough I do understand the problem, i beleive it is a very bad habit to let your code/application/product be limited, because of the tools you use. If your tools cannot handle libraries/snippets/etc beyond some function calls, IMO you should get anoter IDE :). But surely we should not let us be held back by these limitations in these IDEs. ------------------------------------------------------------------------ Wed, 24 Aug 2005 15:31:14 +0000 : moshe weitzman Attachment: http://drupal.org/files/issues/form_defaults.patch (2.39 KB) Um, snippits and macros are not a substitute for function complete. Snippits and macros are static entities which you manually create in the IDE and are then available as needed. If syntax changes for an given snippit, you have to manually change it. Thats just annoying enough to make you not use these feature at all. By contrast, function autocomplete just works the second you open a file. If you start working on a Contrib module you never seen before, the IDE introspects and is immediately ready for action. So all that nice PHPDoc that Adrian has written would be nicely used. Not so with snippits or macros. If you guys want to see what the fuss about IDE is, download the free trial of Zend Studio (http://www.zend.com/store/products/zend-studio/) or Komodo (http://www.activestate.com/Products/Komodo/ - note the small link for the OSX alpha. I've tried the alpha and it works). IDEs take a little while to get configured and get comfortable ... Create a project and start typing some common functions. See how the params show up and all our PHPDoc is there for your use. You can also quickly navigate your project via function names, and skip to the spot where a given function is defined. It is a tremendous time saver, and bug preventer. The IDEs above also offer a debugger which lets you step through your code and set breakpoints. Want to know the current value of a variable - just hover over it in your debugger. Once you've gotten the hang of this, you will never debug using print statements again. I see nothing wrong with designing Drupal so that it uses those PHP language features that are friendly to IDEs. Namely, functions, classes, constants, etc. Arrays are very flexible, but that flexibility comes at a price. ------------------------------------------------------------------------ Wed, 24 Aug 2005 16:34:41 +0000 : chx Attachment: http://drupal.org/files/issues/forms_chx.patch (15.59 KB) Like we have core.php for hooks we can have form.php for form API and let that help your hand with any IDE. Here is an updated version of Adrian's patch. User login block reworked. I move on to other parts of user.module. I also created a theme_password() function, so I will post form.inc as well. ------------------------------------------------------------------------ Wed, 24 Aug 2005 16:49:05 +0000 : chx Attachment: http://drupal.org/files/issues/form_0.inc (21.01 KB) ------------------------------------------------------------------------ Wed, 24 Aug 2005 22:23:06 +0000 : gordon +1 I like this patch a lot. but 2 things. * The testarea hook is not supported. This needs to be implemented. * the existing form_*() need to be changed to use the theme_*() so that there is only one place that form items can be created. I think this will be great for theme developers and it will be easier to build forms for module developers. Great job. ------------------------------------------------------------------------ Fri, 26 Aug 2005 09:27:08 +0000 : adrian Attachment: http://drupal.org/files/issues/form_1.inc (24.73 KB) Here is an updated version of the patch. 1) admin / settings has been rewritten 2) Added form_radios and form_checkboxes and form_select and a few others 3) Very strict validation now .. the drupal_get_form function automatically validates any input .. no way to skip that. If it doesn't produce errors, it calls $form_id_execute(), or the optional $callback_execute(). 4) Started the first part of the implementation of the multiple keyword, which is when an element can have multiple values. An example of this would be the 'files' element from upload.module , where more files get added, and also the primary / secondary links configuration of phptemplate. Still needs to be done : The clean_url validation, and any other module with a _settings hook. I haven't integrated chx' user module work at this point. ------------------------------------------------------------------------ Fri, 26 Aug 2005 09:48:14 +0000 : adrian Attachment: http://drupal.org/files/issues/forms_0.patch (37.68 KB) Here's the patch. I'm also still busy with the filter format, which has unique requirements and needs to be a filter_format element type (gets rid of the in-line html) ------------------------------------------------------------------------ Sun, 28 Aug 2005 02:52:59 +0000 : yogadex One thing I like about the current API is that you can inject arbitrary HTML in the middle of a form if you see fit. It's not obvious to me how to do that with the new API. Is there a way? Could I, for example, arrange my form fields in a table with two or three columns? ------------------------------------------------------------------------ Sun, 28 Aug 2005 07:26:13 +0000 : naudefj I would also like to be able to display forms in HTML tables. Here's a great article explaining how forms can be styled using tables - http://www.cs.tut.fi/~jkorpela/forms/tables.html ------------------------------------------------------------------------ Sun, 28 Aug 2005 08:25:06 +0000 : adrian EVERY form will now have a theme function, as every form now has it's own (unique) form-id. You can create a theme_my_form_id() function if you are a developer, and require the form to be differently layed out (like for instance, adding all the element outputs to a table). You can create a mytheme_my_form_id() if you are a theme developer, that can override the form layout. (as is shown by the current node_form) [? function theme_node_form($form) { $output .= ''; $output .= ''; $output .= ''; $output .= form_render($form['author']); $output .= ''; $output .= ''; $output .= form_render($form['options']); $output .= ''; $output .= ''; $output .= ''; $output .= form_render($form_render); $output .= ''; $output .= ''; return $output; } ?] You can create a phptemplate stub to load it from a template. <?php function phptemplate_node_form($element) { return _phptemplate_default('node_form', $element); } ?> and the current node_form template being : [? <?php print form_render($form['author']) ?> <?php print form_render($form['options']) ?> <?php print form_render($form) ?> ?] NOTE: As a developer, you can even remix forms you didn't design. Infact, it gives you complete themeability over everything. ------------------------------------------------------------------------ Sun, 28 Aug 2005 08:44:27 +0000 : adrian Attachment: http://drupal.org/files/issues/forms_presentation.pdf (78.63 KB) I hate the php filter .. here's the theme_ function .. i don't have time to escape the template right <?php function theme_node_form($form) { $output .= '<div class="node-form">'; $output .= '<div class="admin">'; $output .= '<div class="authored">'; $output .= form_render($form['author']); $output .= '</div>'; $output .= '<div class="options">'; $output .= form_render($form['options']); $output .= '</div>'; $output .= '</div>'; $output .= '<div class="standard">'; $output .= form_render($form_render); $output .= '</div>'; $output .= '</div>'; return $output; } ?> I am attaching my presentation from drupalcon in portland.It has all the examples, although stuff like the validation has changed slightly. ------------------------------------------------------------------------ Sun, 28 Aug 2005 16:53:48 +0000 : adrian Call for help : Code freeze for Drupal 4.7 is coming very quickly , and a lot of work is still needed to get this functionality in before the code freeze happens. As this patch is incredibly important (even if only considered from a security point of view), we need people to help us port all the forms in Drupal at the moment. If anyone is interested in helping, could you please contact me, so we can coordinate. ------------------------------------------------------------------------ Sun, 28 Aug 2005 22:19:40 +0000 : killes(a)www.drop.org You can assign me a module to conver as soon as my revisions patch is in core. I think I'd like to convert profile.module. ------------------------------------------------------------------------ Sun, 28 Aug 2005 23:41:06 +0000 : yogadex Regarding #24: Now I see why you keep the [printed] flag during node_render. Should this line: $output .= form_render($form_render); read: $output .= form_render($form); ??? (That is, I don't see where variable $form_render is coming from). Regarding #25: Must every form be converted in order to include this? Drupal 4.7 will still support the old forms API, yes? I see that this was discussed early in the thread. For those of us with custom modules life would be a lot easier if the old form api sticks around until say a 5.0 release. ------------------------------------------------------------------------ Mon, 29 Aug 2005 00:02:09 +0000 : killes(a)www.drop.org @yogadex: To those of us who roll out the actual security releases till 4am in the morning the disappearance of the old api with 4.7 will provide a lot more sleep. We win, backwards compatibility sucks. ------------------------------------------------------------------------ Mon, 29 Aug 2005 03:50:27 +0000 : adrian The old form api will remain in core for a maximum of one release. another 6-9 months to convert your modules is more than enough time... plus most of the new uber features (cck etc) are going to require you to upgrade to the new system. Meanwhile, new functionality like the install wizards are going to require you to use this form api. ------------------------------------------------------------------------ Mon, 29 Aug 2005 03:59:41 +0000 : adrian And regarding the form_render question .. it's not a variable, it's a small recursive function. the theme function is passed the entire form tree. The designer can choose to remix the form however he chooses. Since elements are nested, and for instance the fieldsets, are named .. you can use form_render($form['elementname']) , which will print that element, and set the printed flag so it won't be printed again. Like in the example where the author and options fieldsets are seperated out from the form and displayed seperately. form_render($form) in turn will print anything that hasn't been printed yet. if you wanted to seperate out the author field on the page, you could use form_render($form['author']['name']) , and it would print that element wherever, and when you then called form_render($form['author']) or form_render($form); , it would not be printed again. ------------------------------------------------------------------------ Mon, 29 Aug 2005 04:24:47 +0000 : yogadex @killes: I certainly don't want you losing sleep ;) I'm glad to hear the new and old will coexist for at least one release. (Unless there's a security issue - that's another story) @adrian: I understand the form_render() function and it's nifty. But there is a typo in both your attached presentation and #24 above, where "$form_render" is written and it should be "$form", if I am reading correctly. Thanks for explaining. ------------------------------------------------------------------------ Tue, 30 Aug 2005 21:14:37 +0000 : chx Attachment: http://drupal.org/files/issues/forms_1.patch (46.21 KB) I merged and rerolled against current HEAD. ------------------------------------------------------------------------ Wed, 31 Aug 2005 13:59:30 +0000 : m3avrck Minor include bug in the node.module patch: define('NODE_NEW_LIMIT', time() - 30 * 24 * 60 * 60); +include_once 'includes/form.inc'; that include_once is missing the './' part. ------------------------------------------------------------------------ Thu, 01 Sep 2005 00:25:22 +0000 : ax that second <?php +include_once 'includes/form.inc'; ?> shouldn't be changed but *removed*, because it is a duplicate of 8 lines above. ------------------------------------------------------------------------ Thu, 01 Sep 2005 00:31:52 +0000 : m3avrck ah yes, an even better catch than mine, touche! ------------------------------------------------------------------------ Thu, 01 Sep 2005 01:14:12 +0000 : chx Not so. The first should be removed 'cos it is hard to patch against the cvs id :) ------------------------------------------------------------------------ Thu, 01 Sep 2005 01:42:27 +0000 : ax allright, ok, of course, the first one. what would be even better would be a working patch. the ones attached (last one and before) don't include form.inc, generate some duplicate functions that result in "PHP Fatal errors: Cannot redeclare functions()" (system_elements(), theme_node_form()), and "PHP Warning: Call-time pass-by-reference has been deprecated - argument passed by value"s. chx: are you working on this? i would much like to help testing the new forms api and getting it into 4.7 because thats exactly what i need for a bigger project at work where we are considering using Drupal as base framework. the one showstopper is the extensability of Drupal forms ... thanks for all the work done up to now, anyway. ------------------------------------------------------------------------ Thu, 01 Sep 2005 02:38:08 +0000 : jvandyk Attachment: http://drupal.org/files/issues/forms_2.patch (47.72 KB) Here's an updated patch against HEAD that is mostly working. Karoly provided the updated node.module. For me, there seems to be an issue in form.inc in the recursive _form_builder() function where it is recursing even when $element is not an array but is a scalar. ------------------------------------------------------------------------ Thu, 01 Sep 2005 02:43:01 +0000 : m3avrck Not to nitpick, but in node.module, the patch has +include_once 'includes/form.inc'; but should read +include_once './includes/form.inc'; for consistency and performance :) ------------------------------------------------------------------------ Thu, 01 Sep 2005 03:24:16 +0000 : webchick I don't know if this is just the Drupal newbie in me talking, but I am getting quite a few errors with this patch (Drupal HEAD, up-to-date as of about 5 minutes ago). I've uploaded the form.inc from #19 and applied the forms_2.patch from #38, and I'm getting errors like the following: *admin/settings:* warning: Invalid argument supplied for foreach() in $DRUPAL_ROOT/includes/form.inc on line 511. *admin/comment/configure, admin/settings/user*: warning: Missing argument 2 for system_settings_form() in $DRUPAL_ROOT/modules/system.module on line 733. *node/add/[anything]*: warning: Cannot use a scalar value as an array in $DRUPAL_ROOT/includes/form.inc on line 406. warning: Invalid argument supplied for foreach() in $DRUPAL_ROOT/includes/form.inc on line 403. warning: Cannot use a scalar value as an array in $DRUPAL_ROOT/includes/form.inc on line 411. warning: Cannot use a scalar value as an array in $DRUPAL_ROOT/includes/form.inc on line 406. warning: Invalid argument supplied for foreach() in $DRUPAL_ROOT/includes/form.inc on line 403. warning: Cannot use a scalar value as an array in $DRUPAL_ROOT/includes/form.inc on line 411. warning: Cannot use a scalar value as an array in $DRUPAL_ROOT/includes/form.inc on line 406. warning: Invalid argument supplied for foreach() in $DRUPAL_ROOT/includes/form.inc on line 403. warning: Cannot use a scalar value as an array in $DRUPAL_ROOT/includes/form.inc on line 411. warning: Cannot use a scalar value as an array in $DRUPAL_ROOT/includes/form.inc on line 406. warning: Invalid argument supplied for foreach() in $DRUPAL_ROOT/includes/form.inc on line 403. warning: Cannot use a scalar value as an array in $DRUPAL_ROOT/includes/form.inc on line 411. warning: Cannot use a scalar value as an array in $DRUPAL_ROOT/includes/form.inc on line 406. warning: Invalid argument supplied for foreach() in $DRUPAL_ROOT/includes/form.inc on line 403. warning: Cannot use a scalar value as an array in $DRUPAL_ROOT/includes/form.inc on line 411. warning: Invalid argument supplied for foreach() in $DRUPAL_ROOT/includes/form.inc on line 435. warning: Cannot use a scalar value as an array in $DRUPAL_ROOT/includes/form.inc on line 448. warning: Invalid argument supplied for foreach() in $DRUPAL_ROOT/includes/form.inc on line 435. warning: Cannot use a scalar value as an array in $DRUPAL_ROOT/includes/form.inc on line 448. warning: Invalid argument supplied for foreach() in $DRUPAL_ROOT/includes/form.inc on line 435. warning: Cannot use a scalar value as an array in $DRUPAL_ROOT/includes/form.inc on line 448. warning: Invalid argument supplied for foreach() in $DRUPAL_ROOT/includes/form.inc on line 435. warning: Cannot use a scalar value as an array in $DRUPAL_ROOT/includes/form.inc on line 448. warning: Invalid argument supplied for foreach() in $DRUPAL_ROOT/includes/form.inc on line 435. warning: Cannot use a scalar value as an array in $DRUPAL_ROOT/includes/form.inc on line 448. --- Any ideas? ------------------------------------------------------------------------ Thu, 01 Sep 2005 03:35:44 +0000 : jvandyk That's why I said "mostly working." :) m3avrck, sorry, I diffed against the copy where I hadn't corrected the ./ thing. ------------------------------------------------------------------------ Thu, 01 Sep 2005 09:47:05 +0000 : adrian Good morning guys. I am looking at those errors now, and i will upload an updated forms.inc in a short while. ------------------------------------------------------------------------ Thu, 01 Sep 2005 22:46:02 +0000 : adrian Attachment: http://drupal.org/files/issues/form_2.inc (26.76 KB) Here is an updated patch and includes/form.inc. Changes : 1 ) Introduce a global $form_variables array, which works exactly like $_POST['edit'] used to, except the values in it are filtered through the form system and only values that have form elements will be in it. Each form being processed on a page resets this array, so they won't interfere with each other. 2) introduce a form_id hidden that gets submitted with the page. This means it can intelligently decide which form to process. 3) Fix the bug above. I wrapped the offending lines in if is_array() , HOWEVER .. at the point I call that code I am fairly certain I am only working on arrays, hence I need to look it over more 4) fixed the user login form on /user. The user login block still doesn't work 5) Added a default css class (and some temporary css in drupal.css) for everything rendered with the form api. All other forms will have a thick red border if they have not been ported. 6) fixed a bug with element_info where it was setting the wrong defaults and then messing up the first form element because of it. 7) finished the filter module 'filter_format' element. Filter forms are printing now. 8) Added the 'weight' element. Could be cool to theme this with something ajaxy, right ? 9) Fixed some bugs with admin/system 10) Updated the following modules : Book, Page, Story, Blog, Node, Taxonomy and a fixed user form. 11) Moved the inclusion of form.inc to it's rightful place in drupal_bootstrap_full. 12) Added a bunch of comments to places. We will still have to go through all the comments and make sure they all 'parse' =) The node form is not actually working or validated at present, and if someone could take a look at that , I'd be much obliged. It's important to get used to the new (strict) workflow for forms. It generally means having to shift some code around, and it makes it harder to write 'bad code'. ------------------------------------------------------------------------ Thu, 01 Sep 2005 23:05:56 +0000 : adrian Attachment: http://drupal.org/files/issues/forms_3.patch (54.13 KB) here's the patch ------------------------------------------------------------------------ Fri, 02 Sep 2005 05:48:44 +0000 : Dries /introduce a form_id hidden that gets submitted with the page. This means it can intelligently decide which form to process./ Does this mean people could change the ID before they submit the form? I'm not sure that is a good idea. ------------------------------------------------------------------------ Fri, 02 Sep 2005 10:04:21 +0000 : adrian The access check is tied to the page, not to the form. If the page has 2 forms on the page, this check is to see which of the two he submitted. Changing the form_id in the hidden will do nothing but cause the form not to execute, at all. ------------------------------------------------------------------------ Fri, 02 Sep 2005 14:04:45 +0000 : m3avrck Running this patch I'm getting this error Fatal error: _drupal_bootstrap_full() [function.require]: Failed opening required './includes/form.inc' Didn't see the file anywhere in the patch, unless I missed something... ------------------------------------------------------------------------ Fri, 02 Sep 2005 14:44:22 +0000 : adrian drupal.org renames the files to avoid conflicts. it's attached as form_2.inc ------------------------------------------------------------------------ Sat, 03 Sep 2005 03:06:21 +0000 : asimmonds Should there be "return_value" and "delta" property constants defined in form.inc? ------------------------------------------------------------------------ Sat, 03 Sep 2005 06:32:57 +0000 : adrian Attachment: http://drupal.org/files/issues/form_3.inc (27.33 KB) Well spotted. Added those defines. ------------------------------------------------------------------------ Sat, 03 Sep 2005 11:53:53 +0000 : nicopepe Attachment: http://drupal.org/files/issues/modif_Form_Inc.php (2.52 KB) Hello I am a newsbe and i am vey concern about the form project. I want first to congratulaed for all this nice work. I propose this code in the attached file. Tell me what you think, and i'll finished it. ------------------------------------------------------------------------ Sun, 04 Sep 2005 14:57:30 +0000 : nicopepe Hello again, I see no reaction to my proposition. Maybe not interresting code ? My aim with that code is : - Open all the possibilities of attribute in an input balise. - Not multiply the number of constant (The key of an element is an attribute name, define by the programmer) - Check within drupal_attributes function if these attributes are correct in an INPUT Balise. - Include a javascript code insertion for form client validation. In fact i would like to create later on, a module with generic javascript for client validation (Date checking, required fields, ...). I have this in mind when i proposed this code. Any answer this time ? ------------------------------------------------------------------------ Sun, 04 Sep 2005 15:11:41 +0000 : clydefrog nicopepe, it is difficult to tell what your changes are because you have not submitted a patch. Take a look at http://drupal.org/patch. Your code might get a better response that way. ------------------------------------------------------------------------ Sun, 04 Sep 2005 18:25:23 +0000 : Dries As an aside, it might be worth looking into the XForms [1]. They have a short introduction for HTML authors [2] in case you don't feel like reading the entire specification. A friend of mine is a member of the XForms working group and told me someone was working on a JavaScript implementation of XForms. More importantly, XForms have (client-side) validation build-in. For example, to specify a valid range you use: <range ref="volume" start="1" end="10" step="0.5"/> Similarly, XForms have the ability to state that a value must be supplied before the form is submitted, have a notion of typed values (eg. you can specify that you expect an URL, integer or date to be entered). XForms also include stuff to introduce wizard-like behavior of filling in a form in stages, include functionality to dynamically add fields (eg. like when you run out of textfields for poll questions), etc. A lot of the stuff we have been talking about. :-) It might be useful to check if our developer API would map gracefully on XForms. It would impose that we are XForms-ready (future proof). It's especially interesting because Mozilla is working on a XForms implementation [3]. (XForms 1.0 support will be included in Firefox 1.1. You can test it using the nightly Firefox builds.) [1] http://www.w3.org/MarkUp/Forms/ [2] http://www.w3.org/MarkUp/Forms/2003/xforms-for-html-authors [3] http://www.mozilla.org/projects/xforms/ ------------------------------------------------------------------------ Mon, 05 Sep 2005 11:15:16 +0000 : nicopepe As a newsbe, i have not install any patch module Yet. So i will do it. For Dries, i will have a look on this Xforms projet, but i just wonder if it is MS compatible (80% of market) ? Maybe before adding this code I should have write my ideas. I think this new form API should consider having an easy way to perform database update as well as new records adding. So in my view, 2 specials “events” are needed : - call an user defined procedure BEFORE the Form is Loaded (non existing yet ?) to read database and set value to the form. - call an user defined procedure AFTER the Form is validate (drupal_execute_form)to write to the database update or new record. I agree with Adrian to use arrays. They allowed flexibility. I think using $form and $element arrays with others keys than theses defined with property constants is a good way. Property constants are used for regular Drupal core functionality, and user defined keys values used for extra functionalities. This means that the $form array should have a well documented structure to be able to extend it. I see here three examples: - Use all JS events (as my code suggest) for client validation. examples : $form['onSubmit'] = "js function name or code..."; $element['onkeydown'] = "js function name or code..."; - Create an $element[‘NonValidationMessage’] Key used in drupal_validate_form user function to return to the user when the validation of an element is not OK (This message could also be used in JS in client validation). - Possibility for a programmer to input in the $element array the name of the database field and database table to store the input value in case of correct validation. Exemple : $form['tablename'] = "mytable"; $form['idValue'] = "value"; $form['idName'] = "myid"; $element['Databasefieldname'] = "myfieldname"; Then create an function to create automatic SELECT STATMENT to read the value before display the form ( When we need to modify value stored in a database) or INPUT SQL statement after validation, I hope i am clear with my ideas. Maybe some of them could be used already in the core. ------------------------------------------------------------------------ Mon, 05 Sep 2005 11:47:08 +0000 : Dries nicopepe: I'm not saying we should implement XForms at this point. I meant to say that (i) XForms looks like an emerging standard and that (ii) the XForms API tries to solve the same problems we are trying to solve with the new form API. It is worth investigating XForms. Being able to generate XForms-compliant code with minimal changes is a nice extra, but certainly not a must at this point. Hope that clarifies my comment. ------------------------------------------------------------------------ Mon, 05 Sep 2005 12:01:28 +0000 : Junyor Dries: It'd probably be more interesting to look at Web Forms 2: http://www.whatwg.org/specs/web-forms/current-work/. It's geared toward the Web, whereas XForms isn't. Web Forms 2 is also more likely to see native implementations. ------------------------------------------------------------------------ Mon, 05 Sep 2005 12:33:38 +0000 : killes(a)www.drop.org Dries: In my book we are trying to solve a securoty issue woth this patch: the case of inserting bogus fields into our html forms. Client side validation is noce from a usability pov but we are trying to do server side. ------------------------------------------------------------------------ Mon, 05 Sep 2005 14:02:58 +0000 : Dries Gerhard: it's obvious that client-side validation is no replacement for server-side validation. :-) Anyway, instead of looking years ahead, let's focus on the patch at hand. ------------------------------------------------------------------------ Mon, 05 Sep 2005 15:06:27 +0000 : nicopepe Client side validation will never replace server side validation for security reason ! I do understand that client side is not yet a priority, but please let doors open for module developper if they want to implement Client side validation in their module. ------------------------------------------------------------------------ Mon, 12 Sep 2005 17:55:57 +0000 : adrian Attachment: http://drupal.org/files/issues/forms.tar.bz2 (0 bytes) Here is an updated, and more complete version of this patch. I had a bit of a SNAFU with the multiples code, and needed to refactor it to work properly. Changes in this version : 1) Introduced a 'process' property. This is used by the radios and checkboxes elements, to call 'expand_radios' and 'expand_checkboxes' functions which turn the single element into a set of elements. 2) admin/themes screen done. 3) deprecated fix_checkboxes, since it was no longer needed. Checkboxes and radios work perfectly now ( they were broken previously). 4) The way to access the form value is through a global $form_values array, that is set up to the structure of the form. It's no longer flattened, and it works perfectly. 5) Introduced an array_walk_recursive function (copied from PHPCompat in PEAR) that allows you to step through the values and do something with each key. (admin/settings uses this property). 6) Moved the old form api into a legacy.inc 7) Did a bunch more forms. In this tarball you will find the 2 new files (legacy.inc and form.inc) and the current patch. ------------------------------------------------------------------------ Mon, 12 Sep 2005 17:57:30 +0000 : adrian Attachment: http://drupal.org/files/issues/Forms.rtf (14.02 KB) Here's the status document of all the forms (rtf export from omnioutliner) It's incomplete, as I haven't finished cataloging all the forms yet. ------------------------------------------------------------------------ Mon, 12 Sep 2005 18:01:25 +0000 : m3avrck No patch in that zip :) ------------------------------------------------------------------------ Mon, 12 Sep 2005 18:54:08 +0000 : m3avrck Also, adrian, is there a way to add events or set variables in javascript, if the submit button on a form has been clicked? Need this functionality for a patch about to (hopefully!) make its way into HEAD soon. ------------------------------------------------------------------------ Mon, 12 Sep 2005 20:51:26 +0000 : adrian Attachment: http://drupal.org/files/issues/forms.patch.txt (108.18 KB) I'm sure it's possible, but I think we prefer attaching events and the like onto forms using the id attribute. here's the patch file. ------------------------------------------------------------------------ Mon, 12 Sep 2005 23:07:59 +0000 : adrian Attachment: http://drupal.org/files/issues/Forms_0.rtf (22.47 KB) Here's a complete list of the forms in drupal that still need changing. Everything that has already been done is not on this list (ie: all the node modules, and the like) Some of the forms mix normal output, and form output in the same page, and switch out depending on certain requirments (the aggregator_list_page was an example of this), making it a fairly intensive process to seperate the two pieces of functionality into clean code. If something is rated simple, it means it only involves changing the functions to arrays, and either returning the array, or returning the final form using drupal_get_form('form_id', $form); If we want any chance of getting this into core before 4.7, I am going to need some volunteers to handle tackling these forms for me. Once again, I am looking for a few good men (where men == female OR men == male .. obviously. =) ). I am going to be concentrating on the really complex forms, and the theme('confirm') form, which is required by a fair amount of things. ------------------------------------------------------------------------ Tue, 13 Sep 2005 03:38:20 +0000 : Jeremy(a)kerneltrap.org I thought I'd give this patch a quick try, but ran into a couple problems. First is very minor, it makes some changes to settings.php that it shouldn't -- something to clean up before the final version. Second, it adds includes for form.inc and legacy.inc, but the files themselves are not actually included in the patch. ------------------------------------------------------------------------ Tue, 13 Sep 2005 11:53:05 +0000 : adrian They are in the tarball i uploaded earlier. ------------------------------------------------------------------------ Tue, 13 Sep 2005 12:07:31 +0000 : Jeremy(a)kerneltrap.org I tried that last night too, but the earlier tarball was 0 bytes and contained no files: Attachment: forms.tar.bz2 (0 bytes) ------------------------------------------------------------------------ Tue, 13 Sep 2005 12:22:52 +0000 : adrian Attachment: http://drupal.org/files/issues/forms.tar_0.bz2 (27.82 KB) That is the weirdest thing .. locally : Benton:~/dev/forms adrian$ ls -l forms.tar.bz2 -rw-r--r-- 1 adrian adrian 28489 Sep 12 15:30 forms.tar.bz2 ------------------------------------------------------------------------ Tue, 13 Sep 2005 15:03:17 +0000 : adrian Attachment: http://drupal.org/files/issues/Forms.html (10.02 KB) Html version of the status list. ------------------------------------------------------------------------ Tue, 13 Sep 2005 15:12:22 +0000 : Dries Guys, please take a look at the status page. If we want the new form API in Drupal 4.7, we'll have to work together pretty much non-stop to complete it! Care to help? Pick an item, migrate to the new form API and upload a patch. ------------------------------------------------------------------------ Tue, 13 Sep 2005 15:26:09 +0000 : Bèr Kessels Can we not exclude this from the code freeze? I mean that we now (last days) focus on all these outstanding patches for 4.7. and then have acollaborative effort on this forms patch. That will at least give us the benefit that we can aim on a stable target. Instead of junting on a still changing core. From what I see a lot of people are focussing very hard at their babies in the queue, to try to push them into 4.7 (that counts for me anyway) So they will not spend any time on the form api yet. ------------------------------------------------------------------------ Tue, 13 Sep 2005 15:41:45 +0000 : webchick For what it's worth, I agree with Bèr. I've been picking away at comment.module to make it compatible with the forms API, but it's very difficult since the patch needed to enact all these changes differs from day to day (today I applied the .tar'ed version and still am getting errors with a few hunks which I need to troubleshoot first in order to make sure my changes aren't causing errors). Once things are more "settled" with the rest of core, it seems like it would be much easier to make a more "stable" patch as a starting point, which would thus ease efforts on the part of the rest of us to get the changes made. This patch represents a HUGE change, and I worry about the idea of trying cramming it in at the last minute. ------------------------------------------------------------------------ Tue, 13 Sep 2005 15:54:29 +0000 : Souvent22 I totally agree with the above comments. With so many chagnes happening so fast, we wake up to a new core each morning, and must make changes accordingly. I don't know what the core will get 'stable' enough before the freeze to really hack through what needs to be done in an efficient manner. We all seem to agree this *needs* to happen, but perhaps a better plan of *'how'* it should happen since it is such a big change would make things easier, more stable, and not make it seem like we're trying to cram a square block through a round hole.

1 0

Returned Mail: [drupal-devel] [feature] Prevent accidentally navigating away from pages where content has changed
by drupal-devel＠drupal.org 15 Sep '05

15 Sep '05

The following message from Bèr Kessels <drupal-devel(a)drupal.org> was not authorized for entry in the 01Lists.com forum. Posts can only be made by registered users of the forum. From: Bèr Kessels <drupal-devel(a)drupal.org> To: drupal-devel(a)drupal.org Subject: [drupal-devel] [feature] Prevent accidentally navigating away from pages where content has changed **************************************** Issue status update for http://drupal.org/node/30220 Post a follow up: http://drupal.org/project/comments/add/30220 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: m3avrck Updated by: Bèr Kessels Status: patch (ready to be committed) In general I dislike the feature. Konqueror somehow dies this, and that is the way it /should/ be. Browsers should take care of this, not the web app. But, since it is only konq. this patch has a good enough additional value :) I would like to see this patch tested with, tinyMCE and HTMLAREA, at least. For I am quite sure this will break these modules so bad that they are near unusable. Which brings me to the next point: using the textarea hook a simple module could take care of this. I would very very much prefer this living in a contrib, or even a core module. Just not "enforced" on me. And the last point: if this is for core, please use that textarea hook too. This is where that hook is for: extending the textareas. Bèr Kessels Previous comments: ------------------------------------------------------------------------ Thu, 01 Sep 2005 22:47:42 +0000 : m3avrck How many times have you been posting a comment or working on a new post when you click on a link in another application and you navigate away from the content you are editing, losing all of your changes? Well I say we introduce a script, much similiar to that of Blogger, that uses the Javascript window.onbeforeunload handler to prevent this from happening. We can set this handler to call a function that compares the contents of all forms when the page loads and then quickly compares that to the current contents just before the user is about to leave the page. If they have changed, we should prompt the user so they don't lose their work. This would save many headaches and degrade nicely for users without Javascript. Additionally, this script should work with the newly introduced JS-based upload feature to prevent navigating away as a file is being uploaded as well. If I have sometime I'll start work on a patch tomorrow, just wanted to get the idea into the queue right now :) ------------------------------------------------------------------------ Sat, 10 Sep 2005 23:48:33 +0000 : StuartDH Sounds like a great feature to me. We regularly get authors and editors telling us that they've lost the last hour of work by accidentally navigating away, and I've even just had one of them email me about it a few minutes ago, so it'd be great if you could put something together for this. Cheers ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:33:58 +0000 : MichelleC Sounds great to me, too. I often have several tabs open doing stuff and it's easy to lose my place and leave a page with unfinished edits. Would this alert you when you're going to close an unsaved tab/window as well? Michelle ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:34:19 +0000 : MrMattles Great for us multitaskers that rush and click the wrong thing ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:36:52 +0000 : chx Very strong -1 as this is IE only. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:38:40 +0000 : praseodymium Konqueror already does this, +1 for the idea in general. ------------------------------------------------------------------------ Mon, 12 Sep 2005 15:07:21 +0000 : webchick Yeah, I think chx is correc that onbeforeunload is an extension developed by MS [1] (it's not in the ECMAScript specification [2]. However, I found out that this is implemented in Mozilla since 1.7 [3] (which corresponds to Firefox 1.0, I think?) and I've tested Blogger.com's version of this functionality in: Firefox 1.0.6 (working) Firefox Deer Park Alpha 1 (working) Opera 8.0.2 (working) Safari 1.3 (does NOT work) Safari 2.0.1 (working) So it seems to be a 'de facto' standard if nothing else. [1] http://msdn.microsoft.com/workshop/author/dhtml/reference/events/onbeforeun… [2] http://www.ecma-international.org/publications/standards/Ecma-262.htm [3] http://www.mozilla.org/status/2004-03-01.html ------------------------------------------------------------------------ Mon, 12 Sep 2005 15:09:15 +0000 : webchick Also, I should clarify... that "does NOT work" in Safari doesn't display any errors or anything, it just doesn't save the form results as expected (so pre-Tiger Safari users are going to be used to this). So I don't see any harm in including this functionality since it seems to degrade gracefully in non-supporting browsers. ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:10:18 +0000 : m3avrck Attachment: http://drupal.org/files/issues/node.module_11.patch (951 bytes) And a patch is ready! Code was based on many example, including this [4] and this [5] along with adding my own thoughts and what not ;) [4] http://www.codestore.org/store.nsf/cmnts/451FA051398A9AF486256EE0000FB7D1?O… [5] http://www.blogger.com/app/scripts/formcheck.js ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:10:45 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck.js (1.54 KB) And here is the JS to throw in misc/. ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:22:56 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_0.js (1.61 KB) Better JS file attached without tabs. Also, tested and working great in FF and IE6. Doesn't work in Opera 8, however no errors are present, I believe this is just because the event handler is not supported. If there is a work around for Opera, please let me know! ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:24:18 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_1.js (1.61 KB) One more try with those tabs. ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:32:48 +0000 : moshe weitzman hmmm. i usually find these prompts annoying. any chance we can attach the behavior only to the node and comment forms? those are the "high risk" areas. what do others think? ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:42:39 +0000 : m3avrck Yes, the JS is only loaded on the node edit/create pages, won't affect any admin/login/etc form. Also, the prompt is unobstrusive and if you goto save a node you don't get a warning that the contents have changed because you are explicity saving the node (hence no reason!). ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:53:01 +0000 : Dries I want some JS-folks to review the code. ------------------------------------------------------------------------ Mon, 12 Sep 2005 22:09:10 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_15.patch (1.94 KB) After talking with Drumm on IRC, this patch makes the integration with Drupal much simpler and it's off by default. Only turned on for the node edit/change page right now but any module/form can easily add this by passing a 'TRUE' parameter. ------------------------------------------------------------------------ Mon, 12 Sep 2005 23:38:35 +0000 : nedjo +1 on idea, js looks generally good, a few suggestions: 1. Instead of writing onsubmit via PHP, use a js addLoadEvent call. 2. Shouldn't the return false in isElementChanged be at the end (outside the switch block)? 3. It's be nice to find a way to make messages like 'You have unsaved changes.' translatable. Pass in global js variables via a t('') call? 4. onbeforeunload event should probably be in a if isJsEnabled test, and should parallel drupal.js's event adding (see addLoadEvent). 5. Comments should be in standard format and in present tense, e.g., /** * Checks to see if a form has been changed after the page loads */ 6. e_ should be just e to match other js files, e.g., autocomplete. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:54:35 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_2.js (1.58 KB) Ok here is an updated JS file. A few notes, I couldn't get the addSubmitEvent() in drupa.js to reliably work so I had to set the isSubmit() true in the PHP creation of the form. If you look at Blogger.com, they do this as well... so either we both missed an obvious way to do this, or that is the most practical. Hopefully some wise JS gurus will chime in with an answer. Same goes for onbeforeunload event, which is completely different then addEvent defined in drupal.js. As for the t('') I agree this would be useful but I'm not sure of the best way to do this. One way would be to put in form() a t('') passed in, and then write this to a var in formcheck.php which returns a JS file type. Thoughts? ------------------------------------------------------------------------ Tue, 13 Sep 2005 14:06:44 +0000 : Junyor FWIW, Opera doesn't support the onbeforeunload event. However, this issue doesn't affect Opera anyway: form contents are retained in history as long as the page isn't closed. IOW, this feature doesn't work in Opera, but it isn't needed either. ------------------------------------------------------------------------ Tue, 13 Sep 2005 14:15:56 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_3.js (1.64 KB) Updated JS styling and JS-killswitch after talking with Unconed on IRC. ------------------------------------------------------------------------ Tue, 13 Sep 2005 14:39:15 +0000 : m3avrck Just to build on Junyor's comment [6] this functionality is built into Opera 8 and it *doesn't* produce any errors. [6] http://drupal.org/#comment-44066 ------------------------------------------------------------------------ Tue, 13 Sep 2005 15:04:37 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_16.patch (2.1 KB) Updated patch to fix possible problem of overwriting onsubmit event handler thanks to Thox on IRC. ------------------------------------------------------------------------ Tue, 13 Sep 2005 15:13:59 +0000 : webchick I tested this and can confirm that it works in *both* versions of Safari that I have access to: 2.0.1 (OS X Tiger) and 1.3 (OS X Panther). So looks like this code goes one better over the Blogger.com stuff, because their stuff doesn't workk in 1.3. Nice job! ;) ------------------------------------------------------------------------ Tue, 13 Sep 2005 15:25:21 +0000 : m3avrck Well I'm gonna set this to commit then. Tested and working on IE, FF, Safari. Doesn't work or break Opera or Konqueror but not needed in these cases. Thox and Unconded have offered thoughts and I've modified code as needed. Doesn't seem like there is anything else left except a commit :)

1 0