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

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

Open
mkrecek234 opened this issue Jun 9, 2023 · 15 comments
Open
Labels

Comments

@mkrecek234
Copy link
Contributor

mkrecek234 commented Jun 9, 2023

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
Copy link
Member

mvorisek commented Jun 9, 2023

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.

@mvorisek mvorisek changed the title CSS issue in develop with tabs and menus Clickable tabs and menus are missing "hand" mouse cursor Jun 9, 2023
@mkrecek234
Copy link
Contributor Author

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
Copy link
Member

mvorisek commented Jun 9, 2023

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

  • by middle button
  • or openable by a click + ctrl (should open a new browser tab/window)
  • or right click and browser context menu "Open Link in a New Tab"

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 dc68adc PR/commit.

@mvorisek mvorisek changed the title Clickable tabs and menus are missing "hand" mouse cursor Clickable tabs and menus are missing "pointer" mouse cursor Jun 11, 2023
@mvorisek
Copy link
Member

mvorisek commented Jun 11, 2023

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
Copy link

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
Copy link
Member

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
Copy link

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 mkrecek234 changed the title Clickable tabs and menus are missing "pointer" mouse cursor Clickable tabs, menus and labels are missing "pointer" mouse cursor Jun 23, 2023
@mkrecek234
Copy link
Contributor Author

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

@mvorisek mvorisek removed their assignment Nov 3, 2023
@mkrecek234
Copy link
Contributor Author

$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:

@mvorisek
Copy link
Member

mvorisek commented Dec 5, 2023

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
Copy link
Contributor Author

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
Copy link
Member

mvorisek commented Dec 5, 2023

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
Copy link
Contributor Author

mkrecek234 commented Dec 5, 2023

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
Copy link
Contributor Author

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
Copy link
Member

mvorisek commented Mar 20, 2024

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 b4708ad#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants