Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Description • 8 years ago

See https://github.com/whatwg/dom/issues/241


...or we may need to first disable the feature, before it is decided what the new name will be.

Disabling would mean just adding [Pref=some.preference] to the .webidl and having that preference being false by default. And then changing rootNode.html.ini
so that we set the pref when testing.

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

We may want to keep the pref on in non-release builds, so aurora and nightly

	Marcia Knous [:marcia]
	Comment 2 • 8 years ago

smaug: Do you know if this affects 47? Can you also clarify what the pref does and will you work on it? Tracking for 48 and 49.

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

I was hoping jocelyn could take a look at this. The property was added in Bug 1254956.
And since that landed for FF48, this shouldn't affect to 47.

The spec work on this is still ongoing. See https://github.com/whatwg/dom/issues/241
So it is not clear what the property will be called. 
(rniwa is from Apple and hayatoito from Google)

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

Thanks for the notice, I will take care of this.
Looks like they're still discussing possible names at this moment.
Safari already unshipped this feature but Blink doesn't have a plan to unship it yet.
I would like to turn it off only for the beta/release build first (though it won't be any difference before we ship FF48 to beta channel) unless we see more bug reports on this or if there's a consensus that all browsers should unship this before the name is finalized from the web compatibility point of view.

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

yeah, that sounds good, disabled on release/beta, enabled on aurora/nightly.
And once we know what the new name will be, change the name and then possibly enable on beta/release too.

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

I will leave this bug for adopting the change in the spec, open Bug 1270387 for disabling this feature in release builds.

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

The conclusion of https://github.com/whatwg/dom/issues/241 is removing this API, and the changeset has already been reverted in https://github.com/whatwg/dom/commit/42a45eb0d1af13e510620b6b29696997f0c7aa3d.

I guess our action here will be removing this entirely including the preference added in Bug 1270387 from FF48, FF49, and FF50.

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

And the spec was changed again...

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

(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #8)
> And the spec was changed again...

Yes... Would need to revise to getRootNode() then.
ref: https://github.com/whatwg/dom/pull/248

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 10 • 8 years ago

Hi Olli,
Since Bug 1270387 has disabled this feature in release builds, and also to consider the release date of FF48, I wonder if we still think this bug is necessary for FF48, and think we may want to consider setting the tracking flag "status-firefox48" from "affected" to either "fix-optional" or "wontfix," according to the explanation in https://wiki.mozilla.org/Platform#Process

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

This isn't needed for FF48, since .rootNode is disabled.
Fixing this should mean also removing the pref for .rootNode.
Hopefully the new method name is web compatible enough that we won't need to disable it.

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 12 • 8 years ago

(In reply to Olli Pettay [:smaug] (on vacation for couple of days) from comment #11)
> This isn't needed for FF48, since .rootNode is disabled.
> Fixing this should mean also removing the pref for .rootNode.
> Hopefully the new method name is web compatible enough that we won't need to
> disable it.

Thanks Olli. I will change "status-firefox48" to wontfix so that this is removed from Release manager's radar.

	Gerry Chang [:gchang]
	Comment 13 • 8 years ago

Hi Hsin-Yi,
Is there any updates on this?

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 14 • 8 years ago

(In reply to Gerry Chang [:gchang] from comment #13)
> Hi Hsin-Yi,
> Is there any updates on this?

No... 
Since the original feature is disabled in release builds, firefox49 is also unaffected. Is this what you want to understand?

	Gerry Chang [:gchang]
	Comment 15 • 8 years ago

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #14)
> (In reply to Gerry Chang [:gchang] from comment #13)
> > Hi Hsin-Yi,
> > Is there any updates on this?
> 
> No... 
> Since the original feature is disabled in release builds, firefox49 is also
> unaffected. Is this what you want to understand?

Yes. Thanks for clarification.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 16 • 8 years ago

Make sure to revert https://hg.mozilla.org/mozilla-central/rev/d2193462357a

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 17 • 8 years ago

Hi John, could you please take care of this? As this causes web-compat issue, I think we'd consider to handle it soon-ish. :)

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

https://dom.spec.whatwg.org/#dom-node-getrootnode

	John Dai[:jdai] Assignee
	Comment 19 • 7 years ago

- Revise rootNode to getRootNode(). 
- Remove Pref=dom.node.rootNode.enabled.
- Update getRootNode() Web Platform Tests expected results.
- Address comment #16.

Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb31190d5e61a97131ec46485fc3c86897690df1&filter-tier=1&group_state=expanded

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 20 • 7 years ago

Comment on attachment 8822586 [details] [diff] [review]
Bug 1269155 - Revise Node.rootNode to Node.getRootNode.

>+nsINode* nsINode::GetRootNode(const mozilla::dom::GetRootNodeOptions& aOptions)
Nit, return value type to its own line, similar to the method 'SubtreeRoot' below.
And I think you don't need mozilla::dom:: here.
There is after all 
using namespace mozilla;
using namespace mozilla::dom;
in the file

>+{
>+  if (aOptions.mComposed) {
>+    return AsContent()->GetContainingShadow();
>+  }
This is not right. First, the method can be called on non-nsIContent objects, so AsContent() would crash. (just call the method on an document object) and GetContainingShadow()
returns ShadowRoot, but we want the shadow including root node of the tree. So you want shadow host's root node recursively. And in practice we want a fast path for the case when
node is in document, effectively something like if (aOptions.mComposed && IsInComposedDoc()) { return OwnerDoc() ); }
The method should be O(1) in other cases expect when we're dealing with shadow nodes which aren't in document

>-  nsINode* RootNode() const
>-  {
>-    return SubtreeRoot();
>-  }
>+  /*
>+   * Get context objectâs shadow-including root if optionsâs composed is true,
>+   * and context objectâs root otherwise.
Hmm, there are some weird characters here after object. Want to fix?

	John Dai[:jdai] Assignee
	Comment 21 • 7 years ago

(In reply to Olli Pettay [:smaug] from comment #20)
> Comment on attachment 8822586 [details] [diff] [review]
> Bug 1269155 - Revise Node.rootNode to Node.getRootNode.
> 
> >+nsINode* nsINode::GetRootNode(const mozilla::dom::GetRootNodeOptions& aOptions)
> Nit, return value type to its own line, similar to the method 'SubtreeRoot'
> below.
> And I think you don't need mozilla::dom:: here.
> There is after all 
> using namespace mozilla;
> using namespace mozilla::dom;
> in the file
> 
> >+{
> >+  if (aOptions.mComposed) {
> >+    return AsContent()->GetContainingShadow();
> >+  }
> This is not right. First, the method can be called on non-nsIContent
> objects, so AsContent() would crash. (just call the method on an document
> object) and GetContainingShadow()
> returns ShadowRoot, but we want the shadow including root node of the tree.
> So you want shadow host's root node recursively. And in practice we want a
> fast path for the case when
> node is in document, effectively something like if (aOptions.mComposed &&
> IsInComposedDoc()) { return OwnerDoc() ); }
> The method should be O(1) in other cases expect when we're dealing with
> shadow nodes which aren't in document

I think I may misunderstand something. Please correct me if I'm wrong. In your review comment,
> The method should be O(1) in other cases expect when we're dealing with shadow nodes which aren't in document
Is this a typo of expect? It should be *except*.
If this isn't a typo, you said we can use 
> if (aOptions.mComposed && IsInComposedDoc()) { return OwnerDoc() ); }
to apply both nodes are in document and aren't in document.
However, IsInComposedDoc() is a method which only make sure nodes are in Document. Could you explain more about why we can use 
> if (aOptions.mComposed && IsInComposedDoc()) { return OwnerDoc() ); }
in document and aren't in document. Thank you.

> 
> >-  nsINode* RootNode() const
> >-  {
> >-    return SubtreeRoot();
> >-  }
> >+  /*
> >+   * Get context objectâs shadow-including root if optionsâs composed is true,
> >+   * and context objectâs root otherwise.
> Hmm, there are some weird characters here after object. Want to fix?
Will do.

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 22 • 7 years ago

Yes, except, not expect.


if (aOptions.mComposed && IsInComposedDoc()) { return OwnerDoc() ); }
is only an optimization. We want to have O(1) behavior for the cases when node is in document, even when node is in shadow tree.
We need to still have the recursive root node finding for the case we're in shadow dom and want to find the composed root. That should be hopefully rare case and can't be O(1) in our current setup

	John Dai[:jdai] Assignee
	Comment 23 • 7 years ago

Hi Olli,
I addressed comment #20 and comment #22. Could you help me to review it again? Thank you.

Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c186d777bec090102076475bd3c848005e02d944&group_state=expanded&filter-tier=1

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 24 • 7 years ago

Comment on attachment 8826516 [details] [diff] [review]
Bug 1269155 - Revise Node.rootNode to Node.getRootNode. v2


>+
>+    nsINode* node = this;
>+    nsINode* iter = nullptr;
>+    ShadowRoot* shadowRootParent = nullptr;
>+    while(node) {
>+      iter = node;
>+      while ((iter = iter->GetParentNode())) {
>+        node = iter;
>+      }
>+
>+      shadowRootParent = ShadowRoot::FromNode(node);
>+      if (!shadowRootParent) {
>+         break;
>+      }
>+      node = shadowRootParent->GetHost();
>+    }
The inner loop should still use SubtreeRoot(), not GetParentNode() loop.
And in that case it wouldn't be a loop anymore, but just
node = node->SubtreeRoot();
shadowRootParent = ShadowRoot::FromNode(node);
if (!shadowRootParent) {
  break;
}
node = shadowRootParent->GetHost();


With that, r+

	John Dai[:jdai] Assignee
	Comment 25 • 7 years ago

Addressed comment #24, and carried r+.

	John Dai[:jdai] Assignee
	Comment 26 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=38b33f0b6a5e5697c016ed0268745c2dea529c77&filter-tier=1&group_state=expanded

	Pulsebot
	Comment 27 • 7 years ago

Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/14a12ac05a4b
Revise Node.rootNode to Node.getRootNode. r=smaug

	Ryan VanderMeulen [:RyanVM]
	Comment 28 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/14a12ac05a4b

	Marco Castelluccio [:marco]
	Comment 29 • 7 years ago

Can we uplift this patch to 52, since it appears to fix the HSBC website (bug 1325357)?

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 30 • 7 years ago

How super weird. Do they really depend on this very new DOM feature ?

	Marco Castelluccio [:marco]
	Comment 31 • 7 years ago

Not sure why it's broken without this patch, but the pushlog in bug 1325357 comment 40 (which was generated to find what fixed the bug in 53) contains this change.

We could ask the user to run mozregression again to find what broke the site between 51 and 52.

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 32 • 7 years ago

That would be good, to actually understand what broke the site.

	Mike Taylor [:miketaylr]
	Comment 33 • 7 years ago

(In reply to Olli Pettay [:smaug] from comment #30)
> How super weird. Do they really depend on this very new DOM feature ?

Node.rootNode broke sites in Chrome and WebKit due to scripts relying on some random rootNode property, so possibly just another instance of this new prop clobbering existing code? 

https://bugs.webkit.org/show_bug.cgi?id=157233
https://bugs.chromium.org/p/chromium/issues/detail?id=600950

If anyone has access to HSBC UK, it would be interesting to search for all instances of rootNode on the site, and if possible download the scripts that do have it (and email them to me?).

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 34 • 7 years ago

rootNode was already disabled in bug 1270387, and this bug added getRootNode().
Oh, hmm, Aurora had .rootNode for some time when Nightly didn't, I guess.

	Mike Taylor [:miketaylr]
	Comment 35 • 7 years ago

> If anyone has access to HSBC UK, it would be interesting to search for all instances of rootNode on the site, and if possible download the scripts that do have it (and email them to me?).

Actually, nevermind.

https://www.security.hsbc.co.uk/gsp/saas/Components/default/resources/script/libraries/dtk/dojo/dojo.js

var root;if(!_484&&_483&&_483.rootNode){_484=_483;root=_484.rootNode;...

That's probably enough to break the site.

So, rootNode was only ever dev edition and nightly, and the HSBC bug only repros in dev edition and nightly...

-#ifdef RELEASE_OR_BETA
-pref("dom.node.rootNode.enabled", false);
-#else
-pref("dom.node.rootNode.enabled", true);
-#endif

And this patch fixes the bug in dev edition and nightly...

Do we need beta (currently 52) uplift if the prefs weren't removed until 53?

(I feel like I might be missing something...)

	John Dai[:jdai] Assignee
	Comment 36 • 7 years ago

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1325357 causing the regression.
[User impact if declined]: It will enable in firefox 53.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:  none
[Is the change risky?]: minor risk
[Why is the change risky/not risky?]: We need this on firefox 52 to avoid regression.
[String changes made/needed]: none

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 37 • 7 years ago

I don't understand why we need approval here. HSBC site was broken only in nightly/aurora.

	Marco Castelluccio [:marco]
	Comment 38 • 7 years ago

Yes, there's no need to uplift anymore, since the `dom.node.rootNode.enabled` was only enabled in nightly/aurora.

	Julien Cristau [:jcristau]
	Comment 39 • 7 years ago

Comment on attachment 8833933 [details] [diff] [review]
(Beta) Bug 1269155 - Revise Node.rootNode to Node.getRootNode. (Final) r=smaug

There seems to be some confusion as to whether this is needed on beta, a- for now...

	Chris Mills [:cmills]
	Comment 40 • 7 years ago

I've updated browser support info in appropriate places and made sure the method is property documented:

https://developer.mozilla.org/en-US/docs/Web/API/Node
https://developer.mozilla.org/en-US/docs/Web/API/Node/getRootNode
https://developer.mozilla.org/en-US/docs/Web/API/Node/rootNode

I've also added a note to the Fx53 release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/53