Closed Bug 1276579 Opened 3 years ago Closed 3 years ago

Revise document.createElement[NS] for custom elements

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: yrliou, Assigned: yrliou)

References

()

Details

(Keywords: dev-doc-complete, Whiteboard: btpp-active, dom-ce-m1)

Attachments

(2 files)

[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.
Summary: Revise document.CreateElement[NS] for custom elements → Revise document.createElement[NS] for custom elements
Whiteboard: btpp-active
Blocks: 1276240
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
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)
Attachment #8766728 - Flags: review?(bugs) → review?(wchen)
Hi William,

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

Thanks,
Jocelyn
Flags: needinfo?(wchen)
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.
Attachment #8766728 - Flags: review?(wchen) → review-
Flags: needinfo?(wchen)
svg support need to be removed before this bug.
Depends on: 1274505
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.
Flags: needinfo?(wchen)
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
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?
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/
Attachment #8766728 - Flags: review- → review?(wchen)
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.
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?
Attachment #8766728 - Flags: review?(wchen) → review+
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.
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/
Attachment #8766728 - Flags: review?(bugs)
(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
Flags: needinfo?(wchen)
Attachment #8766728 - Flags: review?(bugs) → review+
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.
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/
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/
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
https://hg.mozilla.org/mozilla-central/rev/fc1d44b2cb7b
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Whiteboard: btpp-active → btpp-active, dom-ce-m1
See Also: → 1294100
Depends on: 1318630
Depends on: 1338889
Depends on: 1294100
See Also: 1294100
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.