Closed Bug 1476947 Opened 3 years ago Closed 3 years ago

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

Categories

(Calendar :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jorgk-bmo, Assigned: darktrojan)

References

Details

Attachments

(3 files, 5 obsolete files)

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
Flags: needinfo?(geoff)
Oh look, a listbox! Sigh…
Flags: needinfo?(geoff)
Attached patch 1476947-category-list-1.diff (obsolete) — Splinter Review
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?
Assignee: nobody → geoff
Status: NEW → ASSIGNED
Attachment #8993566 - Flags: feedback?(philipp)
 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.
(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?
See Also: → 1475817
Summary: Many MoziMill failures on 2018-07-19 → Many MozMill failures on 2018-07-19 caused by listbox removal in bug 1472555
Keywords: leave-open
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/8f2abbbb3f04
temporarily disable failing tests. rs=bustage-fix DONTBUILD
Attached image Screenshot from 2018-07-21 15-21-47.png (obsolete) —
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.
Attached image Screenshot from 2018-07-21 15-23-47.png (obsolete) —
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.
(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.
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.
Blocks: 1475817
(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
Attached patch 1476947-category-list-2.diff (obsolete) — Splinter Review
Okay, that seems to work.
Attachment #8993566 - Attachment is obsolete: true
Attachment #8993566 - Flags: feedback?(philipp)
Attachment #8995844 - Flags: review?(philipp)
Attached patch 1476947-category-list-3.diff (obsolete) — Splinter Review
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.
Attachment #8993883 - Attachment is obsolete: true
Attachment #8993884 - Attachment is obsolete: true
Attachment #8995844 - Attachment is obsolete: true
Attachment #8995844 - Flags: review?(philipp)
Attachment #8995903 - Flags: review?(philipp)
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
Attachment #8995903 - Flags: review?(philipp) → review+
With new KeyboardEvent constructor.
Attachment #8995903 - Attachment is obsolete: true
Attachment #8996629 - Flags: review+
When this is checked in, don't forget to re-enable the tests. :)
Pushed by mozilla@jorgk.com:
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
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 6.5
The checkboxes don't show up on the menus in Windows, but I know why. Will post a patch shortly.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8996880 - Flags: review?(philipp)
Attachment #8996880 - Flags: review?(philipp) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/b69318d4e59a
Follow-up: Add 'menuitem-iconic' class to new menu items so checkmarks appear. r=philipp
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.