483389 - viewportElement methods return the wrong element sometimes

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

a) getting the viewportElement or nearestViewportElement of an element should start looking from the element's parent and should not consider the element itself.
b) getting the viewportElement should not just consider svg elements symbols etc can establish viewports.
c)if a foreignObject ancestor is present it should not stop viewportElement finding the viewport

Comment 1

[Robert Longson [:longsonr]]

Attachment: patch

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  var root = doc.getElementById("root");

You could just use doc.documentElement

+  is(root.farthestViewportElement, null, "root.farthestViewportElement");
+  is(root.nearestViewportElement, null, "root.nearestViewportElement");
+  is(root.viewportElement, null, "root.viewportElement");

are you sure this is supposed to return null? I can't find where this is documented...

Comment 3

[Robert Longson [:longsonr]]

http://www.w3.org/TR/SVG/types.html#InterfaceSVGLocatable and http://www.w3.org/TR/SVG/types.html#InterfaceSVGElement say in all cases

Null if the current element is the outermost 'svg' element.

Comment 4

[Robert Longson [:longsonr]]

The only thing I'm not totally sure on is whether I've made the right decision for foreignObject.

<svg id="svg1">
  <foreignObject>
     <svg id="svg2">
         <g id="g"/>
     </svg>
  </foreignObject>
</svg>

What's the farthestViewport of g? We give svg2 an outer SVG frame so is it that? Or is it svg1? Batik (and this patch) goes for svg1. Opera goes for svg2 I think, although if you ask for the farthestViewport for svg2 you get the foreignObject in Opera and I don't see that that makes sense so I'm not sure I trust Opera. On the other hand Batik doesn't really support foreignObject. As usual the spec is no help at all as far as I can tell.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I would say 'null'. Chances are people are expecting it to return the nearest ancestor which doesn't have an SVG parent.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

BTW your foreignObject test is not valid, since the <g> lacks an <svg> parent, right?

Comment 7

[Robert Longson [:longsonr]]

(In reply to comment #5)
You are talking about the farthestViewport of svg2 (rather than g) in that answer aren't you?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sorry yes. The farthest viewport of "svg2" would be null, and the farthest viewport of "g" would be svg2.

Comment 9

[Robert Longson [:longsonr]]

(In reply to comment #8)
The idea was to have a test with invalid markup and determine what we should do with it since the spec says the methods don't throw. I'm going to go with returning null for all viewports in that case.

Comment 10

[Robert Longson [:longsonr]]

I'll do a new patch to take foreignObject into account properly.

Comment 11

[Robert Longson [:longsonr]]

Attachment: address review comments

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  if (element) {
+    element.swap(*aFarthestViewportElement);
   }

Is this 'if' needed?

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+<svg xmlns="http://www.w3.org/2000/svg" width="750"
+     xmlns:xlink="http://www.w3.org/1999/xlink" version="1.1" id="root">

Nit: You don't need the 'id' anymore.

Comment 14

[Robert Longson [:longsonr]]

Attachment: address further review comments

Comment 15

[Robert Longson [:longsonr]]

checked in http://hg.mozilla.org/mozilla-central/rev/7cb9b924b8de