atk4 / ui

Robust and easy to use PHP Framework for Web Apps
https://atk4-ui.readthedocs.io
MIT License
440 stars 104 forks source link

Clickable tabs, menus and labels are missing "pointer" mouse cursor #2068

Open mkrecek234 opened 1 year ago

mkrecek234 commented 1 year ago

Expected behaviour: A mouse cursor indicating that it is clickable/link + hover effect for menu items. See ui.agiletoolkit.org (release version) where the issue is not present.

Found in recent develop

mvorisek commented 1 year ago

Thank you for the report. This is exactly what I was unifying in the past month and something slipped...

In 4.0 https://ui.agiletoolkit.org/demos/interactive/tabs.php the tabs are clickable, but are not links, ie. cannot be opened in a new window... In this is question if they should be, as they can be part of a modal for example, which cannot be opened itself in a new window.

In 4.0 the tab tags are a, now they are div. In 4.0, the click/hover event is cancelled, thus the div seems to be fine/better (like in Button for example), but the mouse cursor needs to be of course fixed.

mkrecek234 commented 1 year ago

Opening in a new window would only make sense if - whenever the user selects a tab - it would automatically change the browser's URL adding a parameter of the currently active tab. As we do not have this, I would not make it possible to open in new window.

The latter feature I will put in a separate issue/proposal.

mvorisek commented 1 year ago

This is technically already possible with VP/Callback, the problem is the tab is not clickable:

If you open the 4.0 demo, you will find there was a tag, but none of these action was possible. This is why a a tag was not right.

I am closing #2070 in favor of this issue for these reasons.

This issue/mouse cursor is present since https://github.com/atk4/ui/commit/dc68adca624d07a551de94061f12d5088e7f1e0b PR/commit.

mvorisek commented 1 year ago

Hi @lubber-de, looking into the Fomantic-UI Tabs demos - some demos like https://fomantic-ui.com/modules/tab.html#centered-item - have the same problem.

IMHO a tag is not semantically the most correct when the tab has no public link/anchor like when in a temporary modal for example.

Is a tag required to be used for tab or can/should this be fixed officially?

I tried to add ui button class to the tab which fixes the cursor problem, but button adds an extra margin visually...

Adding a link to the menu/tab items fixes the cursor as it matches then .ui.menu .link.item:hover selector. Is that the recommended solution or should the selector be extended/simplified to .ui.menu .item:hover officially?

lubber-de commented 1 year ago

Yes, using either one of the following selectors will show the pointer and is intended in SUI since 2014 according to git log

.ui.link.menu .item:hover,
.ui.menu .dropdown.item:hover,
.ui.menu .link.item:hover,
.ui.menu a.item:hover

https://github.com/fomantic/Fomantic-UI/blob/41577bcbfff653772c8a9bacdefc800aaf9d785a/src/definitions/collections/menu.less#L421

mvorisek commented 1 year ago

Yes, I am aware of the current styling selector(s) but it seems inconsistent to me because the module adds a handler for the click:

https://github.com/fomantic/Fomantic-UI/blob/8c329bab785d3d59dc2f884e99ef3d36c5abe125/src/definitions/modules/tab.js#L214

on

https://github.com/fomantic/Fomantic-UI/blob/8c329bab785d3d59dc2f884e99ef3d36c5abe125/src/definitions/modules/tab.js#L144

and the styling, ie. cursor mouse icon etc., should match the JS selectors.


We should also probably use childrenOnly: true and test nested tabs using Behat.


https://dev.agiletoolkit.org/demos/collection/grid.php - the menu item (button) click events are registered by us, thus if .ui.menu .item:hover selector is not wanted, we have to probably deal with the cursor icon by ourself.

lubber-de commented 1 year ago

The .tab() is initiated on a custom menu item selector (usually .ui.tabular.menu .item) Of course we could automatically check if each of the given items is either an a tag or has the link class or it's parent has got the link class (as in link menu)

