837211 - Add (preffed-off) support for some -webkit CSS properties, as aliases

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

A few months ago I wrote some patches to support some -webkit- prefixed CSS properties in order to help with analysis of Web sites that weren't functioning correctly in Firefox for Android (i.e., figuring out whether certain failures were solely due to -webkit- prefixed properties).

It turns out that this is useful for debugging site problems as well; the easiest way to test whether a particular site problem is due to -webkit- prefixes or due to other reasons is to see what happens if those -webkit- prefixes are supported.  Likewise, it may also be useful for further analysis work like the analysis we did a few months ago.

So in order to help debug Web sites that aren't functioning correctly in Firefox and analyze how many of the problems are due to different -webkit- prefixed technology (or combinations thereof), I'd like to land this patch in mozilla-central, behind an off-by-default preference.  This would allow those who need to test -webkit- prefix support to simply flip the pref in order to test the behavior of a page.

Comment 1

[Victor Porof [:vporof][:vp]]

I have concerns with exposing such support in a release, even behind an off-by-default pref. Consider this scenario: an addon that sets the pref on by default is released (under whichever name variation or camouflage possible: "Webkit Firefox", "Compatible Fox", etc.). This addon gets increasingly popular due to the lack of public knowledge regarding what vendor prefixes are and the simple perceived effect that "it fixes firefox". The end situation is a presumably redundant off-by-default pref and a consistent "meh, -webkit actually *can* and *does* work anywhere" mentality.

