1589134 - Move <script> tags out of <wizard>s

Assignee

Description

•

5 years ago

This is a follow up to Bug 1584283, where we stopped using <wizard> as a top-level element. Originally, the <script> tags were going to be moved outside of the <wizard>s in that bug, but it turned out to be trickier than it appeared, causing code to be run in a different order than before.

This bug will involve moving the <script> tags, as well as ensuring that the <wizards> are not broken by doing so.

Robin Steuber (she/her) [:bytesized]

Assignee

Comment 1

•

5 years ago

Attached file Bug 1589134 - Move <script> tags out of <wizard>s r=surkov,bgrins — Details

This patch is currently incomplete. It moves the script tags, but does not address the issues that occur when this change is made.

Robin Steuber (she/her) [:bytesized]

Assignee

Updated

•

5 years ago

Assignee: nobody → ksteuber

Phabricator Automation

Updated

•

5 years ago

Attachment #9101685 - Attachment description: Bug 1589134 - Move <script> tags out of <wizard>s → Bug 1589134 - Move <script> tags out of <wizard>s r=surkov,bgrins

Brian Grinstead [:bgrins]

Comment 2

•

5 years ago

I believe the two wizard UIs we have ("Import Data From Another Browser" and "Create Profile") are not heavily tested in automation. Once this and Bug 1584283 land it would be good to include some manual testing to confirm we haven't changed any behavior.

Flags: qe-verify+

Robin Steuber (she/her) [:bytesized]

Assignee

Comment 3

•

5 years ago

@emilio - I am having some unexpected problems that I think suggests a bug in the Custom Element implementation. This problem occurs when moving <script> tags outside of their <wizard> element, into the parent <window> element. If you would like to see this in action, you can update to the latest mozilla-central (you will need the patch from Bug 1584283, which just merged) and apply this patch.

Prior to moving the <script> tags (when they are still within the <wizard> element), I see the MozWizard's components being loaded in this order:

MozWizard::constructor
MozWizard::connectedCallback calls this.delayConnectedCallback() which returns true, so it immediately returns.
MozWizardButtons::constructor
MozWizardButtons::connectedCallback
MozWizard::connectedCallback calls this.delayConnectedCallback() which returns false this time. This lets the function run, completing the Wizard's setup.

After moving the <script> tags, I see this ordering:

MozWizard::constructor
MozWizard::connectedCallback calls this.delayConnectedCallback() which returns false, causing the function to attempt to finish the setup of the uninitialized MozWizardButtons.
MozWizardButtons::constructor
MozWizardButtons::connectedCallback

This leads me to believe that delayConnectedCallback is not working properly. Shouldn't it always delay until after MozWizardButtons::constructor is called?

Flags: needinfo?(emilio)

Emilio Cobos Álvarez (:emilio)

Comment 4

•

5 years ago

Not necessarily, I think... delayConnectedCallback is not a standard thing, it's just "wait until dom content loaded".

I think this is working as expected, to some extent. Before your patch, the order is:

<wizard> upgrade reaction enqueued.
<wizard> connected callback reaction enqueued.
<script> is found. Executing script ends up draining the micro-task queue and running reactions, so the two above run, without content loaded having run.
Find some buttons, enqueue their constructors and connected callbacks...

Now after your patch you don't have that middle step, so the thing looks like:

<wizard> upgrade reaction enqueued.
<wizard> connected callback reaction enqueued.
Find some buttons, enqueue their constructors and connected callbacks...
Finish parsing, domcontentloaded fires.
Drain queue, execute the callbacks.

Per https://html.spec.whatwg.org/#invoke-custom-element-reactions, I think this is working as expected.

This is also the behavior of this simple test-case:

<!doctype html>
<script>
  customElements.define("c-e-1", class extends HTMLElement {
    constructor() {
      super();
      console.log("c-e-1 constructor");
    }

    connectedCallback() {
      console.log("c-e-1 connected");
    }
  });

  customElements.define("c-e-2", class extends HTMLElement {
    constructor() {
      super();
      console.log("c-e-2 constructor");
    }

    connectedCallback() {
      console.log("c-e-2 connected");
    }
  });
</script>
<c-e-1>
  <c-e-2>
  </c-e-2>
</c-e-1>

I agree it's not the best behavior... It seems to me you should have a way to guarantee that all your children are constructed at best, but it seems it's not the case, and this behaves interoperably everywhere...

Flags: needinfo?(emilio)

Brian Grinstead [:bgrins]

Comment 5

•

5 years ago

Thanks Emilio, that makes sense and clears things up. Sounds like we need to:

Fix <wizard> so that it can support unupgraded children buttons (Kirk has a patch up that does this)
Rethink or likely remove entirely the delayConnectedCallback mechanism. It was originally added to better handle timing with XBL construction but since we don't have XBL anymore it might be time to just remove it. I seem to remember it helped with perf in the past as well, so we'll need to check if that's still the case today.

Brian Grinstead [:bgrins]

Comment 6

•

5 years ago

One note regarding removing delayConnectedCallback: for some reason it's needed in the <browser> element (I tried removing it in a patch recently and am getting Assertion failure: ChildID() == aBC->Canonical()->GetInFlightProcessId() (Attempt to modify a BrowsingContext from a child which doesn't own it): https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2b6740725d99fc7ae524f82b860087c2c77f7e5&selectedJob=272240989) which is different then the original reason I added it (Bug 1519394). That said, if we could remove it on all other elements we could move that directly into browser-custom-element.js.

Brian Grinstead [:bgrins]

Updated

•

5 years ago

Blocks: 1590923

Brian Grinstead [:bgrins]

Comment 7

•

5 years ago

(In reply to Brian Grinstead [:bgrins] from comment #5)

Thanks Emilio, that makes sense and clears things up. Sounds like we need to:

Fix <wizard> so that it can support unupgraded children buttons (Kirk has a patch up that does this)

Rethink or likely remove entirely the delayConnectedCallback mechanism. It was originally added to better handle timing with XBL construction but since we don't have XBL anymore it might be time to just remove it. I seem to remember it helped with perf in the past as well, so we'll need to check if that's still the case today.

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1590923 for (2).

Emilio Cobos Álvarez (:emilio)

Comment 8

•

5 years ago

Brian, Anne told me it would be useful to chime in in the relevant spec issues here, or file a new one. I asked him because I was sad I couldn't provide a solution that wasn't a bit hacky :)

Would you or Kirk have time for that?

Flags: needinfo?(bgrinstead)

Brian Grinstead [:bgrins]

Comment 9

•

5 years ago

(In reply to Emilio Cobos Álvarez (:emilio) from comment #8)

Brian, Anne told me it would be useful to chime in in the relevant spec issues here, or file a new one. I asked him because I was sad I couldn't provide a solution that wasn't a bit hacky :)

Would you or Kirk have time for that?

Sure! Anne, did you have any open spec issues in mind that discuss this behavior (from comment 3 / comment 4)?

Flags: needinfo?(bgrinstead) → needinfo?(annevk)

Brian Grinstead [:bgrins]

Comment 10

•

5 years ago

