1241021 - Support camel-cased and webkit-cased CSSStyleDeclaration attributes for getting & setting WebKit prefixed styles

Status: VERIFIED FIXED

(Core :: DOM: CSS Object Model, defect)

Description

[Mike Taylor [:miketaylr]]

Currently we support the following:

>> document.body.style['webkitTransition'] = 'all 1s'
'all 1s'
>> document.body.style['webkitTransition']
'all 1s'

But the following fails to update DOM:

>> document.body.style['WebkitTransition'] = 'all 2s'


Bug 1240611 depends on this, as well as any site using jQuery.fn.css('-webkit-thing', val)

Comment 1

[Mike Taylor [:miketaylr]]

ni? Daniel, per https://bugzilla.mozilla.org/show_bug.cgi?id=1240611#c10.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Note that last I checked in WebKit this would work too:

  document.body.style['wEbkITtranSITION'] = 'all 1s'

I'm really hoping we don't have to support that....

We _can_ add support for the 'WebkitFoo' capitalization style.  We wouldn't even need to BinaryName it to anything, since it would automatically end up calling the same thing as the camelCased style on the C++ side.

Comment 3

[Mike Taylor [:miketaylr]]

(Just playing around in the Safari console only currently support 'webkitFoo' and 'WebkitFoo' capitalizations. So hooray to them for fixing that bug.)

Comment 4

[Daniel Holbert [:dholbert]]

After bug 1236979, this one's probably the next-highest cause of bustage from enabling webkit prefix support. --> Taking.

I think (?) all we need to do here is get the capitalized version to end up in the auto-generated file $OBJDIR/dom/bindings/CSS2Properties.webidl .  I'm hoping we can just include two duplicate lines in that file (one capitalized, one uncapitalized).

Looks like the way to do that is to add a special-case to this file:
 http://mxr.mozilla.org/mozilla-central/source/dom/bindings/GenerateCSS2PropertiesWebIDL.py

Note that it already has a special-case for properties with |prop.startswith("Moz")|. We can just add another special-case for |prop.startswith("Webkit")|.

