Setting element.style.someSvgProperty has no effect

RESOLVED FIXED in mozilla1.9.3a2

Status

()

defect
RESOLVED FIXED
12 years ago
8 years ago

People

(Reporter: jwatt, Assigned: longsonr)

Tracking

({dev-doc-complete})

Trunk
mozilla1.9.3a2
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.2 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(4 attachments, 3 obsolete attachments)

Reporter

Description

12 years ago
Posted image 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', '');
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
Assignee

Comment 2

12 years ago
Bug 117500 implemented this for html
Reporter

Comment 3

12 years ago
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
Posted patch Patch (obsolete) — Splinter Review
Assignee

Comment 5

12 years ago
This is in the style system. You should ask bzbarsky or dbaron to review it.
Assignee

Updated

12 years ago
Summary: Setting element.style.someSvgProperty has no affect → Setting element.style.someSvgProperty has no effect
Assignee

Updated

12 years ago
Attachment #304713 - Flags: superreview?(bzbarsky)
Attachment #304713 - Flags: review?(bzbarsky)
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?)
Assignee

Comment 7

12 years ago
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.
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?
(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.
Probably worth writing a mochitest for all the properties here...
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 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.
Attachment #304897 - Attachment description: Testcase (setting fill-opacity) → Testcase (setting fill-opacity and checking children of element.style)
Assignee

Comment 14

11 years ago
Comment on attachment 304713 [details] [diff] [review]
Patch

removing review request till comment 11 and comment 12 addressed
Attachment #304713 - Flags: superreview?(bzbarsky)
Attachment #304713 - Flags: review?(bzbarsky)

Updated

11 years ago
Duplicate of this bug: 463985
To be clear, comment 12 is the easy way to address comment 11.

Comment 17

11 years ago
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.
> 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

11 years ago
(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

11 years ago
(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.
Reporter

Updated

10 years ago
Flags: wanted1.9.2+
Assignee: general → nobody
QA Contact: ian → general
Assignee

Comment 21

10 years ago
Posted patch update to tip and add tests (obsolete) — Splinter Review
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.
Attachment #304713 - Attachment is obsolete: true
Attachment #405809 - Flags: review?(bzbarsky)
Assignee

Updated

10 years ago
Assignee: nobody → longsonr
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?
Assignee

Comment 23

10 years ago
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?
> So the question is how is document.all made undetectable in gecko?

"painfully".
(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 on attachment 405809 [details] [diff] [review]
update to tip and add tests

r- until we get filter sorted out.
Attachment #405809 - Flags: review?(bzbarsky) → review-
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.
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.
Fwiw, I like the svgFilter approach more, personally.
What about cssFilter, to match cssFloat?
Reporter

Comment 31

10 years ago
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.
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.
Reporter

Comment 33

10 years ago
(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?
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.
> 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.
Dark magic from the First Age is a chore to optimize, however implemented.

/be
Assignee

Updated

10 years ago
Depends on: 523576
Assignee

Updated

10 years ago
Duplicate of this bug: 536779
Assignee

Comment 38

10 years ago
Posted patch address filter issue (obsolete) — Splinter Review
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?
Attachment #405809 - Attachment is obsolete: true
Attachment #419440 - Flags: review?(bzbarsky)
Assignee

Updated

10 years ago
Attachment #419440 - Attachment is patch: true
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?
Assignee

Comment 40

10 years ago
Posted patch remove ifdefSplinter Review
The ifdef in the interface doesn't do anything. It is a leftover from a previous patch attempt.  Now removed.
Attachment #419440 - Attachment is obsolete: true
Attachment #419442 - Flags: review?(bzbarsky)
Attachment #419440 - Flags: review?(bzbarsky)
Assignee

Comment 41

10 years ago
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?
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. :(
Assignee

Updated

10 years ago
Attachment #419442 - Flags: review?(bzbarsky) → review?(dbaron)
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.
Attachment #419442 - Flags: review?(jst)
Attachment #419442 - Flags: review?(dbaron)
Attachment #419442 - Flags: review+
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?
Assignee

Comment 45

10 years ago
Safari did research on this and decided on a dark magic approach (comment 23). I'll ask them whether they have more details.
Assignee

Comment 46

10 years ago
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?
I would be happier with that, personally.
Assignee

Comment 48

10 years ago
This is the magic free version. It's exactly the same as the previous one except that the dark magic changes are gone.
Attachment #425048 - Flags: superreview?(roc)
Attachment #425048 - Flags: review?(dbaron)
Comment on attachment 425048 [details] [diff] [review]
without dark magic

r=dbaron
Attachment #425048 - Flags: review?(dbaron) → review+
Comment on attachment 425048 [details] [diff] [review]
without dark magic

lovely!
Attachment #425048 - Flags: superreview?(roc) → superreview+
Excellent!
Assignee

Comment 52

10 years ago
pushed http://hg.mozilla.org/mozilla-central/rev/bc6f2b598ff9
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Assignee

Updated

10 years ago
Attachment #419442 - Flags: review?(jst)
Assignee

Updated

10 years ago
Keywords: dev-doc-needed
Target Milestone: --- → mozilla1.9.3a2
Depends on: 544720
Assignee

Updated

10 years ago
Blocks: 544848
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
Documented:

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

And mentioned on Fx4 for developers.

Updated

9 years ago
Depends on: 631875

Updated

8 years ago
Depends on: 644597
You need to log in before you can comment on or make changes to this bug.