Bug 42093 – Change handling of -khtml- and -apple- vendor-prefixes

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: CSS

[Peter Beverloo]

Created attachment 61244 [details]
Removing the translation altogether

Currently WebKit supports "-apple" and "-khtml" as an alternative for its own "-webkit" vendor prefix. I believe this behavior should be removed, or at least, phased out. A thread[1] to discuss this has been created on webkit-dev.

The added patch will remove the translation altogether, however, since the thread is still active I'm not marking it for review.

[1] https://lists.webkit.org/pipermail/webkit-dev/2010-July/013519.html

[Peter Beverloo]

Created attachment 61337 [details]
Remove -khtml support, continue supporting two -apple properties

Patch which removes support for all -khtml-prefixed properties, and removes support for all -apple-prefixed properties except for "-apple-dashboard-region" and "-apple-line-clamp".

[Peter Beverloo]

Created attachment 61340 [details]
Remove -khtml support, continue supporting two -apple properties

Typo fix

[WebKit Review Bot]

Attachment 61340 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/css/CSSParser.cpp:5593:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Peter Beverloo]

Created attachment 61342 [details]
Idem, but with a style fix

Style fix.

[Eric Seidel (no email)]

Comment on attachment 61342 [details]
Idem, but with a style fix

WebCore/css/CSSParser.cpp:5583
 +          if (!strcmp(buffer, "-apple-dashboard-region") || !strcmp(buffer, "-apple-line-clamp")) {
This looks wrong.  Previously the variable "length" was updated by "hasPrefix".  That's no longer the case, so the memmove will be wrong.  Do we not have tests for these?  We should add them.

[Peter Beverloo]

Created attachment 61501 [details]
Added 3 test-cases

The hasPrefix function does not touch the length property, therefore I believe
the code to be correct.

I have added three testcases:
 - Testing (limited) support for the -apple vendor prefix.
 - Testing whether the -khtml prefix actually got removed.
 - Testing whether the -webkit-opacity legacy-property got removed.

One test-cases got updated, "apple-prefix.html" which tests -apple-line-clamp
and relied on -khtml.
One test-case got removed, "legacy-opacity-styles.html", now tested by the
case which tests whether the property got removed.

[Eric Seidel (no email)]

Comment on attachment 61501 [details]
Added 3 test-cases

I think the change looks fine.  The tests could be better.  I'd rather see them as scipt-tests, and see them test more conditions (you only test two properties your new tests).

It's entertaining that you're even removing a script test.  Notice how because it doesn't have any boiler plate its easier to more exhaustivly test what it was testing?

You don't test for example that "-webkit-line-clamp" is or is not defined.

[Peter Beverloo]

Created attachment 61710 [details]
Same code, but with two script-tests

Added two script-tests for testing the new behavior.

limited-vendor-prefix-behavior tests for removal of -khtml and limitation of -apple.

no-legacy-webkit-opacity-expected tests whether "-webkit-opacity" is now unsupported.

[Darin Adler]

Removing -webkit-opacity goes against what we have repeatedly told developers for years. We promised them that even when we supported the standard version of the CSS property we’d keep the prefixed version working for a while.

Generally speaking we tell people that we’ll keep the prefixed versions indefinitely, so it’s safe to use them. Once the standard version is available, it’s preferred. There’s no guarantee that we’ll ever remove the prefixed version entirely. We can do it if that opportunity arises organically.

Hyatt, do you really want to remove -webkit-opacity? Has something changed since we set this strategy in place?

[Darin Adler]

This bug is now tracking multiple changes to vendor prefixes that may need to be discussed separately.

[Peter Beverloo]

Created attachment 61779 [details]
Patch reduced to removing -khtml and limiting -apple

Hyatt solely responded to -khtml and -apple, not -webkit-opacity. Therefore I limited the patch to only address that change.

[Darin Adler]

Comment on attachment 61779 [details]
Patch reduced to removing -khtml and limiting -apple

> +            // -webkit-border-*-*-radius worked in Safari 4 and earlier. -webkit-border-radius syntax
> +            // differs from border-radius, so it is remains as a distinct property.

As long as you are moving this code, we should consider fixing "it is remains" here.

> +        // If the prefix is -apple, support two properties to maintain
> +        // backward compatibility with older versions of Safari.
> +        if (!strcmp(buffer, "-apple-dashboard-region") || !strcmp(buffer, "-apple-line-clamp")) {

This comment isn’t quite right. The most important context for this is not Safari itself, but rather Dashboard. But I suppose we are OK with the comment as-is.

[WebKit Commit Bot]

Comment on attachment 61779 [details]
Patch reduced to removing -khtml and limiting -apple

Rejecting patch 61779 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 20713 test cases.
fast/block/positioning/height-change.html -> failed

Exiting early after 1 failures. 5500 tests run.
94.23s total testing time

5499 test cases (99%) succeeded
1 test case (<1%) had incorrect layout

Full output: http://queues.webkit.org/results/3598159

[Peter Beverloo]

Created attachment 62164 [details]
same patch, but with test fixes

Fixes tests, code remains unchanged. Most notably are the flexbox tests, all of which used -khtml- properties.

[Eric Seidel (no email)]

Comment on attachment 62164 [details]
same patch, but with test fixes

LayoutTests/fast/block/positioning/height-change.html: 
 +  <div style="position: relative; display: -apple-box;">
Does this even need "-webkit-" anymore?

LayoutTests/fast/borders/border-image-omit-right-slice.html:17
 +  		-webkit-border-image: url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAoAAAAKCAIAAAACUFjqAAAAAXNSR0IArs4c6QAAAUFJREFUGNMBNgHJ/gPcUkznH/n8lGR5YkMIIMDXFhgoVNnfqeM5dbre55MDGkF/U6tMmi12K6QBpfkB4H3NyPZAiyjm6kp//uPtAA4pZhHPJ8xbkSop4chy5ecPsVSqkC03ozAmk77SOwOHR+fDXmoF5UUJU9YsnVfehg/iLC7p3etC3w3LEA8D0eGf/DZozPPzoSSX6gTVlQ/L4Ylt4SrFiOgqNDQPAPvl/JQuLv33xK66eAgGCn+OAIUu1Fugs2hsZNIB6gPhrZ80q7sXNUEuIE7EGNC5Dc2ExcIe9tseb4r7usAAralzLgdas9URX21wGUqSgDnf/jMydhxMIEx4w9TtAfCQPsbi30EEVABXFJYnOZgfbQK3CLkwg6PDGG27KwOYF7jD+sIHhdwoUyILAF5sv/6bI6S56fbULB5OToUjFpS7UHGrVgAAAABJRU5ErkJggg==) 1 2 3;
I'm certain this no longer needs -webkit-

LayoutTests/fast/css/limited-vendor-prefix-behavior.html:22
 +  shouldBe("document.getElementById('test-no-khtml').style.khtmlUserDrag", "undefined");
You never test that "lineClamp" doesn't get se when we use -khtml-line-clamp.  I would think it's important to test both for khtmlLineClamp and that it doesn't get mapped to lineClamp.

In general this looks good though!

[Peter Beverloo]

Created attachment 62170 [details]
Patch

Eric: The "box" value for the display-property still needs to be prefixed. Same goes for the "border-image" property. Considering "border-image" already is a CR the prefix could be dropped, but that's not in the scope of this bug.

I have fixed the typo Darin pointed out, updated the backward-compatibility comment to say Dashboard rather than Safari and updated the test-case to include the "userDrag" and "lineClamp" properties, which both pass as expected.

[Darin Adler]

Comment on attachment 62170 [details]
Patch

> @@ -5672,9 +5668,9 @@ int cssValueKeywordID(const CSSParserStr
>      buffer[length] = '\0';
>  
>      if (buffer[0] == '-') {
> -        // If the prefix is -apple- or -khtml-, change it to -webkit-.
> -        // This makes the string one character longer.
> -        if (hasPrefix(buffer, length, "-apple-") || hasPrefix(buffer, length, "-khtml-")) {
> +        // If the prefix is -apple, support two properties to maintain
> +        // backward compatibility with the Apple Dashboard.
> +        if (!strcmp(buffer, "-apple-dashboard-region") || !strcmp(buffer, "-apple-line-clamp")) {

This change is incorrect. -apple-dashboard-region and -apple-line-clamp are CSS property names. But this function is for CSS value keywords. So it's pointless to rename these strings, since they are not legal value keywords. I'm not sure which CSS value keywords need to work with the "-apple" prefix.

The rest of the patch looks good.

[Peter Beverloo]

Missed that, excuse me. Should I remove the value-translation for the -apple/-khtml prefixes in total or would another approach be preferred?

[Darin Adler]

(In reply to comment #18)
> Missed that, excuse me. Should I remove the value-translation for the -apple/-khtml prefixes in total or would another approach be preferred?

Someone has to research which keyword values might still need to be handled. Once we're confident there are none, then yes, removing the code altogether is fine.

I tried and could not quickly find anything.

[Peter Beverloo]

Created attachment 62213 [details]
Patch

I removed the changes to the css value-parsing. Updating the cssValueKeywordID method is rather easy and could be done in a follow-up patch. 

Not touching the tests, as using -webkit-box (or any other value) is preferred over -apple-box either way. I will be doing more research to the usage of these properties later on, although they seems to be rarely used, if at all.

[Darin Adler]

Comment on attachment 62213 [details]
Patch

> +        (WebCore::cssValueKeywordID):

Change log now lists this function, but the patch does not modify it.

[WebKit Commit Bot]

Comment on attachment 62213 [details]
Patch

Rejecting patch 62213 from commit-queue.

Failed to run "[u'git', u'svn', u'dcommit']" exit_code: 1
Last 500 characters of output:
M	WebCore/css/CSSParser.cpp
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:

    The following files contain tab characters:

        trunk/LayoutTests/fast/block/positioning/resources/window-height-change-frame-flex.html

    Please use spaces instead to indent.
    If you must commit a file with tabs, use svn propset to set the "allow-tabs" property.
 at /usr/local/git/libexec/git-core/git-svn line 570


Full output: http://queues.webkit.org/results/3561290

[Peter Beverloo]

Created attachment 62220 [details]
Patch

Gah. Updated the changelog per Darin's comment and replaced the tabs which two positioning tests were based on with four spaces.

[WebKit Commit Bot]

Comment on attachment 62220 [details]
Patch

Clearing flags on attachment: 62220

Committed r63854: <http://trac.webkit.org/changeset/63854>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Tony Chang]

Some tests in fast/backgrounds were missed.  The pixel tests on chromium found them.  I'll make a patch to fix.

[Mark Rowe (bdash)]

This completely broke the layout of Safari RSS pages.  See bug 42990 for details.

[Peter Beverloo]

I got cc'ed on these bugs. If you could make a small list of css properties used by the RSS reader I could make a patch to re-introduce these properties.

[Mark Rowe (bdash)]

Sadly it’s not just Safari RSS that uses these properties.  I did a quick grep over the various loose .css files I could see on my system.  Many of them referenced -khtml properties.  Dashboard, system Dashboard widgets, Dashcode templates, QuickLook, and even third-party applications like Yahoo! Messenger all appear to make use of -khtml or -apple properties.

In order to restore the functionality of the affected applications I am going to roll out the WebCore portion of <http://trac.webkit.org/changeset/63854>.  I would suggest that we revisit this change in the following way:
1) Set things up such that all existing properties that support the -khtml or -apple prefix continue to work, but so that newly-added properties do not support the legacy prefixes.
2) Establish a set of tests that make it explicit which of the properties we intend to support with the legacy prefixes.
3) Work to incrementally reduce the set of properties that are supported with the legacy prefix after we determine that individual properties are not used in the wild with the legacy prefixes.

[Mark Rowe (bdash)]

I did that roll-out in <http://trac.webkit.org/changeset/64071>.  Reopening.

[Eric Seidel (no email)]

Comment on attachment 61779 [details]
Patch reduced to removing -khtml and limiting -apple

Cleared Darin Adler's review+ from obsolete attachment 61779 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 62213 [details]
Patch

Cleared Darin Adler's review+ from obsolete attachment 62213 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Peter Beverloo]

abarth took over and landed http://trac.webkit.org/changeset/113795.

[Alexey Proskuryakov]

*** This bug has been marked as a duplicate of bug 83256 ***