New sections tab, multi-edit control block

Bloke · 2012-06-29 21:22:43

That’s much better functionality. The ‘reset-on-cancel’ is far superior to leaving it selected and adding the ‘Go’ button for auto-submit actions. Me likey. Me committee…

Bloke · 2012-06-29 21:35:44

Oh, one thing. In FF (and I assume other browsers), if you:

Select some stuff.
Choose an auto-submit action such as ‘delete’, or anything from the Comments panel.
Hit Cancel in the verify box.
Choose another (or the same) auto-submit action.

you see an additional checkbox in the verify message:

Prevent this page from creating additional dialogs?

Thus it’s tripping the browser’s ‘infinite loop’ dialog security feature.

If anyone does check the box and hits Cancel again, the next time they choose any auto-submit option, no dialog will appear and, further, won’t be able to submit the form because the ‘Go’ button doesn’t appear on auto-submit actions. The only way to restore functionality is to reload the panel.

This behaviour is not a show stopper because the number of times this is going to happen is verging on zero, but thought I’d just document it here so we can refer to it in future in case it crops up.

Last edited by Bloke (2012-06-29 21:36:36)

Gocom · 2012-06-29 22:41:03

Bloke wrote:

This behaviour is not a show stopper because the number of times this is going to happen is verging on zero, but thought I’d just document it here so we can refer to it in future in case it crops up.

One of those reasons why there should be a better dialog implementation than relying to confirm() in place. I’m hoping that one comes with the future modal dialog code (in 4.6 maybe?).

In FF (and I assume other browsers)

In Safari and Chrome such check isn’t present in as strict form (at least on Mac OS). I suppose that in IE it’s possible to disable confirm dialogs altogether with the browser’s security options.

Last edited by Gocom (2012-06-29 22:57:30)

phiw13 · 2012-06-30 01:40:04

Another bug. Seen this in 3 different browsers, two installs (one at r3862 – pretty much default, the other one at r3875 – my usual test install):

Section pane, select a section,
from the drop down: choose ‘Uses Page’
from the secondary dropdown choose a page template (doesn’t matter which one)
go, confirm (ok), etc

Result: the selected section now has a template named ‘1’ (in the table). Click on the link to that template and you get a blank template page… (that is expected, of course).

Now click on the name of the section you just edited, the edit pane comes up and under ‘Uses Page’ is listed the template it originally had.

This seems specific to changing the ‘Uses Page’ action, and specific to the Sections pane; I haven’t seen any such problems on other panes, or with different actions on the Sections pane.

Gocom · 2012-06-30 08:31:07

Ah, it’s the input name, page. The multi-edit select for Uses Page option is named page, but it is reserved/used by pagination too, and the edited section gets a page number instead of page template. The multi-edit option’s name needs to be changed. Might not be a bad idea also to verify the values.

Last edited by Gocom (2012-06-30 08:32:54)

Bloke · 2012-06-30 08:56:49

Gocom wrote:

Might not be a bad idea also to verify the values.

Ack, an extra query, but I suppose you’re right and it’s only on save so the cost isn’t massive. Multi-edit values should probably be checked across the interface actually…

Last edited by Bloke (2012-06-30 08:57:29)

Gocom · 2012-06-30 09:00:20

Shouldn’t those values used in SQL queries be escaped?

