Jocelyn Liu [:jocelyn] [:joliu] Assignee
	Description • 8 years ago

[NewObject]
Element createElement(DOMString localName, optional ElementCreationOptions options
);
[NewObject] Element createElementNS(DOMString? namespace, DOMString qualifiedName , optional ElementCreationOptions options
);

Revise these methods to have an options argument, and implement the logic specified in https://dom.spec.whatwg.org/#dom-document-createelement and https://dom.spec.whatwg.org/#dom-document-createelementns.

	Jocelyn Liu [:jocelyn] [:joliu] Assignee
	Comment 1 • 8 years ago

A more detailed note regarding to the scope of this bug, algorithm specified in https://dom.spec.whatwg.org/#concept-create-element will not be revised by this bug since we haven't implemented 'constructor' in the registry.
We will leave that part unchanged and use setupCustomElement to do the prototype swizzle instead of calling Construct(c) directly.

Below changes should be covered by this bug:
1. Add 'options' argument into createElement[NS] methods
2. if 'is' is specified but the definition is not registered yet, NotFoundError will be throwed directly.
3. remove overloaded createElement[NS] methods
4. revise current mochitest

	Jocelyn Liu [:jocelyn] [:joliu] Assignee
	Comment 2 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/61482/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61482/

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 3 • 8 years ago

Comment on attachment 8766728 [details]
Bug 1276579: Add 'options' argument into |document.createElement[NS]()| methods and remove overloaded |createElement[NS]()| methods.

I wonder if William could review this.
(I don't know how to move the review request in MozReview)

	Jocelyn Liu [:jocelyn] [:joliu] Assignee
	Comment 4 • 8 years ago

Hi William,

It's just a gentle ping, could you help to review my patches?

Thanks,
Jocelyn

	William Chen [:wchen]
	Comment 5 • 8 years ago

Comment on attachment 8766728 [details]
Bug 1276579: Add 'options' argument into |document.createElement[NS]()| methods and remove overloaded |createElement[NS]()| methods.

https://reviewboard.mozilla.org/r/61482/#review59562

::: dom/base/nsDocument.cpp:5614
(Diff revision 1)
> +  /**
> +   * FIXME: SVG namespace case should be removed when we remove SVG-based
> +   *        custom elemen support in bug 1274505.
> +   */
> +  if ((aNameSpaceID != kNameSpaceID_XHTML &&
> +       aNameSpaceID != kNameSpaceID_SVG)

I don't think we should try to support SVG for the new implementation.

::: dom/base/nsDocument.cpp:5648
(Diff revision 1)
>    nsAutoString lcTagName;
>    if (needsLowercase) {
>      nsContentUtils::ASCIIToLower(aTagName, lcTagName);
>    }
>  
> -  return CreateElem(needsLowercase ? lcTagName : aTagName, nullptr,
> +  bool isOptionsProcessed =

I think the new code here should live in NS_NewHTMLElement because custom elements can only be in the HTML namespace in the new specification. It would also avoid duplication of code here and CreateElementNS.

This code is likely going to be performance sensitive so we need to be careful about only doing work for custom elements when we need to. The first thing it should check is aOptions.mIs and don't do any extra work if it's not passed.

::: dom/base/nsDocument.cpp:5740
(Diff revision 1)
>                                              getter_AddRefs(nodeInfo));
>    if (rv.Failed()) {
>      return nullptr;
>    }
>  
> -  nsCOMPtr<Element> element;
> +  bool isOptionsProcessed =

See comments for ::CreateElement

If you move the new code to NS_NewHTMLElement, then you won't have to duplicate code here. I think the only thing you'll need to add here is a check to return NS_ERROR_DOM_NOT_FOUND_ERR if aOption.mIs was passed and the namespace is not HTML because according to the spec the custom element definition will always be null for non-HTML namespace.

::: dom/base/nsIDocument.h:2502
(Diff revision 1)
>                             const nsAString& aLocalName,
>                             mozilla::ErrorResult& aResult);
>    already_AddRefed<nsContentList>
>      GetElementsByClassName(const nsAString& aClasses);
>    // GetElementById defined above
> -  already_AddRefed<Element> CreateElement(const nsAString& aTagName,
> +  virtual already_AddRefed<Element> CreateElement(

Nit: For long signatures, it looks like the local style in this file is to put the return value on its own line followed by an indent. Then I think all the arguments will fit in 80 cols.

::: dom/svg/SVGElementFactory.cpp:117
(Diff revision 1)
>    return tag != nullptr;
>  }
>  
>  nsresult
>  NS_NewSVGElement(Element** aResult, already_AddRefed<mozilla::dom::NodeInfo>&& aNodeInfo,
> -                 FromParser aFromParser)
> +                 FromParser aFromParser, nsAString* aIs)

We don't need to support custom element extension for SVG in the new spec and although the old implementation still lets you register definitions with SVG namespace, I don't think anyone is using it and it should be fine to let it not work in the transition to the new API.

::: dom/webidl/Document.webidl:17
(Diff revision 1)
>  interface nsIDocShell;
>  interface nsILoadGroup;
>  
>  enum VisibilityState { "hidden", "visible", "prerender" };
>  
> +dictionary ElementCreationOptions {

Put https://dom.spec.whatwg.org/#dictdef-elementcreationoptions in the comments here.

	Jocelyn Liu [:jocelyn] [:joliu] Assignee
	Comment 6 • 8 years ago

svg support need to be removed before this bug.

	Jocelyn Liu [:jocelyn] [:joliu] Assignee
	Comment 7 • 8 years ago

https://reviewboard.mozilla.org/r/61482/#review59562

> See comments for ::CreateElement
> 
> If you move the new code to NS_NewHTMLElement, then you won't have to duplicate code here. I think the only thing you'll need to add here is a check to return NS_ERROR_DOM_NOT_FOUND_ERR if aOption.mIs was passed and the namespace is not HTML because according to the spec the custom element definition will always be null for non-HTML namespace.

From the step 5 of https://dom.spec.whatwg.org/#dom-document-createelement, shouldn't we also search for the definition list and return NOT_FOUND_ERROR if definition is null here?
I could call LookupCustomElementDefinition only when 'is' is nullptr here, but I didn't quite get why we would need to defer the lookup to NS_NewHTMLElement when 'is' was passed.

	William Chen [:wchen]
	Comment 8 • 8 years ago

https://reviewboard.mozilla.org/r/61482/#review59562

> From the step 5 of https://dom.spec.whatwg.org/#dom-document-createelement, shouldn't we also search for the definition list and return NOT_FOUND_ERROR if definition is null here?
> I could call LookupCustomElementDefinition only when 'is' is nullptr here, but I didn't quite get why we would need to defer the lookup to NS_NewHTMLElement when 'is' was passed.

When creating HTML elements, both ::CreateElement and ::CreateElementNS use NS_NewHTMLElement. My idea is that you would do step 5 in NS_NewHTMLElement so that we don't duplicate the code. If we do that, then in ::CreateElementNS we only need to handle the case when 'is' is not null and the namespace is not HTML. According to the defintion lookup steps [1], the definition is always null when the namespace is not HTML so we can return NS_ERROR_DOM_NOT_FOUND_ERROR right away.

[1] https://html.spec.whatwg.org/multipage/scripting.html#look-up-a-custom-element-definition

	Jocelyn Liu [:jocelyn] [:joliu] Assignee
	Comment 9 • 8 years ago

https://reviewboard.mozilla.org/r/61482/#review59562

> When creating HTML elements, both ::CreateElement and ::CreateElementNS use NS_NewHTMLElement. My idea is that you would do step 5 in NS_NewHTMLElement so that we don't duplicate the code. If we do that, then in ::CreateElementNS we only need to handle the case when 'is' is not null and the namespace is not HTML. According to the defintion lookup steps [1], the definition is always null when the namespace is not HTML so we can return NS_ERROR_DOM_NOT_FOUND_ERROR right away.
> 
> [1] https://html.spec.whatwg.org/multipage/scripting.html#look-up-a-custom-element-definition

Hi William,

Personally I think calling a function to do step 5 when 'is' was passed in ::CreateElement and ::CreateElementNS directly might have a better readability.
I think it's easier to read checks which are related to the parameter and preference directly in WebAPI functions, and it's easier to match with the spefication documentation.
I tried to put the check into NS_NewHTMLElement, but found that I might need to modify some functions that are not only for |createElement| and |createElementNS| methods (for example, |CreateElem()| )which might slightly decrease the readability of those functions which have other consumers.
And the code of this check need to be scattered at least for the looking up part (::NS_NewHTMLElement) and the throw part (::CreateElem).
Does this make sense to you?

If we wrap the duplicate code into a function and only check the existance of the definition when 'is' was passed, would you still think there would be downsides in terms of performance and code duplication?

	Jocelyn Liu [:jocelyn] [:joliu] Assignee
	Comment 10 • 8 years ago

Comment on attachment 8766728 [details]
Bug 1276579: Add 'options' argument into |document.createElement[NS]()| methods and remove overloaded |createElement[NS]()| methods.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61482/diff/1-2/

	Jocelyn Liu [:jocelyn] [:joliu] Assignee
	Comment 11 • 8 years ago

I revise the patch accordingly excluding moving codes into NS_NewHTMLElement since we're still discussing and hope this patch could help to illustrate my approach mentioned in the Comment 9.
Upload the interdiff since the interdiff between this and last revision is broken in the mozreview.

	William Chen [:wchen]
	Comment 12 • 8 years ago

Comment on attachment 8766728 [details]
Bug 1276579: Add 'options' argument into |document.createElement[NS]()| methods and remove overloaded |createElement[NS]()| methods.

https://reviewboard.mozilla.org/r/61482/#review62410

This new approach looks good to me. Thanks for working on it.

::: dom/base/nsDocument.cpp:5608
(Diff revision 2)
>    }
>    return true;
>  }
>  
> +CustomElementDefinition*
> +nsDocument::LookupCustomElementDefinition(const nsAString& aLocalName,

Can you also use this method in ::SetupCustomElement?

::: dom/base/nsDocument.cpp:13668
(Diff revision 2)
>    mStyleBackendType = StyleBackendType::Gecko;
>  #endif
>  }
> +
> +nsString*
> +nsDocument::CheckCreationOptions(const ElementCreationOptions& aOptions,

I don't feel that this method name matches what it is doing. I don't really have a good suggestion here, maybe call it ::GetCustomElementName?

	Jocelyn Liu [:jocelyn] [:joliu] Assignee
	Comment 13 • 8 years ago

https://reviewboard.mozilla.org/r/61482/#review62410

> I don't feel that this method name matches what it is doing. I don't really have a good suggestion here, maybe call it ::GetCustomElementName?

The main purpose of this method is to check the 'is' passed by application through CreationOptions should be match with a existing definition, 'is' is returned just for future usage.
so GetCustomElementName probably doesn't fit here.
I'll try to come up with a better ::CheckXXX method name here and write comments in the header file.

	Jocelyn Liu [:jocelyn] [:joliu] Assignee
	Comment 14 • 8 years ago

Comment on attachment 8766728 [details]
Bug 1276579: Add 'options' argument into |document.createElement[NS]()| methods and remove overloaded |createElement[NS]()| methods.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61482/diff/2-3/

	Jocelyn Liu [:jocelyn] [:joliu] Assignee
	Comment 15 • 8 years ago

(In reply to Olli Pettay [:smaug] from comment #3)
> Comment on attachment 8766728 [details]
> Bug 1276579: Add 'options' argument into |document.createElement[NS]()|
> methods and remove overloaded |createElement[NS]()| methods.
> 
> I wonder if William could review this.
> (I don't know how to move the review request in MozReview)

This also needs to be reviewed by a DOM peer since I changed the WebIDL file, could you help me review it?

Thanks,
Jocelyn

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 16 • 8 years ago

Comment on attachment 8766728 [details]
Bug 1276579: Add 'options' argument into |document.createElement[NS]()| methods and remove overloaded |createElement[NS]()| methods.

https://reviewboard.mozilla.org/r/61482/#review62638

r+ for the webidl.

	Jocelyn Liu [:jocelyn] [:joliu] Assignee
	Comment 17 • 8 years ago

Comment on attachment 8766728 [details]
Bug 1276579: Add 'options' argument into |document.createElement[NS]()| methods and remove overloaded |createElement[NS]()| methods.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61482/diff/3-4/

	Jocelyn Liu [:jocelyn] [:joliu] Assignee
	Comment 18 • 8 years ago

Comment on attachment 8766728 [details]
Bug 1276579: Add 'options' argument into |document.createElement[NS]()| methods and remove overloaded |createElement[NS]()| methods.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61482/diff/4-5/

	Pulsebot
	Comment 19 • 8 years ago

Pushed by joliu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fc1d44b2cb7b
Add 'options' argument into |document.createElement[NS]()| methods and remove overloaded |createElement[NS]()| methods. r=smaug,wchen

	Carsten Book [:Tomcat]
	Comment 20 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/fc1d44b2cb7b

	Florian Scholz (Open Web Docs)
	Comment 21 • 8 years ago

Doc updates thanks to Will Bamberg:
https://developer.mozilla.org/en-US/Firefox/Releases/50#DOM
https://developer.mozilla.org/en-US/docs/Web/API/Document/createElement
https://developer.mozilla.org/en-US/docs/Web/API/document/createElementNS