Ticket #7975 (closed feature: fixed)

Opened 3 years ago

Last modified 23 months ago

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:
Blocking: Blocked by:

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.

Change History

comment:1 Changed 2 years ago by scott.gonzalez

  • Type changed from enhancement to feature

comment:2 Changed 2 years ago by scott.gonzalez

  • Milestone changed from 1.9.0 to 1.10.0

comment:3 Changed 2 years ago by joern.zaefferer

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.

comment:4 Changed 2 years ago by mikesherov

Sounds like we should close as wontfix then?

comment:5 Changed 2 years ago by selfthinker

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?

Last edited 2 years ago by selfthinker (previous) (diff)

comment:6 Changed 2 years ago by scott.gonzalez

  • Status changed from new to open
  • Version set to 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.

comment:7 Changed 2 years ago by selfthinker

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

comment:8 Changed 2 years ago by selfthinker

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.
Last edited 2 years ago by selfthinker (previous) (diff)

comment:9 Changed 2 years ago by mikesherov

Beautiful.

comment:10 Changed 2 years ago by scott.gonzalez

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 :-)

comment:11 Changed 2 years ago by selfthinker

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.

comment:12 Changed 23 months ago by mikesherov

  • Summary changed from Give option to not support legacy browsers with invalid CSS to Remove invalid CSS for legacy browsers

comment:13 Changed 23 months ago by selfthinker

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! :)

comment:14 Changed 23 months ago by Anika Henke

  • Status changed from open to closed
  • Resolution set to fixed

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

Note: See TracTickets for help on using tickets.