#7975 closed feature (fixed)

Opened December 29, 2011 03:18PM UTC

Closed November 30, 2012 02:51AM UTC

Remove invalid CSS for legacy browsers

Reported by:	selfthinker	Owned by:
Priority:	minor	Milestone:	1.10.0
Component:	ui.css-framework	Version:	1.9.0
Keywords:		Cc:
Blocked by:		Blocking:

Description

The CSS for jQuery UI currently includes a few hacks and invalid CSS to support legacy browsers. While it's a good thing for people who either need to support them or have clients who cannot be educated to understand that websites don't need to look the same in every browser, people who have the power to move the web forward have not much choice.

While it's true that people don't **need** to use the CSS that comes with jQuery UI and/or the theme roller, the vast majority of people do. They rather overwrite things later in their own CSS than change the original source, because it's much more upgrade-proof to do so.

Rather than changing the CSS as a whole, I propose to add an option to the theme roller like

[x] Fully support legacy browsers

I have prepared two branches ("modern-stable" is based on "1-8-stable" and "modern-devel" is based on "master", pull requests will follow) which does the following:

reduce invalid CSS where the use of it was not necessary at all (that reduced the amount of CSS errors from 33 to 24)
moved legacy styles to their own files, so they'd be easier to be removed (through a theme roller option or manually)
moved IE6 fixes to jquery.ui.ancient.css (removing it would reduce the errors to 21)
moved other less important fixes for other older browsers to jquery.ui.legacy.css (removing that would reduce the errors to just 3)
some other enhancements

I left in exactly 3 errors, because IMO they fix more important things:

Removing the opacity filter in .ui-widget-overlay would make it quite useless in IE6-8. Although there would be an easy solution to that: Just add transparency to the background image that is already in use anyway.
I left button.ui-button::-moz-focus-inner (and added input.ui-button::-moz-focus-inner) because it could make a difference in scenarios where a lot of buttons are in a row. Otherwise I think it's an edge case, because all the buttons look very different in all the browsers anyway.
A fourth error appears with the devel version: "0 is not a box-shadow value". But I believe that's rather an error in the validator.

Judging from opinions of the jQuery UI team I read elsewhere, I can imagine chances are not very high that these changes will get merged. But at least I made them so unintrusive that most users wouldn't know the difference, but they make it easier to remove "harmful" bits for people who care about those things.

Attachments (0)

Change History (14)

Changed June 26, 2012 01:22AM UTC by scottgonzalez comment:1

type:	enhancement → feature

Changed October 11, 2012 02:40PM UTC by scottgonzalez comment:2

milestone:	1.9.0 → 1.10.0

Changed November 04, 2012 05:29PM UTC by jzaefferer comment:3

The mentioned pull request is here: https://github.com/jquery/jquery-ui/pull/558

I just closed that as impossible-to-merge: "IE6 fixes got removed for 1.10, and with all the formatting changes its impossible to figure out what is still valid."

We still support IE7 and 8 for 1.10, so along with still have a bunch of legacy CSS. Though I don't think its worth the trouble extending ThemeRoller for this.

I'd still be interested in patches for removing invalid or unncessary CSS declarations, but these should be kept separate. Same as the CSS formatting, which we still need to do, without making more PRs unmergable.

Changed November 04, 2012 05:50PM UTC by mikesherov comment:4

Sounds like we should close as wontfix then?

Changed November 04, 2012 09:58PM UTC by selfthinker comment:5

_comment0:	Wow, finally some movement here. :) \ I guess, yes, the pull request should obviously be closed, although this ticket is still valid. \ \ joern.zaefferer, judging from your comment, you'd be interested in some of the patches in the pull request but not in all of them? If yes, can you please elaborate in which of the following you would be interested in? \ \ The main changes were: \ * changed coding standard to be more readable \ * removed invalid CSS where possible \ * moved CSS for legacy browsers into separate CSS file (I personally would like to get rid of them completely, but that's your call) \ * removed specific font-size and font-family declaration \ \ "I'd still be interested in patches for removing invalid or unncessary CSS declarations, but these should be kept separate." \ What do you mean by "these should be kept separate"? Do you mean they should be in different pull requests? \ \ I'd be happy to make those changes again, but only if it will get merged soon after. Otherwise it's lost time and effort and will be unmergeable again soon. \ \ IMHO, changing the coding standard to having each selector and declaration on a new line is vital, especially for working with git. But this is also the one change which will cause most problems with still open commits and branches which were created before this change. I guess that was what you meant by "Same as the CSS formatting, which we still need to do, without making more PRs unmergable." \ \ So, the next pull request I could send should be exactly that one about the coding standard and nothing else at first (sendiing others only after it has been merged). Unfortunately I don't know anything about the jqueryui team internals, especially regarding timing of merges etc. So, when would that best fit into your schedule so that it causes the least issues? \ → 1352066594079220

