[de-xbl] minimonth (minimonth-header and minimonth)
Categories
(Calendar :: General, enhancement)
Tracking
(Not tracked)
People
(Reporter: mkmelin, Assigned: pmorris)
References
Details
Attachments
(3 files, 17 obsolete files)
|
1.46 KB,
patch
|
Details | Diff | Splinter Review | |
|
112.41 KB,
patch
|
Details | Diff | Splinter Review | |
|
4.00 KB,
patch
|
darktrojan
:
review+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #1484976 +++
De-xbl minimonth: https://searchfox.org/comm-central/source/calendar/base/content/widgets/minimonth.xml
|
Assignee | |
Comment 1•6 years ago
|
||
The minimonth shows up in a lot of places, both on its own and as part of <datepicker> and <datetimepicker>. I manually tested them all:
-
messenger overlay sidebar xul
- id=calMinimonth (minimonth)
-
today pane xul
- id=today-Minimonth (minimonth)
- id=todayMinimonth (minimonth)
-
lightning item iframe xul
- completed (datepicker)
- repeat until (datepicker)
- event start (datetimepicker)
- event end (datetimepicker)
- task start (datetimepicker)
- task due (datetimepicker)
-
calendar event dialog recurrence xul
- x3 readonly previews (minimonth)
- repeat until (datepicker)
-
calendar event dialog reminder xul
- absolute date (datetimepicker)
-
calendar event dialog attendees xul
- start (datetimepicker)
- end (datetimepicker)
-
calendar event dialog timezone xul
- ? (datetimepicker)
-
calendar print dialog xul
- start date (datepicker)
- end date (datepicker)
Some questions I have:
- Why is there a datetimepicker in the time zones dialog? (Create a new event or task, select show timezones in the options menu, click the link and select 'more tixezones' to open the dialog.) The datepicker doesn't appear, it is disabled, and I didn't find a way to make it appear.
- Why is there a second minimonth in the todaypane? I only found one in the UI (in the drop down menu on the right in the heading).
Some methods and a comment for calIObserver exist, but it was not in the XBL <implementation implements="..."> so I have left it out of MozXULElement.implementCustomInterface.
The patch fixes an issue with attribute inheritance in the datepickers code, by adding this.initializeInheritedAttributes(). (For example, in the new/edit task dialog the datetimepickers should be disabled when the checkboxes aren't checked. This is now fixed.)
I found a bug in the custom reminders dialog. The datepicker doesn't show up at all (both before and after this patch). I wasn't able to figure this out, so I'd like to defer fixing it to a follow-up bug, unless there's an obvious fix I'm missing.
|
||
Comment 2•6 years ago
|
||
(In reply to Paul Morris [:pmorris] from comment #1)
- Why is there a second minimonth in the todaypane? I only found one in the UI (in the drop down menu on the right in the heading).
Set in menu View > Today Pane > Show Mini-Month, then the minimonth is directly shown.
|
Reporter | |
Comment 3•6 years ago
|
||
|
|
Assignee | |
Comment 4•6 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #3)
In the commit message, make it ".... bindings to custom element".
Done.
Once you addressed the comments, please have some calendar/ person do the
review.
Will do.
could you ad JsDoc documentation for the widget while you're here. For the
others too
Done.
either this or check for this.hasChildNodes()
Added a check for child nodes. Adding that check puts the constructor vs connectedCallback question in a new light. That check means we ignore all calls to connectedCallback after the first one (by checking to see if child nodes exist), then there's no real difference between connectedCallback and the constructor, other than when they are called. And it probably makes sense to delay things until connection time, to avoid doing work that might not be needed. (So I've moved the event listeners to connectedCallback.)
Of course there may be cases where you want multiple calls to connectedCallback when/if the CE is moved around in the DOM. Then that changes the equation.
remove the extra blank line
Done.
@@ +97,5 @@
// in an error: "NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument"// So we have to use these proxy objects instead.this.calICompositeObserver = this.getCustomInterfaceCallback(Ci.calICompositeObserver);this.calIOperationListener = this.getCustomInterfaceCallback(Ci.calIOperationListener);this.nsIObserver = this.getCustomInterfaceCallback(Ci.nsIObserver);Interesting.
What we've usually done is make a helper object and moved the implementation
into that. Like
https://searchfox.org/comm-central/source/chat/content/conversation-browser.js#212
So I tried moving these to helper objects, but it soon seemed like I was making the code more convoluted and harder to understand, without much benefit. In this case we'd need three helper objects, so now you have 4 different "this"s to keep straight instead of just one. Each helper object needs a reference back to its parent, so they can call its methods, like: this.kMinimonth.methodName(). And when they need to call methods in one of the other helper objects you have this.kMinimonth.calICompositeObserver.methodName(). So the extra hierarchy adds complexity. I'm not sure what the benefits are other than making it easier to see which methods satisfy which interfaces (one could do the same with comments).
So I think it's better to just leave the methods at "top level" in the CE class to keep things simpler. Then use "getCustomInterfaceCallback" to set up a proxy object for adding/removing observers/listeners/etc. That is what it's for, as far as I can tell. (See https://searchfox.org/mozilla-central/source/toolkit/content/customElements.js#368 )
That's what I've done in the patch I'm about to upload, but happy to discuss further if I'm missing something.
@@ +109,5 @@
}});this.addEventListener("keypress", (event) => {if (event.originalTarget.classList.contains("minimonth-day")) {potentially just bail early instead to save some indention
I prefer to avoid early/multiple returns when possible since code is generally easier to follow without them. Not sure the indentation savings are worth adding an early return here. So I've left it as-is.
I'd remove the comment about eslint, and move the { to after :
I find it useful to have all case bodies in their own bodies actually.
Good call, and done.
since you're touching this. seems like a pointless tmp declaration
Done.
@@ +1013,5 @@
Ci.calICompositeObserver,Ci.calIOperationListener,Ci.nsIObserver- ]);
- customElements.define("minimonth", MozMinimonth);
we should rename this, or it might come back to bite us
Custom Elements must have a dash in their name (and there is some bug - well
- that you can't extend the ones without a dash.
"calendar-minimonth" perhaps, and MozCalendarMinimonth
(The sub elements too, accordingly)
Good call. I've done the renaming.
New patch on the way.
|
|
Assignee | |
Comment 5•6 years ago
|
||
This patch addresses Magnus' comments. Ready for review.
BTW, Richard, thanks for the tip on the second minimonth in the today pane.
|
||
Updated•6 years ago
|
|
|
Assignee | |
Comment 6•6 years ago
|
||
Updated the patch to work with current build. And now this bug has landed that fixes the custom reminders dialog:
https://bugzilla.mozilla.org/show_bug.cgi?id=1536517
|
||
Comment 7•6 years ago
|
||
|
|
Assignee | |
Comment 8•6 years ago
|
||
This patch addresses Philipp's review, fixing the alignment and converting the hbox to a div, which involved adding a little CSS to keep the centered text.
|
|
Assignee | |
Comment 9•6 years ago
|
||
This shows the difference between patch 2 and 3 -- the changes I made to address Philipp's review.
|
|
Assignee | |
Comment 10•6 years ago
|
||
And rebased on the most recent TB build.
|
|
Reporter | |
Comment 11•6 years ago
|
||
I think don't want to be using any id for the element internals. That stuff is visible to the outside, so you could have id collisions and get strange results.
|
|
Assignee | |
Comment 12•6 years ago
|
||
This patch lets mozmill tests work with elements that have more than one class, when you're selecting by one of those classes.
|
|
Assignee | |
Comment 13•6 years ago
|
||
This patch removes the ids and uses classes instead. It also fixes the mozmill tests that were using anonids with the minimonth.
|
|
Assignee | |
Comment 14•6 years ago
|
||
This is just the changes made in patch 5 on top of patch 4, to make them easier to review.
|
|
Assignee | |
Comment 15•6 years ago
|
||
And #6 updates it so it will apply to the current build. (There was overlap with https://bugzilla.mozilla.org/show_bug.cgi?id=1538588 )
|
|
Assignee | |
Comment 16•6 years ago
|
||
Here is the try server run. The two failures, Z1 and Z4, on the two mac builds look relevant, but they passed on Windows and Linux. The different failure, Z4, on Linux x64 debug looks possibly relevant, but it passed on Linux x64 opt. How to proceed?
|
|
Reporter | |
Comment 17•6 years ago
|
||
It's usually best to check which tests failed on try, then go through them locally one by one and see if they fail there too.
In https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=236461259&repo=try-comm-central&lineNumber=20721 you have the below. The test is matchin on anonid's that no longer exist, so there you have to adjust the test. (Didn't look at the others, but might be similar problems)
From your obj-dir:
make mozmill-one -C comm/calendar/test/mozmill SOLO_TEST=views/testMultiweekView.js
SUMMARY-UNEXPECTED-FAIL | testMultiweekView.js | testMultiweekView.js::testMultiWeekView
21:03:28 INFO - EXCEPTION: Timeout exceeded for waitForElementNotPresent Lookup: /id("messengerWindow")/id("tabmail-container")/id("tabmail")/id("tabmail-tabbox")/id("tabpanelcontainer")/id("calendarTabPanel")/id("calendarContent")/id("calendarDisplayDeck")/id("calendar-view-box")/id("view-deck")/id("multiweek-view")/anon({"anonid":"mainbox"})/anon({"anonid":"monthgrid"})/anon({"anonid":"monthgridrows"})/[0]/[4]/{"tooltip":"itemTooltip","calendar":"mozmill"}
21:03:28 INFO - at: utils.jsm line 352
21:03:28 INFO - TimeoutError utils.jsm:352 13
21:03:28 INFO - waitFor utils.jsm:407 11
21:03:28 INFO - MozMillController.prototype.waitFor controller.jsm:653 9
21:03:28 INFO - MozMillController.prototype.waitForElementNotPresent controller.jsm:667 8
21:03:28 INFO - testMultiWeekView testMultiweekView.js:116 16
21:03:28 INFO - Runner.prototype.wrapper frame.jsm:556 7
21:03:28 INFO - Runner.prototype._runTestModule frame.jsm:626 14
21:03:28 INFO - Runner.prototype.runTestModule frame.jsm:665 8
21:03:28 INFO - Runner.prototype.runTestDirectory frame.jsm:516 12
21:03:28 INFO - runTestDirectory frame.jsm:671 10
21:03:28 INFO - Bridge.prototype._execFunction server.js:190 15
21:03:28 INFO - Bridge.prototype.execFunction server.js:195 21
21:03:28 INFO - Session.prototype.receive server.js:313 6
21:03:28 INFO - AsyncRead.prototype.onDataAvailable server.js:78 16
21:03:28 INFO - SUMMARY-PASS | testTaskView.js::setupModule
21:03:28 INFO - SUMMARY-PASS | testTaskView.js::teardownTest
21:03:28 INFO - SUMMARY-PASS | testTaskView.js::testTaskView
21:03:28 INFO - SUMMARY-PASS | testWeekView.js::setupModule
21:03:28 INFO - SUMMARY-PASS | testWeekView.js::teardownTest
21:03:28 INFO - SUMMARY-PASS | testWeekView.js::testWeekView
|
|
Reporter | |
Comment 18•6 years ago
|
||
|
|
Reporter | |
Comment 19•6 years ago
|
||
|
|
Assignee | |
Comment 20•6 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #17)
It's usually best to check which tests failed on try, then go through them locally one by one and see if they fail there too.
In https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=236461259&repo=try-comm-central&lineNumber=20721 you have the below. The test is matchin on anonid's that no longer exist, so there you have to adjust the test. (Didn't look at the others, but might be similar problems)
Okay, the tests were passing locally (and in the try run they passed on Linux x64 opt, which is what I have locally). I'm pretty sure I had changed all the minimonth anonid cases in the tests.
I checked again and they still passed locally. I checked 'cal-recurrence' and 'recurrenceRotated' which were the ones that failed on mac, and the one you quoted (views/testMultiweekView.js, which is actually not related to minimonth AFAICT).
Not sure what to do if they are passing locally.
(In reply to Magnus Melin [:mkmelin] from comment #18)
Comment on attachment 9053992 [details] [diff] [review]
mozmill-multiple-classes-0.patchReview of attachment 9053992 [details] [diff] [review]:
Looks good. It needs a bug number though (or it can be a ridealong in this
bug, but you do need to mention a bug number
Okay, I'll make it a ridealong with this bug.
(In reply to Magnus Melin [:mkmelin] from comment #19)
Comment on attachment 9054009 [details] [diff] [review]
minimonth-de-xbl-6.patchReview of attachment 9054009 [details] [diff] [review]:
Looks good to me, please have Fallen/Makemyday review it (once you have the
test failures figured out)
Okay.
::: calendar/base/content/dialogs/calendar-print-dialog.xul
@@ +30,5 @@<script type="application/javascript" src="chrome://calendar/content/calendar-print-dialog.js"/>
<script type="application/javascript" src="chrome://calendar/content/calendar-ui-utils.js"/>
<script type="application/javascript" src="chrome://global/content/printUtils.js"/>
- <script type="application/javascript" src="chrome://messenger/content/customElements.js"/>
is this needed?
Ah, apparently not, good call! I've removed it.
::: calendar/base/content/widgets/calendar-minimonth.js
@@ +24,5 @@
connectedCallback() {if (this.delayConnectedCallback() || this.hasChildNodes()) {return;}// The "name" attributes are used in mozmill tests.please drop this comment
Not sure what it's referring to either
Done. (I had tried using "name" attributes before I fixed multiple classes, then forgot to delete the comment.)
@@ +75,2 @@
this.kMinimonth = this.closest("calendar-minimonth");k is usuall for constants. I see this is prior art, but still odd
Not sure this is a sane setup, with the child reaching into the ancestry to
initialize itself.. but perhaps for another bug.
Hm, but kMinimonth is a constant -- it's never re-assigned, right? I've kept the name, but decided it wasn't worth having a separate custom element for the minimonth header. So I've merged it into the minimonth to simplify things (while we're here).
@@ +204,5 @@
// save references for convenienceif (this.hasAttribute("readonly")) {this.mIsReadOnly = this.getAttribute("readonly") == "true";}with only one user, I don't think there's too much convenience
Okay, removed this prior art.
@@ +214,4 @@
// Add pref observerconst branch = Services.prefs.getBranch("");branch.addObserver("calendar.", this.nsIObserver);please just inline the branch
Done.
@@ +319,5 @@
get extra() {return this.mExtraDate;}/**nit: please add empty line
Done.
@@ +320,5 @@
get extra() {return this.mExtraDate;}/*** returns the first (inclusive) date of the minimonth as a calIDateTime objectFor comments, please capitalize and add period.
Done.
@@ +438,4 @@
/*** End calIOperationListener methods* Start calIObserver methodsfor stuff like these, it's more common to put something like
// calIObserver methods
I changed these, but kept the "end x methods" comments, as I find that explicitness to be helpful.
@@ +704,5 @@
if (aDate.getMonth() == date.getMonth() && aDate.getFullYear() == date.getFullYear()) {
day.setAttribute("aria-label",date.toLocaleDateString(undefined, { day: "numeric" })I think the previous formatting as perferable
Okay, done.
@@ +1002,4 @@
// Remove pref observerlet branch = Services.prefs.getBranch("");branch.removeObserver("calendar.", this.nsIObserver);inline this call too
Done.
Patches and a new try run coming up.
|
|
Assignee | |
Comment 21•6 years ago
|
||
|
|
Assignee | |
Comment 22•6 years ago
|
||
|
|
Assignee | |
Comment 23•6 years ago
|
||
|
|
||
Updated•6 years ago
|
|
|
Assignee | |
Comment 24•6 years ago
|
||
Is this ready for checkin? Reviews are good, and the try server run looks good except for Z1 and Z4 on mac. Do those block checkin? If so, I'm not sure how to make progress since I'm on linux and can't reproduce those test failures locally.
More generally, does an r+ review only include code review or also include 'review' of try server results?
|
|
Reporter | |
Comment 25•6 years ago
|
||
The OSX Z4 failing recurrence tests are not at least present on trunk. Maybe there were at that time.
Perhaps rebase and send off a new try run for them.
When you think the try run looks good, just add checkin-needed to the bug keywords.
Strictly speaking, if you need to do changes needed to get the tests to pass you need a new review. But we're not super strict, and it all depends how much you needed to change.
|
|
Assignee | |
Comment 26•6 years ago
|
||
Hmm, I rebased and did a new try run and it has the same cal-recurrence and recurrenceRotated test failures on mac:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=3cbd51497d917c5aedc29bbaf9f48127c6c66e93&selectedJob=237702193
They're all similar, a timeout while waiting for modal dialog to open. I'll see if I can figure it out.
19:47:57 INFO - SUMMARY-UNEXPECTED-FAIL | testBiweeklyRecurrence.js | testBiweeklyRecurrence.js::testBiweeklyRecurrence
19:47:57 INFO - EXCEPTION: Timeout waiting for modal dialog to open.
19:47:57 INFO - at: utils.jsm line 352
19:47:57 INFO - TimeoutError utils.jsm:352 13
19:47:57 INFO - waitFor utils.jsm:407 11
19:47:57 INFO - WindowWatcher_waitForModalDialog test-window-helpers.js:379 11
19:47:57 INFO - wait_for_modal_dialog test-window-helpers.js:612 17
19:47:57 INFO - handleOccurrencePrompt test-calendar-utils.js:200 5
19:47:57 INFO - testBiweeklyRecurrence testBiweeklyRecurrence.js:101 5
|
|
Reporter | |
Comment 27•6 years ago
|
||
It's probably hard to figure out without testing on a Mac.
Khushil, can you test if these fail locally on Mac too? If they do, you can try adding controller.sleep(2000) in some places to see if there's some timing related issue or not
|
|
Reporter | |
Updated•6 years ago
|
|
|
Assignee | |
Comment 28•6 years ago
|
||
Khushil found that the test failures also occurred locally on mac, and that they were due to the change to mozmill DOM selector code to let it match with one of several classes (mozmill-multiple-classes-1.patch). If he specified all of the classes on an element in the tests, then they passed on mac.
Any ideas on why this would fail on mac but not on other platforms? Seems bizarre.
I think the way forward here is to just specify all classes in the tests, leave the mozmill code unchanged, and get the minimonth conversion landed. Then we can move the mozmill improvement to its own bug. I'll prepare a patch and do a try run.
|
|
Assignee | |
Comment 29•6 years ago
|
||
This version just changes the class selectors in a couple of mozmill tests to include all of the classes for the given element (rather than just one). All calendar tests pass locally.
Try server run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b20cd41bc445575b2bd1cfbaa2d2c8c60180945a
With this patch we no longer need the mozmill-multiple-classes-1.patch (which doesn't work on mac for some reason).
|
|
Assignee | |
Comment 30•6 years ago
|
||
In rebasing, then re-applying, etc. the previous version of the patch, I managed to not include the calendar-minimonth.js file in the patch. (Tests still passed locally because the file was still there, just not tracked by hg.)
This is the corrected patch and here's a try server run for it:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=90d678c6232b4b90376ae6226d034385714a2336
|
|
Assignee | |
Comment 31•6 years ago
|
||
Latest try run has mac-only test failures. Khushil, would you please investigate those?
Also a couple of ESLint errors. (Odd, I didn't have any ESLint messages for those two files in my editor.) It doesn't like the "-" in "<calendar-minimonth". I tried making it "<html:calendar-minimonth" but that did not fix the error. If we can't fix ESLint to not flag this as an error, we could redo it to be |is="calendar-minimonth"| instead.
|
|
Reporter | |
Comment 32•6 years ago
|
||
The xul linting is very new (last week?).
We shouldn't have to update change the name, as it's perfectly fine.
Can it be that it's the id with a dash that is the problem? Especially as there is now both id="todayMinimonth" and id="today-Minimonth" in that file.
You can add the needed files to ignore (like in bug 1542477) if you can't figure it out.
|
|
Assignee | |
Comment 33•6 years ago
|
||
I just checked the eslint issue locally. If I remove the "-" from the "<calendar-minimonth" it passes. If I remove it from the id="today-Minimonth" it still fails. So the linting is complaining about the element name when it shouldn't. Thanks for the tip on ignoring the file. Shall I open a bug about fixing the linting?
|
|
Reporter | |
Comment 34•6 years ago
|
||
Please do. I think you can also change one of the ids so they are not so confusingly similar (even if that doesn't fix the issue)
|
|
Assignee | |
Comment 35•6 years ago
|
||
I found an existing bug for this XUL "-" in custom elements eslint problem: https://bugzilla.mozilla.org/show_bug.cgi?id=1542548
|
|
Assignee | |
Comment 36•6 years ago
|
||
Now omitting the XUL files from eslint-ing and with ids for the calendar-minimonth elements in today-pane.xul that are not almost the same.
|
|
Assignee | |
Comment 37•6 years ago
|
||
Try server run looks good for linux and windows, but mac still has test failures even with the class selectors containing all classes:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c7bdec3d5e1d1d8db2f95169831d56e2b5e3edc6
|
||
Comment 38•5 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/92b567487475
Let mozmill tests work with multiple classes. r=mkmelin
|
||
Comment 39•5 years ago
•
|
||
Looks like the patch had the wrong bug number, sigh :-( - Should have been bug 1544914, see bug 1544914 comment #22.
|
|
||
Updated•5 years ago
|
|
|
Reporter | |
Comment 40•5 years ago
|
||
Khushil, I think you said you found what's not working, but not why it's not working. Can you link to that?
|
||
Comment 41•5 years ago
|
||
This is not triggering: https://searchfox.org/comm-central/source/calendar/test/mozmill/shared-modules/test-calendar-utils.js#195
It gets called from here: https://searchfox.org/comm-central/source/calendar/test/mozmill/cal-recurrence/testBiweeklyRecurrence.js#101
|
|
Reporter | |
Comment 42•5 years ago
|
||
Philipp, any idea why that woudln't work on mac?
|
|
Reporter | |
Comment 43•5 years ago
|
||
Alex, maybe you could check this on the mac?
|
||
Comment 44•5 years ago
|
||
Hey Paul, I'd like to load this patch and try to figure out the failing mozmill tests on macos.
Could you please rebase this from trunk?
|
|
Assignee | |
Comment 45•5 years ago
|
||
Here's a rebased version. Thanks for taking a look, Alex!
|
|
||
Comment 46•5 years ago
|
||
Here's the patch updated with the fix for those mozmill tests failing on macos.
I'm asking for a review from Phillip and a thorough feedback from Paul as I had to change some selectors in the tests.
Apparently, triggering a delete keystroke on a saved calendar event by specifying its parent node, works on Linux and Windows but not on macos.
Please, review it and be sure these changes don't cause issues anywhere else.
Here's the try push: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8e667949d613109e75fb5af8603a7ddde740ff0f
|
|
||
Comment 47•5 years ago
|
||
|
|
||
Comment 48•5 years ago
|
||
Sweet, thanks for the r+
Hey Paul, I'm giving this back to you as the mozmill tests for macos have been fixed.
Heads up, there might be a couple of small linting issues in the 2 tests I fixed.
|
|
Assignee | |
Comment 49•5 years ago
•
|
||
Thanks aleca and Fallen. Aleca's changes look good to me. Glad the problem is solved, and good see this one unblocked. Putting the finishing touches on it are next on my list.
|
|
Assignee | |
Comment 50•5 years ago
|
||
(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #47)
Comment on attachment 9074660 [details] [diff] [review]
For the lookupMethod() things, maybe check if you can use
super.setAttribute(aAttr, aVal); instead. Could cause an infinite loop,
beware.
Good call. This worked and calendar mozmill tests passed locally. Here's a new revision of the patch. Ready to land, assuming try server run is good:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9fd2932bd4a5b51a2ac1b34fdbb5a0e98dd3b307
|
|
Assignee | |
Updated•5 years ago
|
|
|
||
Updated•5 years ago
|
|
|
||
Comment 51•5 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/099f40c736f0
[de-xbl] Convert minimonth bindings to custom elements. r=philipp
|
|
||
Updated•5 years ago
|
|
|
Assignee | |
Comment 52•5 years ago
•
|
||
Mysteriously, the testWeeklyNOccurrences tests didn't fail during the try run but did fail after landing. This patch fixes them.
The changes are the same as those for the other tests that were failing on macOS. So I'd say we can safely forego review of this patch since the previous changes that were similar were reviewed (r=philipp).
Here's the try run that shows that this patch fixed those tests (assuming we're not getting more false successes):
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0841805de49499000dea91ceecad404921c6e5dc
|
||
Comment 53•5 years ago
|
||
FYI testDayView.js and testWeekView.js are still broken. It seems that there's been a bad interaction with something that landed since the revision your Try run was based on.
|
|
||
Comment 54•5 years ago
|
||
|
|
||
Comment 55•5 years ago
|
||
|
|
Assignee | |
Comment 56•5 years ago
|
||
(In reply to Geoff Lankow (:darktrojan) from comment #53)
FYI
testDayView.jsandtestWeekView.jsare still broken. It seems that there's been a bad interaction with something that landed since the revision your Try run was based on.
Thanks for catching this. The same fix worked for those two tests. Here's a new patch with a successful try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=1016ef58bffffeeab933df7246ed363d746b735e
|
|
||
Comment 57•5 years ago
|
||
|
|
||
Updated•5 years ago
|
|
|
||
Comment 58•5 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/b532b2516cbe
Fix some more MozMill tests on macOS. rs=bustage-fix
|
|
||
Updated•5 years ago
|
Description
•