Bug 42093 - Change handling of -khtml- and -apple- vendor-prefixes
: Change handling of -khtml- and -apple- vendor-prefixes
Status: RESOLVED DUPLICATE of bug 83256
: WebKit
CSS
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-07-12 10:39 PST by
Modified: 2012-04-25 09:53 PST (History)


Attachments
Removing the translation altogether (3.39 KB, patch)
2010-07-12 10:39 PST, Peter Beverloo
no flags Review Patch | Details | Formatted Diff | Diff
Remove -khtml support, continue supporting two -apple properties (3.13 KB, patch)
2010-07-13 00:38 PST, Peter Beverloo
no flags Review Patch | Details | Formatted Diff | Diff
Remove -khtml support, continue supporting two -apple properties (3.13 KB, patch)
2010-07-13 00:43 PST, Peter Beverloo
no flags Review Patch | Details | Formatted Diff | Diff
Idem, but with a style fix (3.12 KB, patch)
2010-07-13 00:51 PST, Peter Beverloo
eric: review-
Review Patch | Details | Formatted Diff | Diff
Added 3 test-cases (15.55 KB, patch)
2010-07-14 04:35 PST, Peter Beverloo
no flags Review Patch | Details | Formatted Diff | Diff
Same code, but with two script-tests (17.44 KB, patch)
2010-07-15 13:40 PST, Peter Beverloo
no flags Review Patch | Details | Formatted Diff | Diff
Patch reduced to removing -khtml and limiting -apple (14.19 KB, patch)
2010-07-16 01:38 PST, Peter Beverloo
no flags Review Patch | Details | Formatted Diff | Diff
same patch, but with test fixes (39.11 KB, patch)
2010-07-21 04:46 PST, Peter Beverloo
no flags Review Patch | Details | Formatted Diff | Diff
Patch (39.42 KB, patch)
2010-07-21 05:46 PST, Peter Beverloo
darin: review-
darin: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Patch (38.28 KB, patch)
2010-07-21 11:15 PST, Peter Beverloo
no flags Review Patch | Details | Formatted Diff | Diff
Patch (40.79 KB, patch)
2010-07-21 12:42 PST, Peter Beverloo
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-07-12 10:39:25 PST
Created an attachment (id=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
------- Comment #1 From 2010-07-13 00:38:46 PST -------
Created an attachment (id=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".
------- Comment #2 From 2010-07-13 00:43:14 PST -------
Created an attachment (id=61340) [details]
Remove -khtml support, continue supporting two -apple properties

Typo fix
------- Comment #3 From 2010-07-13 00:45:16 PST -------
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.
------- Comment #4 From 2010-07-13 00:51:32 PST -------
Created an attachment (id=61342) [details]
Idem, but with a style fix

Style fix.
------- Comment #5 From 2010-07-14 01:57:50 PST -------
(From update of attachment 61342 [details])
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.
------- Comment #6 From 2010-07-14 04:35:08 PST -------
Created an attachment (id=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.
------- Comment #7 From 2010-07-15 12:09:43 PST -------
(From update of attachment 61501 [details])
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.
------- Comment #8 From 2010-07-15 13:40:34 PST -------
Created an attachment (id=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.
------- Comment #9 From 2010-07-15 14:44:32 PST -------
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?
------- Comment #10 From 2010-07-15 14:44:55 PST -------
This bug is now tracking multiple changes to vendor prefixes that may need to be discussed separately.
------- Comment #11 From 2010-07-16 01:38:44 PST -------
Created an attachment (id=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.
------- Comment #12 From 2010-07-19 13:06:37 PST -------
(From update of attachment 61779 [details])
> +            // -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.
------- Comment #13 From 2010-07-19 13:33:41 PST -------
(From update of attachment 61779 [details])
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
------- Comment #14 From 2010-07-21 04:46:48 PST -------
Created an attachment (id=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.
------- Comment #15 From 2010-07-21 04:55:52 PST -------
(From update of attachment 62164 [details])
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!
------- Comment #16 From 2010-07-21 05:46:34 PST -------
Created an attachment (id=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.
------- Comment #17 From 2010-07-21 08:15:18 PST -------
(From update of attachment 62170 [details])
> @@ -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.
------- Comment #18 From 2010-07-21 08:24:53 PST -------
Missed that, excuse me. Should I remove the value-translation for the -apple/-khtml prefixes in total or would another approach be preferred?
------- Comment #19 From 2010-07-21 08:28:36 PST -------
(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.
------- Comment #20 From 2010-07-21 11:15:40 PST -------
Created an attachment (id=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.
------- Comment #21 From 2010-07-21 11:23:22 PST -------
(From update of attachment 62213 [details])
> +        (WebCore::cssValueKeywordID):

Change log now lists this function, but the patch does not modify it.
------- Comment #22 From 2010-07-21 12:20:52 PST -------
(From update of attachment 62213 [details])
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
------- Comment #23 From 2010-07-21 12:42:23 PST -------
Created an attachment (id=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.
------- Comment #24 From 2010-07-21 13:39:31 PST -------
(From update of attachment 62220 [details])
Clearing flags on attachment: 62220

Committed r63854: <http://trac.webkit.org/changeset/63854>
------- Comment #25 From 2010-07-21 13:39:37 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #26 From 2010-07-21 14:11:42 PST -------
Some tests in fast/backgrounds were missed.  The pixel tests on chromium found them.  I'll make a patch to fix.
------- Comment #27 From 2010-07-26 12:46:14 PST -------
This completely broke the layout of Safari RSS pages.  See bug 42990 for details.
------- Comment #28 From 2010-07-26 12:52:33 PST -------
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.
------- Comment #29 From 2010-07-26 13:36:57 PST -------
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.
------- Comment #30 From 2010-07-26 13:54:16 PST -------
I did that roll-out in <http://trac.webkit.org/changeset/64071>.  Reopening.
------- Comment #31 From 2010-07-27 04:59:37 PST -------
(From update of attachment 61779 [details])
Cleared Darin Adler's review+ from obsolete attachment 61779 [details] so that this bug does not appear in http://webkit.org/pending-commit.
------- Comment #32 From 2010-07-27 04:59:46 PST -------
(From update of attachment 62213 [details])
Cleared Darin Adler's review+ from obsolete attachment 62213 [details] so that this bug does not appear in http://webkit.org/pending-commit.
------- Comment #33 From 2012-04-25 04:25:01 PST -------
abarth took over and landed http://trac.webkit.org/changeset/113795.
------- Comment #34 From 2012-04-25 09:53:19 PST -------

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