Closed Bug 874919 Opened 11 years ago Closed 11 years ago

window.getComputedStyle() doesn't return used values of width and height for svg

Categories

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

Product:
Core
Component:
DOM: CSS Object Model
Version:
21 Branch
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla29

People

(Reporter: herr.ernst, Assigned: adamncasey)

Details

(Whiteboard: [mentor=bz][lang=c++])

Attachments

(2 files, 2 obsolete files)

getComputedStyle on img vs svg
1.42 KB, text/html
Details
bug874919_2.diff
4.78 KB, patch
Details | Diff | Splinter Review 
window.getComputedStyle should get the used values (that is, in pixels for lengths, see [1]). But for an SVG element with a CSS style of "display: inline" and "width: 100%" it returns "100%" as width, not the used value. See example in test case, where for an img element, it always returns pixel values, but not for the first SVG.

[1] https://developer.mozilla.org/en-US/docs/Web/API/window.getComputedStyle

    
    

    
  
Comment on attachment 752786 [details]
getComputedStyle on img vs svg

><!doctype html>
><title>SVG CSSOM getComputedStyle</title>
>
><script>
>addEventListener("load", function () {
>	var els = document.querySelectorAll("div img, div svg"), p, span;
>	for (var i = 0; i < els.length; i++) {
>		p = document.createElement("p");
>		span = document.createElement("span");		
>		span.appendChild(document.createTextNode("getComputedStyle.width: " + window.getComputedStyle(els[i]).width));		
>		if (!window.getComputedStyle(els[i]).width.match(/\d+px$/)) {
>			span.style.color = "red";
>		}
>		p.appendChild(span);
>		els[i].parentNode.appendChild(p);
>	}
>}, false);
></script>
>
><div style="width: 150px">
><h2>img (display: inline)</h2>
><img style="display: inline; width: 100%" src="https://bugzilla.mozilla.org/extensions/OpenGraph/web/bugzilla.png" alt="Bugzilla"/>
>Some Text
></div>
>
><hr />
>
><div style="width: 150px">
><h2>img (display: block)</h2>
><img style="display: block; width: 100%" src="https://bugzilla.mozilla.org/extensions/OpenGraph/web/bugzilla.png" alt="Bugzilla"/>
>Some Text
></div>
>
><hr />
>
><div style="width: 150px">
><h2>svg (display: inline)</h2>
>   <svg style="display: inline; width: 100%">
>      <circle cx="120" cy="120" r="120" fill="blue" />
>   </svg>
>   Some Text
></div>
>
><hr />
>
><div style="width: 150px">
><h2>svg (display: block)</h2>
>   <svg style="display: block; width: 100%">
>      <circle cx="120" cy="120" r="120" fill="blue" />
>   </svg>
>   Some Text
></div>

        Attachment #752786 -
        Attachment mime type: text/plain → text/html

    
  
Component: General → DOM
Product: Firefox → Core

    
    

    
  
We only return computed styles for "display:inline", but we should do something different for SVG probably, yes.
Component: DOM → DOM: CSS Object Model
Whiteboard: [mentor=bz][lang=c++]

    
    

    
  
Hi I am interested in working on this bug,can anyone assign it to me and guide me on how to get started with it?Thanks a lot.

    
    

    
  
Hi, I am interested in working on this bug. So, I request you to assigm it to me and guide me on how ti get started with because I am a newbie.

Thanks in advance,
A.Anup

    
    

    
  
MikeLin, Anup, are you still interested in the bug?

    
    

    
  
Hi, in case of MikeLin and Anup aren't interested in this bug anymore, I'm willing to work on it. Thanks a bunch!

    
    

    
  
Since there hasn't been any activity for a while I thought it'd be OK to start working on it..
I've got a test (more or less equivalent to the attachment on comment 1) which is currently passing.

The current solution is to simply add another check for whether the mInnerFrame is of type eSVG when we work out whether we should calculate the width/height.
(in nsComputedDOMStyle.cpp ~ line 3713).

I thought about perhaps making the SVG Outer frame respond positively to IsFrameOfType(eReplaced), since in my head that made some sense, however I think that causes issues further afield.

I'm not sure whether we want to add a similar check for MathML. Opera returns a resolve value when getComputedStyle is called on a MathML element, however the width: 100% is not applied.

I'm currently running more tests just to check I haven't broken anything else, then I'll submit a patch

    
  
