Closed
Bug 804091
Opened 12 years ago
Closed 12 years ago
IMAP account option "Synchronize the most recent" not saving correctly
Categories
(Thunderbird :: Account Manager, defect)
Tracking
(thunderbird18 fixed, thunderbird19 fixed, thunderbird-esr1719+ fixed)
RESOLVED
FIXED
Thunderbird 20.0
People
(Reporter: benjaoming, Assigned: aceman)
References
Details
Attachments
(3 files, 3 obsolete files)
1.09 KB,
patch
|
neil
:
review+
mconley
:
review+
standard8
:
approval-comm-aurora+
standard8
:
approval-comm-beta+
standard8
:
approval-comm-esr17-
|
Details | Diff | Splinter Review |
778 bytes,
patch
|
aceman
:
review+
standard8
:
approval-comm-esr17+
|
Details | Diff | Splinter Review |
7.61 KB,
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 2•12 years ago
|
||
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 mozilla.org ? Do you get anything in Tools->error console after the field is reverted to the old value?
Reporter | ||
Comment 4•12 years ago
|
||
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)
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
Status: UNCONFIRMED → NEW
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 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'.
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 10•12 years ago
|
||
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-
![]() |
Assignee | |
Comment 11•12 years ago
|
||
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)
Updated•12 years ago
|
Attachment #676293 -
Flags: review?(neil) → review+
Attachment #676293 -
Flags: review?(mconley)
![]() |
Assignee | |
Comment 12•12 years ago
|
||
Good stuff for my test-account-settings-infrastructure.js test file ;)
Flags: in-testsuite?
Comment 13•12 years ago
|
||
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!
Attachment #676293 -
Flags: review?(mconley) → review+
Keywords: checkin-needed
Whiteboard: [please leave open for the test]
Comment 14•12 years ago
|
||
https://hg.mozilla.org/comm-central/rev/7a16616f5729
Keywords: checkin-needed
![]() |
Assignee | |
Comment 15•12 years ago
|
||
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)
![]() |
Assignee | |
Comment 17•12 years ago
|
||
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.
Attachment #676293 -
Flags: approval-comm-esr17?
Comment 18•12 years ago
|
||
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.
Attachment #676293 -
Flags: approval-comm-esr17?
Attachment #676293 -
Flags: approval-comm-esr17+
Attachment #676293 -
Flags: approval-comm-beta+
Attachment #676293 -
Flags: approval-comm-aurora+
Comment 19•12 years ago
|
||
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
status-thunderbird18:
--- → fixed
Target Milestone: --- → Thunderbird 20.0
Comment 20•12 years ago
|
||
https://hg.mozilla.org/releases/comm-aurora/rev/563ed3a4f3de
status-thunderbird19:
--- → fixed
Comment 21•12 years ago
|
||
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?
Attachment #676293 -
Flags: approval-comm-esr17+ → approval-comm-esr17-
Updated•12 years ago
|
tracking-thunderbird-esr17:
--- → 18+
Comment 22•12 years ago
|
||
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+
![]() |
Assignee | |
Comment 23•12 years ago
|
||
(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)
Updated•12 years ago
|
![]() |
Assignee | |
Comment 24•12 years ago
|
||
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)
Comment 25•12 years ago
|
||
(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)
Comment 26•12 years ago
|
||
(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
Flags: needinfo?(mconley)
![]() |
Assignee | |
Comment 27•12 years ago
|
||
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)
![]() |
Assignee | |
Comment 28•12 years ago
|
||
[Approval Request Comment] Standard8, will this apply to TB17?
Attachment #698803 -
Flags: review+
Attachment #698803 -
Flags: approval-comm-release?
![]() |
Assignee | |
Comment 29•12 years ago
|
||
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?
![]() |
||
Comment 30•12 years ago
|
||
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.
![]() |
Assignee | |
Comment 31•12 years ago
|
||
Standard8, can you sort out the flags and check in where needed?
Flags: needinfo?(mbanner)
Comment 32•12 years ago
|
||
Yes, I'll sort them out in the next flag triage that I'll do.
Flags: needinfo?(mbanner)
Comment 33•12 years ago
|
||
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+
Comment 34•12 years ago
|
||
Comment on attachment 698803 [details] [diff] [review] patch for TB17 https://hg.mozilla.org/releases/comm-esr17/rev/dcc1993f148f
Updated•12 years ago
|
status-thunderbird-esr17:
--- → fixed
![]() |
Assignee | |
Comment 36•12 years ago
|
||
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 37•12 years ago
|
||
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 38•12 years ago
|
||
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-
![]() |
Assignee | |
Comment 39•12 years ago
|
||
That is what I say in previous comments:) It does fail for me sometimes and I don't know what is the problem.
![]() |
Assignee | |
Comment 40•12 years ago
|
||
Thanks for the debugging. Let's try this.
Attachment #716136 -
Attachment is obsolete: true
Attachment #716744 -
Flags: review?(mconley)
Comment 41•12 years ago
|
||
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]
Comment 42•12 years ago
|
||
https://hg.mozilla.org/comm-central/rev/b2693dc700cc
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [check in the test only]
You need to log in
before you can comment on or make changes to this bug.
Description
•