539655 - Edit Recurrence dialog: preview not updated changing repeating interval by clicking on textbox buttons

Status: RESOLVED FIXED

(Calendar :: Dialogs, defect)

Description

[Decathlon]

The bug is reproducible with textboxes that set repeating interval of weekly and monthly rules, but not for daily one. For yearly rules I don't know how to test because on my monitor I can see a maximum number of eight months in the preview (resizing the dialog).

Reproducible: Always

Steps to Reproduce:
1. create a new event and select "Repeat" -> "Custom...";
2. select Repeat "Weekly" or "Monthly";
3. click the buttons with little arrows inside textbox which select the interval repeating rule.

Actual Results:
in recurrence dates preview, days in bold don't change.

Expected Results:
preview should change immediately after a new value changes in the textbox.


Changing the textbox with the keyboard immediately updates the preview, apart from bad behavior described in bug 521644.

Comment 1

[Decathlon]

Attachment: proposal

Seems that the problem is the same as initially reported in bug 469742 related to textbox inside reminder dialog. 
In the Edit Recurrence dialog, textboxes have "oninput" attribute that reacts only when a character is typed (with the keyboard), but doesn't react when buttons are pressed. To get them work with the buttons, should be used the "oncommand" attribute (either "onselect" or a listener or what else?).

The daily textbox works as expected because the radiogroup with id="daily-group" has the "oncommand" attribute that handles the command.

The textbox for reminder length in Set up Reminder dialog works in the same way, with radiogroup "reminder-relation-radiogroup" which has the attribute "onselect" that updates the reminder list when the user clicks the buttons in the textbox.
http://mxr.mozilla.org/comm-central/source/calendar/base/content/dialogs/calendar-event-dialog-reminder.xul#85.

So, I've thought to do the same, moving the "oncommand" attribute on the "period-deck" deck (that is at the top of the hierarchy for the controls that have to update the preview) and deleting every other "oncommand" attribute in the underlying hierarchy.
Not sure whether the issue needs a different solution.

Comment 2

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

Comment on attachment 422345 [details] [diff] [review]
proposal

The solution looks very promising and is very elegant.

Unfortunately, I'm having slight issues with it.

I can't really say in which cases, since it seems quite random sometimes. But here goes:

When changing the textbox value by keyboard (i.e entering a number then focusing away), the preview is not updated. It may be sufficient to add an oninput to the deck.

The same goes for using the arrow keys, this might be be solvable using the same method.

There is a toolkit bug on the number textboxes, so as long as this works, with exception of what is described in bug 521644, I'm happy.

r- for now to solve the number entering issues. If its totally bug 521644's fault, I'll be happy to r+. Just let me know.

Comment 3

[Decathlon]

Attachment: previous patch with a fix for bug 521644

It seems that the issue related to the keyboard is bug 521644. To verify that I've attached the same patch but with the fix for bug 521644 which you proposed in bug 469742 comment 3 (the original Calendar bug corresponding to the toolkit bug for textboxes).
I ask for review, but the goal is only testing the first patch without bug 521644.

Instead I can't reproduce the issue you mentioned about the arrow buttons.
Could you please report exactly the steps to reproduce (even if not always occurs)? I will try to reproduce it.

About bug 521644 but for textboxes in the Edit Recurrence dialog (this bug), I've tried a fix with "onkeyup" property instead of "oninput" that is a bit less hacky than the fix you proposed in bug 469742 comment 3, and seems working fine with the keyboard and with the arrow keys on the keyboard (up and down keys) as well (instead "oninput" and your fix don't work with the arrow keys).
I've also attached a patch here with that fix. Could you try it? Does it needs another bug for that specific issue?

Comment 4

[Decathlon]

Attachment: patch with a fix for bug 521644 based on "onkeyup" property

Comment 5

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

Comment on attachment 431851 [details] [diff] [review]
patch with a fix for bug 521644 based on "onkeyup" property

This solution looks good, its much better than the timeout one and I can't reproduce any issues I had with it. Standard8 will check this in for me to test the hg hooks.

Comment 6

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/016ad0250fe1