Re: [PATCH] zoneconf.cgi: Improve the usability of the zone configuration by marking assigned NICs in the zone color. The highlighting is initially applied to the static HTML output, and JavaScript is used to dynamically follow changes made by the user.

["Daniel Weismüller" <daniel.weismueller@ipfire.org>]

[-- Attachment #1: Type: text/plain, Size: 7949 bytes --]

Hello
Regardless of the advice and improvement requests of others, I have 
applied the patch to my IPFire.
So far everything works as expected and I could not find any errors or 
misbehaviour.

Greetings
Daniel

Am 10.11.2020 um 13:37 schrieb Leo-Andres Hofmann:
> Discussion: https://lists.ipfire.org/pipermail/development/2020-October/008567.html
>
> Signed-off-by: Leo-Andres Hofmann <hofmann(a)leo-andres.de>
> ---
>   html/cgi-bin/zoneconf.cgi | 89 +++++++++++++++++++++++++++++----------
>   1 file changed, 67 insertions(+), 22 deletions(-)
>
> diff --git a/html/cgi-bin/zoneconf.cgi b/html/cgi-bin/zoneconf.cgi
> index d99a3e611..2501901c4 100644
> --- a/html/cgi-bin/zoneconf.cgi
> +++ b/html/cgi-bin/zoneconf.cgi
> @@ -26,7 +26,7 @@ require '/var/ipfire/general-functions.pl';
>   require "${General::swroot}/lang.pl";
>   require "${General::swroot}/header.pl";
>   
> -my $css = <<END
> +my $extraHead = <<END
>   <style>
>   	table {
>   		width: 100%;
> @@ -64,19 +64,19 @@ my $css = <<END
>   	}
>   
>   	td.green {
> -		background-color: $Header::colourgreen;
> +		background-color: $Header::colourgreen !important;
>   	}
>   
>   	td.red {
> -		background-color: $Header::colourred;
> +		background-color: $Header::colourred !important;
>   	}
>   
>   	td.blue {
> -		background-color: $Header::colourblue;
> +		background-color: $Header::colourblue !important;
>   	}
>   
>   	td.orange {
> -		background-color: $Header::colourorange;
> +		background-color: $Header::colourorange !important;
>   	}
>   
>   	td.topleft {
> @@ -112,6 +112,44 @@ my $css = <<END
>   		margin-top: 1em;
>   	}
>   </style>
> +
> +<script type="text/javascript">
> +	//highlight interface access selection, call this from "onchange" event of selection element
> +	function highlightAccess(selectObj) {
> +		if(!(selectObj && ('zone' in selectObj.dataset) && ('mac' in selectObj.dataset))) {
> +			return; //required parameters are missing
> +		}
> +
> +		var zoneColor = selectObj.dataset.zone.trim().toLowerCase(); //zone color (red/green/blue/orange) CSS class
> +
> +		function colorParentCell(obj, color, enabled = true) { //find nearest parent table cell of "obj" and set its CSS color class
> +			do {
> +				obj = obj.parentElement;
> +			} while(obj && (obj.nodeName.toUpperCase() !== 'TD'));
> +			if(obj && (['green', 'red', 'orange', 'blue'].includes(color))) {
> +				obj.classList.toggle(color, enabled);
> +			}
> +		}
> +
> +		//handle other associated input fields
> +		if(selectObj.type.toUpperCase() === 'RADIO') { //this is a radio button group: clear all highlights
> +			let radios = document.getElementsByName(selectObj.name);
> +			radios.forEach(function(button) {
> +				if(button.nodeName.toUpperCase() === 'INPUT') { //make sure the found element is a button
> +					colorParentCell(button, zoneColor, false); //remove css
> +				}
> +			});
> +		} else { //this is a dropdown menu: enable/disable additional VLAN tag input box
> +			let tagInput = document.getElementById('TAG ' + selectObj.dataset.zone + ' ' + selectObj.dataset.mac); //tag input element selector
> +			if(tagInput) {
> +				tagInput.disabled = (selectObj.value !== 'VLAN'); //enable tag input if VLAN mode selected
> +			}
> +		}
> +
> +		//if interface is assigned, highlight table cell in zone color
> +		colorParentCell(selectObj, zoneColor, (selectObj.value !== 'NONE'));
> +	}
> +</script>
>   END
>   ;
>   
> @@ -162,7 +200,7 @@ foreach (@nics) {
>   	}
>   }
>   
> -&Header::openpage($Lang::tr{"zoneconf title"}, 1, $css);
> +&Header::openpage($Lang::tr{"zoneconf title"}, 1, $extraHead);
>   &Header::openbigbox('100%', 'center');
>   
>   ### Evaluate POST parameters ###
> @@ -312,8 +350,8 @@ if ($cgiparams{"ACTION"} eq $Lang::tr{"save"}) {
>   print <<END
>   <form method='post' enctype='multipart/form-data'>
>   	<table>
> -		<tr>
> -		<td class="h narrow topleft" /td>
> +	<tr>
> +		<td class="h narrow topleft"></td>
>   END
>   ;
>   
> @@ -332,7 +370,7 @@ foreach (@zones) {
>   		my $red_restricted = ($uc eq "RED" && ! ($red_type eq "STATIC" || $red_type eq "DHCP"));
>   
>   		if ($red_restricted) {
> -			print "<td class='h textcenter $_'>$uc ($red_type)</td>";
> +			print "\t\t<td class='h textcenter $_'>$uc ($red_type)</td>\n";
>   
>   			next; # We're done here
>   		}
> @@ -350,7 +388,7 @@ foreach (@zones) {
>   	}
>   
>   	print <<END
> -		<td class='h textcenter $_'>$uc</br>
> +		<td class='h textcenter $_'>$uc<br>
>   			<select name="MODE $uc">
>   				<option value="DEFAULT" $mode_selected{"DEFAULT"}>$Lang::tr{"zoneconf nicmode default"}</option>
>   				<option value="BRIDGE" $mode_selected{"BRIDGE"}>$Lang::tr{"zoneconf nicmode bridge"}</option>
> @@ -361,7 +399,7 @@ END
>   ;
>   }
>   
> -print "</tr>";
> +print "\t</tr>\n";
>   
>   my $slightlygrey = "";
>   
> @@ -370,12 +408,13 @@ foreach (@nics) {
>   	my $nic = $_->[1];
>   	my $wlan = $_->[2];
>   
> -	print "<tr><td class='h narrow textcenter'>$nic<br>$mac</td>";
> +	print "\t<tr>\n\t\t<td class='h narrow textcenter'>$nic<br>$mac</td>\n";
>   
>   	# Iterate through all zones and check if the current NIC is assigned to it
>   	foreach (@zones) {
>   		my $uc = uc $_;
>   		my $dev_name = $ethsettings{"${uc}_DEV"};
> +		my $highlight = "";
>   
>   		if ($dev_name eq "") { # Again, skip the zone if it is not activated
>   			next;
> @@ -391,9 +430,10 @@ foreach (@nics) {
>   
>   				if ($mac eq $ethsettings{"${uc}_MACADDR"}) {
>   					$checked = "checked";
> +					$highlight = $_;
>   				}
>   
> -				print "<td class='textcenter $slightlygrey'><input type='radio' id='PPPACCESS $mac' name='PPPACCESS' value='$mac' $checked></td>";
> +				print "\t\t<td class='textcenter $highlight $slightlygrey'><input type='radio' id='PPPACCESS $mac' name='PPPACCESS' value='$mac' data-zone='RED' data-mac='$mac' onchange='highlightAccess(this)' $checked></td>\n";
>   				next; # We're done here
>   			}
>   		}
> @@ -430,21 +470,26 @@ foreach (@nics) {
>   
>   		$access_selected{"NONE"} = ($access_selected{"NATIVE"} eq "") && ($access_selected{"VLAN"} eq "") ? "selected" : "";
>   		my $vlan_disabled = ($wlan) ? "disabled" : "";
> +		
> +		# If the interface is assigned, hightlight table cell
> +		if ($access_selected{"NONE"} eq "") {
> +			$highlight = $_;
> +		}
>   
>   		print <<END
> -			<td class="textcenter $slightlygrey">
> -				<select name="ACCESS $uc $mac" onchange="document.getElementById('TAG $uc $mac').disabled = (this.value === 'VLAN' ? false : true)">
> -					<option value="NONE" $access_selected{"NONE"}>- $Lang::tr{"zoneconf access none"} -</option>
> -					<option value="NATIVE" $access_selected{"NATIVE"}>$Lang::tr{"zoneconf access native"}</option>
> -					<option value="VLAN" $access_selected{"VLAN"} $vlan_disabled>$Lang::tr{"zoneconf access vlan"}</option>
> -				</select>
> -				<input type="number" class="vlanid" id="TAG $uc $mac" name="TAG $uc $mac" min="1" max="4095" value="$zone_vlan_id" $field_disabled>
> -			</td>
> +		<td class="textcenter $highlight $slightlygrey">
> +			<select name="ACCESS $uc $mac" data-zone="$uc" data-mac="$mac" onchange="highlightAccess(this)">
> +				<option value="NONE" $access_selected{"NONE"}>- $Lang::tr{"zoneconf access none"} -</option>
> +				<option value="NATIVE" $access_selected{"NATIVE"}>$Lang::tr{"zoneconf access native"}</option>
> +				<option value="VLAN" $access_selected{"VLAN"} $vlan_disabled>$Lang::tr{"zoneconf access vlan"}</option>
> +			</select>
> +			<input type="number" class="vlanid" id="TAG $uc $mac" name="TAG $uc $mac" min="1" max="4095" value="$zone_vlan_id" $field_disabled>
> +		</td>
>   END
>   ;
>   	}
>   
> -	print "</tr>";
> +	print "\t</tr>\n";
>   
>   	if ($slightlygrey) {
>   		$slightlygrey = "";