Project: chromium ▼ Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.
Issue 463116 SVGElement should not support offsetWidth/offsetHeight/etc
Starred by 7 users Reported by bzbar...@mit.edu, Mar 2 2015 Back to list
Status: Fixed
Owner:
█ phil...@opera.com
Email to this user bounced
Closed: Feb 2016
Cc:
█ tanchow....@gmail.com
█ ed@chromium.org
ashej...@chromium.org
paint-bugs@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug

Blocked on:
issue 504578


Sign in to add a comment
 
UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:39.0) Gecko/20100101 Firefox/39.0

Example URL:
data:text/html,<svg></svg><script>alert(document.querySelector("svg").offsetWidth)</script>

Steps to reproduce the problem:
1. Try to get offsetWidth/offsetHeight on a non-HTML element

What is the expected behavior?
Returns undefined, since per http://dev.w3.org/csswg/cssom-view/#extensions-to-the-htmlelement-interface these properties only exist for HTML elements.

What went wrong?
SVG elements have an offsetWidth and offsetHeight.  There is no specification for how these should behave, since http://dev.w3.org/csswg/cssom-view/#dom-htmlelement-offsetwidth and http://dev.w3.org/csswg/cssom-view/#dom-htmlelement-offsetheight both assume the CSS box model, which SVG does not have.

Does it occur on multiple sites: Yes

Is it a problem with a plugin? No 

Did this work before? N/A 

Does this work in other browsers? Yes 

Chrome version: 42.0.2311.11 (Official Build) dev (64-bit)  Channel: n/a
OS Version: OS X 10.9
Flash Version: Shockwave Flash 16.0 r0

Please either remove this non-standard extension or propose an actual specification for it.
Comment 1 by tkent@chromium.org, Mar 3 2015
Cc: phil...@opera.com
Labels: -Cr-Content Cr-Blink-DOM Cr-Blink-SVG
Comment 2 by phil...@opera.com, Mar 3 2015 
Yep, I noticed this when rewriting Element.idl recently and added a FIXME, among very many others.

I've asked the bots how they would feel about this change:
https://codereview.chromium.org/969303002
Comment 3 by andrer...@gmail.com, Mar 3 2015 
For me, it is clear that the definition is incomplete and must be changed, or better refined. I don't see why you should break a working feature that might be in use on multiple sites, just because it's not in the technical specification.

I'm an end user (web dev) and I feel like I triggered this ticket. I am using a jQuery svg:visible selector which works in Chrome right now, but not in Firefox. Therefore I opened a Bugzilla report, but instead of fixing it, they claimed that this fault is actually a feature. I find this kind of cheesy, if you know what I mean.
Comment 4 by phil...@opera.com, Mar 3 2015 
More important than not being in the spec is that it isn't in Firefox or IE. Do you have an alternative code path for them, and could you not use that code path for all browsers?

I haven't checked exactly what these attributes do for SVG, at first glance I guessed that they would be zero/null, but I guess not?
Comment 5 by f...@opera.com, Mar 3 2015 
> I haven't checked exactly what these attributes do for SVG, at first glance I guessed that they would be zero/null, but I guess not?

AFAICS, they would return 0 (with the side-effect of a layout update of course...)
Comment 6 by ashej...@chromium.org, Mar 3 2015
Cc: -phil...@opera.com ashej...@chromium.org
Owner: phil...@opera.com
Status: Assigned
Changing the status to Assigned as CL is up for review.

Assigning it to "philipj@opera.com", feel free to change accordingly.

Thank you!
Comment 7 by bzbar...@mit.edu, Mar 3 2015 
> AFAICS, they would return 0 

Not in the testcase I provided.  Though that one _does_ in fact have a CSS box, so could be defined usefully, and perhaps should be.
Comment 8 by f...@opera.com, Mar 3 2015 
Right, the outermost SVG root would be special in that regard.
Comment 9 by phil...@opera.com, Mar 4 2015 
> could be defined usefully, and perhaps should be

