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.

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.

[Jonatan Schlag]

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

Hi,

thirst thank you for working on this so fast. I will comment on this
patch inline, but first one note on the commit message.

The commit message should be split up. When you use
'git commit -sv' an editor will open and in this editor, the first line
makes up the subject of the mail the lines after the second makes up
the commit message. The first line should not be longer than 50
characters to avoid, a subject like in this E-Mail.

For further information on a good commit message see: 
https://blog.ipfire.org/post/how-to-write-a-good-commit-message


"Leo-Andres Hofmann" hofmann(a)leo-andres.de – 10. November 2020 13:37
> Discussion: 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;
Why do we need !important here? When it does not work without we
should, investigate why. Using important break the specificity of the
CSS, which will make the CSS harder to maintain.
> }
> 
> 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);
> + }
> + }
Why is this function part of the other function?
> +
> + //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
> ;

Could you put this in a separate file (zoneconf.js) please? It's hard to read here and we should try to at least avoid introducing new mixing of languages (Perl and js). Also, we can use formatter and linter of this separate file easier. This should apply to the CSS as well, but that is not your mistake and therefore not part of the patch.
> 
> @@ -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";
@Micheal or somebody else, do we have a function to print lines
intended like in the network code of ipfire 3.x?
Something like print_indented <number of tabs> <string>
> 
> 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>
Should be not part of this patch, but does not matter much. You can
use 'git add -i' to pick those changes and but them in a separate patch
like "cleaning up code".
> <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

This is very hard to read, could we split this up please?
> }
> }
> @@ -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
> ;
> }
Same here, when you only change the indention it is hard to see the
actual change of the javascript. 

To sum things up, please do not get upset about my comments. It might be sometimes annoying, but doing things in a high-quality way makes our work in a few years when we might have a look at this again easier.
Greetings Jonatan
> 
- print "</tr>";
+ print "\t</tr>\n";

if ($slightlygrey) {
$slightlygrey = "";
--
2.27.0.windows.1

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.

[Michael Tremer]

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

Hello,

> On 11 Nov 2020, at 11:55, Jonatan Schlag <jonatan.schlag(a)ipfire.org> wrote:
> 
> Hi,
> 
> thirst thank you for working on this so fast. I will comment on this
> patch inline, but first one note on the commit message.
> 
> The commit message should be split up. When you use
> 'git commit -sv' an editor will open and in this editor, the first line
> makes up the subject of the mail the lines after the second makes up
> the commit message. The first line should not be longer than 50
> characters to avoid, a subject like in this E-Mail.
> 
> For further information on a good commit message see: 
> https://blog.ipfire.org/post/how-to-write-a-good-commit-message

Good pointers.

> "Leo-Andres Hofmann" hofmann(a)leo-andres.de – 10. November 2020 13:37
>> Discussion: 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;
> Why do we need !important here? When it does not work without we
> should, investigate why. Using important break the specificity of the
> CSS, which will make the CSS harder to maintain.
>> }
>> 
>> 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);
>> + }
>> + }
> Why is this function part of the other function?
>> +
>> + //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
>> ;
> 
> Could you put this in a separate file (zoneconf.js) please? It's hard to read here and we should try to at least avoid introducing new mixing of languages (Perl and js). Also, we can use formatter and linter of this separate file easier. This should apply to the CSS as well, but that is not your mistake and therefore not part of the patch.

Hmm, I get that the JS is messy. But I am not sure if a second file helps.

I would say since this is an extension of an existing file, we should keep it as it is.

>> 
>> @@ -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";
> @Micheal or somebody else, do we have a function to print lines
> intended like in the network code of ipfire 3.x?
> Something like print_indented <number of tabs> <string>

*Michael

The web UI is built like this now. I understand your idea, but I would be against a major rewrite of this now, because it is a lot of work for almost no benefit to the user. Since we usually change the CGI files not very much, I guess we can rather cope with a little bit extra overhead here.

