1193337 - Checkboxes in doorhangers are not working

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[u421692]

1. Open http://popuptest.com/popuptest1.html
2. Try to check and uncheck "Don't ask again for this site"

Expected result:
You can easily change the state of the checkbox.

Actual result:
Checkboxes in doorhangers are not working
PS: The only way I was able to make this work was by changing the orientation of the device, and was only able to check or uncheck once.

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=72835344333f&tochange=15155971639c

regressed from bug 1173624 ?

Comment 1

[:Margaret Leibovic]

Martyn, can you look into this?

Comment 2

[Chenxia Liu [:liuche]]

It's still possible to change the state of doorhanger dropdown menus (such as input selection in the Firefox Hello doorhanger).

Comment 3

[Martyn Haigh (:mhaigh)]

Attachment: MozReview Request: Bug 1193337 - Checkboxes in doorhangers are not working; r?liuche

Bug 1193337 - Checkboxes in doorhangers are not working; r?liuche

I couldn't get the old method to work properly, so I have refactored it, this one should be more performant and easier to understand.

Comment 4

[Martyn Haigh (:mhaigh)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1826af76a0cf

Comment 5

[Martyn Haigh (:mhaigh)]

Wrong link in previous comment

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b73d7f62968

Comment 6

[Chenxia Liu [:liuche]]

Comment on attachment 8647475 [details]
MozReview Request: Bug 1193337 - Checkboxes in doorhangers are not working; r?liuche

https://reviewboard.mozilla.org/r/15997/#review14577

Nice!

(If you feel like running any profiling, I'd be curious what the differences between these two are. I wonder if the Path approach is actually more performant because we don't have to copy from another canvas. Although save/restore might be expensive. Android Settings > Dev options > Profile GPU rendering > bars - when the bars cross the green line, we've dropped a frame.)

::: mobile/android/base/widget/RoundedCornerLayout.java:53
(Diff revision 1)
> -        if (changed) {
> +        RectF r = new RectF(0, 0, w, h);

Nit: final

::: mobile/android/base/widget/RoundedCornerLayout.java:61
(Diff revision 1)
> -        Bitmap offscreenBitmap = Bitmap.createBitmap(canvas.getWidth(), canvas.getHeight(), Bitmap.Config.ARGB_8888);
> +        canvas.save();

You could add a check here for whether the size has changed, and skip save/restore/clip if it hasn't.

And if it gets called only when there are size changes, add a comment.

Comment 7

[Martyn Haigh (:mhaigh)]

Attachment: Old method

This is using the original method

Comment 8

[Martyn Haigh (:mhaigh)]

Attachment: new method

This is the performance using the new method as detailed in this patch.  We're still crossing the line, but we're doing so less severly and for less frames.

Comment 9

[Martyn Haigh (:mhaigh)]

url:        https://hg.mozilla.org/integration/fx-team/rev/2b21997e32784d2c3210d03f64b4a1264b084931
changeset:  2b21997e32784d2c3210d03f64b4a1264b084931
user:       Martyn Haigh <[e-mail]>
date:       Tue Aug 18 15:09:25 2015 +0100
description:
Bug 1193337 - Checkboxes in doorhangers are not working; r=liuche

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2b21997e3278

Comment 11

[u421692]

Verified as fixed on latest Nightly

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Looks like Fx42 is affected, but this was never uplifted to Fx42. There might be regressions from this change now too.

Comment 14

[Chenxia Liu [:liuche]]

Comment on attachment 8647475 [details]
MozReview Request: Bug 1193337 - Checkboxes in doorhangers are not working; r?liuche

Approval Request Comment
[Feature/regressing bug #]: Originally regressed by bug 1173624 which landed in 42, but this was mistakenly not uplifted to 42.
[User impact if declined]: Checkboxes in doorhangers will not work, so users will lose some level of control over some of their data choices
[Describe test coverage new/current, TreeHerder]: green Beta try run - https://treeherder.mozilla.org/#/jobs?repo=try&revision=54ff8cec05f9
[Risks and why]: low, has been baking for a full cycle. Bug 1201081 would still be present, but it only affects a small number of devices
[String/UUID change made/needed]: none

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Recent regression; tracking this here rather than in the duplicate bug.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8647475 [details]
MozReview Request: Bug 1193337 - Checkboxes in doorhangers are not working; r?liuche

Green try run on m-b, fix for recent regression. Approving this so it can make it into the next mobile beta build.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f31f7273e82a

Comment 18

[u421692]

Verified as fixed on Firefox 42 Beta 6