? ? Pending

User tests: Successful: Unsuccessful:

avatar brianteeman
brianteeman
3 Oct 2018

Role=alert has specific meaning and specific associated actions with assistive technology.

In all the cases in this PR these are simply on screen messages and should not have the role=alert - as a result there is no need to use the custom element as it is simply css - no js required etc

avatar brianteeman brianteeman - open - 3 Oct 2018
avatar brianteeman brianteeman - change - 3 Oct 2018
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 3 Oct 2018
Category Administration com_associations com_banners com_categories com_config com_contact com_content com_csp com_fields com_finder com_installer com_joomlaupdate
avatar zwiastunsw
zwiastunsw - comment - 3 Oct 2018

This PR requires a case-by-case examination.
I haven't analyzed all cases. I have checked several incidents.
These are messages that appear on the pages dynamically after some user action. The user of assistive technology must be informed about the appearance of these messages. Otherwise, he will not know at all that a message has appeared.
Therefore, they should have a role attribute. The use of custom elements is highly recommended here.

avatar brianteeman
brianteeman - comment - 3 Oct 2018

These are messages that appear on the pages dynamically after some user action.

Please tell me which one as I did check each one and did not see any. Remember that if there is a page load then it is not a dynamic message. There is zero need for a custom element at all for any of these as they are not alerts, they are not dismissable, they are not pop-ups. They are one css class and zero javascripts. They should never have been given the role = alert.

avatar zwiastunsw
zwiastunsw - comment - 3 Oct 2018

I will take over again.
Because, of course, you are right that when a page is reloaded (e.g. after a search), these are not dynamic messages. I absolutely do not question the correctness of your PR.

avatar zwiastunsw
zwiastunsw - comment - 3 Oct 2018

In fact, in all cases these are usually messages. Good work.

avatar zwiastunsw
zwiastunsw - comment - 3 Oct 2018

I have tested this item successfully on 0b46457


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

avatar zwiastunsw zwiastunsw - test_item - 3 Oct 2018 - Tested successfully
avatar brianteeman
brianteeman - comment - 3 Oct 2018

Thanks for testing. It is so important to test the code before commenting on if it is correct.

avatar zwiastunsw
zwiastunsw - comment - 3 Oct 2018

I haven't checked the files before. There were so many of them that I was convinced that you had uploaded all the messages. I'm sorry.

avatar brianteeman brianteeman - change - 3 Oct 2018
Labels Added: ?
avatar Quy
Quy - comment - 3 Oct 2018

I have tested this item successfully on 4b25515


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

avatar Quy Quy - test_item - 3 Oct 2018 - Tested successfully
avatar chmst
chmst - comment - 3 Oct 2018

I have tested this item successfully on 4b25515


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

avatar chmst chmst - test_item - 3 Oct 2018 - Tested successfully
avatar Quy Quy - change - 3 Oct 2018
Status Pending Ready to Commit
avatar Quy
Quy - comment - 3 Oct 2018

RTC


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

avatar laoneo laoneo - change - 3 Oct 2018
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2018-10-03 20:10:52
Closed_By laoneo
Labels Added: ?
avatar laoneo laoneo - close - 3 Oct 2018
avatar laoneo laoneo - merge - 3 Oct 2018
avatar laoneo
laoneo - comment - 3 Oct 2018

Thanks

avatar brianteeman
brianteeman - comment - 3 Oct 2018

Thanks

avatar dgrammatiko
dgrammatiko - comment - 4 Oct 2018

This was totally wrong! You reverted to Bootstrap alerts when the aim was to remove this dependency for alerts!
The correct solution here is to revert this PR and assign the role attribute in the custom element only if there is a dismiss button

avatar brianteeman
brianteeman - comment - 4 Oct 2018

no - I am not using bootstrap alerts just css - there is no dependency at all. And even your suggestion is wrong. An alert should not use a dismiss button or require any keyboard interaction. And finally not one of the messages touched in this PR should have a role=alert. Sorry you didnt understand this basic accessibility. https://www.w3.org/TR/wai-aria-1.1/#alert