>> 
>> 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>
> Should be not part of this patch, but does not matter much. You can
> use 'git add -i' to pick those changes and but them in a separate patch
> like "cleaning up code".
>> <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
> 
> This is very hard to read, could we split this up please?
>> }
>> }
>> @@ -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
>> ;
>> }
> Same here, when you only change the indention it is hard to see the
> actual change of the javascript. 
> 
> To sum things up, please do not get upset about my comments. It might be sometimes annoying, but doing things in a high-quality way makes our work in a few years when we might have a look at this again easier.
> Greetings Jonatan
>> 
> - print "</tr>";
> + print "\t</tr>\n";
> 
> if ($slightlygrey) {
> $slightlygrey = "";
> --
> 2.27.0.windows.1
> 
> 

Thank you for Jonatan for reviewing this :)

And thank you even more for Leo-Andres for working on this.

Best,
-Michael

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.

[Leo Hofmann]

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

Hi Jonatan,

thanks for your feedback. I'm not very familiar with your workflow yet, I hope you will excuse my mistakes.
I guess I'll have to submit this patch a second time as well. If you don't mind, could you please comment on my idea for the second attempt:

Patch 1: Only fix HTML output e.g. missing "<", indentation and so on.
I remember Michael suggested this in the past, but since it only affected a few lines this time, I thought it was okay.

I just noticed another issue with the original zoneconf source. Some HTML elements are created with whitespace in their id tag, which isn't allowed by HTML standard.
Should I fix this in the first patch as well, or would you like this to be another patch?

Patch 2 (or 3?): Add highlighting feature

This patch would rely on the changes introduced by the first patch. What's the best way to make sure they don't get separated?

I will answer your comments below! I have also added a few specific questions.

Regards
Leo

Am 11.11.2020 um 12:55 schrieb Jonatan Schlag:
> Hi,
>
> thirst thank you for working on this so fast. I will comment on this
> patch inline, but first one note on the commit message.
>
> The commit message should be split up. When you use
> 'git commit -sv' an editor will open and in this editor, the first line
> makes up the subject of the mail the lines after the second makes up
> the commit message. The first line should not be longer than 50
> characters to avoid, a subject like in this E-Mail.
>
> For further information on a good commit message see:
> https://blog.ipfire.org/post/how-to-write-a-good-commit-message
>
>
> "Leo-Andres Hofmann" hofmann(a)leo-andres.de – 10. November 2020 13:37
>> Discussion: 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;
> Why do we need !important here? When it does not work without we
> should, investigate why. Using important break the specificity of the
> CSS, which will make the CSS harder to maintain.

This ensures that the highlighting always takes precendence over the table row colors.
I found that this was not always reliable during the tests.

I can easily fix this by using modern css selectors like nth-child. This would also simplify the Perl code, because I can do the odd-even row colors in CSS as well.
But this might break compatibility with legacy browsers. What is your opinion on this?
Should this be included the first general clean-up patch?

>> }
>>
>> 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);
>> + }
>> + }
> Why is this function part of the other function?
Because it is specific to this highlighting function. I want to avoid cluttering the global namespace with auxiliary functions.
>> +
>> + //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
>> ;
> Could you put this in a separate file (zoneconf.js) please? It's hard to read here and we should try to at least avoid introducing new mixing of languages (Perl and js). Also, we can use formatter and linter of this separate file easier. This should apply to the CSS as well, but that is not your mistake and therefore not part of the patch.
Where should I put this? Would html/include/zoneconf.js be the right place? Are there any licensing headers I need to add?
>> @@ -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";
> @Micheal or somebody else, do we have a function to print lines
> intended like in the network code of ipfire 3.x?
> Something like print_indented <number of tabs> <string>
That would be great! I know this isn't really important, but creating clean HTML makes it much easier for me to check my work.
>> 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>
> Should be not part of this patch, but does not matter much. You can
> use 'git add -i' to pick those changes and but them in a separate patch
> like "cleaning up code".
>> <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
> This is very hard to read, could we split this up please?
I'll split it!
> }
> }
> @@ -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
> ;
> }
> Same here, when you only change the indention it is hard to see the
> actual change of the javascript.
>
> To sum things up, please do not get upset about my comments. It might be sometimes annoying, but doing things in a high-quality way makes our work in a few years when we might have a look at this again easier.
> Greetings Jonatan
> - print "</tr>";
> + print "\t</tr>\n";
>
> if ($slightlygrey) {
> $slightlygrey = "";
> --
> 2.27.0.windows.1
>
>

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.

[Michael Tremer]

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

Hi,

> On 11 Nov 2020, at 16:44, Leo Hofmann <hofmann(a)leo-andres.de> wrote:
> 
> Hi Jonatan,
> 
> thanks for your feedback. I'm not very familiar with your workflow yet, I hope you will excuse my mistakes.

Absolutely. This is just to let you know what was different than expected.

Also feel free to ask any questions whenever you are unsure :)

> I guess I'll have to submit this patch a second time as well. If you don't mind, could you please comment on my idea for the second attempt:
> 
> Patch 1: Only fix HTML output e.g. missing "<", indentation and so on.
> I remember Michael suggested this in the past, but since it only affected a few lines this time, I thought it was okay.

I do this sometimes, too. A few probably can never be avoided.

I wouldn’t have complained about this as long as this is in or close to the lines that are being changed.

> I just noticed another issue with the original zoneconf source. Some HTML elements are created with whitespace in their id tag, which isn't allowed by HTML standard.
> Should I fix this in the first patch as well, or would you like this to be another patch?

Please do this in another patch. That allows us to patch this very quickly because it is very easy to review.

> Patch 2 (or 3?): Add highlighting feature

> This patch would rely on the changes introduced by the first patch. What's the best way to make sure they don't get separated?

Just send them in one go.

If you are using “git send-email” it will automatically generate a patchset which then looks like this:

  https://patchwork.ipfire.org/project/ipfire/list/?series=1600

> I will answer your comments below! I have also added a few specific questions.
> 
> Regards
> Leo

-Michael

> Am 11.11.2020 um 12:55 schrieb Jonatan Schlag:
>> Hi,
>> 
>> thirst thank you for working on this so fast. I will comment on this
>> patch inline, but first one note on the commit message.
>> 
>> The commit message should be split up. When you use
>> 'git commit -sv' an editor will open and in this editor, the first line
>> makes up the subject of the mail the lines after the second makes up
>> the commit message. The first line should not be longer than 50
>> characters to avoid, a subject like in this E-Mail.
>> 
>> For further information on a good commit message see:
>> https://blog.ipfire.org/post/how-to-write-a-good-commit-message
>> 
>> 
>> "Leo-Andres Hofmann" hofmann(a)leo-andres.de – 10. November 2020 13:37
>>> Discussion: 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;
>> Why do we need !important here? When it does not work without we
>> should, investigate why. Using important break the specificity of the
>> CSS, which will make the CSS harder to maintain.
> 
> This ensures that the highlighting always takes precendence over the table row colors.
> I found that this was not always reliable during the tests.
> 
> I can easily fix this by using modern css selectors like nth-child. This would also simplify the Perl code, because I can do the odd-even row colors in CSS as well.
> But this might break compatibility with legacy browsers. What is your opinion on this?
> Should this be included the first general clean-up patch?
> 
>>> }
>>> 
>>> 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);
>>> + }
>>> + }
>> Why is this function part of the other function?
> Because it is specific to this highlighting function. I want to avoid cluttering the global namespace with auxiliary functions.
>>> +
>>> + //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
>>> ;
>> Could you put this in a separate file (zoneconf.js) please? It's hard to read here and we should try to at least avoid introducing new mixing of languages (Perl and js). Also, we can use formatter and linter of this separate file easier. This should apply to the CSS as well, but that is not your mistake and therefore not part of the patch.
> Where should I put this? Would html/include/zoneconf.js be the right place? Are there any licensing headers I need to add?
>>> @@ -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";
>> @Micheal or somebody else, do we have a function to print lines
>> intended like in the network code of ipfire 3.x?
>> Something like print_indented <number of tabs> <string>
> That would be great! I know this isn't really important, but creating clean HTML makes it much easier for me to check my work.
>>> 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>
>> Should be not part of this patch, but does not matter much. You can
>> use 'git add -i' to pick those changes and but them in a separate patch
>> like "cleaning up code".
>>> <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
>> This is very hard to read, could we split this up please?
> I'll split it!
>> }
>> }
>> @@ -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
>> ;
>> }
>> Same here, when you only change the indention it is hard to see the
>> actual change of the javascript.
>> 
>> To sum things up, please do not get upset about my comments. It might be sometimes annoying, but doing things in a high-quality way makes our work in a few years when we might have a look at this again easier.
>> Greetings Jonatan
>> - print "</tr>";
>> + print "\t</tr>\n";
>> 
>> if ($slightlygrey) {
>> $slightlygrey = "";
>> --
>> 2.27.0.windows.1

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.

[Michael Tremer]

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

Hello,

> On 13 Nov 2020, at 09:23, Leo Hofmann <hofmann(a)leo-andres.de> wrote:
> 
> Good morning,
> 
> thanks again for the encouragement and feedback! I think that I have implemented all suggestions and I am almost ready to submit again.

Great. Feel free to submit that :)

> I moved the JavaScript to html/include/zoneconf.js. Is there a file list to which I have to add this file? So that it is included in the build process.

Yes, that would go into html/html/… and has to be added to config/rootfiles/common/web-user-interface.

If that sounds complicated, just submit the patch and I can take care of it.

Best,
-Michael

> 
> Regards
> Leo
> 
> 
> Am 11.11.2020 um 18:01 schrieb Michael Tremer:
>> Hi,
>> 
>> 
>>> On 11 Nov 2020, at 16:44, Leo Hofmann <hofmann(a)leo-andres.de>
>>>  wrote:
>>> 
>>> Hi Jonatan,
>>> 
>>> thanks for your feedback. I'm not very familiar with your workflow yet, I hope you will excuse my mistakes.
>>> 
>> Absolutely. This is just to let you know what was different than expected.
>> 
>> Also feel free to ask any questions whenever you are unsure :)
>> 
>> 
>>> I guess I'll have to submit this patch a second time as well. If you don't mind, could you please comment on my idea for the second attempt:
>>> 
>>> Patch 1: Only fix HTML output e.g. missing "<", indentation and so on.
>>> I remember Michael suggested this in the past, but since it only affected a few lines this time, I thought it was okay.
>>> 
>> I do this sometimes, too. A few probably can never be avoided.
>> 
>> I wouldn’t have complained about this as long as this is in or close to the lines that are being changed.
>> 
>> 
>>> I just noticed another issue with the original zoneconf source. Some HTML elements are created with whitespace in their id tag, which isn't allowed by HTML standard.
>>> Should I fix this in the first patch as well, or would you like this to be another patch?
>>> 
>> Please do this in another patch. That allows us to patch this very quickly because it is very easy to review.
>> 
>> 
>>> Patch 2 (or 3?): Add highlighting feature
>>> 
>>> This patch would rely on the changes introduced by the first patch. What's the best way to make sure they don't get separated?
>>> 
>> Just send them in one go.
>> 
>> If you are using “git send-email” it will automatically generate a patchset which then looks like this:
>> 
>>   
>> https://patchwork.ipfire.org/project/ipfire/list/?series=1600
>> 
>> 
>> 
>>> I will answer your comments below! I have also added a few specific questions.
>>> 
>>> Regards
>>> Leo
>>> 
>> -Michael
>> 
>> 
>>> Am 11.11.2020 um 12:55 schrieb Jonatan Schlag:
>>> 
>>>> Hi,
>>>> 
>>>> thirst thank you for working on this so fast. I will comment on this
>>>> patch inline, but first one note on the commit message.
>>>> 
>>>> The commit message should be split up. When you use
>>>> 'git commit -sv' an editor will open and in this editor, the first line
>>>> makes up the subject of the mail the lines after the second makes up
>>>> the commit message. The first line should not be longer than 50
>>>> characters to avoid, a subject like in this E-Mail.
>>>> 
>>>> For further information on a good commit message see:
>>>> 
>>>> https://blog.ipfire.org/post/how-to-write-a-good-commit-message
>>>> 
>>>> 
>>>> 
>>>> "Leo-Andres Hofmann" 
>>>> hofmann(a)leo-andres.de
>>>>  – 10. November 2020 13:37
>>>> 
>>>>> Discussion: 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;
>>>>> 
>>>> Why do we need !important here? When it does not work without we
>>>> should, investigate why. Using important break the specificity of the
>>>> CSS, which will make the CSS harder to maintain.
>>>> 
>>> This ensures that the highlighting always takes precendence over the table row colors.
>>> I found that this was not always reliable during the tests.
>>> 
>>> I can easily fix this by using modern css selectors like nth-child. This would also simplify the Perl code, because I can do the odd-even row colors in CSS as well.
>>> But this might break compatibility with legacy browsers. What is your opinion on this?
>>> Should this be included the first general clean-up patch?
>>> 
>>> 
>>>>> }
>>>>> 
>>>>> 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);
>>>>> + }
>>>>> + }
>>>>> 
>>>> Why is this function part of the other function?
>>>> 
>>> Because it is specific to this highlighting function. I want to avoid cluttering the global namespace with auxiliary functions.
>>> 
>>>>> +
>>>>> + //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
>>>>> ;
>>>>> 
>>>> Could you put this in a separate file (zoneconf.js) please? It's hard to read here and we should try to at least avoid introducing new mixing of languages (Perl and js). Also, we can use formatter and linter of this separate file easier. This should apply to the CSS as well, but that is not your mistake and therefore not part of the patch.
>>>> 
>>> Where should I put this? Would html/include/zoneconf.js be the right place? Are there any licensing headers I need to add?
>>> 
>>>>> @@ -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";
>>>>> 
>>>> @Micheal or somebody else, do we have a function to print lines
>>>> intended like in the network code of ipfire 3.x?
>>>> Something like print_indented <number of tabs> <string>
>>>> 
>>> That would be great! I know this isn't really important, but creating clean HTML makes it much easier for me to check my work.
>>> 
>>>>> 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>
>>>>> 
>>>> Should be not part of this patch, but does not matter much. You can
>>>> use 'git add -i' to pick those changes and but them in a separate patch
>>>> like "cleaning up code".
>>>> 
>>>>> <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
>>>>> 
>>>> This is very hard to read, could we split this up please?
>>>> 
>>> I'll split it!
>>> 
>>>> }
>>>> }
>>>> @@ -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
>>>> ;
>>>> }
>>>> Same here, when you only change the indention it is hard to see the
>>>> actual change of the javascript.
>>>> 
>>>> To sum things up, please do not get upset about my comments. It might be sometimes annoying, but doing things in a high-quality way makes our work in a few years when we might have a look at this again easier.
>>>> Greetings Jonatan
>>>> - print "</tr>";
>>>> + print "\t</tr>\n";
>>>> 
>>>> if ($slightlygrey) {
>>>> $slightlygrey = "";
>>>> --
>>>> 2.27.0.windows.1
>>>> 

[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.

[Leo-Andres Hofmann]

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

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 = "";
-- 
2.27.0.windows.1

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]

[-- 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 = "";