But, you don't necessary have to use tabular menu items as selector trigger, it can be anything, so the intention is to not modify any of the menu items (or lets say the "tab triggers") inside of the tab module.

So, if you want to use a div as menu item add the link class to either each div or the parent menu. Also beware that a (tabular) menu could possibly contain items which are not supposed to be clicked (like a header, so always adding the pointer cursor to menu items is also not intended.

mkrecek234 commented 8 months ago

@mvorisek Any news on this topic? Usability is suffering if the hover effect is no longer working on those menus.

mkrecek234 commented 7 months ago

$this->addClass('link') in the MenuItem class would do it for me as a quick fix solution - I think it is a fair assumption that any MenuItem is linked and as such should trigger the pointer cursor and a hover effect: https://github.com/atk4/ui/blob/33ec62ca0e3ad8d331657867aa8e9c8d24b2dc34/src/MenuItem.php#L25

mvorisek commented 7 months ago

Please understand that Tabs have the same clickable issue. I won't approve any change if they will not address this for everything. Otherwise we will have mess of some parts working, some parts not, some parts fixes this way, some other.

⚠ It probably needs some fixes in FUI, must work with disabled items, must work middle button/right click (to open in a new tab) etc. So first, the problem must be perfectly understood/debugged on almost all possible usecases with some real demos. Then fix FUI and atk4/ui demos thru a PR with discussion summarizing all your findings.

mkrecek234 commented 7 months ago

With my proposed fix, also tabs are working properly with a hand pointer then without issue. The right-click issue for tab is another issue: This PR only addresses the UI bug that clickable menu items do not show a hand pointer and no longer have a hover effect as before. Please specify which parts would then not be working, as in my tests I cannot see this.

mvorisek commented 7 months ago

Sorry, I have no time to explain this again an again. To fix something, you need to understand and fix all usecases, There are like 20 different usecases. Menus, buttons, tabs, accordion ... And of them can be disabled etc. This is one topic that must be fixed at one in order to keep the code and UX consistency. Is that clear?

mkrecek234 commented 7 months ago

Sorry but I do not understand what you mean: (active or disabled) Tabs, (active or disabled) MenuItems, Accordions, Disabled Buttons ALL behave correctly with this fix showing a proper mouse hand pointer (if active) then, and where available also activating the hover effect. I cannot see any issue arising from this fix, but it solves the UI bug. Please let me know which unwanted side-effect this fix would imply other than solving the UI issue. I'd be happy to have a better proposal if this does not suit your needs to fix the UI problem.

BTW this was your own proposal you asked @lubber-de to fix the issue: "Adding a link to the menu/tab items fixes the cursor as it matches then .ui.menu .link.item:hover selector. Is that the recommended solution or should the selector be extended/simplified to .ui.menu .item:hover officially?" And @lubber-de also proposed exactly that: "So, if you want to use a div as menu item add the link class to either each div or the parent menu."

mkrecek234 commented 7 months ago

FYI The "right-click/middle click" topic is covered in issue #2070 and should be treated there - the right-click topic is far more complex, as this would also involve more situations eventually, not just Accordions, Tabs but for example also Table onRowClick events. I consider #2070 a "nice-to-have" but not critical, whereas this UI issue is in fact completely against FUI or web app UX standards if not fixed.

mvorisek commented 3 months ago

Have a look at https://github.com/atk4/ui/tree/fix_2068_mouse_clickable_pointer. The "link" class is not a general solution because for at least this two reasons:

  1. in https://github.com/atk4/ui/commit/b4708ad4fa92a3e3eaa00c0717303e1aa7da05a8#diff-eeeb7a49fbda4a38e5f79f764164efbc5b268bfa9c505c27c4c9fd39de2b22b9L118 the a tag name cannot be removed, as div.ui.item.link class itself is not enough - figure how to fix it, probably some extra class is needed or the parent element needs another class
  2. what guarantees the MenuItem class is really clickable? The "link" class solution can easily leak to a situation where the div is shown as "clickable", but in reality, it is not.