Last Comment Bug 374216 - Setting element.style.someSvgProperty has no effect
: Setting element.style.someSvgProperty has no effect
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: mozilla1.9.3a2
Assigned To: Robert Longson
:
Mentors:
http://www.w3.org/TR/DOM-Level-2-Styl...
: 463985 536779 (view as bug list)
Depends on: 523576 544720 613500 631875 644291 644597 649823
Blocks: 390483 544848
  Show dependency treegraph
 
Reported: 2007-03-16 07:48 PDT by Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
Modified: 2011-04-14 21:04 PDT (History)
15 users (show)
jwatt: wanted1.9.2+
longsonr: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (458 bytes, image/svg+xml)
2007-03-16 07:48 PDT, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
no flags Details
Patch (13.88 KB, patch)
2008-02-21 04:23 PST, Takeshi Kurosawa
no flags Details | Diff | Review
Testcase (setting fill-opacity and checking children of element.style) (589 bytes, image/svg+xml)
2008-02-21 20:32 PST, Takeshi Kurosawa
no flags Details
update to tip and add tests (23.45 KB, patch)
2009-10-11 17:27 PDT, Robert Longson
bzbarsky: review-
Details | Diff | Review
address filter issue (27.95 KB, patch)
2009-12-29 11:13 PST, Robert Longson
no flags Details | Diff | Review
remove ifdef (27.92 KB, patch)
2009-12-29 11:52 PST, Robert Longson
dbaron: review+
Details | Diff | Review
without dark magic (22.71 KB, patch)
2010-02-03 12:44 PST, Robert Longson
dbaron: review+
roc: superreview+
Details | Diff | Review

Description Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2007-03-16 07:48:35 PDT
Created attachment 258784 [details]
testcase

Setting SVG CSS properties using the shortcut method doesn't work. For example you should be able to do this:

  element.style.fill = 'lime';

but currently we need to do this:

  element.style.setProperty('fill', 'lime', '');
Comment 1 Takeshi Kurosawa 2008-02-20 19:30:42 PST
http://mxr.mozilla.org/seamonkey/source/layout/style/nsCSSPropList.h#528 says

> // XXX treat SVG's CSS Properties as internal for now.
> // Do we want to create an nsIDOMSVGCSS2Properties interface?

We need to create the interface.

BTW, SVG 1.1 Spec say nothing about |CSS2Properties| interface...
http://www.w3.org/TR/DOM-Level-2-Style/css.html#CSS-CSS2Properties
Comment 2 Robert Longson 2008-02-20 23:54:54 PST
Bug 117500 implemented this for html
Comment 3 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2008-02-21 01:47:37 PST
It says it indirectly, taken. The SVGStylable has a 'style' member of type CSSStyleDeclaration, and it's the definition for this interface that requires "implementations of a certain level" to implement CSS2Properties. See:

http://www.w3.org/TR/SVG11/types.html#InterfaceSVGStylable
http://www.w3.org/TR/DOM-Level-2-Style/css.html#CSS-CSSStyleDeclaration
Comment 4 Takeshi Kurosawa 2008-02-21 04:23:26 PST
Created attachment 304713 [details] [diff] [review]
Patch
Comment 5 Robert Longson 2008-02-21 04:31:24 PST
This is in the style system. You should ask bzbarsky or dbaron to review it.
Comment 6 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-02-21 09:35:41 PST
I wonder if the interface order (SVG vs. NS) should be the other way around and SVG should be frozen (and not ifdefed).  In fact, given that we've started to check some of the "SVG" properties in non-SVG code, too, I'm wondering if we should just remove the SVG ifdefs from the style system.  That said, those changes could be followup patches...

Is there any standard that defines this interface?  (e.g., some SVG spec?)
Comment 7 Robert Longson 2008-02-21 09:42:36 PST
All we have for the interface definition are our inferences from reading between the lines of the comment 3 specifications as far as I know.
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2008-02-21 09:52:31 PST
My real questions are:

1)  Do other SVG UAs support this?
2a) If not, aren't we decreasing interop?
2b) If they do, have we verified that we agree with them on the casing (and
    naming in general) of these properties, since there is no specification that
    defines said casing and naming?
Comment 9 Takeshi Kurosawa 2008-02-21 20:31:45 PST
(In reply to comment #8)
> My real questions are:
> 
> 1)  Do other SVG UAs support this?
> 2a) If not, aren't we decreasing interop?
> 2b) If they do, have we verified that we agree with them on the casing (and
>     naming in general) of these properties, since there is no specification
> that
>     defines said casing and naming?
> 

Opera 9 and WebKit nightly do. And their casing rules seem same as our rule.
Adobe SVG Viewer 3 and Batik 1.7 seem don't.
Comment 10 Takeshi Kurosawa 2008-02-21 20:32:55 PST
Created attachment 304897 [details]
Testcase (setting fill-opacity and checking children of element.style)
Comment 11 Boris Zbarsky [:bz] (Out June 25-July 6) 2008-02-21 20:36:54 PST
Probably worth writing a mochitest for all the properties here...
Comment 12 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-02-21 20:42:55 PST
You should probably just change the "domProp" lines in layout/style/test/property_database.js to match these property names rather than say "null".
Comment 13 Takeshi Kurosawa 2008-02-21 20:44:48 PST
Comment on attachment 304897 [details]
Testcase (setting fill-opacity and checking children of element.style)

Previous test case shows children of |element.style|.
# WebKit doesn't expose shortcut properties.
## Opera does.
Comment 14 Robert Longson 2008-04-06 15:56:16 PDT
Comment on attachment 304713 [details] [diff] [review]
Patch

removing review request till comment 11 and comment 12 addressed
Comment 15 Aiko 2008-11-10 01:00:49 PST
*** Bug 463985 has been marked as a duplicate of this bug. ***
Comment 16 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-11-10 08:05:59 PST
To be clear, comment 12 is the easy way to address comment 11.
Comment 17 Aiko 2008-11-15 01:45:14 PST
An interesting pitfall was just brought up in the ES-Discuss list: naively adding a "filter" property to .style without further precautions would break websites which try to use IE's DirectX filters if such a property is found.
Comment 18 Boris Zbarsky [:bz] (Out June 25-July 6) 2008-11-16 07:31:30 PST
> would break websites which try to use IE's DirectX filters if such a property
> is found

Are there really sites that do object-detection on that particular property instead of on other IE things?
Comment 19 Will Pittenger 2008-11-16 11:21:10 PST
(In reply to comment #17)
> An interesting pitfall was just brought up in the ES-Discuss list: naively
> adding a "filter" property to .style without further precautions would break
> websites which try to use IE's DirectX filters if such a property is found.
You might want to create a test site using something like that.  Get it to work in IE.  Then see how various Gecko powered browser deal with the same site.
Comment 20 Aiko 2008-11-17 01:26:10 PST
(In reply to comment #18)
> Are there really sites that do object-detection on that particular property
> instead of on other IE things?

Seems likely, given all that preaching of the "use feature detection, not browser detection" mantra. For example, yahoo-dom-event.js contains this line:

if(typeof el.style.filter=='string'){el.style.filter='alpha(opacity='+val*100+')'

You'd have to ask Maciej Stachowiak for a definitive description of the problems Apple ran into.
Comment 21 Robert Longson 2009-10-11 17:27:57 PDT
Created attachment 405809 [details] [diff] [review]
update to tip and add tests

We need to move on this as it was identified at the SVG Open as a barrier to interoperability.

Includes changes suggested in comment 12 to add tests.
Comment 22 Boris Zbarsky [:bz] (Out June 25-July 6) 2009-10-11 19:26:33 PDT
So what's the story on the filter thing?  Esp. given comment 20?

Other than that, I'd prefer the inclusion of nsIDOMNSCSS2Properties.h in various places in this patch be unconditional.  And perhaps file a followup bug on addressing comment 6?
Comment 23 Robert Longson 2009-10-12 02:45:15 PDT
According to Maciej...

I believe the only property for which we did something special deliberately was "filter". What we did effectively is to make "filter" return a String object that's undetectable in the same way as document.all. The reason we did this originally was, we found some sites that tested for elt.style.filter to decide whether to use IE's DirectX filters to create transparency, or opacity. Of course, it's kind of bogus to check filter before checking opacity, but we found enough affected sites at the time that we felt we had to do the undetectability hack.

If any other SVG CSS properties are missing, it's probably an oversight.

So the question is how is document.all made undetectable in gecko?
Comment 24 Boris Zbarsky [:bz] (Out June 25-July 6) 2009-10-12 12:21:05 PDT
> So the question is how is document.all made undetectable in gecko?

"painfully".
Comment 25 Brendan Eich [:brendan] 2009-10-12 12:39:13 PDT
(In reply to comment #24)
> > So the question is how is document.all made undetectable in gecko?
> 
> "painfully".

It's not that painful to implement. The resulting behavior isn't pretty, but it did the job for document.all. See

http://mxr.mozilla.org/mozilla-central/search?string=JSRESOLVE_DETECTING

in particular the two lines in nsDOMClassInfo.cpp:

# line 8858 -- if (flags & JSRESOLVE_DETECTING  || !(flags & JSRESOLVE_QUALIFIED)) {
# line 9062 -- !(flags & JSRESOLVE_DETECTING) && !hasAll) {

/be
Comment 26 Boris Zbarsky [:bz] (Out June 25-July 6) 2009-10-15 12:52:01 PDT
Comment on attachment 405809 [details] [diff] [review]
update to tip and add tests

r- until we get filter sorted out.
Comment 27 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-10-18 14:12:09 PDT
Let's not make 'filter' undetectable. Maciej suggested, and I agree, that we should just expose this property with a different name to the usual one. I propose elem.style.svgFilter. I'll post to www-svg about it.
Comment 28 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-10-18 14:18:54 PDT
Hmm, now that I think about it, I'm not so sure that renaming the property is a great idea. Since we already the dark magic, maybe we should just reuse it here and keep the property name consistent.
Comment 29 Boris Zbarsky [:bz] (Out June 25-July 6) 2009-10-18 14:58:33 PDT
Fwiw, I like the svgFilter approach more, personally.
Comment 30 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-10-18 15:18:20 PDT
What about cssFilter, to match cssFloat?
Comment 31 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2009-10-18 16:23:17 PDT
What's the downside to the "dark magic" approach? I'm really not keen on the idea of a different property name. Having exceptions to conventions seems very undesirable from an author point of view.
Comment 32 Mike Shaver (:shaver -- probably not reading bugmail closely) 2009-10-18 17:03:55 PDT
It is very much the recommendation of ECMA TG1 and I believe our own JavaScript team that we not use the dark magic any more.  I think cssFilter is a fine idea.
Comment 33 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2009-10-18 17:13:30 PDT
(In reply to comment #32)
> It is very much the recommendation of ECMA TG1 and I believe our own JavaScript
> team that we not use the dark magic any more.

Why is that?
Comment 34 Mike Shaver (:shaver -- probably not reading bugmail closely) 2009-10-18 17:17:04 PDT
Because it makes it hard to reason about the effects of code transformations on the behaviour of the program.  There is a ton of detail in the es-discuss list archives if you're really curious.
Comment 35 Boris Zbarsky [:bz] (Out June 25-July 6) 2009-10-18 17:22:02 PDT
> What's the downside to the "dark magic" approach?

One that I can think off offhand is that every time the property is accessed you _will_ fall off trace, pretty sure.
Comment 36 Brendan Eich [:brendan] 2009-10-18 17:23:00 PDT
Dark magic from the First Age is a chore to optimize, however implemented.

/be
Comment 37 Robert Longson 2009-12-27 10:34:40 PST
*** Bug 536779 has been marked as a duplicate of this bug. ***
Comment 38 Robert Longson 2009-12-29 11:13:39 PST
Created attachment 419440 [details] [diff] [review]
address filter issue

This is the dark magic approach. Nothing seems universally popular according to previous comments but this matches what Safari does (comment 23) and won't require authors to do something special to get filters to work.

bz do I need to get an additional reviewer for the dark magic code?
Comment 39 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2009-12-29 11:23:23 PST
The #ifdefs in the interface definition seem like they could be a bad idea.  Would it be better to either (a) instead ifdef whether to implement the interface or (b) ifdef nothing?
Comment 40 Robert Longson 2009-12-29 11:52:04 PST
Created attachment 419442 [details] [diff] [review]
remove ifdef

The ifdef in the interface doesn't do anything. It is a leftover from a previous patch attempt.  Now removed.
Comment 41 Robert Longson 2010-01-14 09:32:59 PST
Boris. Is reviewing this patch percolating up your todo list or would you like me to try dbaron as he has shown some interest in this too?
Comment 42 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-01-14 09:41:16 PST
If dbaron has the bandwidth, I'm perfectly happy with him reviewing this.  My review situation right now is ... very swamped, with several patches that are higher priority than this. :(
Comment 43 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-02-01 16:44:34 PST
Comment on attachment 419442 [details] [diff] [review]
remove ifdef

r=dbaron, but the nsDOMClassInfo stuff here to make 'filter' undetectable should get review from somebody who knows that code.

Also, please make the style in dom/interfaces/css/Makefile.in consistent with the surrounding lines.
Comment 44 Johnny Stenback (:jst, jst@mozilla.com) 2010-02-02 16:16:55 PST
I'd really rather not add any more dark magic here myself either, just because we have the code and use it for document.all doesn't mean we should do more of it, plus with this code it'll be hard for websites to detect whether or not it's running in a Firefox version that supports filters or not. Inventing and using the undetectability of a property made sense for document.all given how widely used that property was, but this doesn't seem anywhere near as bad to me yet. I'm strongly leaning towards either using a different name or taking the hit of the name conflict here w/o undetectablity and letting sites deal.

Do we have any feel for what the number of sites that would break are if we just go ahead and use the name "filter", or how hard it might be to get those sites to change?
Comment 45 Robert Longson 2010-02-02 21:02:59 PST
Safari did research on this and decided on a dark magic approach (comment 23). I'll ask them whether they have more details.
Comment 46 Robert Longson 2010-02-03 12:25:34 PST
Safari's change for dark magic is http://trac.webkit.org/changeset/15557

I've checked out http://www.housingmaps.com/ and it seems no different with or without dark magic (the popup windows appear just the same) so perhaps google maps has changed in the last 4 years.

Should we just land the patch without the dark magic and then keep the option to add dark magic later if we need to?
Comment 47 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-02-03 12:31:12 PST
I would be happier with that, personally.
Comment 48 Robert Longson 2010-02-03 12:44:26 PST
Created attachment 425048 [details] [diff] [review]
without dark magic

This is the magic free version. It's exactly the same as the previous one except that the dark magic changes are gone.
Comment 49 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-02-03 13:24:37 PST
Comment on attachment 425048 [details] [diff] [review]
without dark magic

r=dbaron
Comment 50 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-02-03 15:40:38 PST
Comment on attachment 425048 [details] [diff] [review]
without dark magic

lovely!
Comment 51 Johnny Stenback (:jst, jst@mozilla.com) 2010-02-03 15:49:00 PST
Excellent!
Comment 52 Robert Longson 2010-02-06 07:57:51 PST
pushed http://hg.mozilla.org/mozilla-central/rev/bc6f2b598ff9
Comment 53 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-02-23 00:54:43 PST
Someone might want to sort through these sites and check which ones may have been broken:
http://philip.html5.org/data/if-style-filter.txt
Comment 54 Eric Shepherd [:sheppy] 2010-11-09 12:43:27 PST
Documented:

https://developer.mozilla.org/en/DOM/element.style

And mentioned on Fx4 for developers.

Note You need to log in before you can comment on or make changes to this bug.