(bz, can you confirm that the above plan is sane & I'm not missing something? Also, I'll probably tag you for review, if that's OK.)

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: part 1: refactor existing logic

Yeah, the strategy described above seems to work.

Here's the first patch -- just refactoring the existing logic.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: part 2: give webkit-prefixed properties an extra (capitalized) entry in CSS2Properties.webidl

and here's the second patch, to give these properties a second capitalized entry in this webidl file.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: the new CSS2Properties.webidl

Here's what CSS2Properties.webidl looks like after this change, BTW. (Note the back-to-back lines for each webkit property.)

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: part 1: refactor existing logic into a helper function (to generate each line of WebIDL)

Er, the previous 'part 1' added a stray/harmless semicolon, and `part 2` removed it.

--> Reposting patches, now without the semicolon ever existing.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: part 2: give webkit-prefixed properties an extra (capitalized) entry in CSS2Properties.webid

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8715090 [details] [diff] [review]
part 1: refactor existing logic into a helper function (to generate each line of WebIDL)

>+def generatePropLine (name, prop, id, flags, pref, proptype):

Drop the space before '(', please.

generatePropLine is not quite right either, since it generates two lines for some properties (e.g. anything that has a '-' in it but isn't prefixed, or "float").

We could hoist hoist the |if prop != name and name[0] != "-":| block and the preceding comment out of generatePropLine, and just make sure to always output the BinaryName bits and the '_' prefix on the identifier; I think they should be no-ops in the case when name == prop.

Alternately, we could come up with a better name for generatePropLine that doesn't imply it only generates one line.

r=me either way, but I'd like to see the updated patch before you check it in, especially if you take the more-refactoring approach.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8715091 [details] [diff] [review]
part 2: give webkit-prefixed properties an extra (capitalized) entry in CSS2Properties.webid

r=me

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to Boris Zbarsky [:bz] from comment #10)
> Comment on attachment 8715090 [details] [diff] [review]
> part 1: refactor existing logic into a helper function (to generate each
> line of WebIDL)
> 
> >+def generatePropLine (name, prop, id, flags, pref, proptype):
> 
> Drop the space before '(', please.

Oops, sorry about that. (thanks for catching)

> generatePropLine is not quite right either, since it generates two lines for
> some properties

Good point. I've re-refactored this so that most of the logic is in the caller, and this function now really does just generate one line.  (I've also shortened the name to just "generateLine", to avoid any allegiance to 'prop' vs. 'name' variables.)

New patches coming up.

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: part 1, v2: refactor existing logic into a helper function (to generate each line of WebIDL)

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: part 2, v2: give webkit-prefixed properties an extra (capitalized) entry in CSS2Properties.webid [r=bz]

Here's the updated part 2, rebased on the new part 1. (No serious changes since previous version, so carrying forward r+.)

Comment 15

[Daniel Holbert [:dholbert]]

I verified that the new patches generate an identical CSS2Properties.webidl file to the old patches, BTW (attached in comment 7).

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8715444 [details] [diff] [review]
part 1, v2: refactor existing logic into a helper function (to generate each line of WebIDL)

r=me

Comment 17

[Daniel Holbert [:dholbert]]

Thanks for the review!

I have two small automated-test patches coming up (to get these DOM accessors tested alongside the normal ones in test_initial_storage.html), too.

Comment 18

[Daniel Holbert [:dholbert]]

Attachment: part 3: Refactor CSS mochitest test_initial_storage.html to perform its DOM accessor consistency-checks via a helper-function

Comment 19

[Daniel Holbert [:dholbert]]

Attachment: part 4: Extend mochitest test_initial_storage.html to test that uppercase Webkit DOM accessors for css properties are functional

The idea here is that part 3 refactors out the DOM accessor checks to all happen in a single function, so that part 4 only has to modify that one spot to add a special case for webkit aliases (which gets it tested at various points).

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8715535 [details] [diff] [review]
part 3: Refactor CSS mochitest test_initial_storage.html to perform its DOM accessor consistency-checks via a helper-function

r=me, though using backtick strings here could have been nicer for the message:

  `(${messagePrefix}) consistency between decl.getPropertyValue(${sproperty}) and decl.${sinfo.domProp}`

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8715540 [details] [diff] [review]
part 4: Extend mochitest test_initial_storage.html to test that uppercase Webkit DOM accessors for css properties are functional

r=me with the same note about backticks.

Comment 22

[Daniel Holbert [:dholbert]]

Thanks, I made that change & confirmed that the test still works as-expected.

Comment 23

[Daniel Holbert [:dholbert]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2fda42c1e26

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4ee0ec4ebbd
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f4f6726f274
https://hg.mozilla.org/integration/mozilla-inbound/rev/8173f9c3a3b9
https://hg.mozilla.org/integration/mozilla-inbound/rev/47cefafd3a2a

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a4ee0ec4ebbd
https://hg.mozilla.org/mozilla-central/rev/4f4f6726f274
https://hg.mozilla.org/mozilla-central/rev/8173f9c3a3b9
https://hg.mozilla.org/mozilla-central/rev/47cefafd3a2a

Comment 26

[Daniel Holbert [:dholbert]]

BTW: this didn't quite make it into today's Nightly -- but it'll be in the next one.

Comment 27

[Daniel Holbert [:dholbert]]

Comment on attachment 8715444 [details] [diff] [review]
part 1, v2: refactor existing logic into a helper function (to generate each line of WebIDL)

Requesting approval to get this fixed on Aurora, since we're still early in the Aurora release cycle, and I'd like for our Developer Edition audience to have a better experience and not run into site breakage due to this bug.

Approval Request Comment [Applies to all 4 patches here]
[Feature/regressing bug #]: Webkit-prefixed CSS support (enabled in bug 1213126. This feature will be automatically disabled in beta & release builds for now, as of bug 1238827, so neither this regression nor this fix should end up affecting beta or release. So, this backport is purely to make life better for our Aurora users.)

[User impact if declined]:
 *  Site breakage, when webkit CSS support is enabled (as it is on Aurora), at any site that checks for webkit CSS support and then expects it can do something like "jQuery.fn.css('-webkit-thing', val)" (which JQuery translates into an uppercase WebkitThing DOM accessor under the hood).
 * In particular: breakage in sites which use the "RoyalSlider" photo-gallery JS library; see bug 1240611, bug 1244464.

[Describe test coverage new/current, TreeHerder]:
 * Tests are included to be sure that the new DOM accessors are active & that they work.

[Risks and why]:
 * Low-risk. This is effectively a minimal change to a list of web-exposed DOM getters (adding new ones with a different capitalization alongside existing ones).  (Though the fix happens via modifying the python script which generates that list.)
 * There's a small risk that sites could break due to seeing the new API (which can happen with any web-facing API change).  For example, if a site checks for a "WebkitFoo" DOM accessor (which we'll now have), and then does something that we don't support. I'm skeptical that we'll see much of this sort of thing, though, and I think there will be a much greater number of sites that become *un*-broken as a result of this change than sites that become broken.

[String/UUID change made/needed]: None.

Comment 28

[Liz Henry (:lizzard) (relman/hg->git project)]

Webkit related work aimed at 46 aurora, marking affected for 46.

Comment 32

[Mike Taylor [:miketaylr]]

Verifying that this fixes known WebKit support regressions -- thanks Daniel!

Comment 33

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8715444 [details] [diff] [review]
part 1, v2: refactor existing logic into a helper function (to generate each line of WebIDL)

Support for WebKit, includes new tests; please uplift to aurora.

Comment 34

[Liz Henry (:lizzard) (relman/hg->git project)]

All 4 patches please for uplift.

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/814bf3adecda
https://hg.mozilla.org/releases/mozilla-aurora/rev/44db9d762d0e
https://hg.mozilla.org/releases/mozilla-aurora/rev/243fd5ca8437
https://hg.mozilla.org/releases/mozilla-aurora/rev/4d0523077f33