avatar dgrammatiko
dgrammatiko - comment - 4 Oct 2018

Sorry you're wrong. You're reverting the Bootstrap decoupling

avatar brianteeman
brianteeman - comment - 4 Oct 2018

css is not coupling!!! and all your code is at best failing accessibility and html standards and at worse completely misunderstanding the basics.

all the custom elements need review and extensive changes - right now they are a terrible implementation of what is an exciting new technology. I would rather be coupled with anything than have this implementation.

avatar dgrammatiko
dgrammatiko - comment - 4 Oct 2018

css is not coupling!!!

Yeah sure

all the custom elements need review and extensive changes

I was about to fulfil my promise to fix the custom elements but I guess after the warm welcome that won't happen

By the way either removing the line that sets the role or create a logic for that attribute in the custom element was all it needed here. But you know better, back to Bootstrap...
If you're about to have bootstrap alerts and custom elements alerts you should drop the custom elements all together because the idea was to decouple all the components (meaning also removing the css for them) from the BS.
Obviously you have no clue how to do it correctly and just reverting to J3 codebase

avatar brianteeman
brianteeman - comment - 4 Oct 2018

I was about to fulfil my promise to fix the custom elements but I guess after the warm welcome that won't happen

Maybe you shouldnt keep saying you are not contributing any more then

By the way either removing the line that sets the role or create a logic for that attribute in the custom element was all it needed here. But you know better, back to Bootstrap...

No it wasnt or thats what I would have done. And it is not bootstrap!!! Doesnt matter how many times you say it.

Obviously you have no clue how to do it correctly and just reverting to J3 codebase

pot kettle black

avatar C-Lodder
C-Lodder - comment - 4 Oct 2018

@brianteeman The role is being set here in the custom element: https://github.com/joomla-projects/custom-elements/blob/master/src/js/alert/alert.js#L18

You have indeed reverted back to BS alerts as it's Bootstrap that targets the .alert class. And it's still being imported: https://github.com/joomla/joomla-cms/blob/4.0-dev/administrator/templates/atum/scss/bootstrap.scss#L29

The custom elements target the following:

joomla-alert { ... }

https://github.com/joomla-projects/custom-elements/blob/master/src/scss/alert/alert.scss#L66

avatar brianteeman
brianteeman - comment - 4 Oct 2018

no i have correctly added css markup to a div. If these were bootstrap alerts then they would have the role=alert. These are NOT alerts. There is zero need for a custom element here. But if you want to have 180 lines of javascript code (actually more to add all the conditions to allow the element to be usable) instead of 20 characters of perfectly valid markup then go ahead - bust your balls - apparently I know nothing about anything. At least I know that you cannot put a div inside a span. You cannot focus on an a element if there is no href - no matter how many tabindex you add. If you put aria hidden on a button there is no point adding text inside it for screenreaders. You cannot add focus to an alert. You cannot have outline-none and expect a keyboard user to have a clue where they are. You cannot have a button which is a link and expect it work as a button for keyboard users. A modal must be dismissable using the esc key. A field label must be in the same element as the field input etc etc Do I need to continue explaining again why these custom elements and web components are wrong. Use a keyboard to navigate, use a screenreader to navigate, read the specifications and best practices and finally confirm everything with experts such as the W3C Web Platform WG co-chair,

avatar C-Lodder
C-Lodder - comment - 4 Oct 2018

How do you intend on dismissing the alerts (or whatever you want to call them) then?

Just because the CE's aren't fully accessible, does not make the concept of them wrong. They simply need fixing, not removing

avatar brianteeman
brianteeman - comment - 4 Oct 2018

These are not alerts - they should not be dismissed. Please read the code and the comments of those that tested not the rant of someone who also didnt read the code

why would you want to dismiss this
image

I am NOT saying CE are wrong technology - I am just saying that for the code changed in this PR they are not needed.

