1213818 - Align document.title for SVG documents with HTML spec

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Blocked for the moment on <https://github.com/whatwg/html/issues/247>.

Comment 2

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b339d01a3c58

This assumes the spec change is accepted.  If not, it's just a matter of changing InsertChildAt to AppendChildTo.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8672704 [details] [diff] [review]
Patch

>+  if (aNamespace == kNameSpaceID_SVG) {

Do other UAs also restruct to children of the root here?

>+      if (child->GetNameSpaceID() == kNameSpaceID_SVG &&
>+          child->NodeInfo()->NameAtom() == nsGkAtoms::title) {

  if (child->IsSVGElement(nsGkAtoms::title)) {

>+  return static_cast<Element*>(list->Item(0, false));

Please use AsElement() here.  That will also assert that this is in fact an element....

You should probably rename this method to GetTitleElement.  And it's not clear to me that we still want to pass in the namespace, as opposed to GetTitleElement just doing the GetRootElement() thing itself; seems like it might simplify callers some to do it that way.

Also, please fix the documentation to describe what the method actually does, since it's no longer just getting the first element with the given type.

>+  if (rootElement->GetNameSpaceID() == kNameSpaceID_SVG &&
>+      rootElement->NodeInfo()->NameAtom() == nsGkAtoms::svg) {

  if (rootElement->IsSVGElement(nsGkAtoms::svg)) {

>+  } else if (rootElement->GetNameSpaceID() == kNameSpaceID_XHTML) {

  if (rootElement->IsHTMLElement()) {

>+      NS_NewSVGElement(getter_AddRefs(title), titleInfo.forget(),

Can fail, no?  Need to deal if it does.

Comment 4

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch v2

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e98a32507c8f

(In reply to Boris Zbarsky [:bz] from comment #3)
> Do other UAs also restruct to children of the root here?

IE does, Chrome does not.  Since <title> in SVG applies to the parent element, not the document, IE's behavior makes far more sense, unless the SVG spec does not match reality here: http://www.w3.org/TR/SVG/struct.html#DescriptionAndTitleElements

Comment 5

[Robert Longson [:longsonr]]

Comment on attachment 8673075 [details] [diff] [review]
Patch v2

>+  if (rootElement->GetNameSpaceID() == kNameSpaceID_XUL) {

better as

if (rootElement->IsXULElement()) {

Comment 6

[Boris Zbarsky [:bzbarsky]]

> IE does, Chrome does not.

Please file a bug on Chrome?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8673075 [details] [diff] [review]
Patch v2

>+      if (!title) {
>         return NS_OK;

Hmm.  I guess that's what we used to do for HTML too.  A bit weird, but alright.

>+  if (rootElement->GetNameSpaceID() == kNameSpaceID_XUL) {

IsXULElement(), both places.

>+      {
>+        nsRefPtr<mozilla::dom::NodeInfo> titleInfo;

I wouldn't bother with this extra scope.  It just makes sure the titleInfo ref goes away before we do head->AppendChildTo(), but I don't think there's a reason to enforce that.

>+   * specification, or null if there isn't one.  For SVG documents, this is 

No, this method doesn't care what sort of _document_ this is.  It just cares about the namespace and localname of the root element.  So this should be "For documents whose root element is an <svg:svg>" or some such.

r=me with those fixes

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6736becee155

Comment 9

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I forgot to wait for the actual spec change to be accepted before landing, but it's sort of a no-brainer (makes more sense *and* matches only existing implementation), so it doesn't really matter.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6736becee155

Comment 11

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

https://code.google.com/p/chromium/issues/detail?id=543061