1496704 - TEST-UNEXPECTED-FAIL | [snip]/mozmill/composition/test-image-insertion-dialog.js

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

This is of course an old hat, see bug 1246094, but now it's failing permanently.

Since we're currently a little busted, we can see the failure in try runs.

Still OK here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=da8414931b1ff8e187527ea29230a4b15821909b
M-C: 301f1711aab2fabca34a6c80d6bf4f4277

Started to fail here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&selectedJob=203587524&revision=58967297bae7dde7ff65ee196bf39fa3e9c42145
M-C: a7cd9a613cac1a27d38aafea2e053b7825

Those are one M-C merge apart:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=301f1711aab2fabca34a6c80d6bf4f4277&tochange=a7cd9a613cac1a27d38aafea2e053b7825

The only thing I can see that might be vaguely related is
d8313ee5547e - Brian Grinstead - Bug 1496137 - Handle asynchronous XBL construction of <radio> elements beneath <radiogroup>;r=jaws

Aceman, we don't have enough bustage yet :-(

Comment 1

[Jorg K (CEST = GMT+2)]

OK, I ran at at M-C rev d8313ee5547e and it failed. The changset just before was fine.
https://hg.mozilla.org/mozilla-central/rev/d8313ee5547e

SUMMARY-UNEXPECTED-FAIL | c:\mozilla-source\comm-central\comm\mail\test\mozmill\composition\test-image-insertion-dialog.js | test-image-insertion-dialog.js::test_image_insertion_dialog_persist
  EXCEPTION: We should persist the previously selected value
    at: test-folder-display-helpers.js line 2955
       assert_true test-folder-display-helpers.js:2955 11
       insert_image test-image-insertion-dialog.js:59 5

The failing line is
      assert_true(mwc.window.document.getElementById("noAltTextRadio").selected,

Brian, would it be possible that you lost the persistence of the selection when launching a/the panel again, or do we just have to wait async now?

Comment 2

[Brian Grinstead [:bgrins]]

(In reply to Jorg K (GMT+2) from comment #1)
> The failing line is
>      
> assert_true(mwc.window.document.getElementById("noAltTextRadio").selected,
> 
> Brian, would it be possible that you lost the persistence of the selection
> when launching a/the panel again, or do we just have to wait async now?

I don't expect that waiting async would fix this. Does it?

I wonder if the persisted attribute doesn't get restored until after the radiogroup connectedCallback runs, or something. That would lead it to reset `this.selectedIndex = 0;` in connectedCallback.

Can you check if the "noAltTextRadio" element is part of the `var children = this._getRadioChildren();` array in the init() function? And if so, does it have the "selected" attr at that time.

Comment 3

[Jorg K (CEST = GMT+2)]

First of all I noticed some unhealthy-looking errors:

JavaScript error: chrome://global/content/bindings/radio.xml, line 29: TypeError: control.radioChildConstructed is not a function

I'm looking at

  init() {
    this._radioChildren = null;

    if (this.getAttribute("disabled") == "true")
      this.disabled = true;

    var children = this._getRadioChildren();
    var length = children.length;
    for (var i = 0; i < length; i++) {
      dump(`${children[i].getAttribute("name")} ${children[i].getAttribute("value")} ${children[i].getAttribute("selected")}\n`);

and added a dump to it. But how can I work out whether "noAltTextRadio" is one of the children. They don't seem to have names, and the value is empty. So all that prints is true without context.

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to Jorg K (GMT+2) from comment #3)
> First of all I noticed some unhealthy-looking errors:
> 
> JavaScript error: chrome://global/content/bindings/radio.xml, line 29:
> TypeError: control.radioChildConstructed is not a function

Hm, so I guess when the radio XBL binding <constructor> is running the <radiogroup> custom element hasn't been upgraded... My best guess is that this is Bug 1470242. Can you try replacing this `if` block:

https://searchfox.org/mozilla-central/rev/924e3d96d81a40d2f0eec1db5f74fc6594337128/toolkit/content/widgets/radio.xml#27-28

With:

```
          if (control) {
            customElements.upgrade(control);
            control.radioChildConstructed(this);
          }
```

And let me know if that changes anything?

> I'm looking at
> 
>   init() {
>     this._radioChildren = null;
> 
>     if (this.getAttribute("disabled") == "true")
>       this.disabled = true;
> 
>     var children = this._getRadioChildren();
>     var length = children.length;
>     for (var i = 0; i < length; i++) {
>       dump(`${children[i].getAttribute("name")}
> ${children[i].getAttribute("value")}
> ${children[i].getAttribute("selected")}\n`);
> 
> and added a dump to it. But how can I work out whether "noAltTextRadio" is
> one of the children. They don't seem to have names, and the value is empty.
> So all that prints is true without context.

Maybe: `console.log(children[i].id, children[i])`? But given the problem you mention above where radioChildConstructed isn't defined I wouldn't think you'd be calling into this function, at least in that instance.

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: 1496704-disable-test.patch

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f34a77fd9416
temporarily disable failing test test-image-insertion-dialog.js. rs=bustage-fix

Comment 7

[Jorg K (CEST = GMT+2)]

Your code snipped above the JS errors go away.

With the debug you suggested I now see:
altTextRadio [object XULElement]
noAltTextRadio [object XULElement] true
altTextRadio [object XULElement]
noAltTextRadio [object XULElement] true
altTextRadio [object XULElement]
noAltTextRadio [object XULElement] true
actualSizeRadio [object XULElement]
customSizeRadio [object XULElement] true
actualSizeRadio [object XULElement]
customSizeRadio [object XULElement] true
actualSizeRadio [object XULElement]
customSizeRadio [object XULElement] true
=== before assert

So it looks like the panel with the radio buttons loads and the noAltTextRadio is on it and true. Hmm, I don't understand why neither .selected nor .getAttribute("selected") picks up that value.

Comment 8

[Jorg K (CEST = GMT+2)]

Actually, our test has five asserts and only the first one fails. That's fishy.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: 1496704-partly-disable-test.patch

More lightweight disabling of only one assert.

Comment 10

[Jorg K (CEST = GMT+2)]

I should mention: "TypeError: control.radioChildConstructed is not a function" is not related to the test, it happens when TB starts. Sorry.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/327ff9d24bbd
re-enable test-image-insertion-dialog.js apart from one failing assert. rs=bustage-fix

Comment 12

[Magnus Melin [:mkmelin]]

Attachment: bug1496704_test-image-insert-dialog_noAltTextRadio.patch

Add value to make the radio group selection work

Comment 13

[Magnus Melin [:mkmelin]]

Attachment: bug1496704_test-image-insert-dialog_noAltTextRadio.patch

Fix an indention

Comment 14

[Jorg K (CEST = GMT+2)]

Comment on attachment 9075120 [details] [diff] [review]
bug1496704_test-image-insert-dialog_noAltTextRadio.patch

Very nice.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/924dc8252612
add value to altTextRadioGroup radio widgets, to make the radiogroup selection work properly. r=jorgk

Comment 16

[Jorg K (CEST = GMT+2)]

TB 68 beta 4:
https://hg.mozilla.org/releases/comm-beta/rev/cc2da28f9de2c9119854d6d4527bb70bec89c232