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.0] Improve a11y for systeminformation #21724

Merged
merged 22 commits into from
Dec 18, 2018

Conversation

chmst
Copy link
Contributor

@chmst chmst commented Aug 19, 2018

JAT has found several a11y issues in the SystemInformation View:

This PR replaces #21628 which I have deleted - my mistake. Could someone please close the old PR? Sorry for inconvenience.

Summary of Changes

Remove the < form > tag, as this is not a form.
Remove empty Table Footers
Replace the < fieldset > Tag by < div >
Remove the < legend > by Table < caption >
Improve the Table header with some css, add a new scss file for com_admin

th {
font-weight: bold;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line contains trailing whitespace

// com-admin

.sysinfo {
.table {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line should be indented with spaces, not tabs

@chmst chmst changed the title Replace PR 21628 - improve a11y for systeminformation [4.0] Improve a11y for systeminformation Aug 19, 2018
@ghost
Copy link

ghost commented Aug 19, 2018

#21628 is closed as requested.

@brianteeman
Copy link
Contributor

Looks good - can you fix hound please

@wojsmol
Copy link
Contributor

wojsmol commented Aug 19, 2018

Additionally we have a merge conflict in this files:

administrator/templates/atum/css/template-rtl.min.css
administrator/templates/atum/css/template.min.css 

font-weight: bold;
}
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Files should end with a trailing newline

@chmst
Copy link
Contributor Author

chmst commented Aug 20, 2018

@wojsmol can you help me? I don't know how to resolve conflicts in .min.css files.

@wojsmol
Copy link
Contributor

wojsmol commented Aug 20, 2018

@chmst see chmst#1

@wojsmol
Copy link
Contributor

wojsmol commented Aug 20, 2018

@chmst I send PR reverting my last changes.

@chmst
Copy link
Contributor Author

chmst commented Aug 20, 2018

All right. I don't understand the conflicts, because the .css and min.css files should be generated from the scss files.

@wojsmol
Copy link
Contributor

wojsmol commented Aug 20, 2018

accept my last PR then I prepare good one.

@chmst
Copy link
Contributor Author

chmst commented Aug 20, 2018

It is merged. Is there something else I have to do?

@wojsmol
Copy link
Contributor

wojsmol commented Aug 20, 2018

chmst#3

@joomla-cms-bot joomla-cms-bot added the Language Change This is for Translators label Aug 22, 2018
$focus-highlight-color: #39f;
$focus-highlight-color: #39f;

// Breadcrumbs

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line contains trailing whitespace

@joomla-cms-bot joomla-cms-bot removed the Language Change This is for Translators label Sep 4, 2018
@chmst
Copy link
Contributor Author

chmst commented Sep 4, 2018

@wojsmol hope that the conflict is resolved now. Thank you for your patience

@baernm
Copy link

baernm commented Sep 8, 2018

I have tested this item ✅ successfully on cd203ad


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

@wojsmol
Copy link
Contributor

wojsmol commented Sep 8, 2018

@chmst Please remove administrator/components/com_media/package-lock.json from this PR.

@laoneo
Copy link
Member

laoneo commented Oct 18, 2018

@brianteeman all ok here from your side?

@brianteeman
Copy link
Contributor

@laoneo it was good when I first checked it - not sure if there have been significant changes since

@laoneo
Copy link
Member

laoneo commented Oct 18, 2018

Do you mind to have another look?

@chmst
Copy link
Contributor Author

chmst commented Oct 18, 2018

Wait a moment. There is another PR which has changed something in the library. I can resolve this as soon as the other change is merged.

@brianteeman
Copy link
Contributor

@laoneo I will put it on my todo for tonight

@carcam
Copy link

carcam commented Nov 23, 2018

I have tested this item ✅ successfully on cf368e5

I have tested this change in Joomla 4 Alpha 5 release succesfully. There are no visible differences in the View System Information before and after patch.

I have also tested with Wave Automatic Test and I got the error of an empty th which belongs to the php info about mysqlnd statistics which was empty in my server but I do not think it's related with this PR.


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

@chmst
Copy link
Contributor Author

chmst commented Nov 23, 2018

@carcam thank you for testing. And good finding of the empty in the php-info. Let's make another PR for the sysinfo model, to avoid empty table headers.

@Quy
Copy link
Contributor

Quy commented Nov 23, 2018

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Nov 23, 2018
@zwiastunsw
Copy link
Contributor

I tested this patch. Fieldset and legend elements have been rightly removed. But...
The "caption" element should be used in the table instead of the legend element. Without the caption element, users of screen readers will not get the information contained in the table.

@laoneo laoneo added this to the Joomla 4.0 milestone Nov 24, 2018
@wilsonge
Copy link
Contributor

I'm going to merge this as a good piece of progress even though it would appear there's some more to do here from the comments by @zwiastunsw

@wilsonge wilsonge merged commit 6918943 into joomla:4.0-dev Dec 18, 2018
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Dec 18, 2018
@chmst
Copy link
Contributor Author

chmst commented Dec 19, 2018

@wilsonge thank you! So I can continue without raising conflicts ;)

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.