Fix click event bubbling in Table::onRowClick() II.

[mkrecek234]

fixes #1858

related original #1655 PR

[mvorisek]

I do not belive this PR fixes the problem correctly, what will happen if you will add checkbox somewhere instead of a button? Why to keep checkbox and remove dropdown?

also, how can this be the most easily reproduced on demos/_unit-test/grid-rowclick.php demo?

[mkrecek234]

I do not belive this PR fixes the problem correctly, what will happen if you will add checkbox somewhere instead of a button? Why to keep checkbox and remove dropdown?

also, how can this be the most easily reproduced on demos/_unit-test/grid-rowclick.php demo?

I tested all of that in live settings - checkbox works perfectly - so do href links. The action buttons with this code here also work without issues, whereas in the current develop they don't work. Let's please merge this. You can easily reproduce if you add action buttons to your repro code while also adding onRowClick.

[mvorisek]

I am not objecting the issue, but missing test and I am really worried why .ui.checkbox is ok, but .ui.dropdown not.

[mkrecek234]

I am not objecting the issue, but missing test and I am really worried why .ui.checkbox is ok, but .ui.dropdown not.

The dropdown event logic seems to be different and I have not really grasped it - if you stop propagating on .dropdown or .atk-menu-actions or the like, the actions dropdown menu items won't work. If you don't do it, they work, and so does clicking on the down-arrow. However, if you click to the "Actions..." text left of the arrow, it will fire also the onRowClick, Same happens if you add the td.collapsing. Looks as if the event logic for that is run a bit different than usual. Let's please merge this for now, as today we are facing a more severe bug. We can keep the issue open though.

[mvorisek]

Michael, in #1655 I have put a lot of efford to cover the original issue with a test. The only positive side of your PR now is it does not break the tests, but the unanswered questions here are major and will probably lead to another issue.

What needs to be done is to reproduce the new issue with a test. Otherwise the problem can be reintroduced very soon.

The currently proposed solution seems "coded to satisfy tests and my usecase". If such hacky solution is needed, it will need a test for every kept/removed selector/UI component. I doubt so, if we cannot find a good solution quickly, we can revert #1655 and recreate the original PR with a test for this issue.

[mkrecek234]

This is why I suggest to adjust to the original proposed which did not include dropdown and button. I invested some time to get JS events for the menu item but have not found a solution. As this is a minor issue and we have many more critical things to address we should not try to rework the whole action button JS side now. We can safely keep support for links and checkout though, perfectly working.

[mkrecek234]

Suggestion - the original issue was for Checkbox and Link, and also the original PR - both issues were solved successfully.
You provided tests for those uses cases, so PR should be complete and merged that way.

Since you added two more selectors (button and dropdown) which were untested and which actually created a problem which was not present before, we should keep this in a separate issue open and address it separately. The JS logic as said before for menu items in Table is different (and addresses to the most part non-bubbling events) and we can look at it separately.

[mvorisek]

Suggestion - the original issue was for Checkbox and Link, and also the original PR - both issues were solved successfully. You provided tests for those uses cases, so PR should be complete and merged that way.

Yes, having the tests has shown the full behaviour and if the problem is impossible to solve, with a little documentation in test & GH issue, it can be merged. But, I do not think it is impossible :)

In

ui/src/Table.php

Line 518 in e105350

$this->on('click', 'tbody>tr', $action);

we can probably exclude the unwanted events with https://stackoverflow.com/questions/5420399/determine-the-elements-which-an-event-has-bubbled-through no need to even add the handlers to excluded the problematic selectors.

I am also quite curious why the menu/dropdown button is allowed to bubble in the first place, as when any event is added

ui/src/View.php

Lines 999 to 1000 in a036f67

	$eventStatements['preventDefault'] = $defaults['preventDefault'] ?? true;
	$eventStatements['stopPropagation'] = $defaults['stopPropagation'] ?? true;

it should prevent it to bubble. Maybe no our event is added (only SUI adds it).