I very much agree that the above is a pessimistic scenario, but plausible. Restricting the functionality up until Aurora (I wouldn't go as far as Beta, even) is safer if such a patch is ever going to land.

Comment 2

[Jason Smith [:jsmith]]

(In reply to Victor Porof [:vp] from comment #1)
> I have concerns with exposing such support in a release, even behind an
> off-by-default pref. Consider this scenario: an addon that sets the pref on
> by default is released (under whichever name variation or camouflage
> possible: "Webkit Firefox", "Compatible Fox", etc.). This addon gets
> increasingly popular due to the lack of public knowledge regarding what
> vendor prefixes are and the simple perceived effect that "it fixes firefox".
> The end situation is a presumably redundant off-by-default pref and a
> consistent "meh, -webkit actually *can* and *does* work anywhere" mentality.
> 
> I very much agree that the above is a pessimistic scenario, but plausible.
> Restricting the functionality up until Aurora (I wouldn't go as far as Beta,
> even) is safer if such a patch is ever going to land.

Certainly, I think that's a valid concern. The only need for site investigation is an ability to do it, but it's not needed on a release channel. In fact, we could have this pref available for Nightly only. That would be sufficient and not take the risks you note.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yeah, I was thinking about something like that, though I was concerned about the risk of ending up with something that doesn't compile when it's merged to Beta on merge day.

Comment 4

[hsablonniere]

Could this be done outside of the browser? With some kind of prefix-free like bookmarklet?

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to hsablonniere from comment #4)
> Could this be done outside of the browser? With some kind of prefix-free
> like bookmarklet?

Not with anywhere near the same accuracy (e.g., most interactions with script).

Comment 6

[Masatoshi Kimura [:emk]]

bool PrefEnabled()
{
#ifdef MOZ_RELEASE_BUILD
  return false;
#else
  return Preferences::GetBool("layout.css.webkit-prefix.enabled");
#endif
}

would be safe enough.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 1: Add ALPHA_BUILD as the opposite of RELEASE_BUILD so we can be careful about having included the definitions we want, and define MOZ_RELEASE_BUILD and MOZ_ALPHA_BUILD in C++.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2: Add flags field to CSS_PROP_ALIAS macro.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 3: Add flag to control whether CSS properties are enabled (applicable to aliases as well), so that properties can be disabled without substantive code change.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 4: Off-by-default preference, available only in nightly/aurora versions, for testing the effect of supporting -webkit- prefixes or unprefixed properties.

Note that this does not emulate the WebKit quirk of supporting
element.style["-webkit-animation"] or supporting the uppercase
element.style.WebkitAnimation (etc.) as opposed to the lowercase (and
enumerable) element.style.webkitAnimation.

Note that this also does not add aliases for transition or animation
events.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 5: Add test that -webkit- prefixes are unsupported.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 733679 [details] [diff] [review]
patch 2:  Add flags field to CSS_PROP_ALIAS macro.

r=me

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 733680 [details] [diff] [review]
patch 3:  Add flag to control whether CSS properties are enabled (applicable to aliases as well), so that properties can be disabled without substantive code change.

>+            raise StandardError("unexpected flag" + flag)

You probably want a space before the closing '"'.

r=me

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 733681 [details] [diff] [review]
patch 4:  Off-by-default preference, available only in nightly/aurora versions, for testing the effect of supporting -webkit- prefixes or unprefixed properties.

r=me

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 733682 [details] [diff] [review]
patch 5:  Add test that -webkit- prefixes are unsupported.

r=me

Comment 16

[(not currently active) Ted Mielczarek]

Comment on attachment 733678 [details] [diff] [review]
patch 1:  Add ALPHA_BUILD as the opposite of RELEASE_BUILD so we can be careful about having included the definitions we want, and define MOZ_RELEASE_BUILD and MOZ_ALPHA_BUILD in C++.

Review of attachment 733678 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +3911,5 @@
>      GRE_MILESTONE=`tail -n 1 "$_topsrcdir"/config/milestone.txt 2>/dev/null || tail -1 "$_topsrcdir"/config/milestone.txt`
>  fi
>  AC_SUBST(GRE_MILESTONE)
>  
> +if echo "$GRE_MILESTONE" | grep -q "a"; then

We should probably just consolidate this and the rules.mk logic.

@@ +3914,5 @@
>  
> +if echo "$GRE_MILESTONE" | grep -q "a"; then
> +    AC_DEFINE(MOZ_ALPHA_BUILD)
> +else
> +    AC_DEFINE(MOZ_RELEASE_BUILD)

Can we make these match the PREF_PPFLAGS versions? Having one with a MOZ_ prefix and the other without seems confusing.

Comment 17

[Sunil]

Hi, what is the status of this bug? Is this already checked into nightly builds? If not, any ETA?

Thanks.

Comment 18

[Aaron Train [:aaronmt]]

(In reply to Sunil from comment #17)
> Hi, what is the status of this bug? Is this already checked into nightly
> builds? If not, any ETA?
> 
> Thanks.

It is a work in progress as per the review above. There is nothing checked in.

Comment 19

[Sunil]

Hi, 
Is there any interest in taking this to completion just to see how many sites might get fixed in Firefox OS? I am more looking at Gmail to begin with.

Moreover, there's a comment in one of the attachments 'Note that this also does not add aliases for transition or animation events.'

Is there any guidance how to go about doing that if I want to play with the patch and I can report back.

Thanks.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

My current inclination is not to land the patches; apologies for wasting code reviewers' time on them.  There seemed to me to be less interest in doing this sort of testing, and I'm also more concerned about other parties enabling -webkit- prefixes without talking to us if the patch were in.

(I'd also need to post a revised patch 1 for re-review; it's in my patch queue, though.)

Comment 21

[Sunil]

IMHO, if this patch was in (and available as part of the nightly builds behind a preference, hence beyond alpha builds), then tech evangelism might become easier as the guilty party can quickly see that if they added support for either non prefixed/moz prefixed CSS prop, then their web app can work on Firefox OS and Fennec too (I know reality is never this easy, but atleast webkit only app developers can quickly try their app on Fennec).  

And potentially also add support for webkit transition and animation events, for the same reason as above. 

In any case, will appreciate pointers on how to add alias for webkit transition and animation events so that I can do some testing.

Thanks.

Comment 22

[Daniel Holbert [:dholbert]]

dbaron's been good enough to keep his patches for this bug unbitrotted in his patch queue.  I'd like to land them, in support of bug 1170774 (for better compatibility with the web-as-it-stands, particularly on mobile).

I'm planning on doing the following:
 (1) Obsoleting patch 1, since we don't want any ALPHA_BUILD dependency for these aliases anymore -- we're planning on shipping them in release builds.
 (2) Posting up-to-date versions of patches 2-5 (taken from dbaron's patch queue) on this bug.
 (3) Sanity-checking the differences between those up-to-date patches vs. the reviewed patches here, and (with dbaron's blessing) fixing up any issues that I notice.
 (4) Landing these patches on dbaron's behalf [with dbaron as patch-author] and closing out this bug.

dbaron, I'm pretty sure you're on board with this, but let me know if you have any thoughts/concerns.

Comment 23

[Daniel Holbert [:dholbert]]

Attachment: patch 2 v2: Add flags field to CSS_PROP_ALIAS macro. [r=bz]

Changes in the updated version of this patch all look good. The changes I'm seeing from diffing this patch vs. its old version on this bug are as follows:

 (1) DO_PROP has been changed (in some other bug) to take 'flags', so this patch-version just directly passes 'flags' through to it, which makes sense.  (In the old patch series, this didn't happen until part 3.)

 (2) CSS_PROP_ALIAS expressions are already multi-line now, so this patch doesn't have to make them multi-line anymore. (In the old patch version, the replaced expressions were giant one-liners.)

 (3) We have more CSS_PROP_ALIAS expressions than we used to. (And we're adding a 0 flags value to each of them.)

These diffs all seem sensible.

Comment 24

[Daniel Holbert [:dholbert]]

Attachment: patch 3 v2: Add flag to control whether CSS properties are enabled (applicable to aliases as well), so that properties can be disabled without substantive code change. [r=bz]

Changes in the updated version of this patch (ignoring contextual differences:
- Typo fix in some new python code (s/false/False/ & whitespace fix)
- No need to add 'flags' anymore since it's already present, as noted above.
- The numeric value of CSS_PROPERTY_DISABLED_BY_DEFAULT is larger, since its previous value has been taken.

All good.

Comment 25

[Daniel Holbert [:dholbert]]

Attachment: patch 4 v2: Off-by-default preference, for supporting -webkit- prefixes or unprefixed properties [r=bz]

The only non-contextual change in this patch is that MOZ_ALPHA_BUILD has been renamed to ALPHA_BUILD, which is unimportant because we're not going to use that #define anyway, per above notes about skipping patch 1.

I made one change to this patch -- I edited the commit message to remove the mention of nightly/aurora.  I'll post an interdiff with my own minor code-tweaks (mostly removing that nightly/alpha restriction) which I plan to fold into this patch when landing.

Comment 26

[Daniel Holbert [:dholbert]]

Attachment: interdiff for dholbert's tweaks to patch 4

Here are my tweaks to part 4, which I plan to fold in before landing:
 - I removed the ALPHA_BUILD switch (leaving behind the formerly-ALPHA_BUILD-only code).
 - I moved the -webkit aliases to the bottom nsCSSPropAliasList.h (where they were, in the previously-reviewed patch version here), so that we don't end up with -moz aliases followed by -webkit aliases followed by more -moz aliases.

(Let me know if you meant for the order to be as it was in your unbitrotted version -- I'm assuming it wasn't an explicit choice & was just an artifact of unbitrotting.)

Note: for simplicity, I'm keeping the preference name ("layout.css.test-webkit-prefixes") unchanged for now, but we'll probably want to rename it to something more definitive-sounding before this pref ships.

Comment 27

[Daniel Holbert [:dholbert]]

Attachment: patch 5 v2: Add test that -webkit- prefixes are unsupported. [r=bz]

Changes in the updated version of this patch:
 - updated to modern mochitest manifest-file.

(Note: since ultimately we *will* want to support these properties by default, we'll need to tweak this mochitest at that point.  Perhaps we'll toggle the pref to disable these properties before running the mochitest, or something.)

Comment 28

[Daniel Holbert [:dholbert]]

I imagine we probably want to remove the unprefixed properties from part 4 before landing, too ("text-size-adjust", "appearance", "user-select").

We can always add them back later if web-compat demands it; but for now, it's probably simplest to only concern ourselves with -webkit prefixed properties here.

Comment 29

[Daniel Holbert [:dholbert]]

Note: I needed to do a bit more rebasing locally this morning in part 1, to fix bitrot from bug 1206569 [which, among other things, renamed CSS2PropertiesProps.h to PythonCSSProps.h]. I won't bother to repost just for that bitrot-fix at this point.

Also: the current patch stack hit a silly test-failure in a preliminary Try run that I kicked off last night[1], and I filed bug 1208136 to fix that.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0e85628c5ce

Comment 30

[Daniel Holbert [:dholbert]]

Here's a green try run, with these patches layered on top of my patch in bug 1208136, FWIW (on one platform):
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=4675d846127b

Comment 31

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #26)
> Note: for simplicity, I'm keeping the preference name
> ("layout.css.test-webkit-prefixes") unchanged for now

(I'm tentatively planning on renaming this to "layout.css.webkit-prefixed-aliases.enabled" -- that's what I'm using locally.)

Comment 32

[Daniel Holbert [:dholbert]]

Two more small changes I've made locally in my interdiff for part 4:
 (1) I'm naming the pref "layout.css.prefixes.webkit" for this feature now, since that's how we've been naming preferences for prefixed css features: http://mxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.js?rev=f33b94206b39#2292

 (2) I'll now be adding this pref to libpref/init/all.js , set to false by default, instead of making it a "hidden" pref. (Otherwise we'll fail tests -- intentionally, as of the patch for bug 1208136.)

(I won't bother re-requesting review for these changes, though if anyone objects to this pref name, feel free to speak up; and we can always adjust it later on, too.)

Comment 33

[Daniel Holbert [:dholbert]]

dbaron: just to be on the safe side, could you confirm that you're OK with me pushing your unbitrotted patches here? (with my interdiff tweaks folded in)

I'm confident that they're landable, particularly given that this will be preffed off by default for now; but I want to make sure I've got your approval to land them.

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I guess I don't think there's any need for patches 2 and 3 anymore.  (They also somewhat conflict with the work in bug 1069192, which adds a different mechanism to do roughly the same thing.)  And probably not patch 5 either, actually.

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Daniel Holbert [:dholbert] from comment #32)
> Two more small changes I've made locally in my interdiff for part 4:
>  (1) I'm naming the pref "layout.css.prefixes.webkit" for this feature now,
> since that's how we've been naming preferences for prefixed css features:
> http://mxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.
> js?rev=f33b94206b39#2292
> 
>  (2) I'll now be adding this pref to libpref/init/all.js , set to false by
> default, instead of making it a "hidden" pref. (Otherwise we'll fail tests
> -- intentionally, as of the patch for bug 1208136.)

Those both sound fine.

Comment 36

[Daniel Holbert [:dholbert]]

(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #34)
> I guess I don't think there's any need for patches 2 and 3 anymore.  (They
> also somewhat conflict with the work in bug 1069192, which adds a different
> mechanism to do roughly the same thing.)

OK, thanks -- that makes sense.

> And probably not patch 5 either, actually.

Sure, since we're planning on enabling these before too long, no sense in adding a test that asserts they're disabled.  Sounds good.

I'll plan on landing part 4 on its own, then -- with the interdiff folded in -- after a local sanity-check to make sure it works as expected & a run through Try.

Comment 37

[Daniel Holbert [:dholbert]]

Green try run, with this bug's tweaked 'part 4' patch & bug 1211101's mochitests for these properties (property_database.js additions):  https://treeherder.mozilla.org/#/jobs?repo=try&revision=54d02f56d9e2

Comment 38

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e44fb274be90

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e44fb274be90

Comment 43

[Daniel Holbert [:dholbert]]

[updating summary to reflect the final intent of this bug (this is no longer just for debugging site problems)]

Comment 45

[Sebastian Zartner [:sebo]]

I've added compatibility notes to the pages below:

https://developer.mozilla.org/en-US/docs/Web/CSS/animation
https://developer.mozilla.org/en-US/docs/Web/CSS/animation-delay
https://developer.mozilla.org/en-US/docs/Web/CSS/animation-direction
https://developer.mozilla.org/en-US/docs/Web/CSS/animation-duration
https://developer.mozilla.org/en-US/docs/Web/CSS/animation-fill-mode
https://developer.mozilla.org/en-US/docs/Web/CSS/animation-iteration-count
https://developer.mozilla.org/en-US/docs/Web/CSS/animation-name
https://developer.mozilla.org/en-US/docs/Web/CSS/animation-play-state
https://developer.mozilla.org/en-US/docs/Web/CSS/animation-timing-function
https://developer.mozilla.org/en-US/docs/Web/CSS/text-size-adjust
https://developer.mozilla.org/en-US/docs/Web/CSS/transform
https://developer.mozilla.org/en-US/docs/Web/CSS/transform-origin
https://developer.mozilla.org/en-US/docs/Web/CSS/transform-style
https://developer.mozilla.org/en-US/docs/Web/CSS/transition
https://developer.mozilla.org/en-US/docs/Web/CSS/transition-delay
https://developer.mozilla.org/en-US/docs/Web/CSS/transition-duration
https://developer.mozilla.org/en-US/docs/Web/CSS/transition-property
https://developer.mozilla.org/en-US/docs/Web/CSS/transition-timing-function
https://developer.mozilla.org/en-US/docs/Web/CSS/border-radius
https://developer.mozilla.org/en-US/docs/Web/CSS/border-top-left-radius
https://developer.mozilla.org/en-US/docs/Web/CSS/border-top-right-radius
https://developer.mozilla.org/en-US/docs/Web/CSS/border-bottom-left-radius
https://developer.mozilla.org/en-US/docs/Web/CSS/border-bottom-right-radius
https://developer.mozilla.org/en-US/docs/Web/CSS/-moz-appearance
https://developer.mozilla.org/en-US/docs/Web/CSS/background-clip
https://developer.mozilla.org/en-US/docs/Web/CSS/background-origin
https://developer.mozilla.org/en-US/docs/Web/CSS/background-size
https://developer.mozilla.org/en-US/docs/Web/CSS/border-image
https://developer.mozilla.org/en-US/docs/Web/CSS/box-shadow
https://developer.mozilla.org/en-US/docs/Web/CSS/box-sizing
https://developer.mozilla.org/en-US/docs/Web/CSS/user-select

And I added a release note to 44:

https://developer.mozilla.org/en-US/Firefox/Releases/44

David, can you please have a look whether that's ok?

Sebastian

Comment 46

[Sebastian Zartner [:sebo]]

Daniel, as David seems to be quite busy, could you please help and quickly review the pages I listed in comment 45?

Sebastian

Comment 47

[Daniel Holbert [:dholbert]]

Sorry, I'm quite busy up as well (in part because I might be out of the office for some or all of next week to serve on a jury, depending on whether I'm called in or not -- and I've got a backlog of other work & requests to get through before then).

(I'll leave this in my needinfo queue, but I might not get to it right away.)

Comment 48

[Sebastian Zartner [:sebo]]

Fine for me. Take your time. I just wanted to make sure it's not forgotten.
(And getting a "I'm busy" answer is better than no answer. :-) )

Sebastian

Comment 49

[Mike Taylor [:miketaylr]]

> (In reply to Sebastian Zartner [:sebo] from comment #45)
> I've added compatibility notes to the pages below:
...
> https://developer.mozilla.org/en-US/docs/Web/CSS/-moz-appearance
....
> 

It looks like you're just documenting the aliases added by this patch (I see some of the ones added later like -webkit-filter are also documented) -- everything looks good except -moz-appearance. That was removed in Bug 1249134.

Comment 50

[Sebastian Zartner [:sebo]]

Sorry for the delay! 

(In reply to Mike Taylor [:miketaylr] from comment #49)
> > (In reply to Sebastian Zartner [:sebo] from comment #45)
> > I've added compatibility notes to the pages below:
> ...
> > https://developer.mozilla.org/en-US/docs/Web/CSS/-moz-appearance
> ....
> > 
> 
> It looks like you're just documenting the aliases added by this patch (I see
> some of the ones added later like -webkit-filter are also documented)

Yes, documentation for other aliases was done separately for each of the bugs scattered over Bugzilla (-webkit-filter was implemented in bug 1236506). Unfortunately not all aliases are referenced correctly and implementers tend to forget to set the dev-doc-needed flag, which made them a bit hard to find.
So, if you see -webkit alias bugs I've missed to document, please add the dev-doc-needed flag, so their documentation can be tracked.

> everything looks good except -moz-appearance. That was removed in Bug
> 1249134.

Thank you for the info! I've removed the note from -moz-appearance again.
With that done I set this to dev-doc-complete now.

Sebastian