Index: include/txp_section.php
===================================================================
--- include/txp_section.php	(revision 3877)
+++ include/txp_section.php	(working copy)
@@ -512,7 +512,7 @@
 	function section_delete()
 	{
 		$selected  = ps('selected');
-		$with_articles = safe_rows('Section, Count(*) AS count', 'textpattern', "Section in ('".join("','", $selected)."') GROUP BY Section");
+		$with_articles = safe_rows('Section, Count(*) AS count', 'textpattern', "Section in ('".join("','", doSlash($selected))."') GROUP BY Section");
 		$protected[] = 'default';
 		$del['success'] = $del['error'] = array();

@@ -601,7 +601,7 @@

 		switch ($method)
 		{
-			case 'delete';
+			case 'delete':
 				return section_delete($selected);
 				break;

@@ -610,24 +610,24 @@
 				$val = ps('uses_page');
 				break;

-			case 'changecss';
+			case 'changecss':
 				$key = 'css';
 				$val = ps('css');
 				break;

-			case 'changeonfrontpage';
+			case 'changeonfrontpage':
 				$key = 'on_frontpage';
-				$val = ps('on_front_page');
+				$val = (int) ps('on_front_page');
 				break;

-			case 'changesyndicate';
+			case 'changesyndicate':
 				$key = 'in_rss';
-				$val = ps('in_rss');
+				$val = (int) ps('in_rss');
 				break;

-			case 'changesearchable';
+			case 'changesearchable':
 				$key = 'searchable';
-				$val = ps('searchable');
+				$val = (int) ps('searchable');
 				break;

 			default:
@@ -636,13 +636,13 @@
 				break;
 		}

-		$selected = safe_column('name', 'txp_section', "name IN ('".join("','", $selected)."')");
+		$selected = safe_column('name', 'txp_section', "name IN ('".join("','", doSlash($selected))."')");

 		if ($selected and $key)
 		{
 			foreach ($selected as $id)
 			{
-				if (safe_update('txp_section', "$key = '".doSlash($val)."'", "name = '$id'"))
+				if (safe_update('txp_section', "$key = '".doSlash($val)."'", "name = '".doSlash($id)."'"))
 				{
 					$changed[] = $id;
 				}

Gocom · 2012-06-30 09:01:43

Bloke wrote:

Ack, an extra query, but I suppose you’re right and it’s only on save so the cost isn’t massive. Multi-edit values should probably be checked across the interface actually…

You could do two queries globally right before $step(); is called and store the values. No extra queries and txp_css and txp_pages tables are accessed on all pages anyways.

Last edited by Gocom (2012-06-30 09:06:25)

Bloke · 2012-06-30 14:22:30

Gocom wrote:

You could do two queries globally right before $step(); is called and store the values.

True. Or what about a Constraint? Or is this not an appropriate place for it?

Shouldn’t those values used in SQL queries be escaped?

Hell yes! And how did the case statements ever work with semicolons instead of colons?! I need my eyes testing. It’s like that in a few panels so I’d better fix that sharpish. Thanks eagle-eyes.

Last edited by Bloke (2012-06-30 14:22:55)

Gocom · 2012-06-30 15:19:34

Bloke wrote:

True. Or what about a Constraint? Or is this not an appropriate place for it?

Could be cool to have, but I’m not sure if it’s really needed. As pages and styles go, you can’t actually change public-facing template or style loading, making extending of those two rather pointless. Maybe Articles panel can benefit from constraints, but I don’t think much else can.

And how did the case statements ever work with semicolons instead of colons?!

That’s PHP and its inconsistent alternative syntax structures for you.

Last edited by Gocom (2012-06-30 15:27:41)

phiw13 · 2012-07-01 02:20:17

Gocom wrote:

Ah, it’s the input name, page. The multi-edit select for Uses Page option is named page, but it is reserved/used by pagination too, and the edited section gets a page number instead of page template. The multi-edit option’s name needs to be changed. Might not be a bad idea also to verify the values.

Right, thanks for debugging! Following r3877, this now works correctly.

And fwiw, I haven’t found any more issues :-).

philwareham · 2012-07-01 11:16:18

Hi.

Back home now and sifting through all the work that’s been done.

The categories page potentially has multiple instances of id="multi-option-changeparent" which needs to be addressed.

Bloke · 2012-07-01 19:52:54

philwareham wrote:

The categories page potentially has multiple instances of id="multi-option-changeparent"

Best I can do is add a static numeric counter to the end of the ID in the multi_edit function. It’ll seem a bit weird to have multi-edit-changeblah-1 on pages that only have one control, but does that cause any problems? Or is there a better solution I’m not seeing?

EDIT: or add an optional select_id attribute to the args in multi_edit() which is mixed into the default ID somewhere. e.g. multi-option-changeparent-article (if function supplied with article as the select_id parameter).

EDIT2: OK, that last one works better. I’ll stick with that for now.

Last edited by Bloke (2012-07-01 20:26:24)

Gocom · 2012-07-01 20:55:18

Bloke wrote:

EDIT2: OK, that last one works better. I’ll stick with that for now.

I would have gone with the first one personally. Please note that the IDs are temporary and nothing outside the multi-edit controller can use those (not w/ event based hacks). If you check the page with a DOM inspector you will see that the .multi-option blocks and IDs aren’t actually even present in the document. They are not stored as hidden blocks in the page, but in memory by JavaScript.

The best option might actually be use randomized IDs altogether. The $methods are in order, and indexes could be used the IDs. Appending an static instance number to IDs would avoid collisions.

Last edited by Gocom (2012-07-01 20:58:36)

Bloke · 2012-07-01 21:03:38

Gocom wrote:

IDs are temporary and nothing outside the multi-edit controller can use those

Ah, good point. In which case, how about…

Index: textpattern/lib/txplib_html.php
===================================================================
--- textpattern/lib/txplib_html.php	(revision 3885)
+++ textpattern/lib/txplib_html.php	(working copy)
@@ -876,12 +876,13 @@
  * @param  string  $dir Sorting direction
  * @param  string  $crit Search criterion
  * @param  string  $search_method Search method
- * @param  string  $id_suffix Added to the HTML id attribute; useful if more than one similar control placed per page
  * @return string  HTML
  */

-	function multi_edit($options, $event=null, $step=null, $page='', $sort='', $dir='', $crit='', $search_method='', $id_suffix='')
+	function multi_edit($options, $event=null, $step=null, $page='', $sort='', $dir='', $crit='', $search_method='')
 	{
+		static $me_counter = 1;
+
 		$html = $methods = array();
 		$methods[''] = gTxt('with_selected_option');

@@ -906,7 +907,7 @@
 				if (isset($option['html']))
 				{
 					$html[$value] = '<div class="multi-option" id="multi-option-'.
-						txpspecialchars($value).($id_suffix ? '-'.txpspecialchars($id_suffix) : '').'">'.$option['html'].'</div>';
+						txpspecialchars($value).$me_counter++.'">'.$option['html'].'</div>';
 				}
 			}
 			else
Index: textpattern/include/txp_category.php
===================================================================
--- textpattern/include/txp_category.php	(revision 3885)
+++ textpattern/include/txp_category.php	(working copy)
@@ -184,7 +184,7 @@
 				form(
 					join('',$array).
 					hInput('type',$area).
-					n.multi_edit($methods, 'category', 'cat_category_multiedit', '', '', '', '', '', $area)
+					n.multi_edit($methods, 'category', 'cat_category_multiedit')
 					,'', '', 'post', 'category-tree', '', 'category_'.$area.'_form'
 				);
 		}

Textpattern CMS

Textpattern CMS support forum

#76 2012-06-29 21:22:43

Re: New sections tab, multi-edit control block

#77 2012-06-29 21:35:44

Re: New sections tab, multi-edit control block

#78 2012-06-29 22:41:03

Re: New sections tab, multi-edit control block

#79 2012-06-30 01:40:04

Re: New sections tab, multi-edit control block

#80 2012-06-30 08:31:07

Re: New sections tab, multi-edit control block

#81 2012-06-30 08:56:49

Re: New sections tab, multi-edit control block

#82 2012-06-30 09:00:20

Re: New sections tab, multi-edit control block

#83 2012-06-30 09:01:43

Re: New sections tab, multi-edit control block

#84 2012-06-30 14:22:30

Re: New sections tab, multi-edit control block

#85 2012-06-30 15:19:34

Re: New sections tab, multi-edit control block

#86 2012-07-01 02:20:17

Re: New sections tab, multi-edit control block

#87 2012-07-01 11:16:18

Re: New sections tab, multi-edit control block

#88 2012-07-01 19:52:54

Re: New sections tab, multi-edit control block

#89 2012-07-01 20:55:18

Re: New sections tab, multi-edit control block

#90 2012-07-01 21:03:38

Re: New sections tab, multi-edit control block

Board footer