Boris, would you like us to try moving these to HTMLElement, or to ask for some spec change?
Comment 10 by bzbar...@mit.edu, Mar 4 2015 
Given that I think these are fundamentally broken APIs that we should encourage people not to use (in favor of getBoundingClientRect), the former is probably better in my ideal world.  I realize that this is a lot more painful for you, of course, and I don't feel strongly enough about it to actually push for that as opposed to specifying behavior on non-HTML things.  

None of which answers your question, I know.  Sorry.
Comment 11 by phil...@opera.com, Mar 4 2015 
OK, I think I'm going to start with adding use counters for when these offset* attributes return a non-null or non-zero value for something other than a HTMLElement. Hopefully usage should be low, but sometimes we are unpleasantly surprised...
Project Member Comment 12 by bugdroid1@chromium.org, Mar 13 2015 
The following revision refers to this bug:
  http://src.chromium.org/viewvc/blink?view=rev&rev=191864

------------------------------------------------------------------
r191864 | philipj@opera.com | 2015-03-13T19:42:00.815674Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt?r1=191864&r2=191863&pathrev=191864
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/webexposed/global-interface-listing-expected.txt?r1=191864&r2=191863&pathrev=191864
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/dom/Element.idl?r1=191864&r2=191863&pathrev=191864
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/html/HTMLElement.idl?r1=191864&r2=191863&pathrev=191864
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/frame/UseCounter.h?r1=191864&r2=191863&pathrev=191864

Add UseCounters for Element.offset*

Add HTMLElement.offset* (which is per spec) to shadow Element.offset*,
so that the new counters will only trigger for non-HTML elements.

It seems likely that usage could be high without actually implying that
removal would break anything, but wait to see the data...

BUG= 463116 

Review URL: https://codereview.chromium.org/1007683002
-----------------------------------------------------------------
Comment 13 by phil...@opera.com, Jul 10 2015
Cc: ed@chromium.org tanchow....@gmail.com
Issue 508865 has been merged into this issue.
Comment 14 by phil...@opera.com, Jul 10 2015
Blockedon: chromium:504578
Comment 16 by phil...@opera.com, Jul 10 2015 
Boris, did you discover this due to an interop problem, or just by happenstance? If the former this should be given higher priority.
Comment 17 by bzbar...@mit.edu, Jul 10 2015 
I don't quite remember, sorry.  It wasn't a site in the wild being broken, if that's the question, but it might have been someone filing a bug on us because they had a jsfiddle or something that worked in Blink but not in Gecko.
Comment 18 by phil...@opera.com, Jul 10 2015 
Thanks, that's good to know. Even if breakage isn't found in the wild we should still fix it, of course.
Project Member Comment 19 by bugdroid1@chromium.org, Jul 14 2015 
The following revision refers to this bug:
  http://src.chromium.org/viewvc/blink?view=rev&rev=198857

------------------------------------------------------------------
r198857 | tanay.c@samsung.com | 2015-07-14T12:30:33.536866Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt?r1=198857&r2=198856&pathrev=198857
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/webexposed/global-interface-listing-expected.txt?r1=198857&r2=198856&pathrev=198857
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/dom/Element.idl?r1=198857&r2=198856&pathrev=198857
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/svg/SVGElement.idl?r1=198857&r2=198856&pathrev=198857
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/html/element-offset-expected.txt?r1=198857&r2=198856&pathrev=198857
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/frame/UseCounter.cpp?r1=198857&r2=198856&pathrev=198857
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/html/element-offset.html?r1=198857&r2=198856&pathrev=198857
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/frame/UseCounter.h?r1=198857&r2=198856&pathrev=198857

Deprecate Element.offsetParent/offsetTop/offsetLeft/offsetWidth/offsetHeight

Intent to Deprecate: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/Hi2ni3HdXTw

BUG= 504578 , 463116 

Review URL: https://codereview.chromium.org/1231533002
-----------------------------------------------------------------
Comment 20 by phil...@opera.com, Nov 4 2015
Labels: Hotlist-Interop
Summary: SVGElement should not support offsetWidth/offsetHeight/etc (was: offsetWidth/offsetHeight are apparently supported on non-HTML elements)
In issue 504578 these attributes were removed from Element, but they remain on SVGElement. Renaming to track what remains.
Comment 22 by fris...@jeka.info, Nov 5 2015 
Probably worth to add some history here:

See #c3 : Someone tried JQuery pseudo-CSS |svg:visible| ,
had unexpected results in Firefox and filed a bug against Mozilla
https://bugzilla.mozilla.org/show_bug.cgi?id=1137730

quoting  http://jquery-2.1.4.js :

jQuery.expr.filters.hidden = function( elem ) {
	return elem.offsetWidth <= 0 && elem.offsetHeight <= 0;
};
jQuery.expr.filters.visible = function( elem ) {
	return !jQuery.expr.filters.hidden( elem );
};

JQuery has this logic since 1.3.2. Formerly CSS |display| and |visibility| was used.
http://blog.jquery.com/2009/02/20/jquery-1-3-2-released/#visible-hidden-overhauled

So people can't use |svg:visible| or |svg:hidden| since 2009-02, because it fails in IE and Mozilla.
JQuery had fixed this code long time ago if people complained.
Comment 23 by phil...@opera.com, Nov 5 2015 
Thanks #22, I took that and turned it into a test that clearly shows the difference between Blink, Edge and Gecko on this issue:
https://jsfiddle.net/fy1zp4kk/

Only in Blink does the circle seem to be hidden, and that's because offsetWidth and offsetHeight return 0. If they return undefined, the hidden selector will never match, like in Edge and Gecko.

This is of course a bug in jQuery, but it was closed as wontfix as SVG is not a priority:
http://bugs.jquery.com/ticket/12587
Project Member Comment 24 by bugdroid1@chromium.org, Nov 6 2015 
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3ea8c5bfc445dcd51c0e16bb2f5eef0b0345f0d1

commit 3ea8c5bfc445dcd51c0e16bb2f5eef0b0345f0d1
Author: philipj <philipj@opera.com>
Date: Fri Nov 06 16:37:02 2015

Deprecate SVGElement.offsetParent/offsetTop/offsetLeft/offsetWidth/offsetHeight

Intent to Deprecate:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/JlAEmQpWMWA/63acYbmdDwAJ

Simply using [Deprecate] is unfortunately not supported.

Tests are updated to avoid the attributes to the extent possible.

BUG= 463116 

Review URL: https://codereview.chromium.org/1405283006

Cr-Commit-Position: refs/heads/master@{#358333}

[modify] http://crrev.com/3ea8c5bfc445dcd51c0e16bb2f5eef0b0345f0d1/third_party/WebKit/LayoutTests/css3/flexbox/flexitem-expected.txt
[modify] http://crrev.com/3ea8c5bfc445dcd51c0e16bb2f5eef0b0345f0d1/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-display-expected.txt
[modify] http://crrev.com/3ea8c5bfc445dcd51c0e16bb2f5eef0b0345f0d1/third_party/WebKit/LayoutTests/fast/dom/shadow/shadow-dom-event-dispatching-svg-in-shadow-subtree-expected.txt
[modify] http://crrev.com/3ea8c5bfc445dcd51c0e16bb2f5eef0b0345f0d1/third_party/WebKit/LayoutTests/fast/events/click-focus-anchor.html
[modify] http://crrev.com/3ea8c5bfc445dcd51c0e16bb2f5eef0b0345f0d1/third_party/WebKit/LayoutTests/fast/repaint/resources/text-based-repaint.js
[modify] http://crrev.com/3ea8c5bfc445dcd51c0e16bb2f5eef0b0345f0d1/third_party/WebKit/LayoutTests/svg/W3C-SVG-1.1-SE/text-intro-09-b.svg
[modify] http://crrev.com/3ea8c5bfc445dcd51c0e16bb2f5eef0b0345f0d1/third_party/WebKit/LayoutTests/svg/animations/resources/SVGTestCase.js
[modify] http://crrev.com/3ea8c5bfc445dcd51c0e16bb2f5eef0b0345f0d1/third_party/WebKit/LayoutTests/svg/css/cursor-replace.svg
[modify] http://crrev.com/3ea8c5bfc445dcd51c0e16bb2f5eef0b0345f0d1/third_party/WebKit/LayoutTests/svg/custom/animate-reference-crash-expected.txt
[modify] http://crrev.com/3ea8c5bfc445dcd51c0e16bb2f5eef0b0345f0d1/third_party/WebKit/LayoutTests/svg/custom/detached-outermost-svg-crash-expected.txt
[modify] http://crrev.com/3ea8c5bfc445dcd51c0e16bb2f5eef0b0345f0d1/third_party/WebKit/LayoutTests/svg/custom/font-platformDestroy-crash.svg
[modify] http://crrev.com/3ea8c5bfc445dcd51c0e16bb2f5eef0b0345f0d1/third_party/WebKit/LayoutTests/svg/custom/pointer-events-invalid-fill.svg
[modify] http://crrev.com/3ea8c5bfc445dcd51c0e16bb2f5eef0b0345f0d1/third_party/WebKit/LayoutTests/svg/custom/touch-events.html
[modify] http://crrev.com/3ea8c5bfc445dcd51c0e16bb2f5eef0b0345f0d1/third_party/WebKit/LayoutTests/svg/dynamic-updates/resources/SVGTestCase.js
[modify] http://crrev.com/3ea8c5bfc445dcd51c0e16bb2f5eef0b0345f0d1/third_party/WebKit/LayoutTests/svg/foreignObject/fO-percentage-height-style-expected.txt
[modify] http://crrev.com/3ea8c5bfc445dcd51c0e16bb2f5eef0b0345f0d1/third_party/WebKit/LayoutTests/svg/foreignObject/fO-percentage-height-style.html
[modify] http://crrev.com/3ea8c5bfc445dcd51c0e16bb2f5eef0b0345f0d1/third_party/WebKit/LayoutTests/svg/text/inline-text-destroy-attributes-crash.xhtml
[modify] http://crrev.com/3ea8c5bfc445dcd51c0e16bb2f5eef0b0345f0d1/third_party/WebKit/Source/core/frame/UseCounter.cpp
[modify] http://crrev.com/3ea8c5bfc445dcd51c0e16bb2f5eef0b0345f0d1/third_party/WebKit/Source/core/svg/SVGElement.idl
Project Member Comment 25 by bugdroid1@chromium.org, Feb 24 2016 
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6887a15a769e9de73628346f67a4bfff71b553be

commit 6887a15a769e9de73628346f67a4bfff71b553be
Author: philipj <philipj@opera.com>
Date: Wed Feb 24 10:40:29 2016

Update SVG tests ahead of offset* removal

This converts some SVG tests to use getBoundingClientRect() instead of
the deprecated offset* attributes, so that the removal CL is minimized
and thus easier to revert in case of trouble.

smil-scheduled-in-inactive-document-crash.html didn't actually run the
problematic code because of a early finishJSTest(), which was moved.

BUG= 463116 
R=fs@opera.com

Review URL: https://codereview.chromium.org/1729073002

Cr-Commit-Position: refs/heads/master@{#377261}

[modify] https://crrev.com/6887a15a769e9de73628346f67a4bfff71b553be/third_party/WebKit/LayoutTests/css3/flexbox/flexitem-expected.txt
[modify] https://crrev.com/6887a15a769e9de73628346f67a4bfff71b553be/third_party/WebKit/LayoutTests/css3/flexbox/flexitem.html
[modify] https://crrev.com/6887a15a769e9de73628346f67a4bfff71b553be/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-display-expected.txt
[modify] https://crrev.com/6887a15a769e9de73628346f67a4bfff71b553be/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-display.html
[modify] https://crrev.com/6887a15a769e9de73628346f67a4bfff71b553be/third_party/WebKit/LayoutTests/fast/dom/shadow/resources/event-dispatching.js
[modify] https://crrev.com/6887a15a769e9de73628346f67a4bfff71b553be/third_party/WebKit/LayoutTests/fast/dom/shadow/shadow-dom-event-dispatching-svg-in-shadow-subtree-expected.txt
[modify] https://crrev.com/6887a15a769e9de73628346f67a4bfff71b553be/third_party/WebKit/LayoutTests/fast/events/resources/focus-anchor-by-mouse.js
[modify] https://crrev.com/6887a15a769e9de73628346f67a4bfff71b553be/third_party/WebKit/LayoutTests/resources/check-layout-th.js
[modify] https://crrev.com/6887a15a769e9de73628346f67a4bfff71b553be/third_party/WebKit/LayoutTests/resources/check-layout.js
[modify] https://crrev.com/6887a15a769e9de73628346f67a4bfff71b553be/third_party/WebKit/LayoutTests/svg/animations/smil-scheduled-in-inactive-document-crash.html
[modify] https://crrev.com/6887a15a769e9de73628346f67a4bfff71b553be/third_party/WebKit/LayoutTests/svg/as-iframe/svg-in-iframe.html
[modify] https://crrev.com/6887a15a769e9de73628346f67a4bfff71b553be/third_party/WebKit/LayoutTests/svg/custom/animate-svgsvgelement.html
[modify] https://crrev.com/6887a15a769e9de73628346f67a4bfff71b553be/third_party/WebKit/LayoutTests/svg/custom/size-follows-container-size.html
Project Member Comment 26 by bugdroid1@chromium.org, Feb 24 2016 
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b515424de0aa3d62c4d64f4a73cf0bff137539c8

commit b515424de0aa3d62c4d64f4a73cf0bff137539c8
Author: philipj <philipj@opera.com>
Date: Wed Feb 24 13:40:38 2016

Remove SVGElement.offsetParent/offsetTop/offsetLeft/offsetWidth/offsetHeight

Intent to Remove:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/jjwLLSG_hGY/Ovi-nvEeDwAJ

BUG= 463116 

Review URL: https://codereview.chromium.org/1726743002

Cr-Commit-Position: refs/heads/master@{#377283}

[modify] https://crrev.com/b515424de0aa3d62c4d64f4a73cf0bff137539c8/android_webview/tools/WebViewShell/test/webexposed/global-interface-listing-expected.txt
[modify] https://crrev.com/b515424de0aa3d62c4d64f4a73cf0bff137539c8/third_party/WebKit/LayoutTests/svg/custom/animate-reference-crash-expected.txt
[modify] https://crrev.com/b515424de0aa3d62c4d64f4a73cf0bff137539c8/third_party/WebKit/LayoutTests/svg/custom/detached-outermost-svg-crash-expected.txt
[modify] https://crrev.com/b515424de0aa3d62c4d64f4a73cf0bff137539c8/third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt
[modify] https://crrev.com/b515424de0aa3d62c4d64f4a73cf0bff137539c8/third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt
[modify] https://crrev.com/b515424de0aa3d62c4d64f4a73cf0bff137539c8/third_party/WebKit/Source/core/frame/Deprecation.cpp
[modify] https://crrev.com/b515424de0aa3d62c4d64f4a73cf0bff137539c8/third_party/WebKit/Source/core/frame/UseCounter.h
[modify] https://crrev.com/b515424de0aa3d62c4d64f4a73cf0bff137539c8/third_party/WebKit/Source/core/svg/SVGElement.idl
Comment 27 by phil...@opera.com, Feb 24 2016
Labels: M-50
Status: Fixed
Comment 28 by phil...@opera.com, Apr 25 2016 
Some fallout in issue 606124.
► Sign in to add a comment