Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4] Strange module selection. #30798

Closed
ReLater opened this issue Sep 28, 2020 · 33 comments
Closed

[4] Strange module selection. #30798

ReLater opened this issue Sep 28, 2020 · 33 comments

Comments

@ReLater
Copy link
Contributor

ReLater commented Sep 28, 2020

Steps to reproduce the issue

  • Maybe issue is not related to J4 core but I don't know for sure.

  • Updated a J 3.9.21 stepwise to Joomla 4 (current nightly).

  • Deactivate a module extension on Extensions > Manage where a module exists on Site Modules (image 000).

  • Go to Site Modules and select some modules.

  • I can select modules without issues one by one that are ordered in the list before "Newsletter".

  • See images 001 to 003 for modules that are ordered after module "Newsletter".

  • Green arrow: Here I clicked (select and deselect).

  • Orange arrow: The wrong selection afterwards.

image 000

000

image 001

001

image 002

002
002

image 003

003

System information (as much as possible)

System Information
Setting Value
PHP Built On Windows NT DELL-M6800 10.0 build 19041 (Windows 10) AMD64
Database Type mysql
Database Version 10.4.11-MariaDB
Database Collation utf8mb4_general_ci
Database Connection Collation utf8mb4_general_ci
Database Connection Encryption None
Database Server Supports Connection Encryption No
PHP Version 7.3.16
Web Server Apache/2.4.41 (Win64) OpenSSL/1.1.1c PHP/7.3.16
WebServer to PHP Interface apache2handler
Joomla! Version Joomla! 4.0.0-beta5-dev Development [ Mañana ] 15-September-2020 19:15 GMT
User Agent Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/85.0.4183.121 Safari/537.36 Edg/85.0.564.63

@ReLater
Copy link
Contributor Author

ReLater commented Oct 4, 2020

I have tested it now with a pure online nightly Joomla 4 and it has exactly the same issue.

  • Create modules in Position "breadcrumbs" or so if you have no demo data installed.
  • Create a modul "Articles Category".
  • Go to "Extensions manager" and deactivate "Articles Category" extension.
  • Go back to modules manager.
  • Fiddle like described in previous [4] Strange module selection. #30798 (comment)

04-10-_2020_05-44-50

Additional find: One can't reorder the deactivated module with Drag&Drop.

@Quy
Copy link
Contributor

Quy commented Oct 28, 2020

@dgrammatiko Can you please advise changes to the following code to only add the click event to rows containing a checkbox input? Thank you!

  class JMultiSelect {
    constructor(formElement) {
      this.tableEl = document.querySelector(formElement);

      if (this.tableEl) {
        this.boxes = [].slice.call(this.tableEl.querySelectorAll('input[type=checkbox]'));
        this.rows = [].slice.call(document.querySelectorAll('tr[class^="row"]'));
        this.checkallToggle = document.querySelector('[name="checkall-toggle"]');

        this.onCheckallToggleClick = this.onCheckallToggleClick.bind(this);
        this.onRowClick = this.onRowClick.bind(this);

        if (this.checkallToggle) {
          this.checkallToggle.addEventListener('click', this.onCheckallToggleClick);
        }

        if (this.rows.length) {
          this.rows.forEach((row) => {
            row.addEventListener('click', this.onRowClick);
          });
        }
      }
    }
<tr class="row0" data-draggable-group="none">
  <td class="text-center">
  </td>
</tr>
<tr class="row1" data-draggable-group="none">
  <td class="text-center">
   <label for="cb3"><span class="sr-only">Select </span></label><input autocomplete="off" type="checkbox" id="cb3" name="cid[]" value="130" onclick="Joomla.isChecked(this.checked);">
  </td>
</tr>

@dgrammatiko
Copy link
Contributor

@Quy sure, the problem is that for disabled rows the checkbox needs to render with a disabled attribute instead of skipping it. Basically the conditional here

<?php if ($item->enabled > 0) : ?>
<?php echo HTMLHelper::_('grid.id', $i, $item->id); ?>
<?php endif; ?>
is wrong. It always need to render a checkbox but if the row is disabled the checkbox needs to also have a disabled attribute (eg <?php echo HTMLHelper::_('grid.id', $i, $item->id); ?> needs to return always a checkbox but if needed with a disabled attribute)

@Quy
Copy link
Contributor

Quy commented Oct 28, 2020

Added the disabled attribute to checkbox. Clicking the checkbox does not toggle it, however, clicking the row/check all items checkbox toggles the checkbox.

30798

@dgrammatiko
Copy link
Contributor

dgrammatiko commented Oct 28, 2020

you need something like:

      if (this.boxes[currentCheckBox].disabled) {
        return;
      }

@Quy
Copy link
Contributor

Quy commented Oct 28, 2020

Almost there. The disabled checkbox can still be toggled using the Check All Items checkbox at the top. Thank you!

@brianteeman
Copy link
Contributor

This appears to be resolved now as the last issue reported is no longer valid and this should be closed

@ReLater
Copy link
Contributor Author

ReLater commented May 21, 2021

If you say so I believe it ;-)

@ReLater ReLater closed this as completed May 21, 2021
@Quy
Copy link
Contributor

Quy commented May 21, 2021

This is reproducible.

Install Testing Sample Data.
Go to System > Manage > Extensions
Search and disable Wrapper - Site.
Go to System > Manage > Site Modules
Click Wrapper row.
The checkbox below it is checked.

@Quy Quy reopened this May 21, 2021
@ReLater
Copy link
Contributor Author

ReLater commented May 21, 2021

Thanks @Quy for reopening! I have confused this with another selection failure pr (menu assignment)

@brianteeman
Copy link
Contributor

What am I doing wrong then

modules

@dgrammatiko
Copy link
Contributor

@brianteeman try to select the next module after the disabled one

@brianteeman
Copy link
Contributor

ah - it selects the next two. got it now

@infograf768
Copy link
Member

See demo of problem here in the NOTE
#34238 (comment)

@infograf768
Copy link
Member

infograf768 commented May 28, 2021

Why don't we simply do

						<td class="text-center">
							<?php echo HTMLHelper::_('grid.id', $i, $item->id, false, 'cid', 'cb', $item->title); ?>
						</td>

imho, it does not matter if we can tick the disabled item checkbox and use Actions as it does not change its disabled status.

@brianteeman
Copy link
Contributor

thats not a good solution as it provides a negative user experience which was the entire point in the checkbox being removed in the first place.

@bembelimen
Copy link
Contributor

TBH, I'm more surprised when the checkbox is removed at all, because then I have to think and guess and have to make a connection: no checkbox? why? mhh, because it's disabled (says the icon at least).

If there would be an disabled checkbox with information why it's disabled, then I don't have to think myself...

@infograf768
Copy link
Member

thats not a good solution as it provides a negative user experience which was the entire point in the checkbox being removed in the first place.

Certainly less negative than what we have now. The module can anyway be edited although it is disabled. Taking off the checkbox makes no sense.

Concerning the explanation, I guess using a real tooltip as proposed here would do the job.#34238 (comment)

@dgrammatiko
Copy link
Contributor

dgrammatiko commented May 28, 2021

thats not a good solution as it provides a negative user experience which was the entire point in the checkbox being removed in the first place.

Whoever removed the checkbox for the disabled items did a very poor job as they broke at least 3 scripts: drag and drop, multiselect and select all. Can it be done without a checkbox for disabled items? Yes but all 3 scripts need to be adjusted and honestly, it's far easier to bringback the checkbox as I already guided @Quy in my comments above

@bembelimen
Copy link
Contributor

thats not a good solution as it provides a negative user experience which was the entire point in the checkbox being removed in the first place.

Whoever removed the checkbox for the disabled items did a very poor job as they broke at least 3 scripts: drag and drop, multiselect and select all. Can it be done without a checkbox for disabled items? Yes but all 3 scripts need to be adjusted and honestly, it's far easier to bringback the checkbox as I already guided @Quy in my comments above

  • Deletion of this entry is also not possible anymore...

@infograf768
Copy link
Member

The use of <?php if ($item->enabled > 0) : ?> for the checkbox was introduced in J3.5.0
#8020

@chmst
Copy link
Contributor

chmst commented May 28, 2021

If a component is disabled, we remove the link in the menu to this component. No Tooltip, no ghost componetnt - disabled means disabled. Why not hide disabled module instances from the list?

@dgrammatiko
Copy link
Contributor

dgrammatiko commented May 28, 2021

Why not hide disabled module instances from the list?

Actually, I wouldn't expect the list all modules to display only rows for modules that are enabled. In fact, I would expect when a module row indicated that the module is disabled and I enable (publish) that row then the module will be enabled automatically instead of having to go to the extensions to enable the module and come back to publish the row. It seems that we're making things hard for no reason

Also if you hide rows like that in case I uninstall a module I have no way to remove any remaining related data in the database

@chmst
Copy link
Contributor

chmst commented May 28, 2021

I see this: A disabled module is not in the select screen (com_modules&view=select).
Because it is disabled and may not be used - maybe old PHP version or whatever the reason is for the administrator.
If another user could publish it in the same way as a unpublished module in (com_modules&view=modules), this would be contrary to the intention of disabling the module.

@chmst
Copy link
Contributor

chmst commented May 28, 2021

In any case, re-adding the checkbox and disabling it so that no action is possible would at least solve the problem of the missing checkbox

@infograf768
Copy link
Member

In any case, re-adding the checkbox and disabling it so that no action is possible would at least solve the problem of the missing checkbox

Except, as @bembelimen rightfully remarked, that one can't delete the instance of the module concerned via the admin UI.
Very different from not displaying the menu link to the component.

@chmst
Copy link
Contributor

chmst commented May 29, 2021

Just saying: In j3 disabled module items are not shown in the list. It is true that the items remain as ghost in the database.

@infograf768
Copy link
Member

The difference in J3 is that a blocked module can't be disabled.
Therefore the test should be done with a non-blocked module.

@ReLater
Copy link
Contributor Author

ReLater commented May 29, 2021

In j3 disabled module items are not shown in the list.

In my case they are shown:
29-05-_2021_11-18-07

@infograf768
Copy link
Member

Yep. Same for Most Read because it is not blocked.

Screen Shot 2021-05-29 at 11 20 54

@infograf768
Copy link
Member

J3 list of protected modules

Screen Shot 2021-05-29 at 11 23 02

@Fedik
Copy link
Member

Fedik commented May 29, 2021

I have added checkbox, please test #34273

@Fedik
Copy link
Member

Fedik commented May 29, 2021

closing it, because PR

@Fedik Fedik closed this as completed May 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants