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

VERIFIED FIXED in Firefox 46

Status

()

Core
DOM: CSS Object Model
VERIFIED FIXED
2 years ago
10 months ago

People

(Reporter: miketaylr, Assigned: dholbert)

Tracking

(Blocks: 1 bug)

unspecified
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 unaffected, firefox46 fixed, firefox47 fixed)

Details

(URL)

Attachments

(5 attachments, 4 obsolete attachments)

68.45 KB, text/plain
Details
3.64 KB, patch
Details | Diff | Splinter Review
1.42 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
3.82 KB, patch
Details | Diff | Splinter Review
1.46 KB, patch
Details | Diff | Splinter Review
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)
ni? Daniel, per https://bugzilla.mozilla.org/show_bug.cgi?id=1240611#c10.
Flags: needinfo?(dholbert)
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.
(Just playing around in the Safari console only currently support 'webkitFoo' and 'WebkitFoo' capitalizations. So hooray to them for fixing that bug.)
Blocks: 1241588
Blocks: 1244464
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.)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Flags: needinfo?(bzbarsky)
Created attachment 8715078 [details] [diff] [review]
part 1: refactor existing logic

Yeah, the strategy described above seems to work.

Here's the first patch -- just refactoring the existing logic.
Flags: needinfo?(dholbert)
Flags: needinfo?(bzbarsky)
Attachment #8715078 - Flags: review?(bzbarsky)
Created attachment 8715079 [details] [diff] [review]
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.
Attachment #8715079 - Flags: review?(bzbarsky)
Created attachment 8715080 [details]
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.)
Attachment #8715080 - Attachment mime type: text/x-csrc → text/plain
Attachment #8715078 - Attachment is patch: true
Created attachment 8715090 [details] [diff] [review]
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.
Attachment #8715078 - Attachment is obsolete: true
Attachment #8715079 - Attachment is obsolete: true
Attachment #8715078 - Flags: review?(bzbarsky)
Attachment #8715079 - Flags: review?(bzbarsky)
Attachment #8715090 - Flags: review?(bzbarsky)
Created attachment 8715091 [details] [diff] [review]
part 2: give webkit-prefixed properties an extra (capitalized) entry in CSS2Properties.webid
Attachment #8715091 - Flags: review?(bzbarsky)
Attachment #8715090 - Attachment description: part 1: refactor existing logic → part 1: refactor existing logic into a helper function (to generate each line of WebIDL)
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.
Attachment #8715090 - Flags: review?(bzbarsky) → review+
Comment on attachment 8715091 [details] [diff] [review]
part 2: give webkit-prefixed properties an extra (capitalized) entry in CSS2Properties.webid

r=me
Attachment #8715091 - Flags: review?(bzbarsky) → review+
(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.
Created attachment 8715444 [details] [diff] [review]
part 1, v2: refactor existing logic into a helper function (to generate each line of WebIDL)
Attachment #8715444 - Flags: review?(bzbarsky)
Created attachment 8715450 [details] [diff] [review]
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+.)
Attachment #8715090 - Attachment is obsolete: true
Attachment #8715091 - Attachment is obsolete: true
Attachment #8715450 - Flags: review+
I verified that the new patches generate an identical CSS2Properties.webidl file to the old patches, BTW (attached in comment 7).
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
Attachment #8715444 - Flags: review?(bzbarsky) → review+
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.
Created 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
Attachment #8715535 - Flags: review?(bzbarsky)
Created 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

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).
Attachment #8715540 - Flags: review?(bzbarsky)
Attachment #8715540 - Attachment description: part 4: xtend mochitest test_initial_storage.html to test that uppercase Webkit DOM accessors for css properties are functional → part 4: Extend mochitest test_initial_storage.html to test that uppercase Webkit DOM accessors for css properties are functional
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}`
Attachment #8715535 - Flags: review?(bzbarsky) → review+
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.
Attachment #8715540 - Flags: review?(bzbarsky) → review+
Thanks, I made that change & confirmed that the test still works as-expected.
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2fda42c1e26

Comment 24

2 years ago
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

2 years ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
BTW: this didn't quite make it into today's Nightly -- but it'll be in the next one.
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.
Attachment #8715444 - Flags: approval-mozilla-aurora?
Webkit related work aimed at 46 aurora, marking affected for 46.
status-firefox45: --- → unaffected
status-firefox46: --- → affected
Duplicate of this bug: 1244464
No longer blocks: 1244464
No longer blocks: 1241588
Duplicate of this bug: 1241588
No longer blocks: 1240611
Duplicate of this bug: 1240611
Verifying that this fixes known WebKit support regressions -- thanks Daniel!
Status: RESOLVED → VERIFIED
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.
Attachment #8715444 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
All 4 patches please for uplift.

Comment 35

2 years ago
bugherderuplift
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
status-firefox46: affected → fixed

Updated

2 years ago
Depends on: 1248444
Blocks: 1259345

Updated

10 months ago
Depends on: 1332281
You need to log in before you can comment on or make changes to this bug.