Actually, I think I found them (#809 and then the linked issues from https://github.com/w3c/webcomponents/issues/809#issuecomment-488093265). We will read up on that and see if we have something to add based on our experience here.

Flags: needinfo?(annevk)

Robin Steuber (she/her) [:bytesized]

Assignee

Comment 11

•

5 years ago

@emilio - One more question: MozWizard is currently relying on being able to access its MozWizardPage children in its connectedCallback function. It seems that, in general, we cannot rely on children being loaded in time for connectedCallback, so I am concerned. But the code works consistently, a fact that we believe is due to the prototype cache.

Is it safe for us to rely on this? It sounds to me like, since the first pass over chrome private XHTML documents will always create all the prototype elements from the source or from the prototype cache, we can assume that the MozWizardPages will be there before MozWizard::connectedCallback runs. But if this behavior is due to some sort of race condition, and is not guaranteed, I should probably change this implementation to delay this code until the children are guaranteed to be present.

Flags: needinfo?(emilio)

Emilio Cobos Álvarez (:emilio)

Comment 12

•

5 years ago

I'm not familiar with the prototype cache enough to say for sure. If it upgrades elements then it should probably work consistently, though it's a subtle thing to rely on...

Flags: needinfo?(emilio)

Brian Grinstead [:bgrins]

Comment 13

•

5 years ago

(In reply to Emilio Cobos Álvarez (:emilio) from comment #12)

I'm not familiar with the prototype cache enough to say for sure. If it upgrades elements then it should probably work consistently, though it's a subtle thing to rely on...

needinfoing Brendan to see if he knows the answer to Comment 11.

I also remembered that we have some similar-ish initialization code for <radiogroup><radio /></radiogroup> which expects this behavior:

Parent radio connectedCallback expects children to be available (although sometimes they are upgraded and sometimes not) https://searchfox.org/mozilla-central/rev/2a355e56c490768be676689de18629485c9d699b/toolkit/content/widgets/radio.js#138-148,161
Child radio is upgraded with customElements.upgrade: https://searchfox.org/mozilla-central/rev/2a355e56c490768be676689de18629485c9d699b/toolkit/content/widgets/radio.js#422
Child radio signals to parent radiogroup it's connected: https://searchfox.org/mozilla-central/rev/2a355e56c490768be676689de18629485c9d699b/toolkit/content/widgets/radio.js#523

Because of (2), I think this would work (mostly) correctly even if the children weren't available in (1), but it's been running this way for quite some time and I think it'd be OK for <wizard> to have this assumption.

That said, it's already a lot of hoops to jump through and I think it gets even more complex in the web standard (no children at connection) case, so it would be good to weigh in on the spec issue to see if there's anything we could do to make this easier / less error prone.

Flags: needinfo?(bdahl)

Brendan Dahl [:bdahl]

Comment 14

•

5 years ago

(In reply to Kirk Steuber (he/him) [:bytesized] from comment #11)

@emilio - One more question: MozWizard is currently relying on being able to access its MozWizardPage children in its connectedCallback function. It seems that, in general, we cannot rely on children being loaded in time for connectedCallback, so I am concerned. But the code works consistently, a fact that we believe is due to the prototype cache.

Is it safe for us to rely on this? It sounds to me like, since the first pass over chrome private XHTML documents will always create all the prototype elements from the source or from the prototype cache, we can assume that the MozWizardPages will be there before MozWizard::connectedCallback runs. But if this behavior is due to some sort of race condition, and is not guaranteed, I should probably change this implementation to delay this code until the children are guaranteed to be present.

I don't think we should rely on that behavior and it sounds like there may be a bug with the prototype cache. Ideally there should be no way to tell if something uses the prototype cache or not, so we don't introduce weird chrome html only behavior.

If you have a simple example of the different behavior could you try adding a test to https://searchfox.org/mozilla-central/source/dom/prototype/tests/chrome/test_prototype_document.xhtml and I'll take a look at fixing it.

Flags: needinfo?(bdahl)

Pulsebot

Comment 15

•

5 years ago

Pushed by ksteuber@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b76033ea35ef
Move <script> tags out of <wizard>s r=bgrins

Natalia Csoregi [:nataliaCs]

Comment 16

•

5 years ago

bugherder

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

Status: NEW → RESOLVED

Closed: 5 years ago

status-firefox72: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla72

Rares Doghi, Desktop QA

Comment 17

•

5 years ago

Hello, we are trying to Verify this issue, are there any Steps we can use or maybe can someone help us with a reduced test case on this matter ?

Flags: needinfo?(ksteuber)

Robin Steuber (she/her) [:bytesized]

Assignee

Comment 18

•

5 years ago

This patch changed the implementation of Firefox wizards, of which there are currently two. It would be good to test them both.

The first is the profile wizard, which can be accessed by navigating to "about:profiles" and pressing the "Create new profile" button. A successful completion of this wizard should result in another entry in the profile list on the same page.

The second is the migration wizard, which can be opened via the bookmarks window: Hamburger Menu -> Library -> Bookmarks -> Show All Bookmarks. From there, click Import and Backup -> Import data from another browser. A successful completion of this wizard should result in the specified data being available in the browser in the appropriate places (bookmarks and history would be good ones to check).

In addition to primary functionality, we would like to ensure that the appearance of the wizard pages have not changed. They should look the same before and after this patch. Button texts and behaviors should also be the same ("Back", "Next", "Finish", "Cancel").

Flags: needinfo?(ksteuber)

Rares Doghi, Desktop QA

Comment 19

•

5 years ago

This patch seems to be working correctly in our latest Nightly build 72.0a1 (2019-11-11) on Windows 10, Mac OsX 10.15 and Ubuntu 16.04.

Thanks for the extra info, Both the Create a profile wizard and the Migration from a different browser work correctly. I will update the flags.

Status: RESOLVED → VERIFIED

status-firefox72: fixed → verified

Flags: qe-verify+