1366517 - TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\composition\test-newmsg-compose-identity.js | test-newmsg-compose-identity.js::test_editing_identity

Status: RESOLVED FIXED

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

Description

[Jorg K (CEST = GMT+2)]

TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\composition\test-newmsg-compose-identity.js | test-newmsg-compose-identity.js::test_editing_identity

First seen Sat May 20, 2017, 19:25:00:

https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=4ae874d44245158a802e05253b0d353b1fd98164

Log says:

INFO -  SUMMARY-PASS | test-newmsg-compose-identity.js::test_compose_from_composer
INFO -  SUMMARY-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\composition\test-newmsg-compose-identity.js | test-newmsg-compose-identity.js::test_editing_identity
INFO -    EXCEPTION: The From address does not have the correct label: 'ustom <[e-mail]>' != 'custom <[e-mail]>'.
INFO -      at: test-folder-display-helpers.js line 2890
INFO -         assert_true test-folder-display-helpers.js:2890 11
INFO -         assert_equals test-folder-display-helpers.js:2877 3
INFO -         checkCompIdentity test-newmsg-compose-identity.js:90 5
INFO -         test_editing_identity test-newmsg-compose-identity.js:157 3

Totally weird, what ate the "c"??

No C-C push since previous successful run.

M-C last good: 8d60d0f825110cfb646ac31dc16dc01170
M-C first bad: 5b74bbf20e803e299790d266fc6ebf5d53

Range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8d60d0f825110cfb646ac31dc16dc01170&tochange=5b74bbf20e803e299790d266fc6ebf5d53

Comment 1

[Jorg K (CEST = GMT+2)]

This is the bit of the test that fails:

  let customName = "custom";
  let customEmail = "[e-mail]";
  let identityCustom = customName + " <" + customEmail + ">";

  compWin.click(compWin.eid("msgIdentity"));
  compWin.click_menus_in_sequence(compWin.e("msgIdentityPopup"),
                                  [ { command: "cmd_customizeFromAddress" } ]);

  compWin.type(compWin.e("msgIdentityPopup").value, identityCustom);

  checkCompIdentity(compWin, account.defaultIdentity.key, identityCustom, identityCustom);

So we type |custom <[e-mail]>| into the widget, and the "c" got missing since identityList.label returns |ustom <[e-mail]>|. Weird.

Comment 2

[Jorg K (CEST = GMT+2)]

I watched the test locally, I can see that when "typing" |custom <[e-mail]>| into the addressing widget, the leading "c" does *not* appear.

Comment 3

[:aceman]

OK, I determined something has changed in the identity menulist. You can see it by running TB and clicking the "Customize From Address". There is an error about .select() not being a function and .inputField being null on document.getElementById("msgIdentity").

Is there any recent m-c change in this area? Editable menulist.

Comment 4

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #3)
> Is there any recent m-c change in this area? Editable menulist.
Aceman, thanks for looking into it. I run a C-C build after every M-C merge, so the regression range is always only one M-C push, in this case this massive push:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8d60d0f825110cfb646ac31dc16dc01170&tochange=5b74bbf20e803e299790d266fc6ebf5d53

Sadly that particular push caused three bustages: Bug 1366507, this bug here and bug 1366556.

Sometimes it's easy to spot the regressing bug in the pushlog, sometimes it's not.

Comment 5

[Jorg K (CEST = GMT+2)]

Looking at the range closely (and ignoring all the servo/geckodriver things), this looks like bug 1366250.
Olli, could this be related?
Or perhaps bug 1343912 since it appears to be a timing issue, Ehsan?

Basically the problem is that we simulate typing into an editable menulist and the first character gets eaten and not entered.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

bug 1366250 looks possible.

Where is the test? Is it calling element.focus() and then key events synchronously?
And how is it sending the key events?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

I assume some key events in the test are dispatched using element.dispatchEvent, but that doesn't mimic what happens when user types something.
Something like http://searchfox.org/mozilla-central/rev/15edcfd962be2c8cfdd0b8a96102150703bd4ac5/testing/mochitest/tests/SimpleTest/EventUtils.js#773 would be better.
But maybe I'm wrong. Could you show where and how key events are dispatched?

Comment 8

[:aceman]

