1399723 - Checkbox has 3 states – empty / full (white background) / with check-mark.

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

[Karl Dubost💡 :karlcow]

Probably since the release of Bug 1352238 (Native theme for form controls on Firefox Android), a new issue has emerged on Firefox Android.


(needs window regression)

Steps to Reproduce:

Navigate to: http://m.focus.de/.
Tap the “User Account” icon.
Tap the "Jetzt registrieren" (“Register now”) button.
Scroll down the page to the options under the registration form.
Tap one of the option a few times and observe behavior.
Expected Behavior:
Check-box has 2 states – empty / with check-mark.

Actual Behavior:
Check-box has 3 states – empty / full (white background) / with check-mark.

Comment 1

[Louis Chang [:louis]]

I will investigate the bug. Leave the ni to remind myself.

Comment 2

[Julien Cristau [:jcristau]]

webcompat regression in 57, tracking.

Comment 3

[Mats Palmgren (inactive)]

Hmm, this is a surprising regression.  I don't see how adding a theme
could have changed the control states like this.

I'd be interested in knowing what the values of 'eventState.mStates' are here:
http://searchfox.org/mozilla-central/rev/2c9a5993ac40ec1db8450e3e0a85702fa291b9e2/widget/android/nsNativeThemeAndroid.cpp#171

Comment 4

[Louis Chang [:louis]]

I think this is the same issue with bug 1399776 that we do not paint the default background-color: white for checkbox. I have tested with the patch on bug 1399776 and it looks normal.
But still looking why the area of dotted line is strange.

(In reply to Mats Palmgren (:mats) from comment #3)
> Hmm, this is a surprising regression.  I don't see how adding a theme
> could have changed the control states like this.
> 
> I'd be interested in knowing what the values of 'eventState.mStates' are
> here:
> http://searchfox.org/mozilla-central/rev/
> 2c9a5993ac40ec1db8450e3e0a85702fa291b9e2/widget/android/nsNativeThemeAndroid.
> cpp#171

Here I list out all the events that I think are relevant to the issue in three cases:
1) If we click on an empty checkbox, we will first receive NS_EVENT_STATE_ACTIVE then receive NS_EVENT_STATE_FOCUS, NS_EVENT_STATE_HOVER and NS_EVENT_STATE_CHECKED.
2) If we unfocus the checkbox, we will receive nothing.
3) If we click on the checked checkbox, we will receive NS_EVENT_STATE_ACTIVE then receive NS_EVENT_STATE_FOCUS and NS_EVENT_STATE_HOVER.

Comment 5

[Mats Palmgren (inactive)]

(In reply to Louis Chang [:louis] from comment #4)
> I think this is the same issue with bug 1399776 that we do not paint the
> default background-color: white for checkbox. I have tested with the patch
> on bug 1399776 and it looks normal.

OK, sounds good.

Comment 6

[Louis Chang [:louis]]

Except the strange area of dotted line, I think there is still something wrong with state handling. Since currently we paint grey hover color when state is NS_EVENT_STATE_HOVER. So when we try to unfocus a checked checkbox, the background color will change from grey to white. And if we click again it will become a grey but unchecked checkbox. Maybe we should paint the grey background color only when the state is NS_EVENT_STATE_CHECKED?

Besides, I'm curious what's the purpose of hover event on mobile phone? Since on mobile phone we have nothing to hover (unless using emulator...). Maybe we don't need to handle hover event?

Comment 7

[Mats Palmgren (inactive)]

Yeah, I think we can just ignore NS_EVENT_STATE_HOVER.

Or we could try changing it to NS_EVENT_STATE_ACTIVE instead?
http://searchfox.org/mozilla-central/rev/1c13d5cf85f904afb8976c02a80daa252b893fca/widget/android/nsNativeThemeAndroid.cpp#48,135
I think that would give feedback that the control is pressed.
I see that's how we paint it on OSX for example:
0. default state: white background (no checkmark)
1. press mouse button -> grey background (no checkmark)
2. release mouse button -> blue background + checkmark
3. press mouse button again -> darker blue background + checkmark
4. release mouse button -> default state

Whatever you think looks best is fine with me.

Comment 8

[Mats Palmgren (inactive)]

(In reply to Louis Chang [:louis] from comment #4)
> 2) If we unfocus the checkbox, we will receive nothing.

Do you mean that after clicking on a checkbox (to make it focused),
then clicking somewhere on the page background does not unfocus it?
That sounds a bit odd.

(In reply to Louis Chang [:louis] from comment #6)
> Except the strange area of dotted line, 

Are you referring to the dotted outline from our :-moz-focusring rule?
What's strange about it?

Comment 9

[Mats Palmgren (inactive)]

(Not sure if the active state is worth signalling for checkbox/radio buttons
on a phone though, since presumably the user's finger is covering most/all
of the button anyway when pressing it.)

Comment 10

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Louis Chang [:louis] from comment #6)
> Besides, I'm curious what's the purpose of hover event on mobile phone?
> Since on mobile phone we have nothing to hover (unless using emulator...).
> Maybe we don't need to handle hover event?

The user could be using a mouse or a stylus that supports hovering, but maybe we don't need to indicate that visually.

Comment 11

[Louis Chang [:louis]]

(In reply to Mats Palmgren (:mats) from comment #8)
> Do you mean that after clicking on a checkbox (to make it focused),
> then clicking somewhere on the page background does not unfocus it?
> That sounds a bit odd.

Not really. What I mean is when we click somewhere outside the checkbox we will receive a eventState but not includes NS_EVENT_STATE_ACTIVE,  NS_EVENT_STATE_HOVER, NS_EVENT_STATE_FOCUS. (Which will cause the repaint of checkbox without hover background color and result in checked checkbox with white background color)

> Are you referring to the dotted outline from our :-moz-focusring rule?
> What's strange about it?

Yeah, the focusring rule: http://searchfox.org/mozilla-central/source/layout/style/res/forms.css#593.
The strange rectangle area as shown in the picture of https://webcompat.com/issues/9864. I guess it's somehow related to bug 1400050.

Comment 12

[Louis Chang [:louis]]

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

Comment 13

[Louis Chang [:louis]]

Attachment: Bug 1399723 - Draw a grey background color when event state is active instead of hover.

Review commit: https://reviewboard.mozilla.org/r/181672/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181672/

Comment 14

[Louis Chang [:louis]]

(In reply to Mats Palmgren (:mats) from comment #7)
> Or we could try changing it to NS_EVENT_STATE_ACTIVE instead?

Ya, so I think this is good enough for now.
So I will first fix the grey background issue in this bug and fix the focusring issue in bug 1400050 (if it is the same problem).

Comment 15

[Mats Palmgren (inactive)]

Comment on attachment 8910179 [details]
Bug 1399723 - Draw a grey background color when event state is active instead of hover.

https://reviewboard.mozilla.org/r/181672/#review187174

::: commit-message-a0eb2:1
(Diff revision 1)
> +Bug 1399723 - Draw a gery background color when state is active instead of hover. r?mats

s/gery/grey/

Comment 16

[Louis Chang [:louis]]

Comment on attachment 8910179 [details]
Bug 1399723 - Draw a grey background color when event state is active instead of hover.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181672/diff/1-2/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4a59e79b7e94
Draw a grey background color when event state is active instead of hover. r=mats

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4a59e79b7e94

Comment 19

[Karl Dubost💡 :karlcow]

mats, louis,

another 3 states issues which is not resolved by the latest build.
https://webcompat.com/issues/6220

It seems to be css related, but we didn't nail it down yet.

Comment 20

[Louis Chang [:louis]]

Ok, I can reproduce the problem on my physical machine. But I think it is different issue with the webcompat issues listed here (9808, 9816, 9803, 9901, 9864). Still looking for the root cause.

Comment 21

[Astley Chen (inactive)]

Karl, could you open a new bug for tracking?

Comment 22

[Mike Taylor [:miketaylr]]

(In reply to Astley Chen [:astley] (UTC+8) from comment #21)
> Karl, could you open a new bug for tracking?

(ni? Karl for Comment #21)

Comment 23

[Karl Dubost💡 :karlcow]

Yup. Doing that now.
Opened Bug 1403410.

Thanks.