Support from Acquia helps fund testing for Drupal Acquia logo

Comments

echoz’s picture

Status: Active » Needs review
FileSize
6.12 KB

This patch removes remaining prefixed border-radius and prefixed box-shadow found in Seven and views-admin.theme.css

echoz’s picture

Issue tags: +d8mux-css-cleanup

tagging

echoz’s picture

Re-submitting since stuck at "postponed" after 7hrs, where later patches were queued normally.

LewisNyman’s picture

Issue tags: -d8mux-css-cleanup

#3: remove-prefixed-css-2005520-3.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, remove-prefixed-css-2005520-3.patch, failed testing.

LewisNyman’s picture

Status: Needs work » Needs review

#3: remove-prefixed-css-2005520-3.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +d8mux-css-cleanup

The last submitted patch, remove-prefixed-css-2005520-3.patch, failed testing.

echoz’s picture

Status: Needs work » Needs review
FileSize
5.36 KB

Re-roll

LewisNyman’s picture

Status: Needs review » Needs work

I found a few more instances of prefixed properties. I don't think we can touch the joyride.css, you could submit an issue in the Github repo. I have no idea about simpletest but I'm sure we can loose it there as well.

/core/modules/tour/css/joyride-2.0.3.css:
/core/modules/simpletest/files/css_test_files/css_input_without_import.css.unoptimized.css
/core/modules/simpletest/files/css_test_files/css_input_without_import.css.optimized.css
/core/modules/simpletest/files/css_test_files/css_input_without_import.css
/core/themes/seven/ie.css // This one is quite funny
echoz’s picture

Status: Needs work » Needs review
FileSize
8.37 KB

Thanks Lewis. This patch adds the 3 files you noted from simpletest. I also deleted the MS filter lines as IE9 supports box shadow, MS filter no longer needed.

That ie.css (shudder), I spotted that recently, and it can go. Needs another issue, although unrelated to this one.

Status: Needs review » Needs work

The last submitted patch, remove-prefixed-css-2005520-10.patch, failed testing.

echoz’s picture

Status: Needs work » Needs review
FileSize
8.37 KB

Ooops, patching that optimized file got me.

Status: Needs review » Needs work
Issue tags: -d8mux-css-cleanup

The last submitted patch, remove-prefixed-css-2005520-12.patch, failed testing.

echoz’s picture

Status: Needs work » Needs review
Issue tags: +d8mux-css-cleanup
echoz’s picture

is the optimized file, css_input_with_import.css.optimized.css, a generated file so perhaps we're not supposed to patch it?

LewisNyman’s picture

I spoke to Berdir about this on IRC, it sounds like we should be patching the optimised file. The CSS files are tests to check to see if the unoptimised CSS comes back correctly once it's optimised. If we change the unoptimised we should change the optimised to match.

echoz’s picture

Ok, thanks for checking on that. It is patched in #12 and it came back green.

LewisNyman’s picture

Status: Needs review » Reviewed & tested by the community

Fantastic, may I suggest we add a section on prefixed properties to our coding standards?

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

We still have some usages of -webkit-box-shadow and -moz-box-shadow in ckeditor.admin.css and seven's ie.css (why they're in ie.css is kinda beyond me :D )

echoz’s picture

Status: Needs work » Needs review
FileSize
8.92 KB

Gee, I don't know what process I began searching for these properties with, but it sure needed some work, thanks @LewisNyman and @alexpott. This patch adds ckeditor.admin.css. I believe we could follow up with prefixed gradients http://lea.verou.me/2013/04/can-we-get-rid-of-gradient-prefixes/ which might need discussion where our line is.

I'm creating another issue to delete seven's ie.css.

echoz’s picture

Status: Needs review » Needs work
Issue tags: -d8mux-css-cleanup

The last submitted patch, remove-prefixed-css-2005520-20.patch, failed testing.

echoz’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, remove-prefixed-css-2005520-20.patch, failed testing.

echoz’s picture

Status: Needs work » Needs review
Issue tags: +d8mux-css-cleanup
LewisNyman’s picture

Status: Needs review » Reviewed & tested by the community

Thanks Alex, I thought the ckeditor CSS was part of the ckeditor plugin.

Did a global grep for -webkit- prefixes. The only places that -webkit-border-radius or -webkit-box-shadow show up are in third party libs.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 6d8aacf and pushed to 8.x. Thanks!

Wim Leers’s picture

Status: Fixed » Active

Why were lines with those properties in core/modules/simpletest/files/css_test_files/* removed? They were there to guarantee that CSS aggregation wouldn't break them, and it's entirely possible that sites which need to support older browsers still want to use these.

echoz’s picture

@Wim Leers, I assume you mean the "filter" and "-ms-filter". I noted removing them in #10. They are obsolete proprietary MS code, although I don't know the nuances of simpletest. Do we need to revert and re-patch leaving them in? If that's the case, it seems like we don't want to remove anything from simpletest, as it's more likely someone would still use the much more modern -webkit-border-radius.

LewisNyman’s picture

Issue summary: View changes
Issue tags: +CSS, +frontend
sqndr’s picture

Status: Active » Closed (duplicate)

There's a new issue, #1664268 to remove the remaining vendor prefixes. Closing this one, since the latest patch is outdated as well.