(In reply to Olli Pettay [:smaug] from comment #6)
> Where is the test? Is it calling element.focus() and then key events
> synchronously?
> And how is it sending the key events?

The failing test code is in comment 1 (https://dxr.mozilla.org/comm-central/rev/9285e79dfd098b4d5e6de6c12550e72c98387ce4/mail/test/mozmill/composition/test-newmsg-compose-identity.js#139). .type() and .click() are standard functions of the mozmill controller. click_menus_in_sequence is defined at https://dxr.mozilla.org/comm-central/rev/9285e79dfd098b4d5e6de6c12550e72c98387ce4/mail/test/mozmill/shared-modules/test-window-helpers.js#957 .

After clicking "cmd_customizeFromAddress" item, this code is run in base TB:

  var customizeMenuitem = document.getElementById("cmd_customizeFromAddress");
  customizeMenuitem.setAttribute("disabled", "true");
  customizeMenuitem.setAttribute("checked", "true");
  var identityElement = document.getElementById("msgIdentity");
  identityElement.removeAttribute("type");
  identityElement.editable = true;
  identityElement.focus();
  identityElement.value = identityElement.selectedItem.value;
  identityElement.select();     <-- error starts here
  identityElement.inputField.placeholder = getComposeBundle().getFormattedString("msgIdentityPlaceholder", [identityElement.selectedItem.value]);

As I wrote in comment 3, the last 2 calls suddenly fail after last batch of m-c pushes.
Also, the display of the meunitem after getting "checked"="true" is broken (checkbox overlaps the label).

So before we get to where the 'c' got eaten the question may be why menulist with "editable"="true" is not having these 2 functions working. The failure of the 2 last calls can be seen by manually operating the menulist element in TB. The errors are not seen in the test run. So there may be some timing problems.

Comment 9

[:aceman]

(In reply to :aceman from comment #8)
> Also, the display of the meunitem after getting "checked"="true" is broken
> (checkbox overlaps the label).

OK, the styling of the checkbox may be unrelated, I think we had some other changes where we needed to decide which menulists have icons and which not and reserve the space. This could be forgotten.

Comment 10

[:aceman]

The "select" function still exists in menulist.xml in binding "menulist-editable". Are we too fast to call it right after changing the menulist to editable so the binding wasn't yet fully applied?

Comment 11

[Richard Marti (:Paenglab)]

(In reply to :aceman from comment #9)
> (In reply to :aceman from comment #8)
> > Also, the display of the meunitem after getting "checked"="true" is broken
> > (checkbox overlaps the label).
> 
> OK, the styling of the checkbox may be unrelated, I think we had some other
> changes where we needed to decide which menulists have icons and which not
> and reserve the space. This could be forgotten.

This is unrelated. The menuitem is forced to be a "checkmenuitem" and this shows the checkbox.

Comment 12

[:aceman]

Yes, the checkbox is broken already in TB52. Time for a new bug :)

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

So the test does not dispatch any key events?

If so, does it call .focus() somewhere? Looks like so.
Does flushing layout explicitly help?
(Just accessing element.offsetLeft should do the trick)

I wonder if focus() should flush in chrome docs since those may need XBL to be flushed.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

identityElement.value = identityElement.selectedItem.value;

What is the value of identityElement.selectedItem.value; ?
How is that value set?

Comment 15

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #8)
> The failure of the 2 last calls can be seen by manually operating the
> menulist element in TB.
Indeed:
TypeError: identityElement.select is not a function  MsgComposeCommands.js:4675:3 (code as per comment #8).
Do we need the .select()?

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: 1366517-insert-timeout.patch

This fixes the test. The "TypeError: identityElement.select is not a function" is also gone. But instead I get:

TypeError: identityElement.inputField is null - MsgComposeCommands.js:4676:44

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

So the code expects that focus() flushes layout.
Does 
identityElement.offsetLeft;
before .focus() fix this?

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Also, what kind of element is identityElement? Is that something which has XBL bound to it?

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Attachment: chrome_focus.diff

Possible temporary solution to ensure chrome docs, which may use XBL, get the layout flush.

Does this help?

Comment 20

[Jorg K (CEST = GMT+2)]

Comment on attachment 8869818 [details] [diff] [review]
chrome_focus.diff

Thank you. This fixes the test and all errors seen in manual operation. Can we have this quickly ;-)

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8869818 [details] [diff] [review]
chrome_focus.diff

I think we need to take this for now. Dealing with XBL is tricky.

Comment 22

[:aceman]

(In reply to Olli Pettay [:smaug] from comment #18)
> Also, what kind of element is identityElement? Is that something which has
> XBL bound to it?

menulist, it has bindings in mozilla/toolkit/content/widgets/menulist.xml.

(In reply to Olli Pettay [:smaug] from comment #13)
> So the test does not dispatch any key events?
> 
> If so, does it call .focus() somewhere? Looks like so.
> Does flushing layout explicitly help?

The key events may be kinda artificial. Please follow from here:
https://dxr.mozilla.org/comm-central/rev/9285e79dfd098b4d5e6de6c12550e72c98387ce4/mail/test/resources/mozmill/mozmill/extension/resource/modules/controller.js#413

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Where are the events dispatched?

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Aha, perhaps it is https://dxr.mozilla.org/comm-central/source/mail/test/resources/mozmill/mozmill/extension/resource/modules/events.js#116 ?

Comment 25

[:aceman]

I think so.

Comment 26

[Jorg K (CEST = GMT+2)]

Olli, can you please get this landed, we're waiting for it.
There is no commit message, otherwise I'd request checkin.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

About to land.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/87380800d291
because of XBL usage, limit not-flushing-always-during-focus() to non-chrome, r=ehsan

Comment 29

[Jorg K (CEST = GMT+2)]

Thanks.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/87380800d291