Skip to content

[4.0] Admin menu redundant link#30696

Merged
infograf768 merged 3 commits intojoomla:4.0-devfrom
brianteeman:rh3
Sep 21, 2020
Merged

[4.0] Admin menu redundant link#30696
infograf768 merged 3 commits intojoomla:4.0-devfrom
brianteeman:rh3

Conversation

@brianteeman
Copy link
Contributor

When adjacent links go to the same location this results in additional navigation and repetition for keyboard and screen reader users. WCAG 2.4.4

The parent menu item for Smart Search is the same as the first child menu item.

Although the first one is disabled by the js and only acts as means to expand the submenu its still a problem for some assistive technologies.

This PR follows the pattern of all other core components and changes the parent link to index.php?option=com_finder

This can be tested either by performing a clean installation or by manually applying the sql in the update script.

Note that "database fix" will not apply the change in the update script as it is a content change not a structural change.

before

image

after

image

When adjacent links go to the same location this results in additional navigation and repetition for keyboard and screen reader users. WCAG 2.4.4

The parent menu item for Smart Search is the same as the first child  menu item.

Although the first one is disabled by the js and only acts as means to expand the submenu its still a problem for some assistive technologies.

This PR follows the pattern of all other core components and changes the parent link to index.php?option=com_finder

This can be tested either by performing a clean installation or by manually applying the sql in the update script.

Note that "database fix" will not apply the change in the update script as it is a content change not a structural change.
@ghost
Copy link

ghost commented Sep 20, 2020

I have tested this item ✅ successfully on 2e49b38


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30696.

@infograf768
Copy link
Member

I am not sure about that
UPDATE #__menuSETlink='index.php?option=com_finder' WHERE id=13;

default com_finder menu in 3.x is not at id 13 but 18, which may mean that an update would chnage com_newsfeeds link instead of com_finder, if I do not mistake.

@brianteeman
Copy link
Contributor Author

That is a different issue if correct

@richard67
Copy link
Member

richard67 commented Sep 20, 2020

No, it is not a different issue. You have to change the WHERE clause in your update SQL script to not use the ID, except if you can be sure that on any installation with any long update history it always has the same ID. This can not be granted, and so the golden rule is to never use the ID to identify a menu item for an update with an SQL update script.

If you want I can advise with a proper WHEREclause.

@richard67
Copy link
Member

I suggest to use the following where clause:

WHERE `menutype`='main' AND `title`='com_finder' AND `link`='index.php?option=com_finder&view=index'

@richard67
Copy link
Member

For example if someone had a Joomla 1.5 or so installation and updated it to 2.5, then the menuitem ID would be 21, see update SQL 2.5.0-2011-12-22.sql in the Joomla_2.5.28-Stable-Full_Package.zip, and it will still be 21 now after the years and all updates.

@alikon
Copy link
Contributor

alikon commented Sep 20, 2020

it seems more correct imho, the only left question is about the alternative admin menus that can be configured .....

@richard67
Copy link
Member

richard67 commented Sep 20, 2020

it seems more correct imho, the only left question is about the alternative admin menus that can be configured .....

@alikon Those will not have menutype='main'.

And no, it is not more correct, it is just correct, because using the ID as criteria in the WHERE clause is DEFINITELY wrong, see my previous comment about ID of that menu item being 21 when coming from pre-2.5 in the update history, and as @infograf768 wrote it is 18 on a new installed staging.

@alikon
Copy link
Contributor

alikon commented Sep 20, 2020

sorry my bad .... i've not checked deep that alternative admin menu have a proper different menutype

@brianteeman
Copy link
Contributor Author

Happy to update - you should note that the code was copied from
administrator\components\com_admin\sql\updates\mysql\4.0.0-2019-05-05.sql

brianteeman and others added 2 commits September 20, 2020 11:09
…0-09-19.sql

Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
…0-2020-09-19.sql

Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
@richard67
Copy link
Member

you should note that the code was copied from
administrator\components\com_admin\sql\updates\mysql\4.0.0-2019-05-05.sql

@brianteeman OMG!!! I have to check and if possible fix that! Thank you for that information,

@alikon
Copy link
Contributor

alikon commented Sep 20, 2020

oh shit ..... i need to ask to myself where i was when that s..... was proposed/merged

@richard67
Copy link
Member

@alikon Initially it was that one: #24801 . But let's continue that discussion somewhere else in order not to pollute this PR with off-topic. I will check and if necessary make an issue or PR.

@alikon
Copy link
Contributor

alikon commented Sep 20, 2020

sure right......ping me if you need help/tests on that matter

@alikon
Copy link
Contributor

alikon commented Sep 20, 2020

I have tested this item ✅ successfully on f579ff9


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30696.

@richard67
Copy link
Member

richard67 commented Sep 20, 2020

@alikon Did you do a real test? Or just code review? Am preparing real test, but it takes time. It needs to test new installation with this PR applied and also update SQL statements test on a 4.0-dev for both MySQL and PostgreSQL.

@alikon
Copy link
Contributor

alikon commented Sep 20, 2020

i've made a quick test on my current staging 4.0-dev.... probably this is not enough....well i'll remove my test

@alikon
Copy link
Contributor

alikon commented Sep 20, 2020

I have not tested this item.

too much quick test


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30696.

@richard67
Copy link
Member

@alikon No, it's ok. I'll do the complete test. From you it's ok with 1 real test and a code review.

@richard67
Copy link
Member

@alikon In your case, as you are an SQL expert with both MySQL and PostgreSQL, a code review could be sufficient ;-) Just wanted to know how you had tested, that's why I had asked. If you had said you have done all real tests I could have decided to be lazy and do the code review only ;-)

@richard67
Copy link
Member

I have tested this item ✅ successfully on f579ff9

Tested

@infograf768
Copy link
Member

I have tested this item ✅ successfully on f579ff9


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30696.

@infograf768
Copy link
Member

rtc
happy i could help...


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30696.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Sep 20, 2020
@infograf768 infograf768 added this to the Joomla 4.0 milestone Sep 20, 2020
@infograf768 infograf768 merged commit 730b123 into joomla:4.0-dev Sep 21, 2020
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Sep 21, 2020
@infograf768
Copy link
Member

Tks

@brianteeman
Copy link
Contributor Author

thanks

@brianteeman brianteeman deleted the rh3 branch September 21, 2020 14:15
sakiss pushed a commit to sakiss/joomla-cms that referenced this pull request Oct 16, 2020
* [4.0] Admin menu redundant link

When adjacent links go to the same location this results in additional navigation and repetition for keyboard and screen reader users. WCAG 2.4.4

The parent menu item for Smart Search is the same as the first child  menu item.

Although the first one is disabled by the js and only acts as means to expand the submenu its still a problem for some assistive technologies.

This PR follows the pattern of all other core components and changes the parent link to index.php?option=com_finder

This can be tested either by performing a clean installation or by manually applying the sql in the update script.

Note that "database fix" will not apply the change in the update script as it is a content change not a structural change.

* Update administrator/components/com_admin/sql/updates/mysql/4.0.0-2020-09-19.sql

Co-authored-by: Richard Fath <richard67@users.noreply.github.com>

* Update administrator/components/com_admin/sql/updates/postgresql/4.0.0-2020-09-19.sql

Co-authored-by: Richard Fath <richard67@users.noreply.github.com>

Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants