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

Remove user agent css classes in fe_page by default #482

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@Toflar
Copy link
Member

commented May 10, 2019

Personally, I have not a single Contao installation where I need these CSS classes.
If I really need to do things differently depending on the browser I always go for feature detection which is safer anyway.

The major disadvantage of using these classes is that they have to be different depending on the user agent that requests the page. This is why we've implemented rendering this insert tag as ESI fragment. This means that the whole page can be served from the cache and the reverse proxy then has to execute a separate request to the application only to get these CSS classes. On every single page!

I'm very convinced that for more than 90% of all the websites using Contao, these CSS classes are irrelevant which is why I think we should optimize the default behaviour for those.

People who really need these classes can still re-add {{ua::class}}. You don't even need to do it in the fe_page but you can add the insert tag to e.g. the layout if you know that it's only needed for a certain layout.

@leofeyer

This comment has been minimized.

Copy link
Member

commented May 10, 2019

The following scripts rely on the user agent CSS classes:

Script Scope Classes used Comment
responsive.css FE .ie7, .ie8 No problem, as we are no longer supporting IE8
basic.css BE .win Used to apply a different margin to radio buttons on Windows -> no feature detection possible
main.css BE .mac Used to apply a different top position to checkboxes on macOS -> no feature detection possible
simplemodal.css BE .ios Various iOS fixes -> can probably be replaces with another solution

But there is also the .mobile class and AFAIK this one is heavily in use.

I'm very convinced that for more than 90% of all the websites using Contao, these CSS classes are irrelevant

I have just checked my projects and I find me using the classes in about 50% of them.

@Toflar

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

BE is irrelevant. There we still add it. We're just talking about fe_page here. The back end isn't cached anyway.

@fritzmg

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

We mainly use them for fixing inconsistencies with the rendering of various form elements - not just input fields, but also <legend> or <button> elements can be a pain.

@Toflar

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

Examples please.

@fritzmg

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

e.g. <select> elements. All the browser put a different, visual padding to the left (and right). Thus, if you want the text within the <select> to align with other input fields like regular text input fields, you'll need to manually move the <select> around.

For example:

Screenshot_2019-05-10 Anmeldung En GardE

This is a Screenshot from FireFox where the <select> for the "Anrede" was moved 4px to the left. Otherwise it would look like this:

Screenshot_2019-05-10 Anmeldung En GardE(1)

On IE, the <select> needs to be moved 2px to the left. No adjustment is needed for Chrome.

Having said that: I am all for removing them 😉. Contao could provide a js_useragent template instead, which would add a user agent CSS class via JavaScript and can be enabled when needed.

@Toflar

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

Contao could provide a js_useragent template instead, which one can enable when needed.

👍 We could do that, it's just completely unnessecary to ask the application to parse the user agent string on every request. You could implement the exact same code in JS using navigator.userAgent if you really want to read from the user agent string rather than going for feature detection.

@fritzmg

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

Yep, exactly my thoughts. No need to waste server resources for purely client related things.

(Also, I hate styling forms. With all the advances and unification in browser technology - simple form elements are still a pain.)

@fritzmg

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

One thing to note: if we really implement this as a js_useragent template, which would be included before </body>, then you might get FouC.

@Toflar

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

Yeah I don't really get why anyone would ever use these classes really. If the forms don't look the same in different browsers then that's what it is.
It's just not a good default. Definitely not. Even in your form example, why would you have an ESI request on every single page if it's only relevant for one form? I mean you could even add {{ua::class}} to the form generator CSS class settings and you'll get it on <form class="...">.
Or the page layout, or the page...everywhere, just not by default in the fe_page because it's useless for most pages 😄

@fritzmg

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

Yeah - I'd also only add the JavaScript on pages where it is necessary.

@leofeyer leofeyer added the feature label May 13, 2019

@leofeyer leofeyer added this to the 4.8.0 milestone May 13, 2019

Update core-bundle/src/Resources/contao/templates/frontend/fe_page.html5
Co-Authored-By: Leo Feyer <github@contao.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.