Closed Bug 1399723 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Widget: Android, defect, major)

57 Branch
ARM
Android
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 + fixed

People

(Reporter: karlcow, Assigned: lochang)

References

()

Details

(Keywords: regression, Whiteboard: [webcompat])

Attachments

(1 file)

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.
Flags: webcompat?
Summary: Check-box has 3 states – empty / full (white background) / with check-mark. → Checkbox has 3 states – empty / full (white background) / with check-mark.
Flags: needinfo?(lochang)
I will investigate the bug. Leave the ni to remind myself.
webcompat regression in 57, tracking.
Blocks: 1352238
No longer depends on: 1352238
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
Assignee: nobody → lochang
Flags: needinfo?(lochang)
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.
(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.
Depends on: 1399776
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?
Flags: needinfo?(mats)
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.
Flags: needinfo?(mats)
(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?
Flags: needinfo?(lochang)
(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.)
(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.
(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.
Flags: needinfo?(lochang)
(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 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/
Attachment #8910179 - Flags: review?(mats) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4a59e79b7e94
Draw a grey background color when event state is active instead of hover. r=mats
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4a59e79b7e94
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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.
Flags: needinfo?(mats)
Flags: needinfo?(lochang)
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.
Flags: needinfo?(lochang)
Karl, could you open a new bug for tracking?
(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)
Flags: needinfo?(kdubost)
See Also: → 1403410
Yup. Doing that now.
Opened Bug 1403410.

Thanks.
Flags: needinfo?(kdubost)
Flags: needinfo?(mats)
You need to log in before you can comment on or make changes to this bug.