avatar dgrammatiko
dgrammatiko - comment - 4 Oct 2018

Just comment out

@import "../../../../media/vendor/bootstrap/scss/alert";
and rebuild the css. Then you'll realise what I and Charlie we try to say here. You are reverting back to bootstrap dependency when the idea behind the Custom Elements was to decouple from that monolithic framework. I will not comment on the other parts, I'll just state that there was a successful audit for each of these elements before merging by the group that was supposed to take care of accessibility. Blaming me for that part, just because I wrote the js part, is really annoying. And you keep doing that crap for couple years now

avatar laoneo
laoneo - comment - 4 Oct 2018

I guess both sides have right. It couples to bootstrap but it is also not an alert. So what do we do? Adding a new element which represents an empty list information box?

avatar mbabker
mbabker - comment - 4 Oct 2018

Being decoupled from a third party CSS framework is one thing. But basically saying "you cannot use <div class="alert"> in the Joomla core layouts because "this is Bootstrap's convention" is going to go nowhere. Who's doing custom elements for my card elements in Install from Web (using Bootstrap's card component and the busted as hell .card-header that has been misused throughout this template)? Who's doing elements to replace <span class="badge">? This whole "decouple Bootstrap" thing and requiring CE for everything to avoid "simple" markup because it's the convention that Bootstrap uses is a prime example of overengineering.

If the direction of the web is that everything has to be JavaScript because "plain" HTML is bad then I don't want to work on web facing code anymore.

avatar brianteeman
brianteeman - comment - 4 Oct 2018

it is not coupled to bootstrap. it uses the same classes that bootstrap uses. That is NOT coupling. It will not break if you remove bootstrap. You just need to have those classes in your own template. Again - please read the code and the items that are touched by this pr before commenting - you make yourselves look like idiots because you are commenting on a mistaken understanding

avatar C-Lodder
C-Lodder - comment - 4 Oct 2018

@mbabker I get what is trying to be achieved here. You basically want a <div> with the same styling as an alert but do not want any JS for interactions. So just a styled box.

If that IS the case, then use the styling from the Custom Element, not bootstrap. Cause right now, you have Custom Element alerts CSS, and Bootstrap alert CSS. They both provide the same result but just use different naming convensions. No point in providing 2 things that do exactly the same thing...right?

Look at bootstrap as a prime example. They have the JS which is for alert interactions, and CSS for the styling. It's separated. You can use both the JS and CSS or just the CSS. Right now you're using BS for non-interactive alerts, and Custom Elements (which use different CSS) for interactive alerts.

@brianteeman
I do not expect you of all people to be calling me an idiot but instead accepting this as a heathly discussion. I have read the code. I understand what you're trying to do here....even if your comments deviate from the origin PR description.

avatar C-Lodder
C-Lodder - comment - 4 Oct 2018

If it's ONLY the role="alert" that is the actual issue here, then (if correct), only add it if the alert/message is an error, else make it an argument.

avatar dgrammatiko
dgrammatiko - comment - 4 Oct 2018

I guess both sides have right. It couples to bootstrap but it is also not an alert

I think you need to re evaluate what the custom elements are actually doing. So they consist of a custom tag (totally valid) some css and some js. In the case of alerts if attribute dismiss is not set then the js is not doing much (only appending a class for the fade in effect and setting the role attribute). If the role attribute is not needed in this case just move https://github.com/joomla-projects/custom-elements/blob/master/src/js/alert/alert.js#L18 to the line before https://github.com/joomla-projects/custom-elements/blob/d266618c35e925db8fcf921c9a7b49d1c32c7cdd/src/js/alert/alert.js#L28. There you have it, problem solved. Everything else in this PR is totally irrelevant and WRONG

avatar dgrammatiko
dgrammatiko - comment - 4 Oct 2018

But basically saying "you cannot use <div class="alert"> in the Joomla core layouts because "this is Bootstrap's convention" is going to go nowhere.

Well, depends on what is considered Bootstrap.css. I thought the idea was to strip the bc css from the components since people agreed to use custom elements. If that's falsie then you will be better off reverting all the custom elements because effectively they're just superfluous bloat.

avatar mbabker
mbabker - comment - 4 Oct 2018

If you're saying that <div class="card"> is Bootstrap and therefore must be a CE or cannot be used by Joomla, then yes, I think that assumption is wrong. Because right now I basically see you complaining anytime markup is introduced or modified that follows the Bootstrap structure or uses class names that are used in the Bootstrap CSS, even if the class names are generic and would represent the exact same component in another framework.

The JavaScript stuff is trickier and most likely all belongs as CE's. I'm not against that. But if you're saying we have to introduce a CE for cards, or badges, or other non-JavaScript components (or components that do have a JavaScript element but not used in a particular circumstance, such as those in this PR) because "that's Bootstrap markup and that's crap and we can't allow that" then that's where I'm heavily in disagreement with whatever it is you're pushing for.

avatar dgrammatiko
dgrammatiko - comment - 4 Oct 2018

If you're saying that <div class="card"> is Bootstrap and therefore must be a CE

By Components I mean the ones that have interaction (eg require javascript: modals, tabs, alerts, etc). Card is plain html and css and should never be a custom element because there is nothing that needs javascript there...

avatar brianteeman
brianteeman - comment - 4 Oct 2018

and in this pr there is nothing that uses js and is just html and css - so why are you insisting it is wrong?

avatar mbabker
mbabker - comment - 4 Oct 2018

And yet these alerts which don't need JavaScript are a problem...

avatar mbabker
mbabker - comment - 4 Oct 2018

(Everyone knows I don't do frontend stuff primarily and am always 18-24 months behind the hottest and latest trends, so I really don't grasp some of these CE things yet, but seeing the way things are being reacted to with 4.0 development really doesn't get me interested in learning more about them or finding use cases for them in my own work)

avatar C-Lodder
C-Lodder - comment - 4 Oct 2018

and in this pr there is nothing that uses js and is just html and css - so why are you insisting it is wrong?

See: #22476 (comment)

avatar dgrammatiko
dgrammatiko - comment - 4 Oct 2018
avatar brianteeman
brianteeman - comment - 4 Oct 2018

sorry Charlie that comment says nothing other than uses some different css classes. i am happy to do that. doesnt address why dimitris says this is completely wrong or why you say i should be a ce with an option not to have the role or the dismiss button

avatar brianteeman
brianteeman - comment - 4 Oct 2018

@dgrammatiko so what are you showing there is that you can reeinvent a round wheel

avatar C-Lodder
C-Lodder - comment - 4 Oct 2018

I'll let you guys continue to battle this out. Said my piece

avatar dgrammatiko
dgrammatiko - comment - 4 Oct 2018

@dgrammatiko so what are you showing there is that you can reinvent a round wheel

How the heck a plain html markup is considered reinvent a round wheel?
Anyways what I'm pointing out is that at some point we agreed to drop the js components of bootstrap together with their related css. With this PR what you did was to revert to J3 practices: eg total dependency on bootstrap.
Then again if that is the case, drop the bloody custom elements altogether (is just alerts and tabs related to bootstrap) and carry on with the beloved bootstrap.

avatar dgrammatiko
dgrammatiko - comment - 4 Oct 2018

Also I'm out, it's pretty obvious we're not communicating here

avatar brianteeman
brianteeman - comment - 4 Oct 2018

Also I'm out, it's pretty obvious we're not communicating here

Clearly because this does not have total dependency on bootstrap and does not use any js component from bootstrap.

Again I will repeat the main problem here is that you and the a11y team misunderstood role=alert and implemented in areas where it should not be. This pr is NOT removing the custom element from where it is supposed to be used

avatar ciar4n
ciar4n - comment - 4 Oct 2018

This argument comes up time and again and is always unwinnable.

On one side.... An element is coupled with a framework if by removing that framework, that element no longer functions as it should. Valid.

On the other... Last checked Bootstrap does not have a copyright on class names like alert and card. So is using the same classes as one framework really coupling? Valid.

Unfortunately, this argument will continue to surface because Joomla sits in a no man's land between Bootstrap coupling and not.

Personal opinion... HTML output and it's classes will never be agnostic. The only real solution to an agnostic Joomla is move all HTML output from core and move it to the template.

avatar dgrammatiko
dgrammatiko - comment - 4 Oct 2018

@brianteeman I wrote the stupid solution 3 times already, why is it so hard to get that? It is just about moving one line of code...

avatar brianteeman
brianteeman - comment - 4 Oct 2018

that is not a solution

avatar dgrammatiko
dgrammatiko - comment - 4 Oct 2018

Why? it doesn't do what you need to achieve here? Elaborate

avatar mbabker
mbabker - comment - 4 Oct 2018

With this PR what you did was to revert to J3 practices: eg total dependency on bootstrap.

@dgrammatiko This is the type of comment that's creating problems. Because to me it is basically saying "you cannot use HTML and/or CSS structures if they are 'owned' by Bootstrap". So to me it's implying "if I can find this CSS class in bootstrap.css then I should not use it in the J4 template or create a custom element wrapping it so it can be replaced by another UI framework". And that's problematic because there are a lot of generic class names in the framework, and HTML is pretty generic on its own too. And that's where the feeling of overengineering I hinted at before comes from, it basically feels like we're bending over backwards to create our own UI framework so that <div class="alert"> only exists in some JavaScript component that can supposedly be replaced (how I have no idea, again I don't grasp CE stuff) and that <div class="alert"> cannot exist in a core HTML producing file.

avatar brianteeman
brianteeman - comment - 4 Oct 2018

Why? it doesn't do what you need to achieve here? Elaborate

Because loading a javascript file to do nothing is a complete waste of time and resources

avatar dgrammatiko
dgrammatiko - comment - 4 Oct 2018

@brianteeman you're always loading that script. ALWAYS! But then again my solution doesn't depend on freaking javascript The js is used only for the dismissible button. Instead of making wrong assumptions you'd be better reading the code and understand what is going on there...

avatar dgrammatiko
dgrammatiko - comment - 4 Oct 2018

@mbabker the problem is that when we did the alerts we didn't removed the relative scss import. Can you still use the .alert class? Sure, as long as you also importing that scss.

The whole dispute here is that there should be only one css file for this component. If you end up using both the ce and the bs, as I kept repeating, just revert the custom elements.

avatar brianteeman
brianteeman - comment - 4 Oct 2018

of course the js is being used how else does it know not to use code to create the dismiss button

avatar dgrammatiko
dgrammatiko - comment - 4 Oct 2018

I give up. The js will append a dismiss button only if there is an attribute dismiss...

avatar brianteeman
brianteeman - comment - 4 Oct 2018

so as you confirm the js is being used even if it is not going to result in anything

avatar dgrammatiko
dgrammatiko - comment - 4 Oct 2018

Are you stupid? Did you checked the link for the codepen I posted above? Is there any javascript there?

avatar brianteeman
brianteeman - comment - 4 Oct 2018

Think what you are saying

avatar zwiastunsw
zwiastunsw - comment - 4 Oct 2018

Again I will repeat the main problem here is that you and the a11y team misunderstood role=alert and implemented in areas where it should not be.

My two cents. I do not want to interfere in this discussion, but some of the 'arguments' are irritating. Where did the a11y team misunderstand the attribute role="alert"?
Did the a11y team mark ALL messages with attributes of role="alert"? Did the a11y team accept this attribute somewhere?
I realize that this is not relevant to this discussion but weigh your words. I am fed up with the constant clinging of the a11y team.

avatar brianteeman
brianteeman - comment - 4 Oct 2018

@zwiastunsw I am sorry if that was not the case. I am only basing that on the comments that this had been previously approved by the a11y team

Add a Comment

Login with GitHub to post a comment