Assignee: nobody → adamncasey

    
    

    
  


  
Most of the mochitests and reftests were checked, however I had some issues running them on this machine, so if this could be run on the Try server for me that would be very useful

    
    

    
  
Comment on attachment 8344247 [details] [diff] [review]
Patch containing a unit test and code fix for this bug

I recommend setting a feedback or review flag to ensure that this gets looked at. Thanks for the patch, Adam!

        Attachment #8344247 -
        Flags: feedback?(bzbarsky)

    
    

    
  
Comment on attachment 8344247 [details] [diff] [review]
Patch containing a unit test and code fix for this bug

This doesn't seem quite right to me.  Here's what the spec says:

  If the property applies to the element or pseudo-element and the resolved
  value of the 'display' property is not 'none', the resolved value is the used
  value. Otherwise the resolved value is the computed value.

Now typically, the width/height properties do not apply to inline frames, except replaced inlines.  From the point of view of CSS, outermost <svg> is a replaced element.  Similar for <math>.

What this patch does is make us use the resolved value for all SVG and MathML frames.  But most of at least the former do not in fact have width apply to them, do they?  Think things like paths, rects, etc.

So this is the right place to change, but the wrong change, imo.

        Attachment #8344247 -
        Flags: feedback?(bzbarsky) → feedback+

    
    

    
  
So I looked, and it looks like the fact that width/height apply to outermost SVG is only encoded in its ComputeSize function.

We could either explicitly check for outermost SVG here or add some new frame type flag that it would claim to be.  Explicitly checking seems simplest to me.

I wonder which MathML elements actually have width apply to them...

    
    

    
  
Oh, and you're right that making IsFrameOfType(eReplaced) return true for outer SVG is likely to cause issues elsewhere in the system. :(

    
    

    
  
Ah, I see.

Adding a new nsIFrame enumeration value (along the lines of eSVGOuterFrame..) and checking for that in this location seems like it might work.

Actually, eSVGContainer already exists, and seems to bring the behaviour in line with other browsers (and the spec?). However I'm not sure if other elements which could be inside of another SVG use the Frame Type eSVGContainer.

I'll dig into whether MathML will encounter the same problem.

Thanks for your comments, they're very helpful

    
    

    
  
Thank you for working on this!

If we plan to just special-case this for nsSVGOuterFrame, I'd rather do that via do_QueryFrame than by adding a flag that only that one frame type has, fwiw.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch 2 (obsolete)
        — 
          — Splinter Review
      

    


  
Only checks for the outer SVG frame, fixing the issues in the last patch.

Ignores MathML aspects of the bug. As suggested by bzbarsky@mit.edu, will file appropriate query to clear up this part of the MathML/MathML & CSS spec.

Tests include test which passes in current Firefox, to clear up what was wrong about the previous patch. Not sure if this is helpful.

        Attachment #8344247 -
        Attachment is obsolete: true

        Attachment #8345560 -
        Flags: review?(bzbarsky)

    
    

    
  
Comment on attachment 8345560 [details] [diff] [review]
Patch 2

         !(mInnerFrame->IsFrameOfType(nsIFrame::eReplaced))) {
+
+      // An outer SVG frame should behave the same as eReplaced in this case
+      if(mInnerFrame->GetType() != nsGkAtoms::svgOuterSVGFrame) {

Seems like this would be more readable:

         !(mInnerFrame->IsFrameOfType(nsIFrame::eReplaced)) &&
         // An outer SVG frame should behave the same as eReplaced in this case
         mInnerFrame->GetType() != nsGkAtoms::svgOuterSVGFrame) {

>+        // however some browsers return "0px", and this test shouldn't fail if the behaviour of firefox changes.

It sure should, no?  And it looks like it will, which is good.  Just drop that comment.

>+        ok(style.width.match(/(\d+%$)|(auto$)/), 

I don't think you nee dthose inner capturing parens.  But I'd like to see some '^' in that regexp, and the same for the shouldUseUsed case.

>\ No newline at end of file

Please add one.

r=me with those nits, and thank you!

        Attachment #8345560 -
        Flags: review?(bzbarsky) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug874919_2.diff
        — 
          — Splinter Review
      

    


  
Fixed the nits suggested by bz

        Attachment #8345560 -
        Attachment is obsolete: true

    
  
Keywords: checkin-needed

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/92f73b019839
Status: UNCONFIRMED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: