1476947 - Many MozMill failures on 2018-07-19 caused by listbox removal in bug 1472555

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

Now that Calendar is back online it's suffering daily bustage again :-(

First seen: Thu, Jul 19, 2018, 12:41:34,
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=67048a6b1ab4e514e640d2e73ac7dde8b7e47355

Strangely only on Linux and Mac.

TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1532001708/build/tests/mozmill/cal-recurrence/testAnnualRecurrence.js | testAnnualRecurrence.js::testAnnualRecurrence
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1532001708/build/tests/mozmill/cal-recurrence/testBiweeklyRecurrence.js | testBiweeklyRecurrence.js::testBiweeklyRecurrence
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1532001708/build/tests/mozmill/cal-recurrence/testDailyRecurrence.js | testDailyRecurrence.js::testDailyRecurrence
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1532001708/build/tests/mozmill/cal-recurrence/testLastDayOfMonthRecurrence.js | testLastDayOfMonthRecurrence.js::testLastDayOfMonthRecurrence
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1532001708/build/tests/mozmill/cal-recurrence/testWeeklyNRecurrence.js | testWeeklyNRecurrence.js::testWeeklyNRecurrence
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1532001708/build/tests/mozmill/cal-recurrence/testWeeklyUntilRecurrence.js | testWeeklyUntilRecurrence.js::testWeeklyUntilRecurrence
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1532001708/build/tests/mozmill/cal-recurrence/testWeeklyWithExceptionRecurrence.js | testWeeklyWithExceptionRecurrence.js::testWeeklyWithExceptionRecurrence
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1532001698/build/tests/mozmill/testAlarmDefaultValue.js | testAlarmDefaultValue.js::testDefaultAlarms
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1532001698/build/tests/mozmill/testLocalICS.js | testLocalICS.js::testLocalICS
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1532001698/build/tests/mozmill/testTodayPane.js | testTodayPane.js::testTodayPane

M-C last good: 5a8107262015714d2907a85abc24c847ad
M-C first bad: 183ee39bf309cd8463d8db5b5c8eb232cd

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5a8107262015714d2907a85abc24c847ad&tochange=183ee39bf309cd8463d8db5b5c8eb232cd

Comment 1

[Geoff Lankow (:darktrojan)]

Oh look, a listbox! Sigh…

Comment 2

[Geoff Lankow (:darktrojan)]

Attachment: 1476947-category-list-1.diff

Here I'm replacing the listbox and the textbox above it with a proper menu. There's now a prompt instead of the textbox. Before I go ahead and add proper strings to it, can you see any problems?

Comment 3

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

 I'm having trouble picturing this just from the code. So we are replacing an inline category editor with a prompt? If possible I'd like to keep it inline to not take the user out of context. Maybe a screenshot would make me reconsider.

Comment 4

[Jorg K (CEST = GMT+2)]

(In reply to Geoff Lankow (:darktrojan) from comment #1)
> Oh look, a listbox! Sigh…
Sigh, indeed. We're only at 14 parts and counting in bug 1475817 getting rid of listboxes :-( - Quick action here would be appreciated since it's messing up the tree. Why doesn't the failure show on Windows?

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: 1476947-disable-tests.patch

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8f2abbbb3f04
temporarily disable failing tests. rs=bustage-fix DONTBUILD

Comment 7

[Geoff Lankow (:darktrojan)]

Attachment: Screenshot from 2018-07-21 15-21-47.png

This is a screenshot with my changes. I imagine the first item being labelled "Custom category…" or something like it. "Add category…" seems wrong as we don't actually add a new category to the list of categories.

Comment 8

[Geoff Lankow (:darktrojan)]

Attachment: Screenshot from 2018-07-21 15-23-47.png

For comparison, this is what the dropdown looks like on beta. It's particularly ugly here because I have dark menus but the checkboxes and textbox aren't dark.

Comment 9

[[:MakeMyDay]]

(In reply to Geoff Lankow (:darktrojan) from comment #7)
> Created attachment 8993883 [details]
> Screenshot from 2018-07-21 15-21-47.png
> 
> This is a screenshot with my changes. I imagine the first item being
> labelled "Custom category…" or something like it. "Add category…" seems
> wrong as we don't actually add a new category to the list of categories.

The issue here is not what's on the screeenshot (which looks ok) but what's not. The add category think allows you to add a separate catagory inline and not with a prompt like in your patch. This is what Philipp was referring to, I think. Can you make that an editable box again?

n.b.: the category added here is intentionally just added for this meeting, not to the persisted list in the pref. But it should be added to the list in the screenshot (and I just verified it still does in b10), so that you can add more then one actegory and e.g. uncheck one again if you mistyped.

Comment 10

[Geoff Lankow (:darktrojan)]

It didn't occur to me until after I'd posted the patch, that I might be able to just add a textbox to the menu. Which I did, but the menu steals keyboard events and they never make it to the textbox. (That might be platform-specific.) Back to square one.

Comment 11

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Geoff Lankow (:darktrojan) from comment #10)
> It didn't occur to me until after I'd posted the patch, that I might be able
> to just add a textbox to the menu. Which I did, but the menu steals keyboard
> events and they never make it to the textbox. (That might be
> platform-specific.) Back to square one.

I had this issue in quick folder move, there is a way to add a textbox that gets input events. It feels a little hackish, maybe you can make it less hackish than I did: https://github.com/kewisch/quickmove-extension/blob/master/chrome/content/quickmove.js

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/ignorekeys

Comment 12

[Geoff Lankow (:darktrojan)]

Attachment: 1476947-category-list-2.diff

Okay, that seems to work.

Comment 13

[Geoff Lankow (:darktrojan)]

Attachment: 1476947-category-list-3.diff

Now I regret trying to fix this in this way. I discovered the task tab has a button with this binding, and of course, XUL being the weird inconsistent thing that it is, the menulist behaves differently.

Comment 14

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8995903 [details] [diff] [review]
1476947-category-list-3.diff

Review of attachment 8995903 [details] [diff] [review]:
-----------------------------------------------------------------

::: calendar/base/content/calendar-task-view.js
@@ +235,5 @@
> +                event.preventDefault();
> +
> +                let code = event.key == "ArrowUp" ? KeyboardEvent.DOM_VK_UP : KeyboardEvent.DOM_VK_DOWN;
> +                let keyEvent = document.createEvent("KeyboardEvent");
> +                keyEvent.initKeyEvent("keydown", true, true, null, false, false, false, false, code, 0);

Can you use the more verbose KeyboardEvent constructor? https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/KeyboardEvent

Comment 15

[Geoff Lankow (:darktrojan)]

Attachment: 1476947-category-list-4.diff

With new KeyboardEvent constructor.

Comment 16

[Geoff Lankow (:darktrojan)]

When this is checked in, don't forget to re-enable the tests. :)

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/500711034996
Replace category listbox with menulist. r=philipp
https://hg.mozilla.org/comm-central/rev/a5b7dd2dccb8
Backed out changeset 8f2abbbb3f04 to re-enable tests. a=backout

Comment 18

[Geoff Lankow (:darktrojan)]

The checkboxes don't show up on the menus in Windows, but I know why. Will post a patch shortly.

Comment 19

[Geoff Lankow (:darktrojan)]

Attachment: 1476947-category-checkboxes-1.diff

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b69318d4e59a
Follow-up: Add 'menuitem-iconic' class to new menu items so checkmarks appear. r=philipp