Closed Bug 1414521 Opened 3 years ago Closed 3 years ago

Port bug 1412361 to calendar: Remove checkbox-baseline and checkbox-radio xbl bindings, fix failing test testTodayPane.js

Categories

(Calendar :: General, enhancement, P3)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mkmelin, Assigned: Paenglab)

References

Details

Attachments

(2 files, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #1412361 +++

Used here: https://dxr.mozilla.org/comm-central/source/calendar/base/content/widgets/calendar-widgets.xml#256

Something is probably broken.
And this too: Bug 1411640 - Consolidate <radio> bindings and styling across platforms.
Attached patch checkbox-baseline.patch (obsolete) — Splinter Review
This fixes the in toolkit removed binding like done at https://hg.mozilla.org/mozilla-central/rev/31b3ba0e8491#l4.12

But I get the error:

TypeError: aPeriod.listItem.getCheckbox(...).setChecked is not a function[Learn More]  agenda-listbox.js:92:5
	agendaListbox.addPeriodListItem chrome://calendar/content/agenda-listbox.js:92:5
	agendaListbox.init chrome://calendar/content/agenda-listbox.js:35:5
	agendaListbox.setupCalendar chrome://calendar/content/agenda-listbox.js:686:5
	onLoad chrome://calendar/content/today-pane.js:33:9

When I remove everything in this function, more elements are shown in the today pane, but I get this errors:

ReferenceError: reference to undefined property "listItem"[Learn More]  agenda-listbox.js:727:9
TypeError: curPeriod.listItem is undefined[Learn More] agenda-listbox.js:727:9
	agendaListbox.refreshPeriodDates chrome://calendar/content/agenda-listbox.js:727:9
	updatePeriod chrome://calendar/content/today-pane.js:395:9
	setDay chrome://calendar/content/today-pane.js:367:9
	setDaywithjsDate chrome://calendar/content/today-pane.js:337:9
	onchange chrome://messenger/content/messenger.xul:1:1
	fireEvent chrome://calendar/content/widgets/minimonth.xml:971:13
	update chrome://calendar/content/widgets/minimonth.xml:1104:17
	set_value chrome://calendar/content/widgets/minimonth.xml:407:1
	setDay chrome://calendar/content/today-pane.js:365:64
	initializeMiniday chrome://calendar/content/today-pane.js:117:9
	onLoad chrome://calendar/content/today-pane.js:34:9
Assignee: nobody → richard.marti
Attachment #8925261 - Flags: feedback?(philipp)
Attachment #8925261 - Flags: feedback?(mkmelin+mozilla)
Hmm, is this the reason why
mozmake -C calendar/test/mozmill SOLO_TEST=testTodayPane.js mozmill-one
fails? See bug 1414533 comment #2.
(In reply to Richard Marti (:Paenglab) from comment #2)
> TypeError: aPeriod.listItem.getCheckbox(...).setChecked is not a
> function[Learn More]  agenda-listbox.js:92:5

Yes setChecked was implemented in checkbox-baseline
Comment on attachment 8925261 [details] [diff] [review]
checkbox-baseline.patch

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

I haven't tested it but looks correct I think.
Attachment #8925261 - Flags: feedback?(mkmelin+mozilla) → feedback+
(In reply to Magnus Melin from comment #4)
> Yes setChecked was implemented in checkbox-baseline
Magnus is referring to this:
https://hg.mozilla.org/mozilla-central/rev/31b3ba0e8491#l1.52
Summary: Port 1412361 to calendar: Remove checkbox-baseline and checkbox-radio xbl bindings → Port 1412361 to calendar: Remove checkbox-baseline and checkbox-radio xbl bindings, fix failing test testTodayPane.js
Attached patch checkbox-baseline.patch (v2 JK) (obsolete) — Splinter Review
This passes the test. All ideas by Aceman via IRC.

Richard, does this look right?
Attachment #8925305 - Flags: review?(philipp)
Attachment #8925305 - Flags: feedback?(richard.marti)
These are really Aceman's ideas.
Attachment #8925261 - Attachment is obsolete: true
Attachment #8925305 - Attachment is obsolete: true
Attachment #8925261 - Flags: feedback?(philipp)
Attachment #8925305 - Flags: review?(philipp)
Attachment #8925305 - Flags: feedback?(richard.marti)
This fixes the test. There are visual glitches, so I guess we need to fix some CSS or so.
Keywords: leave-open
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/b35420759ed0
Port 1412361 to calendar: Use checkbox instead of checkbox-baseline (all ideas by aceman). rs=bustage-fix
Comment on attachment 8925306 [details] [diff] [review]
checkbox-baseline.patch (v3)

This is an incomplete patch which returns the functionality with a visual glitch. I hope Richard can look into that.
Attachment #8925306 - Flags: review?(philipp)
Summary: Port 1412361 to calendar: Remove checkbox-baseline and checkbox-radio xbl bindings, fix failing test testTodayPane.js → Port bug 1412361 to calendar: Remove checkbox-baseline and checkbox-radio xbl bindings, fix failing test testTodayPane.js
Whiteboard: PLR (post landing review)
You where too fast. I was on uploading the patch with CSS fix.
This fixes the visual glitch.
Attachment #8925307 - Flags: review?(jorgk)
Keywords: leave-open
Comment on attachment 8925307 [details] [diff] [review]
checkbox-baseline-css.patch

Works for me, thanks. Why wasn't the "center no-repeat" needed before?

I wasn't too fast. We had 7 bustages today:
- two I fixed in the morning
- the ">" crashes (bug 1414513, bug 1414544)
- this bug here
- all the Mozmill failures (bug 1414533) caused by the
- prefs issue (bug 1414398)
You want me to be any slower??
Attachment #8925307 - Flags: review?(philipp)
Attachment #8925307 - Flags: review?(jorgk)
Attachment #8925307 - Flags: review+
Sorry, make that 8: Win32 busted, bug 1414507. Sadly I seem to be involved in most of them in some way, so whatever I can close, I will close.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/fe6af7970776
Port 1412361 to calendar: Use checkbox instead of checkbox-baseline (visual fixes). r=jorgk
Status: NEW → RESOLVED
Closed: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 6.0
Whiteboard: PLR (post landing review) → [PLR]
Attachment #8925306 - Flags: review?(philipp) → review+
Attachment #8925307 - Flags: review?(philipp)
Attachment #8925307 - Flags: review+
Whiteboard: [PLR]
You need to log in before you can comment on or make changes to this bug.