_comment0:

Wow, finally some movement here. :) \ I guess, yes, the pull request should obviously be closed, although this ticket is still valid. \ \ joern.zaefferer, judging from your comment, you'd be interested in some of the patches in the pull request but not in all of them? If yes, can you please elaborate in which of the following you would be interested in? \ \ The main changes were: \ * changed coding standard to be more readable \ * removed invalid CSS where possible \ * moved CSS for legacy browsers into separate CSS file (I personally would like to get rid of them completely, but that's your call) \ * removed specific font-size and font-family declaration \ \ "I'd still be interested in patches for removing invalid or unncessary CSS declarations, but these should be kept separate." \ What do you mean by "these should be kept separate"? Do you mean they should be in different pull requests? \ \ I'd be happy to make those changes again, but only if it will get merged soon after. Otherwise it's lost time and effort and will be unmergeable again soon. \ \ IMHO, changing the coding standard to having each selector and declaration on a new line is vital, especially for working with git. But this is also the one change which will cause most problems with still open commits and branches which were created before this change. I guess that was what you meant by "Same as the CSS formatting, which we still need to do, without making more PRs unmergable." \ \ So, the next pull request I could send should be exactly that one about the coding standard and nothing else at first (sendiing others only after it has been merged). Unfortunately I don't know anything about the jqueryui team internals, especially regarding timing of merges etc. So, when would that best fit into your schedule so that it causes the least issues? \ → 1352066594079220

Wow, finally some movement here. :)

I guess, yes, the pull request should obviously be closed, although this ticket is still valid.

joern.zaefferer, judging from your comment, you'd be interested in some of the patches in the pull request but not in all of them? If yes, can you please elaborate in which of the following you would be interested in?

The main changes were:

changed coding standard to be more readable
removed invalid CSS where possible
moved CSS for legacy browsers into separate CSS file (I personally would like to get rid of them completely, but that's your call)
removed specific font-size and font-family declaration

"I'd still be interested in patches for removing invalid or unncessary CSS declarations, but these should be kept separate."

What do you mean by "these should be kept separate"? Do you mean they should be in different pull requests?

I'd be happy to make those changes again, but only if it will get merged soon after. Otherwise it's lost time and effort and will be unmergeable again soon.

IMHO, changing the coding standard to having each selector and declaration on a new line is vital, especially for working with git. But this is also the one change which will cause most problems with still open commits and branches which were created before this change. I guess that was what you meant by "Same as the CSS formatting, which we still need to do, without making more PRs unmergable."

So, the next pull request I could send should be exactly that one about the coding standard and nothing else at first (sending others only after it has been merged). Unfortunately I don't know anything about the jqueryui team internals, especially regarding timing of merges etc. So, when would that best fit into your schedule so that it causes the least issues?

Changed November 05, 2012 02:10AM UTC by scottgonzalez comment:6

status:	new → open
version:	→ 1.9.0

selfthinker: If you send a PR with just the style cleanup, I'll land it immediately. I had actually done this a while ago and it became stale because I didn't have an easy way to verify that nothing broke in ThemeRoller. Since then we've rewritten ThemeRoller in node and can now easily test changes.

Once that lands, you can send another PR to split out legacy code. The stuff in the ancient file can just go away since we've already dropped IE6 support in master. The stuff in the legacy file can be reduced as well. We don't need any prefixed border radius rules and we only need WebKit box shadows. That should make the legacy file pretty small while still covering all the browsers that we support.

Changed November 20, 2012 12:43AM UTC by selfthinker comment:7

I've just done the coding standard changes again. See https://github.com/jquery/jquery-ui/pull/834

Changed November 27, 2012 11:53PM UTC by selfthinker comment:8

_comment0:	Mainly due to the removed IE6 fixes, there are only 3 types of invalid CSS left: \ \ * 1x `::-moz-focus-inner`: I would definitely leave that in and would even extend it for input as well, as described on #7996 \ * 8x `filter`: Most of the microsoft-specific filters enhance the usability but are not essential, only the opacity filter for .ui-widget-overlay is really important. Would you consider removing some of them? Or would you rather not touch them? I am in two minds about that... \ * 8x `zoom`: Although zoom is not one of the "evil" validity issues, it's also not necessary. As we don't need to consider IE6 anymore, swapping every `zoom: 1` with e.g. a `min-height: 1px` should trigger hasLayout as well. I will first check if each occurrence is really needed (it might have been only there for IE6) and then check if min-height can be used instead and change it accordingly. → 1354060477713837

_comment0:

Mainly due to the removed IE6 fixes, there are only 3 types of invalid CSS left: \ \ * 1x `::-moz-focus-inner`: I would definitely leave that in and would even extend it for input as well, as described on #7996 \ * 8x `filter`: Most of the microsoft-specific filters enhance the usability but are not essential, only the opacity filter for .ui-widget-overlay is really important. Would you consider removing some of them? Or would you rather not touch them? I am in two minds about that... \ * 8x `zoom`: Although zoom is not one of the "evil" validity issues, it's also not necessary. As we don't need to consider IE6 anymore, swapping every `zoom: 1` with e.g. a `min-height: 1px` should trigger hasLayout as well. I will first check if each occurrence is really needed (it might have been only there for IE6) and then check if min-height can be used instead and change it accordingly. → 1354060477713837

Mainly due to the removed IE6 fixes, there are only 3 types of invalid CSS left:

1x ::-moz-focus-inner: I would definitely leave that in and would even extend it for input as well, as described on ticket:7996#comment:15
8x filter: Most of the microsoft-specific filters enhance the usability but are not essential, only the opacity filter for .ui-widget-overlay is really important. Would you consider removing some of them? Or would you rather not touch them? I am in two minds about that...
8x zoom: Although zoom is not one of the "evil" validity issues, it's also not necessary. As we don't need to consider IE6 anymore, swapping every zoom: 1 with e.g. a min-height: 1px should trigger hasLayout as well. I will first check if each occurrence is really needed (it might have been only there for IE6) and then check if min-height can be used instead and change it accordingly.

Changed November 28, 2012 12:07AM UTC by mikesherov comment:9

Beautiful.

Changed November 28, 2012 02:21AM UTC by scottgonzalez comment:10

I'd like to keep the filter rules everywhere. If we can swap zoom for min-height, that seems fine. Adding ::-moz-focus-inner for input.ui-button is fine too.

Thanks for sticking with this :-)

Changed November 28, 2012 11:41AM UTC by selfthinker comment:11

Could someone please rename the ticket to "Remove invalid CSS for legacy browsers"? (As the suggested ThemeRoller option should not be done, the current title is obsolete.) Thanks.

Changed November 28, 2012 11:58AM UTC by mikesherov comment:12

summary:	Give option to not support legacy browsers with invalid CSS → Remove invalid CSS for legacy browsers

Changed November 29, 2012 07:54PM UTC by selfthinker comment:13

And my final pull request is this one: https://github.com/jquery/jquery-ui/pull/852

I tested every demo in IE7 and a few random demoes in other browsers. This could potentially still have introduced issues which are not covered by the demoes, though.

I addressed the input issue in #7996.

After that, the final amount of validation errors is 10: 8x filter and 2x ::-moz-...; and 2 warnings: 1x spaces as clip shape separators and 1x -webkit-... All of which, I think, are fine to have! :)

Changed November 30, 2012 02:51AM UTC by Anika Henke comment:14

resolution:	→ fixed
status:	open → closed

Theme: removed or changed occurrences of zoom. Fixes #7975 - Remove invalid CSS for legacy browsers

Because of overlapping issues, this also reverts most of e77edc60 and fixes it in a different way.

Changeset: d7bff010691b542d918245bac11beac2b93b3462

Search and Top Navigation