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

RESOLVED FIXED in mozilla29

Status

()

Core
DOM: CSS Object Model
--
minor
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: herrernst, Assigned: Adam Casey)

Tracking

21 Branch
mozilla29
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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
(Reporter)

Comment 1

5 years ago
Created attachment 752786 [details]
getComputedStyle on img vs svg
(Reporter)

Comment 2

5 years ago
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

Updated

5 years ago
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++]

Comment 4

5 years ago
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!
(Assignee)

Comment 8

4 years ago
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

Updated

4 years ago
Assignee: nobody → adamncasey
(Assignee)

Comment 9

4 years ago
Created attachment 8344247 [details] [diff] [review]
Patch containing a unit test and code fix for this bug

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. :(
(Assignee)

Comment 15

4 years ago
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.
(Assignee)

Comment 17

4 years ago
Created attachment 8345560 [details] [diff] [review]
Patch 2

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+
(Assignee)

Comment 19

4 years ago
Created attachment 8345944 [details] [diff] [review]
bug874919_2.diff

Fixed the nits suggested by bz
Attachment #8345560 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/92f73b019839
Status: UNCONFIRMED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.