Closed Bug 770345 Opened 9 years ago Closed 9 years ago

Changing orientation with preference dialog open prevents preference from being set


(Firefox for Android Graveyard :: General, defect)

13 Branch
Not set


(firefox13 affected, firefox14 affected, firefox15 verified, firefox16 verified, blocking-fennec1.0 -, fennec15+)

Firefox 16
Tracking Status
firefox13 --- affected
firefox14 --- affected
firefox15 --- verified
firefox16 --- verified
blocking-fennec1.0 --- -
fennec 15+ ---


(Reporter: mcomella, Assigned: mcomella)




(1 file, 1 obsolete file)

1) Open Fennec.
2) Click the more options menu (three horizontal lines)
3) Scroll down and select "Settings".
4) On the preferences screen, select "Text Size".
5) Rotate the device so the orientation changes.
6) Click "Small".

Expected: The dialog disappears and "Small", the selected option, is selected in the preferences screen.
Result: The dialog disappears, but the previous option ("Medium" on the default installation) is still selected.

Note that this applies to more complex dialogs as well such as the "Clear private data" checkbox dialog (Nightly only) - I will make another bug for this as there are more issues.

Tested on Galaxy Nexus, Android 4.0.4, and Asus Transformer Prime TF201, Android 4.0.3.
Added the "See also" bug for the PrivateDataPreferences (clear browsing data) dialog.
See Also: → 770351
Added another rotation bug for the AndroidImportPreference dialog.
See Also: → 770355
Rotation bug for the master password preference dialog.
See Also: → 770358
I can reproduce on my Galaxy Nexus (Android 4.1)

This is dumb.
blocking-fennec1.0: --- → ?
tracking-fennec: --- → 15+
blocking-fennec1.0: ? → -
This (and the other rotation bugs) can be fixed by not recreating the activity on orientation change. As we do not use alternate layouts on the Preference screen, I think this is an okay solution (rather than adding the more lengthy but traditional onSaveInstance(...) code). If you disagree, please let me know.

I have (and tested) the code needed to do this, however, I need to fix up my patch queue to get the patch out - I will upload it when I am done.
Assignee: nobody → michael.l.comella
Sounds like this will fix will also address bug 717307 where the instance count is doubled on rotation.
Blocks: 717307
Attached patch Patch (obsolete) — Splinter Review
This test passed mochitest-robotium locally and was tested by hand on both a Galaxy Nexus, Android 4.0.4, and Asus Transformer Prime TF201, Android 4.0.3.

This patch also fixes several other bugs:

Bug 770351
Bug 770355 (though the code will be replaced with another patch - see the bug for details)
Bug 770358

I will look into bug 717307 to see if it fixes that as well.
Bug 717307 still occurs:

0) Run "adb logcat -v time | grep -i geckopreferences" for your device.
1) Open Fennec.
2) Click the menu bar; Select "Settings"
3) Hit the back button

Expected: No strict mode instance count violation.

"07-06 19:32:15.479 E/StrictMode(30095): class org.mozilla.gecko.GeckoPreferences; instances=2; limit=1
07-06 19:32:15.479 E/StrictMode(30095): android.os.StrictMode$InstanceCountViolation: class org.mozilla.gecko.GeckoPreferences; instances=2; limit=1"


Does not seem to be related to orientation change unfortunately.
Comment on attachment 639898 [details] [diff] [review]

Sriram had looked into the orientation issues with GeckoApp, so I'd like to get his feedback on the changes for GeckoPreferences.
Attachment #639898 - Flags: feedback?(sriram)
Sriram pointed out to me what to change but I suppose it would be good for him to specifically look at the patch as well.
Comment on attachment 639898 [details] [diff] [review]

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

I was kind of dictacting the code ;)
This looks good to me.
Please make sure " {} " are as per other parts of code in the file.
Attachment #639898 - Flags: feedback?(sriram) → feedback+
Attached patch Updated patchSplinter Review
Fixed the braces.
Attachment #639898 - Attachment is obsolete: true
Comment on attachment 639972 [details] [diff] [review]
Updated patch

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

This looks good to me.
Probably you need to see if there is anything that can be cleaned up -- like storing in bundle and taking it back -- as this removes the need for that extra step.
Please post a separate patch if you find anything that could be cleanup up.
Attachment #639972 - Flags: review?(sriram) → review+
Besides some unused imports (bug 772527), I could not find anything.
Keywords: checkin-needed
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
Duplicate of this bug: 770358
Duplicate of this bug: 770351
Aurora/Beta nom?
See Also: 770351, 770355, 770358
Comment on attachment 639972 [details] [diff] [review]
Updated patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Unknown.
User impact if declined: Users who change orientation within the preferences activity with a settings dialog open and then try to set the setting will not be able to until they close the dialog, reopen it, and try again. Oftentimes, users will not notice the setting did not change.
Testing completed (on m-c, etc.): Landed on m-c on 7/10.
Risk to taking this patch (and alternatives if risky): Low risk.
String or UUID changes made by this patch: None.
Attachment #639972 - Flags: approval-mozilla-beta?
Attachment #639972 - Flags: approval-mozilla-aurora?
Comment on attachment 639972 [details] [diff] [review]
Updated patch

low risk, still early enough in beta to take this - approving.
Attachment #639972 - Flags: approval-mozilla-beta?
Attachment #639972 - Flags: approval-mozilla-beta+
Attachment #639972 - Flags: approval-mozilla-aurora?
Attachment #639972 - Flags: approval-mozilla-aurora+
checkin-needed for Aurora and Beta.
Keywords: checkin-needed
This landed on m-c when it was still Firefox 16 (see the target milestone), hence it's already on Aurora.
Unable to reproduce the issue described in comment 0 on:

Firefox Mobile 16.0b5
Samsung Galaxy R (Android 2.3.4)

Marking as verified on Firefox Mobile 16
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.