Closed Bug 804091 Opened 9 years ago Closed 8 years ago

IMAP account option "Synchronize the most recent" not saving correctly


(Thunderbird :: Account Manager, defect)

16 Branch
Not set


(thunderbird18 fixed, thunderbird19 fixed, thunderbird-esr1719+ fixed)

Thunderbird 20.0
Tracking Status
thunderbird18 --- fixed
thunderbird19 --- fixed
thunderbird-esr17 19+ fixed


(Reporter: benjaoming, Assigned: aceman)




(3 files, 3 obsolete files)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:16.0) Gecko/20100101 Firefox/16.0
Build ID: 20121010231231

Steps to reproduce:

Try filling in a number of days or weeks. Then navigate to a different menu item and navigate back.

Actual results:

The newly filled-in number is gone, and the old one is back.

Expected results:

New number should always be kept when de-focusing the widget, no matter what is selected instead.
What do you mean by "menu item".
Can you please provide exact steps to reproduce this, i.e. if you put in the number via keyboard or mouse and then where do you click exactly.
Account Settings -> (Some account) -> Synchronization and storage : Synchronize the most recent

If you navigate to another menu item in Account Settings, for instance Junk Settings, after changing the value of the "spin button" widget, and then back again to Synchronization and storage, you'll see that the value is back to the old value.

The behaviour is also present if you click OK directly without de-focusing the spin button.

I have tested other fields, and so far I've only found that the issue is in this particular field.
I can't see the problem on Win XP, TB19. But I'll try linux later.
Are you using the TB version from Ubuntu repository or an official build from ?

Do you get anything in Tools->error console after the field is reverted to the old value?
Running 16.0.1 from Ubuntu repositories. No errors in Error Console.

Screencast here:

(didn't realize the screen cast program included the option of speaking to the mic, should have done that, sry.. first screen cast ever)
So this could be a extension - did you try with extension disabled ?
It seems the value is properly saved on OK, but is lost on switching to Junk settings.
OK, I can reproduce it now writing the exact numbers as in the video. It also needs to be written via keyboard, not clicking the small arrows in the field.
Also mixing into the confusion is that the value is "optimized" when visiting the pane again, e.g. try putting 35 *days* and it gets converted to 5 *weeks* (which you may not notice if you just look at the number field).

I'll see if this "optimizing" code may have a bug that reverts to the older value.
Assignee: nobody → acelists
Ever confirmed: true
So the bug is this:

We only store the value (into accountValues array) of the field in the am-offline.js::onAutosyncChange() function, that is called in 'onchange' attribute of the textbox. But the current code flow is like this:
1. enter value
2. click Junk settings
3. Offline pane values is saved (onAutosyncChange not yet called, so we save the old value)
4. load of Junk pane
5. call of onAutosyncChange(). But the pane is no longer existing. Even if it would, the new value would not be stored.
6. click Offline settings
7. Junk values are stored (irrelevant here).
8. load of Offline pane with stored values. The old value from step 3. is loaded and shown in the field.

This seems to happen because according to , the onchange event is called only when the focus is changed away from the field and that seems to happen only after step 4 (we have a new pane up). I don't think it is a Toolkit bug, we just do strange things with panes in the account manager :)

Expected flow:
Call of onAutosyncChange() must happen before step 3. That is what happens correctly if the value of the textbox is changed via the small arrows, or after the value is entered other setting widget is clicked/focused.

So I need to find out how to force a focus change before step 3 so that onAutosyncChange() is called. I'd like the solution to apply to all panes even though currently this one field is the only one using 'onchange'.
Attached patch WIP patch (obsolete) — Splinter Review
How ugly is this? :) I use opacity=0 for now, because hidden, collapsed, visibility=hidden didn't work. Gecko was probably smart enough to ignore focus for such invisible elements.
I've left some debugging in the so that the flow of function calls can be seen in the Error console.
Attachment #674358 - Flags: feedback?(iann_bugzilla)
Attachment #674358 - Flags: feedback?(neil)
Comment on attachment 674358 [details] [diff] [review]
WIP patch

The problem in the particular case of clicking on the tree is that the tree's XBL changes its selection on mousedown, but that's actually before it's got the focus, since that happens last as part of mousedown default behaviour.

