Closed Bug 1213818 Opened 4 years ago Closed 4 years ago

Align document.title for SVG documents with HTML spec

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: ayg, Assigned: ayg)

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #8672704 - Flags: review?(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.
Attachment #8672704 - Flags: review?(bzbarsky) → review-
Attached patch Patch v2Splinter Review
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
Attachment #8672704 - Attachment is obsolete: true
Attachment #8673075 - Flags: review?(bzbarsky)
Comment on attachment 8673075 [details] [diff] [review]
Patch v2

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

better as

if (rootElement->IsXULElement()) {
> IE does, Chrome does not.

Please file a bug on Chrome?
Flags: needinfo?(ayg)
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
Attachment #8673075 - Flags: review?(bzbarsky) → review+
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.
Flags: needinfo?(ayg)
https://hg.mozilla.org/mozilla-central/rev/6736becee155
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.