Closed
Bug 1476947
Opened 6 years ago
Closed 6 years ago
Many MozMill failures on 2018-07-19 caused by listbox removal in bug 1472555
Categories
(Calendar :: General, defect)
Calendar
General
Tracking
(Not tracked)
RESOLVED
FIXED
6.5
People
(Reporter: jorgk-bmo, Assigned: darktrojan)
References
Details
Attachments
(3 files, 5 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
darktrojan
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Fallen
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•6 years ago
|
Flags: needinfo?(geoff)
Assignee | ||
Comment 2•6 years ago
|
||
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•6 years ago
|
||
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.
Reporter | ||
Comment 4•6 years ago
|
||
(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?
Summary: Many MoziMill failures on 2018-07-19 → Many MozMill failures on 2018-07-19 caused by listbox removal in bug 1472555
Reporter | ||
Comment 5•6 years ago
|
||
Reporter | ||
Updated•6 years ago
|
Keywords: leave-open
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/8f2abbbb3f04
temporarily disable failing tests. rs=bustage-fix DONTBUILD
Assignee | ||
Comment 7•6 years ago
|
||
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.
Assignee | ||
Comment 8•6 years ago
|
||
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•6 years ago
|
||
(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.
Assignee | ||
Comment 10•6 years ago
|
||
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•6 years ago
|
||
(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
Assignee | ||
Comment 12•6 years ago
|
||
Okay, that seems to work.
Attachment #8993566 -
Attachment is obsolete: true
Attachment #8993566 -
Flags: feedback?(philipp)
Attachment #8995844 -
Flags: review?(philipp)
Assignee | ||
Comment 13•6 years ago
|
||
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 14•6 years ago
|
||
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+
Assignee | ||
Comment 15•6 years ago
|
||
With new KeyboardEvent constructor.
Attachment #8995903 -
Attachment is obsolete: true
Attachment #8996629 -
Flags: review+
Assignee | ||
Comment 16•6 years ago
|
||
When this is checked in, don't forget to re-enable the tests. :)
Keywords: leave-open → checkin-needed
Comment 17•6 years ago
|
||
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
Reporter | ||
Updated•6 years ago
|
Target Milestone: --- → 6.5
Assignee | ||
Comment 18•6 years ago
|
||
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 → ---
Assignee | ||
Comment 19•6 years ago
|
||
Attachment #8996880 -
Flags: review?(philipp)
Updated•6 years ago
|
Attachment #8996880 -
Flags: review?(philipp) → review+
Comment 20•6 years ago
|
||
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: 6 years ago → 6 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•