So the thing to do would seem to be to explicitly focus the tree itself. It's probably easiest to do it from the select handler, otherwise you will find it hard to get it to run before the select handler does.
Attachment #674358 - Flags: feedback?(neil) → feedback-
Attached patch patchSplinter Review
Yeah, using a suitable existing element is much better ;) Thanks for the tip.
Attachment #674358 - Attachment is obsolete: true
Attachment #674358 - Flags: feedback?(iann_bugzilla)
Attachment #676293 - Flags: review?(neil)
Attachment #676293 - Flags: review?(neil) → review+
Attachment #676293 - Flags: review?(mconley)
Good stuff for my test-account-settings-infrastructure.js test file ;)
Flags: in-testsuite?
Comment on attachment 676293 [details] [diff] [review]

Good comment, and I can confirm this fixes the bug. Account manager is a funny beast.

Thanks aceman!
Attachment #676293 - Flags: review?(mconley) → review+
Keywords: checkin-needed
Whiteboard: [please leave open for the test]
Attached patch WIP test (obsolete) — Splinter Review
So this should be the test. However, it does not work. I use amc.type() to write the new value into the field. But onchange is not run after that. Maybe this is not equivalent to writing the value by the user. What should I use in a test to simulate that?
Attachment #687515 - Flags: feedback?(mconley)
Duplicate of this bug: 816450
Comment on attachment 676293 [details] [diff] [review]

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: user may not notice that his entered value was ignored.
Testing completed (on c-c, etc.): 
Risk to taking this patch (and alternatives if risky): while this is a new element in the code, it should be trivial to take out if it causes any problems.
Attachment #676293 - Flags: approval-comm-esr17?
Comment on attachment 676293 [details] [diff] [review]

[Triage Comment]
We should take this onto aurora and beta as well so that SM can use it.
Attachment #676293 - Flags: approval-comm-esr17?
Attachment #676293 - Flags: approval-comm-esr17+
Attachment #676293 - Flags: approval-comm-beta+
Attachment #676293 - Flags: approval-comm-aurora+
I landed this on beta now, so that I could get it in SeaMonkey's beta I'm spinning in moments.
Target Milestone: --- → Thunderbird 20.0
Comment on attachment 676293 [details] [diff] [review]

Unfortunately this doesn't apply cleanly to the ESR branch. Aceman, can you take a look at a clean patch for there please?
Attachment #676293 - Flags: approval-comm-esr17+ → approval-comm-esr17-
Comment on attachment 687515 [details] [diff] [review]
WIP test

Review of attachment 687515 [details] [diff] [review]:

I like the look of this.
Attachment #687515 - Flags: feedback?(mconley) → feedback+
(In reply to Mike Conley (:mconley) from comment #22)
> Review of attachment 687515 [details] [diff] [review]:
> -----------------------------------------------------------------
> I like the look of this.

But have you seen comment 15? The test does not work.
Flags: needinfo?(mconley)
Standard8, the patch seems to have landed on comm-release, but not on comm-esr17 (as you noted), so are these branches different now?
Flags: needinfo?(mbanner)
(In reply to :aceman from comment #24)
> Standard8, the patch seems to have landed on comm-release, but not on
> comm-esr17 (as you noted), so are these branches different now?

That was part of the SeaMonkey merge of beta -> release for the next SeaMonkey releases, it won't be in the next Thunderbird releases.
Flags: needinfo?(mbanner)
(In reply to :aceman from comment #23)
> (In reply to Mike Conley (:mconley) from comment #22)
> > Review of attachment 687515 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I like the look of this.
> But have you seen comment 15? The test does not work.

Hm. So, according to the spec[1], the onchange event fires after a form element is changed and then blurred. That might be something to consider.

Where is the test breaking down?


Flags: needinfo?(mconley)
I amc.type() the new value "35" into the field and click on other pane "am-junk.xul". Then the onchange should fire (as the fix is already landed). But that is not true, the field still contains the original value 30. So I think amc.type() does not work somehow. Or does the test work fine for you on Windows?
Flags: needinfo?(mconley)
Attached patch patch for TB17Splinter Review
[Approval Request Comment]
Standard8, will this apply to TB17?
Attachment #698803 - Flags: review+
Attachment #698803 - Flags: approval-comm-release?
Comment on attachment 698803 [details] [diff] [review]
patch for TB17

Not sure which flag we should use for the TB17 branch that is the release branch, not ESR.
Attachment #698803 - Flags: approval-comm-esr17?
It is my understanding that whatever lands on 17.0 ESR should also land on the 17.0 release branch. But, since bug 815302 should be fixed before 17.0.3 is due (i.e, with the 19.0 cycle), pushing it onto the separate branch in comm-release may be redundant now. Your patch should have landed on the regular comm-release already with the recent beta/release merge on the default branch (thus for SeaMonkey 2.15), but won't be included in the 17.0.x comm-release branch in this way.
Standard8, can you sort out the flags and check in where needed?
Flags: needinfo?(mbanner)
Yes, I'll sort them out in the next flag triage that I'll do.
Flags: needinfo?(mbanner)
Comment on attachment 698803 [details] [diff] [review]
patch for TB17

Just comm-esr17 is enough.
Attachment #698803 - Flags: approval-comm-release?
Attachment #698803 - Flags: approval-comm-esr17?
Attachment #698803 - Flags: approval-comm-esr17+
Now we could move the test. Mconley?
Attached patch test v2 (obsolete) — Splinter Review
Patch updated after bitrot. It does pass for me sometimes under linux. If it works fine for you under Windows then we could maybe land it.
Attachment #687515 - Attachment is obsolete: true
Attachment #716136 - Flags: review?(mconley)
Flags: needinfo?(mconley)
Comment on attachment 716136 [details] [diff] [review]
test v2

Review of attachment 716136 [details] [diff] [review]:

I'm happy with this. r=me with some commentary (see below).

::: mail/test/mozmill/account/test-account-settings-infrastructure.js
@@ +332,5 @@
> +  click_account_tree_row(amc, accountRow);
> +
> +  let iframe = amc.e("contentFrame").contentDocument;
> +
> +  let autoSync = iframe.getElementById("autosyncValue");

Please put a comment in here explaining the magic number 30 (it's the default value).

@@ +336,5 @@
> +  let autoSync = iframe.getElementById("autosyncValue");
> +  assert_equals(autoSync.value, 30);
> +
> +  let autoSyncInterval = iframe.getElementById("autosyncInterval");
> +  assert_equals(autoSyncInterval.value, 1);

Same as above - 1 represents "Days", which is the default value.

@@ +351,5 @@
> +
> +  iframe = amc.e("contentFrame").contentDocument;
> +
> +  autoSync = iframe.getElementById("autosyncValue");
> +  assert_equals(autoSync.value, 5);

Explain why this should be 5 (because we translate 35 days into 5 weeks)

@@ +354,5 @@
> +  autoSync = iframe.getElementById("autosyncValue");
> +  assert_equals(autoSync.value, 5);
> +
> +  autoSyncInterval = iframe.getElementById("autosyncInterval");
> +  assert_equals(autoSyncInterval.value, 7);

Explain why this should be 7 (7 == weeks)
Attachment #716136 - Flags: review?(mconley) → review+
Comment on attachment 716136 [details] [diff] [review]
test v2

Hm - just ran this test locally, and it failed:

SUMMARY-UNEXPECTED-FAIL | test-account-settings-infrastructure.js | test-account-settings-infrastructure.js::test_account_onchange_handler
  EXCEPTION: a != b: '30' != '5'.
    at: test-folder-display-helpers.js line 2831
       assert_true test-folder-display-helpers.js 2831
       assert_equals test-folder-display-helpers.js 2818
       subtest_check_onchange_handler test-account-settings-infrastructure.js 355
       Runner.prototype.wrapper frame.js 577
       WindowWatcher_notify test-window-helpers.js 332

Can you please send this to try? Until further notice, r- due to test failure.
Attachment #716136 - Flags: review+ → review-
That is what I say in previous comments:) It does fail for me sometimes and I don't know what is the problem.
Attached patch test v3Splinter Review
Thanks for the debugging. Let's try this.
Attachment #716136 - Attachment is obsolete: true
Attachment #716744 - Flags: review?(mconley)
Comment on attachment 716744 [details] [diff] [review]
test v3

Review of attachment 716744 [details] [diff] [review]:

Yes, this looks good to me - and they pass locally. r=me.
Attachment #716744 - Flags: review?(mconley) → review+
Keywords: checkin-needed
Whiteboard: [please leave open for the test] → [check in the test only]
Closed: 8 years ago
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [check in the test only]
Depends on: 855687
You need to log in before you can comment on or make changes to this bug.