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

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, defect)

Description

[Benjamin Bach]

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.

Comment 1

[:aceman]

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.

Comment 2

[Benjamin Bach]

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.

Comment 3

[:aceman]

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 mozilla.org ?

Do you get anything in Tools->error console after the field is reverted to the old value?

Comment 4

[Benjamin Bach]

Running 16.0.1 from Ubuntu repositories. No errors in Error Console.

Screencast here: http://overtag.dk/screencasts/tb_account_settings.ogv

(didn't realize the screen cast program included the option of speaking to the mic, should have done that, sry.. first screen cast ever)

Comment 5

[Ludovic Hirlimann [:Usul]]

So this could be a extension - did you try with extension disabled ?

Comment 6

[:aceman]

It seems the value is properly saved on OK, but is lost on switching to Junk settings.

Comment 7

[:aceman]

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.

Comment 8

[:aceman]

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 https://developer.mozilla.org/en-US/docs/XUL/Attribute/textbox.onchange , 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'.

Comment 9

[:aceman]

Attachment: WIP patch

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.

Comment 10

[[e-mail]]

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.

Comment 11

[:aceman]

Attachment: patch

Yeah, using a suitable existing element is much better ;) Thanks for the tip.

Comment 12

[:aceman]

Good stuff for my test-account-settings-infrastructure.js test file ;)

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 676293 [details] [diff] [review]
patch

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

Thanks aceman!

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/7a16616f5729

Comment 15

[:aceman]

Attachment: WIP test

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?

Comment 17

[:aceman]

Comment on attachment 676293 [details] [diff] [review]
patch

[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.

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 676293 [details] [diff] [review]
patch

[Triage Comment]
We should take this onto aurora and beta as well so that SM can use it.

Comment 19

[Justin Wood (:Callek)]

I landed this on beta now, so that I could get it in SeaMonkey's beta I'm spinning in moments.

http://hg.mozilla.org/releases/comm-beta/rev/38584725196f?revcount=240

Comment 20

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-aurora/rev/563ed3a4f3de

Comment 21

[Mark Banner (:standard8)]

Comment on attachment 676293 [details] [diff] [review]
patch

Unfortunately this doesn't apply cleanly to the ESR branch. Aceman, can you take a look at a clean patch for there please?

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 687515 [details] [diff] [review]
WIP test

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

I like the look of this.

Comment 23

[:aceman]

(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.

Comment 24

[:aceman]

Standard8, the patch seems to have landed on comm-release, but not on comm-esr17 (as you noted), so are these branches different now?

Comment 25

[Mark Banner (:standard8)]

(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.

Comment 26

[Mike Conley (:mconley) (:⚙️)]

(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?

-Mike

[1]: http://www.w3.org/TR/DOM-Level-2-Events/events.html#Events-eventgroupings-htmlevents

Comment 27

[:aceman]

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?

Comment 28

[:aceman]

Attachment: patch for TB17

[Approval Request Comment]
Standard8, will this apply to TB17?

Comment 29

[:aceman]

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.

Comment 30

[rsx11m]

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.

Comment 31

[:aceman]

Standard8, can you sort out the flags and check in where needed?

Comment 32

[Mark Banner (:standard8)]

Yes, I'll sort them out in the next flag triage that I'll do.

Comment 33

[Mark Banner (:standard8)]

Comment on attachment 698803 [details] [diff] [review]
patch for TB17

Just comm-esr17 is enough.

Comment 34

[Mark Banner (:standard8)]

Comment on attachment 698803 [details] [diff] [review]
patch for TB17

https://hg.mozilla.org/releases/comm-esr17/rev/dcc1993f148f

Comment 35

[:aceman]

Now we could move the test. Mconley?

Comment 36

[:aceman]

Attachment: test v2

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.

Comment 37

[Mike Conley (: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)

Comment 38

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 39

[:aceman]

That is what I say in previous comments:) It does fail for me sometimes and I don't know what is the problem.

Comment 40

[:aceman]

Attachment: test v3

Thanks for the debugging. Let's try this.

Comment 41

[Mike Conley (: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.

Comment 42

[Mark Banner (:standard8)]

https://hg.mozilla.org/comm-central/rev/b2693dc700cc