Closed Bug 958887 Opened 10 years ago Closed 10 years ago

Add support for element.style["css-property-name"] non-standard extension

Categories

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

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
fennec + ---

People

(Reporter: rennie.degraaf, Assigned: bzbarsky)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [parity-IE][parity-webkit][parity-blink])

Attachments

(6 files)

Attached file Test case
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131210132650

Steps to reproduce:

Changing the CSS property background-image via JavaScript does not seem to work in firefox 26.0. It does work in Chromium 27.

The attached test case sets the background-image of a div to a red X, then changes it to a green checkmark using a script. Loading it in Chromium shows the green checkmark; loading it in Firefox shows the red x indicating that the script had no effect. The images used in the test case are SVG, but I got the same result with PNG images.  

Setting the background property, rather than the background-image property, works.


Actual results:

Background image did not change.


Expected results:

Background image should have changed.
https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Properties_Reference

Dashes aren't viable for JavaScript property names, so lots of them are different from the name in CSS. Generally, the dashes are dropped and the names are switched to camel-case. The "background-image" JS property is "backgroundImage". I'm surprised style["background-image"] instead of style.backgroundImage works in Chromium.
Summary: Changing background-image with JavaScript doesn't work → Changing element.style["background-image"] with JavaScript doesn't work
Component: Untriaged → JavaScript Engine
OS: Linux → All
Product: Firefox → Core
Hardware: x86_64 → All
Version: 26 Branch → Trunk
Still invalid per <http://dev.w3.org/csswg/cssom/#cssstyledeclaration>, it seems.
Component: JavaScript Engine → DOM: CSS Object Model
Whiteboard: DUPEME
related to Bug 947887, Bug 573880 ?
> It does work in Chromium 27.

See http://lists.w3.org/Archives/Public/www-style/2012Feb/0655.html but basically Trident, WebKit, and Blink have a non-standard extensions where in addition to the standardized properties on CSSDeclaration they also implement a bunch of other properties.  Gecko and Presto don't do the non-standard bits here.

Of course the only response to my mail was sidetracking by Mr. Adams, about as expected, and in particular no response from anyone at Apple, Google, or Microsoft.  So maybe we should just give up, also implement this non-standard extension, and wait for the spec to catch up.
Flags: needinfo?(zcorpan)
Oh.  I guess I don't adequately understand the difference between foo.style[prop] and foo.style.getPropertyValue(prop).

Sorry for the entropy, and keep up the good work!
IIRC, WebKit/Blink behavior is also case-insensitive, at least in some places (I think for both the hyphenated and camelCased names, though maybe only the camelCased ones).
Whiteboard: DUPEME → [parity-IE] [parity-webkit]
It's not case-insensitive as far as I can tell. http://software.hixie.ch/utilities/js/live-dom-viewer/saved/2732
> Also see https://code.google.com/p/chromium/issues/detail?id=290055

Which is basically being ignored as far as I can tell, right?  :(
The main spec issue here, btw, is whether these properties live on the prototype or on the object itself....
I think there's at least enough desire to mark this bug as an RFE to consider adding this. The JS prop names have always been mildly annoying/confusing and this extension does make some sense.

(In reply to Boris Zbarsky [:bz] from comment #4)
> So maybe we should just give up, also implement this
> non-standard extension, and wait for the spec to catch up.

Is there a bug filed somewhere to add this to spec?
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Changing element.style["background-image"] with JavaScript doesn't work → Add support for element.style["css-property-name"] non-standard extension
Whiteboard: [parity-IE] [parity-webkit] → [parity-IE][parity-webkit][parity-blink]
> Is there a bug filed somewhere to add this to spec?

I haven't filed one.
(In reply to Boris Zbarsky [:bz] from comment #9)
> Which is basically being ignored as far as I can tell, right?  :(

No, it just takes some time to get data from the use counter. It should be available now I think, I've asked davve to have a look.

(In reply to Dave Garrett from comment #11)
> Is there a bug filed somewhere to add this to spec?

There was a thread on www-style and I did add it to the spec, but it's currently commented out in the spec source. There's also https://www.w3.org/Bugs/Public/show_bug.cgi?id=17098
(In reply to Simon Pieters from comment #8)
> It's not case-insensitive as far as I can tell.
> http://software.hixie.ch/utilities/js/live-dom-viewer/saved/2732

...however, webkitFoo and WebkitFoo both work, and it currently works to mix dashes and uppercase e.g. style['border-rightColor'].
What about in IE?

If we do standardize this, I'd like to standardize the least-insane subset possible, obviously, and 'border-rightColor' is over the boundary.  ;)
IE11 is case-sensitive for camelCase but case-insensitive for dashed properties. Mixing dashes and not dashes is not supported.

In IE, fontSize lives on the prototype but font-size is on the object. In Blink both are on the object (but that might change as part of https://code.google.com/p/chromium/issues/detail?id=43394 - not sure).

The current spec says fontSize should be on the prototype. The commented out spec for dashed properties says they should also be on the prototype, only lowercase is supported and no mixing of dashes and uppercase.
Sounds good.  That's the intersection of what Trident and Blink do, so should be reasonable.
See Also: → 732355
Seems Chrome's usage stats confirm that this is a common problem: https://code.google.com/p/chromium/issues/detail?id=290055#c9 (though some people question whether it is because pages set both hyphenated and camel case - I can't say I've personally noticed scripts doing that..)

Regarding border-rightColor and friends, I suppose what's happening there is some "fallback" algorithm that does a hyphen-to-camel-case conversion if unknown property names with hyphens in are used. While this looks messy from an author point of view, and would indeed be crazy to implement by adding all possible variations to the prototype, I think it would make sense to support this too if it's just adding a couple of lines to an algorithm.
(In reply to Hallvord R. M. Steen from comment #19)
> Regarding border-rightColor and friends, I suppose what's happening there is
> some "fallback" algorithm that does a hyphen-to-camel-case conversion if
> unknown property names with hyphens in are used. While this looks messy from
> an author point of view, and would indeed be crazy to implement by adding
> all possible variations to the prototype, I think it would make sense to
> support this too if it's just adding a couple of lines to an algorithm.

Mixed forms are no longer supported in Blink. https://code.google.com/p/chromium/issues/detail?id=334562
That's good news, thank you Simon :-)
(Note that this breaks Google Images touch UI, might be worth prioritising because we haven't been able to get them to fix it this far..)
Assuming we want to make the change at all, of course.
Thanks Simon.

> The consensus on  bug 290055  is that we can't drop the corresponding
functionality because of high usage both on the counters and in
libraries.

To me, this suggests we should seriously consider adding support for this (and getting a compat note somewhere in the relevant spec.)
flagging for triage, this would resolve some compatibility problems, such as bug 793216
tracking-e10s: --- → ?
Yeah, now that this is in the spec, we should do this.

bz/heycam -- how much work does this look like?  (I'm not sure whether the wording about what's on the object vs. prototype is hard to do in our webidl setup.)


No idea what it has to do with e10s, though.
Flags: needinfo?(cam)
Flags: needinfo?(bzbarsky)
Sorry, meant to flag for fennec, not e10s
tracking-fennec: --- → ?
tracking-e10s: ? → ---
> bz/heycam -- how much work does this look like?

I was actually just looking into this.  The current spec wording basically treats these properties like any other IDL attribute on the prototype, as far as I can tell.

Implementing that should be pretty simple.  Patches coming up in a bit.
Flags: needinfo?(bzbarsky)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
One note: This implements distinct specialized getters/setters which call the same C++ implementation.

We could try to do something more complicated in codegen where we'd use the specialized getter/setter for the non-dashed version, though the error reporting would look weird.  I wasn't sure whether that complexity was really worth it, though admittedly it would avoid the need for most of part 3.
tracking-fennec: ? → +
Attachment #8490243 - Flags: review?(peterv) → review+
Attachment #8490244 - Flags: review?(peterv) → review+
Attachment #8490245 - Flags: review?(peterv) → review+
Attachment #8490246 - Flags: review?(peterv) → review+
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: