Closed Bug 1246796 Opened 4 years ago Closed 3 years ago

Add support for elm.style['-webkit-foo'] style CSSOM getters/setters

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: miketaylr, Assigned: miketaylr)

References

(Blocks 1 open bug)

Details

(Whiteboard: dom-triaged)

Attachments

(3 files, 2 obsolete files)

As mentioned in https://github.com/webcompat/web-bugs/issues/1039#issuecomment-181595677, Edge, Blink and WebKit all support something like a "webkit prefixed dashed attribute."

I have a simple patch that hacks support in and confirm that it does in fact make the broken carousel work as expected. 

One thing to note, I don't know if it's possible to make CSS2Properties non-enumerable or whatever, but it seems like WebKit/Blink/Edge all have done that, compared to my local build: https://cloudup.com/c-m8MSDWT3c.

(It may be that we implement a newer version of the WebIDL spec and that's expected, I don't know. Looks kinda ugly.)

I think this is the first instance of this kind of bustage we've seen -- I'd like to try to find more data here.
Boris, assuming we feel like we actually need to do this, would there be much more needed than this patch (other than tests)?
Flags: needinfo?(bzbarsky)
Well, and fixes to the comment above, and updates to the relevant specs, right?  ;)

As far as actual code changes, no.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #3)
> Well, and fixes to the comment above, and updates to the relevant specs,
> right?  ;)

Heh, yeah. Let me do some crawling of existing compat issues and JS corpora to see how much we actually need this and we'll go from there.
AFAICT this should be covered in CSSOM by "dashed attribute". e.style['-moz-appearance'] should also work but doesn't currently.
I suppose making the patch do this instead would more closely match the spec and other browsers:

-    if prop != name and name[0] != "-":
+    if prop != name:
Hmm.  We explicitly exclude vendor-prefixed stuff from the "dashed attribute" thing, because it's the right thing to do and because it's what the spec says to do.  I guess we could stop doing that, but it seems really bizarre to shit prefixed stuff all over the web like that.  :(

In terms of the spec, as I said the spec does NOT include prefixed stuff in its dashed attribute thing.  The spec says:

  For each CSS property property that is a supported CSS property, except
  for properties that have no "-" (U+002D) in the property name, the
  following partial interface applies where dashed attribute is property.

where "supported CSS property" is defined as:

  The term supported CSS property refers to a CSS property that the user
  agent implements, and that is defined to be a case-insensitive property
  in the CSS specification. A supported CSS property must be in its lowercase
  form for the purpose of comparisons this specification.

Prefixed stuff fulfills the "user agent implements" condition, but fails the "defined in the CSS specification" condition.
OK, that was not what I intended the spec to require. Filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=29440

I suppose I could make the spec to exclude vendor-specific properties other than -webkit- ones, if you prefer that, though I'd like to check what Edge does for -ms-foo also.
David, thoughts on whether we should just expose -moz stuff here?
Flags: needinfo?(dbaron)
If we're going to expose unprefixed properties and -webkit- properties, we should probably do -moz- ones as well, just because developers will assume that things are consistent.
Flags: needinfo?(dbaron)
Seems Edge is happy to allow elm.style['-ms-blah'] setting too.
Whiteboard: dom-triaged
I re-wrote my top Alexa JS crawler to "beautify" and inline JS and let it run last night -- it only got 16071 domains because the battery died (oops), but here's the results from that:

https://gist.github.com/miketaylr/a014b68938c31619f8c4

671 tokens in Alexa Top 16071 (but note that token != site)
Blocks: 1170774
Seems there's a framework Marfeel using this style of setter: https://bugzilla.mozilla.org/show_bug.cgi?id=1149160#c2
Blocks: 1147352
I guess we should just give up and ship this crap.
I changed the comments here to imply the spec is explicit about this change. Can we get https://www.w3.org/Bugs/Public/show_bug.cgi?id=29440 fixed accordingly?
Flags: needinfo?(zcorpan)
Comment on attachment 8773344 [details] [diff] [review]
Allow getting/setting dashed attributes that start with "-", for web compatibility. r?bz

r=me, but please add tests.
Attachment #8773344 - Flags: review?(bzbarsky) → review+
Cool, will do.
Flags: needinfo?(miket)
Assignee: nobody → miket
Attachment #8773344 - Attachment is obsolete: true
Flags: needinfo?(miket)
Attachment #8717200 - Attachment is obsolete: true
Oranges look truly intermittant, let's push.
Pushed by mitaylor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2a6a2af5243
Allow getting/setting dashed attributes that start with "-", for web compatibility. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/7df92788ffeb
Update test for CSS prop setters/getters. r=me
https://hg.mozilla.org/mozilla-central/rev/f2a6a2af5243
https://hg.mozilla.org/mozilla-central/rev/7df92788ffeb
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
(In reply to Mike Taylor [:miketaylr] from comment #16)
> I changed the comments here to imply the spec is explicit about this change.
> Can we get https://www.w3.org/Bugs/Public/show_bug.cgi?id=29440 fixed
> accordingly?

Now fixed.
Flags: needinfo?(zcorpan)
Thanks!
Blocks: 1149160
You need to log in before you can comment on or make changes to this bug.