Closed Bug 1352238 Opened 7 years ago Closed 7 years ago

Implement a native theme for checkbox/radio form controls on Android

Categories

(Core Graveyard :: Widget: Android, enhancement, P1)

54 Branch
enhancement

Tracking

(firefox54-, firefox55 wontfix, firefox56+ wontfix, firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox54 - ---
firefox55 --- wontfix
firefox56 + wontfix
firefox57 --- fixed

People

(Reporter: MatsPalmgren_bugz, Assigned: lochang)

References

Details

(Whiteboard: [webcompat])

Attachments

(8 files, 8 obsolete files)

59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
I think we need a native theme for checkbox/radio form controls on Android
in order to be web compatible with the upcoming changes we're doing in bug 605985
and bug 1333482, which aims to make our checkbox/radio controls compatible with
Chrome/Edge.  Those changes are coming in v54, so ideally we would have this
implemented and uplifted to v54 so that we have a coherent set of changes on
all platforms that web developers can adjust to (if needed).
(see also bug 1328474 comment 15 and bug 605985 comment 89 for background)

I'm hoping this is relatively easy to implement if we restrict the theming
support to checkbox/radio only; other controls are less of an issue.

See bug 1340661 comment 8 and bug 1340661 comment 33 for ideas how this
could be implemented.

I'd be happy to answer any questions regarding layout/widget code if that
helps (but I likely won't have time to implement this myself in time for v54).
[Tracking Requested - why for this release]: need this for web compatibility
Tim: please have a look at this one. I'll be in Taipei next week and can discuss more in person. Thanks!
Flags: needinfo?(timdream)
I don't have enough context technically to understand what this needs. Julian, would you might take a look? Thanks.
Flags: needinfo?(timdream) → needinfo?(walkingice0204)
I've posted a WIP that sets up all of the boilerplate. Happy if someone were to take over of it for me. The remaining work is, I believe, to talk to the Android OS to draw the native checkboxes and radio buttons.

Hey snorp, we talked about how to do that a few weeks ago, but I've forgotten... how do we ask the Android / Java layer to draw the checkboxes and radios into the rects we provide?
Flags: needinfo?(snorp)
> how do we ask the Android / Java layer to draw the checkboxes and radios into the rects we provide?

That would be nice, but I don't think we actually need to make it that advanced
in the initial version.  It seems to me we can just continue to use the C++
code we have for painting the check marks.  Or am I missing something?

So I think the quickest path forward would be to:
1. do the backout we did on v53 (bug 1352406) also on trunk
2. remove all the #ifdef's that exclude Android (bug 605985)
3. remove all default styling of checkbox/radio in UA sheets (except margin)
   (this was already done for other platforms in bug 605985)
4. add the Android native theme (Mike's WIP here looks like a great start)

I can make a patch for 2 since I know where those places are...
In bug 1340661, I had a patch that did the native drawing of checkboxes and radios - though it did it at the nsGfx*ControlFrame level, and not the theme level. That patch was never landed, but the technique I'm using in there to draw could be adapted for this purpose.
For part 3, I think all of this should probably be removed:
http://searchfox.org/mozilla-central/rev/b8cce081200129a1307e2a682f121135b5ca1d19/layout/style/res/forms.css#592-671
(but I suspect that doing that will also conflict with the backout in 1
so probably better to do that part first)
Additionally, all (HTML) checkbox/radio styling in 
mobile/android/themes/core/content.css should be removed too.
(In reply to Mike Conley (:mconley) from comment #8)
> In bug 1340661, I had a patch that did the native drawing of checkboxes and
> radios - though it did it at the nsGfx*ControlFrame level, and not the theme
> level.

Great, something like that is what I had in mind for doing the simple initial
version manually.
Hi Joe, this is bug 605985 but for Android platform. It's being shipped on desktop platforms for 54. I wonder if we want to get this to 54. Jet is in Taipei for discussion.
Flags: needinfo?(walkingice0204) → needinfo?(jcheng)
Blocks: 1328474
Mike, I don't see a reason to wait with step 1 here (do the backout we did on v53
(bug 1352406) also on trunk and then uplift that to v54).  It's causing web sites
to break in 54/55 (see bug 1328474), so we might as well do it now.
Flags: needinfo?(mconley)
(In reply to Mats Palmgren (:mats) from comment #12)
> Mike, I don't see a reason to wait with step 1 here (do the backout we did
> on v53
> (bug 1352406) also on trunk and then uplift that to v54).  It's causing web
> sites
> to break in 54/55 (see bug 1328474), so we might as well do it now.

Hold on... I'm confused. We want to apply bug 1352406 to Nightly and Aurora? I'm starting to lose track in all of these landings and backouts... what _exactly_ is the remaining work that's required to let this stuff ride the trains to release? Just this Android stuff?
Flags: needinfo?(mconley) → needinfo?(mats)
The overall plan is to make our implementation of checkbox/radio on Android
behave the same as on other platforms.  There's no other alternative that
can possibly be web-compatible.  The steps to do that is in comment 6.
Flags: needinfo?(mats)
(In reply to Mats Palmgren (:mats) from comment #14)
> The overall plan is to make our implementation of checkbox/radio on Android
> behave the same as on other platforms.  There's no other alternative that
> can possibly be web-compatible.  The steps to do that is in comment 6.

Okay, so just to be explicit, all of the work that we're backing out is currently blocked from re-landing on this bug, and only this bug, correct?
Flags: needinfo?(mats)
I'm not sure what you mean by "blocked from re-landing".  Doing the backouts from bug 1352406
on trunk/aurora isn't blocked on anything.  AFAICT, it can happen now, before the other steps
are completed.  Does that answer your question?
Flags: needinfo?(mats) → needinfo?(mconley)
(In reply to Mats Palmgren (:mats) from comment #16)
> I'm not sure what you mean by "blocked from re-landing".  Doing the backouts
> from bug 1352406
> on trunk/aurora isn't blocked on anything.  AFAICT, it can happen now,
> before the other steps
> are completed.  Does that answer your question?

Sorry, to much mental bit-flipping - I can probably make this clearer:

Yes, let's do the backouts now.

However, the things that we're backing out, I assume we _want_ to land on central and ride the trains at some point. Is this bug the bug that prevents that last bit from occurring?
Flags: needinfo?(mconley) → needinfo?(mats)
(In reply to Mike Conley (:mconley) from comment #17)
> However, the things that we're backing out, I assume we _want_ to land on
> central and ride the trains at some point. Is this bug the bug that prevents
> that last bit from occurring?

To be infinitely clear, I mean - does the _relanding_ of the things that we're backing out rely on this bug being fixed first?
No, I don't think we should re-land the stuff that bug 1352406 backed out,
at least not bug 418833, because the strategy of using background images
in the UA sheet to display the checkmarks simply isn't (and can't be made
to be) web-compatible.  There are a few reasons for this:

First, if an author specifies a background, it trumps the background styles
in the UA sheet, so the checkmarks disappear if they are implemented as
background images (this is bug 1328474).

Second, the UA sheet MUST have -moz-appearance:checkbox/radio for these
controls.  Then, if an author specifies appearance:none, it acts as
a switch to turn off native theme styling and we create an inline-block
box with no styling whatsoever, this is what bug 605985 fixes (except
for Android).  The UA sheet needs to be empty for this reason too.

(I haven't looked at the dependencies on bug 418833 so I don't know if
those can be re-landed in some form after this bug is fixed.)
Flags: needinfo?(mats)
Blocks: 1354336
Filed bug 1357169 to do the backouts for 54 and 55.
It sounds like we don't want to do the native android widget stuff, so clearing the NI
Flags: needinfo?(snorp)
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #21)
> It sounds like we don't want to do the native android widget stuff, so
> clearing the NI

Right. What we need here is to finish the work in attachment 8855346 [details] so that we can paint the controls in C++. 
Mike: are you able to complete this work, or do we need help from Android team?
Flags: needinfo?(mconley)
(In reply to Jet Villegas (:jet) from comment #22)
> (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #21)
> > It sounds like we don't want to do the native android widget stuff, so
> > clearing the NI
> 
> Right. What we need here is to finish the work in attachment 8855346 [details]
> [details] so that we can paint the controls in C++. 
> Mike: are you able to complete this work, or do we need help from Android
> team?

I'm definitely not able to commit the cycles to finish this in the short term. If there are folks from the Android team (especially folks who know how to get our 2d drawing library to draw stuff where the native widget should be), they should take this.
Flags: needinfo?(mconley)
(In reply to Mats Palmgren (:mats) from comment #12)
> Mike, I don't see a reason to wait with step 1 here (do the backout we did
> on v53
> (bug 1352406) also on trunk and then uplift that to v54).  It's causing web
> sites
> to break in 54/55 (see bug 1328474), so we might as well do it now.

These backouts occurred in bug 1357169 and have been completed.
Depends on: 1357169
A kind of webcompat issue related to Firefox Android not having a native theme for input checkbox
https://webcompat.com/issues/6195
The CSS specifies border-radius for this site which Chrome/Safari ignores and Firefox enforces.
Track 54- as we are in the beta cycle and it's likely we won't take this in 54.
[Tracking Requested - why for this release]: Tracking for 56 as I really doubt this can happen in the next few weeks and be ready to go live with 55.
Louis, let's work it out based on the WIP patch from Mike.
Assignee: nobody → lochang
Status: NEW → ASSIGNED
Whiteboard: [webcompat]
Hi Mats,

I have some questions with the bug, could you help me out? Thanks.

I tried to follow steps on comment 6 (remove ifdef, default styling and add native theme on Android), but it seems gecko does not call to nsDisplayThemedBackground::Paint which will create a native theme and call DrawWidgetBackground to draw the checkbox/radio [1] (Only nsDisplayBackgroundColor::Paint and nsDisplayBorder::Paint were called).
Do you have and idea?

[1] http://searchfox.org/mozilla-central/source/layout/painting/FrameLayerBuilder.cpp#6069
Flags: needinfo?(mats)
Does "frame->IsThemed()" return true for the radio/checkbox frames?
It would help if you attach a rollup patch with what you've got so far.
Flags: needinfo?(mats) → needinfo?(lochang)
Basically, I just combine the patches and comment 9 in the bug. And test the patch with some logs, find that gecko doesn't call nsNativeThemeAndroid::DrawWidgetBackground.

Gecko does create a nsNativeThemeAndroid object somewhere and call nsNativeThemeAndroid::ThemeSupportsWidget but widget type is not checkbox or radio.
Flags: needinfo?(mats)
Flags: needinfo?(lochang)
Flags: needinfo?(jcheng)
OK, do you get a call to nsGfxCheckboxControlFrame::BuildDisplayList at all?
(you should, if you don't then there's a -moz-appearance:none somewhere that
needs to be removed).  And IsThemed() should be true in that method.
Once you get to that method, try removing these two lines:
http://searchfox.org/mozilla-central/rev/f1472e5b57fea8d4a7bbdd81c44e46958fe3c1ce/layout/forms/nsGfxCheckboxControlFrame.cpp#125-126

Looking at the patch I'm guessing you missed bullet 3 in comment 6.
I think you need to remove all styles for checkbox/radio in the "CSS theme",
which is this file IIRC:
http://searchfox.org/mozilla-central/source/mobile/android/themes/geckoview/content.css
In particular:
http://searchfox.org/mozilla-central/source/mobile/android/themes/geckoview/content.css#101-102

This example:
data:text/html,<input type=checkbox style="-moz-appearance:none">
should render nothing at all.
Flags: needinfo?(mats)
(In reply to Mats Palmgren (:mats) from comment #32)
> http://searchfox.org/mozilla-central/source/mobile/android/themes/geckoview/
> content.css#101-102

After removing these two lines, gecko paints checkbox/radio with native theme.
Thanks a lot!
Attachment #8884137 - Attachment is obsolete: true
Comment on attachment 8886069 [details]
Bug 1352238 Part 3 - Implement a fake native theme for checkbox/radio form controls on Android.

Hi Mike,

Could you feedback the patch for me? Thanks.

Basically I just combined your patch in Bug 1352238 and Bug 1340661, it seems that the patch can paint checkbox/radio correctly.

Do we still need to implement other functions in nsITheme, or handle the eventStates?
Attachment #8886069 - Flags: feedback?(mconley)
Attached image fennec checkbox screenshot (obsolete) —
checkbox and check mark painted with native theme on fennec.
Comment on attachment 8886069 [details]
Bug 1352238 Part 3 - Implement a fake native theme for checkbox/radio form controls on Android.

This looks good to me at a glance, but I'm really not so good with the 2d drawing API stuff, so I'm not sure what's what in there. You might want additional feedback (or just a review when you're ready) from snorp, who I think has the perfect combination of graphics know-how and Android knowledge.
Attachment #8886069 - Flags: feedback?(mconley) → feedback+
Comment on attachment 8886069 [details]
Bug 1352238 Part 3 - Implement a fake native theme for checkbox/radio form controls on Android.

Hi snorp,

Could you feedback the patch for me? Also some questions below could you help me out? Thanks.

The patch implements the Android native theme for painting checkbox/radio manually. It seems that the patch can paint checkbox/radio correctly.
But I'm not sure if we still need to implement other functions in nsITheme, or handle the eventStates?

If we have to implement it, do you have any suggestions? (I have looked into other platforms, but their implementations are complicated... and have respective implementations using their native functions on each platform)

In addition, do we have any reftest for testing widget rendering on other platforms? Or any suggestions for creating Android reftest?
Attachment #8886069 - Flags: feedback?(snorp)
Flags: needinfo?(snorp)
This is the markup for the https://webcompat.com/issues/8451

```html
<label class="terms-and-conditions clfx">
    <span class="terms-and-conditions-wrapper fl mr5">
        <input name="signup_minireg[tandc_check]" value="0" type="hidden">
        <input tabindex="14" value="1" name="signup_minireg[tandc_check]" id="signup_minireg_tandc_check" type="checkbox">
      </span>
    <span class="tandc-text fl">
        I accept XING's <a href="/app/user?op=tandc&amp;what=dp" target="_blank">Privacy Policy</a> and <a href="/app/user?op=tandc" target="_blank">Terms and Conditions</a>.
      </span>
</label>
```

with width height
* `label`: `282.233px` x `29.7`
* `span`: `15px` x `15.4167px`
* `input`: `9px` x `9px` but with a vertical padding of 2px and horizontal padding of 1px.


On Firefox Android, the user agent CSS is

```css
input[type="radio"], input[type="checkbox"] {
	border: 1px solid var(--form_border) !important;
	padding-inline-start: 1px;
	padding-inline-end: 1px;
	padding-block-start: 2px;
	padding-block-end: 2px;
}
```

while on desktop it is

```css
input[type="checkbox"] {
	display: inline-block;
	-moz-appearance: checkbox;
	margin-block-start: 3px;
	margin-block-end: 3px;
	margin-inline-start: 4px;
	margin-inline-end: 3px;
}

input[type="radio"], input[type="checkbox"] {
	box-sizing: border-box;
	cursor: default;
	padding: unset;
	-moz-binding: unset;
	border: unset;
	background-color: unset;
	color: unset;
}
```

There is a `padding: unset` on Desktop. I'm not sure why Firefox Android has a padding there and a different one for vertical and horizontal, but it might be one of the reasons of our compat issues.

if I do `padding: unset` on Firefox Android, the issue is fixed.

I wonder if fixing the user agent css would solve part of our issues.
ok same thing for https://webcompat.com/issues/8443

I have the feeling that a lot of these Web compatibility issues would disappear if we were just adding padding: unset like we do on desktop.

/* common features of radio buttons and check boxes */

input[type="radio"],
input[type="checkbox"] {
  box-sizing: border-box;
  cursor: default;
  /* unset some values from the general 'input' rule above: */
  padding: unset;
  -moz-binding: unset;
  border: unset;
  background-color: unset;
  color: unset;
}
Flags: needinfo?(miket)
Flags: needinfo?(mats)
https://dxr.mozilla.org/mozilla-central/rev/36f95aeb4c77f7cf3b3366583008cd6e4b6b1dba/mobile/android/themes/geckoview/content.css#186-193

The current rules for Android
input[type="radio"],
input[type="checkbox"] {
  border: 1px solid var(--form_border) !important;
  padding-inline-start: 1px;
  padding-inline-end: 1px;
  padding-block-start: 2px;
  padding-block-end: 2px;
}

I see that comment from Mike Conley 9 months ago (on the changeset which has been reverted since.)
https://bugzilla.mozilla.org/show_bug.cgi?id=418833#c53
(In reply to Louis Chang [:louis] from comment #41)
> Comment on attachment 8886069 [details]
> Bug 1352238 Part 3 - Implement a native theme for checkbox/radio form
> controls on Android.
> 
> Hi snorp,
> 
> Could you feedback the patch for me? Also some questions below could you
> help me out? Thanks.

snorp may have other feedback on this approach, but here's my drive-by feedback...
 
> The patch implements the Android native theme for painting checkbox/radio
> manually. It seems that the patch can paint checkbox/radio correctly.
> But I'm not sure if we still need to implement other functions in nsITheme,
> or handle the eventStates?
> 

I would expect us, at a minimum, to have correct Android implementations for these 3:

  // checkbox:
  bool IsChecked(nsIFrame* aFrame) 

  // radiobutton:
  bool IsSelected(nsIFrame* aFrame) 

  bool IsFocused(nsIFrame* aFrame) 

> If we have to implement it, do you have any suggestions? 

It will be very clear if we have to implement those 3, as the controls won't work at all otherwise.

> In addition, do we have any reftest for testing widget rendering on other
> platforms? Or any suggestions for creating Android reftest?

I recommend starting by enabling these tests:
/mozilla/central/layout/reftests/forms/input/checkbox/reftest.list:

   15: skip-if(Android) == checkbox-baseline.html checkbox-baseline-ref.html # skip-if(Android) because Android use appearance:none by default for checkbox/radio.
   16: skip-if(Android) == checkbox-radio-color.html checkbox-radio-color-ref.html # skip-if(Android) because Android use appearance:none by default for checkbox/radio.
   17  skip == checkbox-radio-auto-sized.html checkbox-radio-auto-sized-ref.html # Disabled until bug 1352238 is fixed.

/mozilla/central/layout/reftests/forms/input/radio/reftest.list

   1: == label-dynamic.html label-dynamic-ref.html
   2: != checked-native.html checked-native-notref.html
   3: fails-if(Android) == checked-appearance-none.html about:blank
   4: fails-if(Android) == unchecked-appearance-none.html about:blank
I don't really know this code at all, but Jet's response looks reasonable.
Flags: needinfo?(snorp)
Comment on attachment 8886069 [details]
Bug 1352238 Part 3 - Implement a fake native theme for checkbox/radio form controls on Android.

https://reviewboard.mozilla.org/r/156866/#review168372

This generally seems fine if it's the approach we want to take. I don't know what the overall strategy should be regarding form controls. Do we try to use the native widgets when we can? If we really want the controls to look "native", we can probably do that with some Java glue, but I don't know about the CSS interactions there.
Attachment #8886069 - Flags: review+
Attachment #8886067 - Flags: review?(mats)
Attachment #8886068 - Flags: review?(mats)
Attachment #8886069 - Flags: review?(mats)
(In reply to Jet Villegas (:jet) from comment #45)
>    15: skip-if(Android) == checkbox-baseline.html checkbox-baseline-ref.html
> # skip-if(Android) because Android use appearance:none by default for
> checkbox/radio.
>    16: skip-if(Android) == checkbox-radio-color.html
> checkbox-radio-color-ref.html # skip-if(Android) because Android use
> appearance:none by default for checkbox/radio.
>    17  skip == checkbox-radio-auto-sized.html
> checkbox-radio-auto-sized-ref.html # Disabled until bug 1352238 is fixed. 
>    3: fails-if(Android) == checked-appearance-none.html about:blank
>    4: fails-if(Android) == unchecked-appearance-none.html about:blank

Thanks!
Enable tests for checkbox/radio controls on Android.
Try results:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b32f770c56b1dfafe1b762485a55e0f2a39e069
Overall the patches fix some reftests, but also break some...
Notice that there are some small padding when appearance is none in these two test cases:
layout/reftests/forms/input/checkbox/checked-appearance-none.html
layout/reftests/forms/input/checkbox/unchecked-appearance-none.html
But if we set the padding: unset rule for Android as mentioned in comment 44, the padding will become smaller. But still have some padding.
Try result with padding: unset
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac76c3561b7acd25b1cb9ff5192cad2db5641f87&selectedJob=120029512
(In reply to Louis Chang [:louis] from comment #54)

Hi Mats,
could you help me out with following problems, thanks.

> But if we set the padding: unset rule for Android as mentioned in comment
> 44, the padding will become smaller. But still have some padding.

It seems that the padding is due to default style in Android UA sheets http://searchfox.org/mozilla-central/source/mobile/android/themes/geckoview/content.css#186-192. Guess we should remove them as well?

> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=ac76c3561b7acd25b1cb9ff5192cad2db5641f87&selectedJob=1
> 20029512

Do you have any idea with other failures?
Attachment #8886069 - Flags: review?(mats)
Attachment #8886068 - Flags: review?(mats)
Attachment #8886067 - Flags: review?(mats)
(In reply to Louis Chang [:louis] from comment #60)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=685ee6bf0463a43d3acfa85dc64d8333f4cd0e72

After remove the default border and padding in UA sheet of Android, there are still some failures need to fix:
1 failure in R7
2 failures in R10
17 percent-overflow-sizing related failures in R12
1 failures in R16
But most of the checkbox/radio reftests enabled in patch Part 4 are passed.
(In reply to Louis Chang [:louis] from comment #61)
> 17 percent-overflow-sizing related failures in R12

It seems the overflow property is broken in the reference. But I am not sure why my patches will influence them.
Comment on attachment 8886067 [details]
Bug 1352238 Part 1 - Make box construction and layout for radio/checkbox elements work the same on Android as on other platforms.

https://reviewboard.mozilla.org/r/156862/#review176104

::: commit-message-6fec4:1
(Diff revision 3)
> +Bug 1352238 Part 1 - Remove ifdef that exclude Android.

This patch needs a better commit message.  For example,
"Bug 1352238 Part 1 - Make box construction and layout for radio/checkbox elements work the same on Android as on other platforms."
Attachment #8886067 - Flags: review?(mats) → review+
Comment on attachment 8886068 [details]
Bug 1352238 Part 2 - Remove default styling of checkbox/radio in UA sheets.

https://reviewboard.mozilla.org/r/156864/#review176106

Here are a few other selectors/rules that I suspect also needs to be modified:
http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/mobile/android/themes/geckoview/content.css#141-147,161,216,236-239,241-243,245-247,259-260,289-298,322

The goal is that no rules in content.css should apply to <input type=radio|checkbox>,
except maybe for :focus.  How does Chrome on Android render focus on radio/checkboxes?

(You can see which rules apply in devtools if you enable "show browser styles" in its settings.)

::: layout/style/res/forms.css
(Diff revision 3)
> -  border: 2px inset ThreeDLightShadow;
> -  background-repeat: no-repeat;
> -  background-position: center;
> -}
> -
> -input[type="radio"]:disabled,

Since there is special styling for :disabled states that we remove here, I assume that we need to implement special painting for that somehow.  (Ditto for :hover:active states.)  I'm guessing WidgetStateChanged should return aShouldRepaint = true for these cases in part 3.

::: mobile/android/themes/geckoview/content.css
(Diff revision 3)
> -textarea,
> -button,
> -xul|button,
> -* > input:not([type="image"]) {
> -  -moz-appearance: none !important;  /* See bug 598421 for fixing the platform */
> -}

I think we should restrict the changes in this bug to <input type=radio|checkbox> only, so we should keep this rule for other elements.
Attachment #8886068 - Flags: review?(mats) → review-
Comment on attachment 8886069 [details]
Bug 1352238 Part 3 - Implement a fake native theme for checkbox/radio form controls on Android.

https://reviewboard.mozilla.org/r/156866/#review176114

I suspect that we also need to fill the area with a background color here
after removing the additional styles in part 2.  Probably also implement
hover state etc.

::: commit-message-5756a:1
(Diff revision 3)
> +Bug 1352238 Part 3 - Implement a native theme for checkbox/radio form controls on Android.

We should probably call it a "fake native theme" to avoid misleading anyone that we're implementing an actual native theme (using native platform colors etc).

::: widget/android/nsNativeThemeAndroid.h:11
(Diff revision 3)
> +#define nsNativeThemeAndroid_h_
> +
> +#include "nsITheme.h"
> +#include "nsNativeTheme.h"
> +
> +class nsNativeThemeAndroid: private nsNativeTheme,

Make the class "final".

::: widget/android/nsNativeThemeAndroid.h:27
(Diff revision 3)
> +  virtual bool GetWidgetPadding(nsDeviceContext* aContext,
> +                                nsIFrame* aFrame,
> +                                uint8_t aWidgetType,
> +                                nsIntMargin* aResult) override;

I think we usually don't add "virtual" for "override" methods, since it's redundant.

::: widget/android/nsNativeThemeAndroid.cpp:35
(Diff revision 3)
> +  aDrawTarget->StrokeRect(devPxRect,
> +    ColorPattern(ToDeviceColor(mozilla::widget::sAndroidBorderColor)));
> +}
> +
> +static void
> +PaintCheckMark(nsIFrame* aFrame,

Are these painting functions the same as in nsGfxCheckboxControlFrame.cpp without any changes?

http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/layout/forms/nsGfxCheckboxControlFrame.cpp#24

If they are not the same then please up split up this patch in two parts: one that copies them unchanged, and another one patch that modifies them, so that I can see what changes you're doing.

Please also remove the original paint code in nsGfxCheckbox/RadioControlFrame.cpp.

I think we can also remove the BuildDisplayList methods since they are no
longer needed.
http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/layout/forms/nsGfxCheckboxControlFrame.cpp#111-136
http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/layout/forms/nsGfxRadioControlFrame.cpp#44-96

::: widget/android/nsNativeThemeAndroid.cpp:133
(Diff revision 3)
> +nsNativeThemeAndroid::nsNativeThemeAndroid()
> +{
> +}
> +
> +nsNativeThemeAndroid::~nsNativeThemeAndroid() {
> +}

nit: these empty methods should probably be in the header instead.

::: widget/android/nsNativeThemeAndroid.cpp:141
(Diff revision 3)
> +
> +nsNativeThemeAndroid::~nsNativeThemeAndroid() {
> +}
> +
> +bool
> +nsNativeThemeAndroid::IsIndeterminate(nsIFrame* aFrame)

Could we use nsNativeTheme::GetIndeterminate instead?
http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/widget/nsNativeTheme.cpp#245

::: widget/android/nsNativeThemeAndroid.cpp:143
(Diff revision 3)
> +}
> +
> +bool
> +nsNativeThemeAndroid::IsIndeterminate(nsIFrame* aFrame)
> +{
> +

nit: spurious empty line

::: widget/android/nsNativeThemeAndroid.cpp:158
(Diff revision 3)
> +                                           nsIFrame* aFrame,
> +                                           uint8_t aWidgetType,
> +                                           const nsRect& aRect,
> +                                           const nsRect& aDirtyRect)
> +{
> +  switch(aWidgetType) {

nit: add a space after the switch keyword please

::: widget/android/nsNativeThemeAndroid.cpp:174
(Diff revision 3)
> +      }
> +      if (IsIndeterminate(aFrame)) {
> +        PaintIndeterminateMark(aFrame, aContext->GetDrawTarget(), aDirtyRect);
> +      }
> +      break;
> +    default:

We should add an assertion here since we shouldn't get any calls here for values that we returned false for in ThemeSupportsWidget. (I think)

::: widget/android/nsNativeThemeAndroid.cpp:184
(Diff revision 3)
> +}
> +
> +NS_IMETHODIMP
> +nsNativeThemeAndroid::GetWidgetBorder(nsDeviceContext* aContext, nsIFrame* aFrame,
> +                                      uint8_t aWidgetType, nsIntMargin* aResult)
> +{

We should set aResult to zeros here.
The easiest way to do that is probably "*aResult = nsIntMargin()".
(aResult is required to be non-null here)

::: widget/android/nsNativeThemeAndroid.cpp:209
(Diff revision 3)
> +NS_IMETHODIMP
> +nsNativeThemeAndroid::GetMinimumWidgetSize(nsPresContext* aPresContext,
> +                                       nsIFrame* aFrame, uint8_t aWidgetType,
> +                                       LayoutDeviceIntSize* aResult,
> +                                       bool* aIsOverridable)
> +{

Ditto for aResult here.  Perhaps we should return a fixed size here though corresponding the size PaintCheckMark draws.

::: widget/android/nsNativeThemeAndroid.cpp:217
(Diff revision 3)
> +
> +NS_IMETHODIMP
> +nsNativeThemeAndroid::WidgetStateChanged(nsIFrame* aFrame, uint8_t aWidgetType,
> +                                     nsIAtom* aAttribute, bool* aShouldRepaint,
> +                                     const nsAttrValue* aOldValue)
> +{

We need to assign aShouldRepaint before returning.

::: widget/android/nsNativeThemeAndroid.cpp:234
(Diff revision 3)
> +                                          nsIFrame* aFrame,
> +                                          uint8_t aWidgetType)
> +{
> +  switch (aWidgetType) {
> +    case NS_THEME_RADIO:
> +      return true;

nit: please remove this return and let it fall through

::: widget/android/nsWidgetFactory.cpp:61
(Diff revision 3)
> +static nsresult
> +nsNativeThemeAndroidConstructor(nsISupports *aOuter, REFNSIID aIID,
> +                                void **aResult)
> +{
> +  nsresult rv;
> +  nsNativeThemeAndroid* inst;

nit: please remove this line and instead write "nsNativeThemeAndroid* inst = ..." where it's initialized.

::: widget/android/nsWidgetFactory.cpp:64
(Diff revision 3)
> +{
> +  nsresult rv;
> +  nsNativeThemeAndroid* inst;
> +
> +  *aResult = nullptr;
> +  if (nullptr != aOuter) {

I assume this was copy-pasted from some old style code.  Nowadays we don't write out explicit checks against "nullptr", just make this "if (aOuter) ...".

::: widget/android/nsWidgetFactory.cpp:70
(Diff revision 3)
> +  if (nullptr == inst) {
> +    rv = NS_ERROR_OUT_OF_MEMORY;
> +    return rv;
> +  }

Please remove this error checking since "new" is infallibe.
Attachment #8886069 - Flags: review?(mats) → review-
The review comments hopefully answers the pending needinfo questions.
Please ask again if not.
Flags: needinfo?(mats)
Comment on attachment 8886068 [details]
Bug 1352238 Part 2 - Remove default styling of checkbox/radio in UA sheets.

https://reviewboard.mozilla.org/r/156864/#review176106

The style of focus on radio/checkbox got from chrome devtools:
select:focus,
input[type="file"]:focus,
input[type="radio"]:focus,
input[type="checkbox"]:focus {
    outline: 5px auto -webkit-focus-ring-color;
    outline-offset: -2px;
}
Should we add something equivalent to style sheet?
Attachment #8892320 - Attachment description: Bug Part 6 - Enable reftests for checkbox/radio form controls on Android. → Bug 1352238 Part 6 - Enable reftests for checkbox/radio form controls on Android.
Attachment #8886086 - Attachment is obsolete: true
Comment on attachment 8886069 [details]
Bug 1352238 Part 3 - Implement a fake native theme for checkbox/radio form controls on Android.

https://reviewboard.mozilla.org/r/156866/#review176114

> Are these painting functions the same as in nsGfxCheckboxControlFrame.cpp without any changes?
> 
> http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/layout/forms/nsGfxCheckboxControlFrame.cpp#24
> 
> If they are not the same then please up split up this patch in two parts: one that copies them unchanged, and another one patch that modifies them, so that I can see what changes you're doing.
> 
> Please also remove the original paint code in nsGfxCheckbox/RadioControlFrame.cpp.
> 
> I think we can also remove the BuildDisplayList methods since they are no
> longer needed.
> http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/layout/forms/nsGfxCheckboxControlFrame.cpp#111-136
> http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/layout/forms/nsGfxRadioControlFrame.cpp#44-96

Sure, so I keep the original painting stuffs in part 3 and split the modification to part 4. The remove of painting code in nsGfxCheckbox/RadioControlFrame.cpp will be in part 5.

Painting codes of PaintCheckboxControl and PaintRadioContro are using Mike's patch in bug 1340661.
And other painting functions basically just change the color to Android hardcoded colors.

For the input argument aPt, I am not sure what value I should use since the orginal value is got by ToReferenceFrame() and I think we can not call it directly here. Do you have any suggestion?
http://searchfox.org/mozilla-central/source/layout/painting/nsDisplayList.h#2788
(In reply to Louis Chang [:louis] from comment #67)
> The style of focus on radio/checkbox got from chrome devtools:
> select:focus,
> [...]
> Should we add something equivalent to style sheet?

Yeah, that seems reasonable.
Please add it around here (in a %ifdef MOZ_WIDGET_ANDROID):
http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/layout/style/res/forms.css#649
so it's near the similar OSX rule.
Comment on attachment 8900166 [details]
Bug 1352238 Part 4 - Implement PaintCheckBox/RadioControl and modify the original painting functions.

https://reviewboard.mozilla.org/r/171552/#review176976

::: widget/android/nsNativeThemeAndroid.cpp:17
(Diff revision 1)
>  NS_IMPL_ISUPPORTS_INHERITED(nsNativeThemeAndroid, nsNativeTheme, nsITheme)
>  
>  using namespace mozilla::gfx;
>  
>  static void
> +PaintCheckboxControl(nsIFrame* aFrame,

Shouldn't this function also have a FillRect call to fill the background with some suitable background color (filling all of aRect)?
(I'm assuming the rendering should have a grey-ish background, a border, and possibly a checkmark.)

::: widget/android/nsNativeThemeAndroid.cpp:19
(Diff revision 1)
> +                     const nsRect& aDirtyRect)
> +{
> +  nsRect rect(nsPoint(aDirtyRect.X(), aDirtyRect.Y()), aFrame->GetSize());

To answer your question about aPt:
I think you can rename 'aDirtyRect' to aRect 'here', and initialize 'rect' from 'aRect'.
Then pass 'aRect' in DrawWidgetBackground to all these Paint* methods.
IOW, we'll always paint the full size of the frame rather than just the area that is dirty.
I think 'aDirtyRect' is provided mostly to enable optimizations,
which isn't particularly important for these (small) controls.
Attachment #8900166 - Flags: review?(mats) → review-
(Quoted the wrong word above, I meant: s/aRect 'here'/'aRect' here/)
Comment on attachment 8886068 [details]
Bug 1352238 Part 2 - Remove default styling of checkbox/radio in UA sheets.

https://reviewboard.mozilla.org/r/156864/#review176992

LGTM.  r=mats
Attachment #8886068 - Flags: review?(mats) → review+
(In reply to Mats Palmgren (:mats) from comment #76)
> Comment on attachment 8900166 [details]
> Bug 1352238 Part 4 - Implement PaintCheckBox/RadioControl and modify the
> original painting functions.

I think we also need code to use different colors for disabled/hover/active states.
Comment on attachment 8900167 [details]
Bug 1352238 Part 5 - Remove painting related methods in nsGfxCheckbox/RadioControlFrame.cpp.

https://reviewboard.mozilla.org/r/171554/#review176996
Attachment #8900167 - Flags: review?(mats) → review+
Comment on attachment 8892320 [details]
Bug 1352238 Part 6 - Enable reftests for checkbox/radio form controls on Android.

https://reviewboard.mozilla.org/r/163284/#review176998
Attachment #8892320 - Flags: review?(mats) → review+
Comment on attachment 8886069 [details]
Bug 1352238 Part 3 - Implement a fake native theme for checkbox/radio form controls on Android.

https://reviewboard.mozilla.org/r/156866/#review177008

r=mats with the nit fixed
(I'll assume that any improvements for states will go in part 4 or later)

::: widget/android/nsNativeThemeAndroid.cpp:121
(Diff revision 4)
> +      if (GetIndeterminate(aFrame)) {
> +        PaintIndeterminateMark(aFrame, aContext->GetDrawTarget(), aDirtyRect);
> +      }
> +      break;
> +    default:
> +      MOZ_ASSERT(0, "Should not get here with a widget type we don't support.");

nit: use MOZ_ASSERT_UNREACHABLE instead
Attachment #8886069 - Flags: review?(mats) → review+
(In reply to Mats Palmgren (:mats) from comment #75)
> Yeah, that seems reasonable.
> Please add it around here (in a %ifdef MOZ_WIDGET_ANDROID):
> http://searchfox.org/mozilla-central/rev/
> 48ea452803907f2575d81021e8678634e8067fc2/layout/style/res/forms.css#649
> so it's near the similar OSX rule.

Here I use property Hightlight to replace -webkit-focus-ring-color as suggestion in
https://stackoverflow.com/questions/7538771/what-is-webkit-focus-ring-color
Not sure if it is appropriate?
(In reply to Mats Palmgren (:mats) from comment #76)
> Shouldn't this function also have a FillRect call to fill the background
> with some suitable background color (filling all of aRect)?
> (I'm assuming the rendering should have a grey-ish background, a border, and
> possibly a checkmark.)

Sorry, I am a little bit unclear about what I should do now... Could you point me out with the following questions? Thanks.

1. How to handle special :disabled states we removed? Is it like adding something below to painting functions:
   if (state == disabled) {
     if (state == hover || state == active) {
          paint padding: 1px;
          paint border: 1px inset ThreeDShadow;
     }
   }

2. What checkbox/radio should look like? The same as checkbox/radio of Chrome on Android? (Guess we are not able to manually (using Moz2D) paint out checkbox/radio the same as checkbox/radio Firefox painted on other platforms since they are painted by specific OS native theme?) Besides, I think there is no background color (white) for checkbox of Firefox in other platforms?

3. Per test result in comment 84. It seems we fixed some reftests in R10 after addressing your comments. There are 1 failure in R7, 1 failure in R10 and 18 overflow related failures in R12 and R16. Do you have any idea about these failures?
Flags: needinfo?(mats)
For the disabled state I think we can just use some grey color when
drawing the border and checkmark.  I don't think hover/active states
are important for controls that are disabled, so use the same there.
For non-disabled controls, in :hover state we can use the same colors
as default but slightly paler? and in :active state we can use the
default colors.
So for 1, just use the same Paint* you have now but vary the colors
slightly for the different states.  I think that's good enough for
the first version.

For 2, I think we should use the simplest possible drawing for now
just so we can ship something to fix the web-compat problems.
So, fill the background, draw a border at the edges and fill the
center with a checkmark, which is pretty much what you have already.
We can polish it in follow-up bugs.

The R7 failure looks like the test renders as expected but not the
reference?  This test, layout/reftests/bugs/1174332-1.html, is
marked as fails-if(gtkWidget) in the manifest -- I think we should
mark it as fails-if for Android too.

The R10 failure seems to indicate that we draw the controls to
small?  The test, layout/reftests/forms/input/checkbox/checkbox-radio-auto-sized.html,
simply checks that the default content size is 11px:
http://searchfox.org/mozilla-central/source/layout/reftests/forms/input/checkbox/checkbox-radio-auto-sized.html
http://searchfox.org/mozilla-central/source/layout/reftests/forms/input/checkbox/checkbox-radio-auto-sized-ref.html
Can you try to increase the size in GetMinimumWidgetSize to 11px?

R12: I didn't look through all the failures, but the ones I looked
at contains no checkbox/radio controls at all.  Are you sure these
are not just intermittents?  Do they pass with all your patches,
but with a new patch on top which reverts content.css file?
If so, it might be that we unintentionally touched a rule related to
scrollbars.  (Let me know if I missed a test that has a checkbox.)

R16: the ua-style-sheet-checkbox-radio-1.html failure looks the same
as R10, so will probably be fixed by increasing the default size.
the overlayscrollbar-sorting-1.html failure looks similar to the R12
failures. (The background isn't painted behind the overlay scrollbar
for some reason.)

We need to figure out what the problem is with the scrollbars.
It might be an existing bug that is uncovered...
Can you try copying nsNativeThemeGTK::WidgetIsContainer to your
theme - does that help?
Flags: needinfo?(mats) → needinfo?(lochang)
(In reply to Mats Palmgren (:mats) from comment #86)
> For the disabled state I think we can just use some grey color when
> drawing the border and checkmark.  I don't think hover/active states
> are important for controls that are disabled, so use the same there.
> For non-disabled controls, in :hover state we can use the same colors
> as default but slightly paler? and in :active state we can use the
> default colors.
> So for 1, just use the same Paint* you have now but vary the colors
> slightly for the different states.  I think that's good enough for
> the first version.
> 
> For 2, I think we should use the simplest possible drawing for now
> just so we can ship something to fix the web-compat problems.
> So, fill the background, draw a border at the edges and fill the
> center with a checkmark, which is pretty much what you have already.
> We can polish it in follow-up bugs.

Ok, really appreciate for your comments. I will study 2d painting functions and try to handle these painting issues.

> The R7 failure looks like the test renders as expected but not the
> reference?  This test, layout/reftests/bugs/1174332-1.html, is
> marked as fails-if(gtkWidget) in the manifest -- I think we should
> mark it as fails-if for Android too.

Ok, I will add a fails-if for this test case on Android in Part 6.

> The R10 failure seems to indicate that we draw the controls to
> small?  
> 
> R16: the ua-style-sheet-checkbox-radio-1.html failure looks the same
> as R10, so will probably be fixed by increasing the default size.

It seems the test case in R10 is passed after returning 11px in GetMinimumWidgetSize according to test result in comment 87.
But ua-style-sheet-checkbox-radio-1.html is still failed. Does that make sense...?

> R12: I didn't look through all the failures, but the ones I looked
> at contains no checkbox/radio controls at all.  Are you sure these
> are not just intermittents?  Do they pass with all your patches,
> but with a new patch on top which reverts content.css file?

I'm not sure if these test cases are intermittents... How can I verify it? (Currently, I only count on the annotation on try result...) However, I have pushed try several times and these tests seem always failed. Besides, it seems the problem is not we mistouch rules related to scrollbars since theses tests still failed after reverting content.css according to test result in comment 88.

> We need to figure out what the problem is with the scrollbars.
> It might be an existing bug that is uncovered...
> Can you try copying nsNativeThemeGTK::WidgetIsContainer to your
> theme - does that help?

It seems the test cases are still failed after copying the implementation of nsNativeThemeGTK::WidgetIsContainer to my patch according to test result in comment 87.
Flags: needinfo?(lochang) → needinfo?(mats)
> But ua-style-sheet-checkbox-radio-1.html is still failed. Does that make sense...?

It looks like it helped though -- it's larger than the controls in comment 84.
Try 13px instead.  That seems more logical actually, 9px + (1px padding + 1px border) * 2.

> I'm not sure if these test cases are intermittents... How can I verify it?

If you click on a test in treehearder and then type 'r' it will retrigger that test.
Repeat as many times you need.
Or you can specify "--rebuild 4" in the try syntax (to run the tests 5 times).

> ... theses tests still failed after reverting content.css according to test result in comment 88.

OK, I don't see any mistakes in those rules either.

> It seems the test cases are still failed after copying the implementation of nsNativeThemeGTK::WidgetIsContainer ...

Hmm, it must be something else then...
I guess we could try returning true in ThemeSupportsWidget too for
the scrollbar values just to see if that makes any difference (and also
remove the assertions we added about only accepting checkbox/radio).

Also, do a try job with no changes at all just to rule out that these tests
are perma-failing on Try for some reason...
Flags: needinfo?(mats)
(In reply to Mats Palmgren (:mats) from comment #90)
> Hmm, it must be something else then...
> I guess we could try returning true in ThemeSupportsWidget too for
> the scrollbar values just to see if that makes any difference (and also
> remove the assertions we added about only accepting checkbox/radio).

It seems this is not the reason either as the try result shown in comment 91.

> Also, do a try job with no changes at all just to rule out that these tests
> are perma-failing on Try for some reason...

As try result without any patch in comment 92, it seems the overflow related failures are not perma-failing.
I am still finding which part of my patches break these tests...


I have tried to paint the border and handle disabled and hover states by painting different background color in the latest part 4 patch. However, I am not sure how we handle the focus state. I have tried the rules used in chrome as we discussed in comment 67, comment 75, but the effect is weird... Maybe keep using the original rules in forms.css?
http://searchfox.org/mozilla-central/source/layout/style/res/forms.css#656
> I am still finding which part of my patches break these tests...

I suspect that just returning non-null from GetTheme() is what
causes the scrollframe problem, but there's no way to avoid that.
I'm guessing we have code that assumes* that scrollbars are always
supported by the theme if it exists, so we probably need to add
some code in the theme for scrollbars to wallpaper this for now.
([*] see for example nsScrollbarFrame::GetXULMargin which never
calls ThemeSupportsWidget)

Does returning 16x16 for scrollbars in GetMinimumWidgetSize make
a difference?

> Maybe keep using the original rules in forms.css?

Yeah, that rule looks fine.
Comment on attachment 8900166 [details]
Bug 1352238 Part 4 - Implement PaintCheckBox/RadioControl and modify the original painting functions.

https://reviewboard.mozilla.org/r/171552/#review179004

r=mats with the nits fixed

::: widget/android/nsNativeThemeAndroid.cpp:24
(Diff revision 2)
> +PaintCheckboxControl(nsIFrame* aFrame,
> +                     DrawTarget* aDrawTarget,
> +                     const nsRect& aRect,
> +                     const EventStates& aState)
> +{
> +  nsRect rect(nsPoint(aRect.X(), aRect.Y()), aFrame->GetSize());

nit: "nsRect rect(aRect)" is simpler and should give the same result  (ditto in the other places)

::: widget/android/nsNativeThemeAndroid.cpp:117
(Diff revision 2)
> +  Rect devPxRect =
> +    ToRect(nsLayoutUtils::RectToGfxRect(rect,
> +                                        aFrame->PresContext()->AppUnitsPerDevPixel()));

nit: can we declare 'twipsPerPixel' and use 'NSRectToRect' as in 'PaintCheckboxControl' for consistency?
Attachment #8900166 - Flags: review?(mats) → review+
(In reply to Mats Palmgren (:mats) from comment #100)
> I suspect that just returning non-null from GetTheme() is what
> causes the scrollframe problem, but there's no way to avoid that.
> I'm guessing we have code that assumes* that scrollbars are always
> supported by the theme if it exists, so we probably need to add
> some code in the theme for scrollbars to wallpaper this for now.
> ([*] see for example nsScrollbarFrame::GetXULMargin which never
> calls ThemeSupportsWidget)

I see...
So does that mean we have to handle NS_THEME_SCROLLBAR in DrawWidgetBackground and do some painting for scroll bar?
Is that so, could you explain more details of how to paint it... (e.g. the scroll bar we expect to look like, what states we need to handle)

> Does returning 16x16 for scrollbars in GetMinimumWidgetSize make
> a difference?

Yeah, It's different [1]. But it seems tests and refs are both wrong (both render green rect without scrollbar). And some other new tests are broken after returning 16x16.
[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e9731f66103743cf543d3afcd81b3159d32cabe&selectedJob=126942335
Flags: needinfo?(mats)
Attached image Checkboxes, innerRadii = 5 (obsolete) —
The checkboxes in the reftest results looks a bit too rounded to me.
I'd prefer a more square look to make them easier to distinguish
from radio controls.  (the screenshot is zoomed 8x)

I think it's "innerRadii(5, 5, 5, 5)" in part 4 that controls this?
Could you try a few smaller values and see if that looks better?
Flags: needinfo?(lochang)
> Yeah, It's different [1]

OK, interesting.

Looking at the R16 result again from comment 91, it looks like the missing
background is 6px wide, which corresponds to the scrollbar CSS sizes here:
http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/mobile/android/themes/geckoview/content.css#33-37
so returning zero in GetMinimumWidgetSize seems like the right thing to do.

Could you try returning 'eTransparent' from GetWidgetTransparency
for "aWidgetType == NS_THEME_SCROLLBAR" and see if that helps?
Flags: needinfo?(mats)
(In reply to Louis Chang [:louis] from comment #103)
> So does that mean we have to handle NS_THEME_SCROLLBAR in
> DrawWidgetBackground and do some painting for scroll bar?

No, I think we should try to keep the scrollbar rendering in CSS for now
and see if we can get that working.
(In reply to Mats Palmgren (:mats) from comment #104)
> Could you try a few smaller values and see if that looks better?

Sure, I tried innerRadii = 3 and the result is as shown in comment 107 (see R5). See if it is ok to you?

(In reply to Mats Palmgren (:mats) from comment #105)
> Could you try returning 'eTransparent' from GetWidgetTransparency
> for "aWidgetType == NS_THEME_SCROLLBAR" and see if that helps?

The differences remain the same as shown in try result in comment 107.
Flags: needinfo?(lochang) → needinfo?(mats)
(In reply to Louis Chang [:louis] from comment #108)
> (In reply to Mats Palmgren (:mats) from comment #104)
> > Could you try a few smaller values and see if that looks better?
> 
> Sure, I tried innerRadii = 3 and the result is as shown in comment 107 (see
> R5). See if it is ok to you?

Better, but maybe still a bit too round?
We should try to make the rounding consistent with other controls,
e.g. <button>, which looks like it has border-radius:2px
http://searchfox.org/mozilla-central/source/mobile/android/themes/geckoview/content.css
http://searchfox.org/mozilla-central/source/mobile/android/themes/geckoview/defines.css
Could you try with 2 and compare the checkbox corner with the other controls?

> (In reply to Mats Palmgren (:mats) from comment #105)
> > Could you try returning 'eTransparent' from GetWidgetTransparency
> > for "aWidgetType == NS_THEME_SCROLLBAR" and see if that helps?
> 
> The differences remain the same as shown in try result in comment 107.

OK, let's skip that then.

I looked a bit closer at the failing tests:

R5:
Three unexpected PASS -- just mark them as PASS in the manifest? They look OK to me.

The last one:
FAIL | http://10.0.2.2:8854/tests/layout/reftests/bugs/315920-17.html == http://10.0.2.2:8854/tests/layout/reftests/bugs/315920-17-ref.html
image comparison, max difference: 1, number of differing pixels: 12

This is just some anti-aliasing (AA) difference -- just add a fuzz factor
in the manifest to make it pass.

R6: all unexpected PASS -- just mark them as PASS in the manifest? They look OK to me.

R12: the failing tests here looks slightly broken to me, e.g.

FAIL | http://10.0.2.2:8854/tests/layout/reftests/percent-overflow-sizing/hScrollSimpleHeightQuirks-1.html == http://10.0.2.2:8854/tests/layout/reftests/percent-overflow-sizing/greenboxhbar.html
image comparison, max difference: 255, number of differing pixels: 1200

http://searchfox.org/mozilla-central/source/layout/reftests/percent-overflow-sizing/hScrollSimpleHeightQuirks-1.html
http://searchfox.org/mozilla-central/source/layout/reftests/percent-overflow-sizing/greenboxhbar.html

Note that the test has 'background:green' on the inner DIV and the reference
on the outer, so the test depends on the scrollbar being fully transparent to
let the inner DIV background visible.

The odd thing is that it appears Android has transparent background on scrollbars:
http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/mobile/android/themes/geckoview/content.css#22-29

These tests are marked as "random-if(transparentScrollbars)" in the manifest:
http://searchfox.org/mozilla-central/source/layout/reftests/percent-overflow-sizing/reftest.list
which I think is defined here:
http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/layout/tools/reftest/reftest.jsm#705
Could you check manually that the scrollbars looks reasonable on some pages?
If so, then we might want to add "|| sandbox.Android" to the expression there
to make these tests PASS?

R13: unexpected PASS -- just mark it as PASS in the manifest? it looks OK to me.

R16: the first fails due to AA -- just add a fuzz factor to make it pass;
the second has "random-if(transparentScrollbars)" in the manifest so should
pass if we make that true on Android.
Flags: needinfo?(mats) → needinfo?(lochang)
(In reply to Mats Palmgren (:mats) from comment #109)
> Better, but maybe still a bit too round?
> We should try to make the rounding consistent with other controls,
> e.g. <button>, which looks like it has border-radius:2px
> http://searchfox.org/mozilla-central/source/mobile/android/themes/geckoview/
> content.css
> http://searchfox.org/mozilla-central/source/mobile/android/themes/geckoview/
> defines.css
> Could you try with 2 and compare the checkbox corner with the other controls?

Actually, the checkbox painted on physical Android machine looks different on try result. Radius 3 or 5 seem no big difference and they all look good to me (not so rounded and compare to other controls look the same)...
However, I will try 2 and see if it is better on try result.
(In reply to Mats Palmgren (:mats) from comment #109) 
> The odd thing is that it appears Android has transparent background on
> scrollbars:

So is it reasonable that we remove this rule?

> Could you check manually that the scrollbars looks reasonable on some pages?
> If so, then we might want to add "|| sandbox.Android" to the expression there
> to make these tests PASS?

So I have tested it manually, scroll bar on most web pages looks normal. But if I simply search something with search bar and scroll in search results page, I can't see scroll bar at all (originally or without patches, scroll bar will appear when I scroll then after a second it will be transparent). Not sure how big the impact is... Is it still ok to add "|| sandbox.Android"? However, after adding this, try result seems good in comment 112.
Flags: needinfo?(lochang) → needinfo?(mats)
(In reply to Louis Chang [:louis] from comment #113)
> (In reply to Mats Palmgren (:mats) from comment #109) 
> > The odd thing is that it appears Android has transparent background on
> > scrollbars:
> 
> So is it reasonable that we remove this rule?

No, I was just surprised over why the background didn't fill the entire
scroll port because clearly the scrollbars are styled w. transparent background.
But I guess there is something else affecting it that I don't quite understand.

> So I have tested it manually, scroll bar on most web pages looks normal. But
> if I simply search something with search bar and scroll in search results
> page, I can't see scroll bar at all (originally or without patches, scroll
> bar will appear when I scroll then after a second it will be transparent).

OK, sounds good.  I guess it's normal that the scrollbars fades away a few
seconds after using them.  If that's the behavior also without these patches
then I don't think we need to worry about it.

> Not sure how big the impact is... Is it still ok to add "||
> sandbox.Android"? However, after adding this, try result seems good in
> comment 112.

Yeah, I think we should go ahead and do that and declare victory.

Karl/Mike can hopefully help to test/verify that the scrollbars work
the same as before in addition to testing checkbox/radio controls.
Flags: needinfo?(mats)
(In reply to Mats Palmgren (:mats) from comment #114)
> Karl/Mike can hopefully help to test/verify that the scrollbars work
> the same as before in addition to testing checkbox/radio controls.

We have Monday (Taipei time) set aside for testing input compat related stuff, so I'll add this to the pile. Thanks!
Flags: needinfo?(miket)
Attached image scrollbar without patches (obsolete) —
Attached image scrollbar with patches (obsolete) —
(In reply to Mats Palmgren (:mats) from comment #114)
> OK, sounds good.  I guess it's normal that the scrollbars fades away a few
> seconds after using them.  If that's the behavior also without these patches
> then I don't think we need to worry about it.

Just make sure we are on the same page. So what I meant is:
Without patches: I can see a scrollbar while scrolling, and after a minute it will fade away. (See picture in comment 116)
With the patches: I can't see any scrollbar while scrolling. (See picture in comment 117)
Oh, it seems I misunderstood then.  That's a problem we need to fix.
The scrollbar background seems wrong too.
It looks like the same issue as in the overflow reftests?
(so debugging one of those might be easier)

Can you make a simple-as-possible testcase that reproduces the problem?
Perhaps dumping the display lists for that test would help understanding
the issue?
(In reply to Mats Palmgren (:mats) from comment #119)
> Oh, it seems I misunderstood then.  That's a problem we need to fix.
> The scrollbar background seems wrong too.
> It looks like the same issue as in the overflow reftests?
> (so debugging one of those might be easier)
> 
> Can you make a simple-as-possible testcase that reproduces the problem?
> Perhaps dumping the display lists for that test would help understanding
> the issue?

Ok, I will first make a simple test case and look into it.
I guess it's nsDisplayScrollInfoLayer::BuildLayer that paint the scrollbar?
The test case will cause the background behind scrollbar transparent on Android.
Attachment #8855346 - Attachment is obsolete: true
Attachment #8855358 - Attachment is obsolete: true
Attachment #8902662 - Attachment is obsolete: true
(In reply to Louis Chang [:louis] from comment #120)
> I guess it's nsDisplayScrollInfoLayer::BuildLayer that paint the scrollbar?

I think ScrollFrameHelper::BuildDisplayList is a better place to start
debugging.  It builds all the display items for the scrollframe,
the background and scrollbars included.  Dumping the resulting display
list from that method might give some clues about the problem.
Perhaps someone from the Painting team can help with the remaining
scrollbar painting issue?

Matt, can you suggest someone?

(Briefly, the remaining issue is that the background behind the scrollbars
isn't painted properly.  It seems that merely adding a theme (GetTheme()
now returns non-null) affects the scrollbar painting somehow, even though
ThemeSupportsWidget(<scrollbar-things>) returns false.)
Flags: needinfo?(matt.woodrow)
I think there is only nsScrollbarFrame::GetXULMargin[1] the place you mentioned that will use theme to paint scrollbar.
And I try to return 32 (the value use in OSX) in GetMinimumWidgetSize and the scrollbar does appear on Android.
However, in patch part 7, I think maybe we can try to bypass that part by checking the return value of GetMinimumWidgetSize?
I have tested on Android and the scrollbar does appear as well. Also the try result looks good on Android:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eec0e07c28af4e41878bd4cafcbd5df04f91dabb&selectedJob=128244769
(The failures in try result are originally annotated as fuzzy or fails-if(Android) and are changed to == in my patch.)
But I haven't check if this will break scrollbar on other platforms yet (try result in comment 132).

[1] http://searchfox.org/mozilla-central/source/layout/xul/nsScrollbarFrame.cpp#164
OK, cancelling the n-i for now since we have a lead...

So, on Android we used to depend on rv != NS_OK so that we execute
the nsBox::GetXULMargin(aMargin) instead?
Now that GetTheme() returns non-null we have rv == NS_OK and we don't
reach that, and nsBox::GetXULMargin is what picks up the CSS 'margin':
http://searchfox.org/mozilla-central/rev/999385a5e8c2d360cc37286882508075fc2078bd/layout/xul/nsBox.cpp#353
Is that correct?

Looking at nsScrollbarFrame::GetXULMargin I tend to think that
the right fix would be to check ThemeSupportsWidget, i.e. something like:

    nsITheme* theme = presContext->GetTheme();
    if (theme && theme->ThemeSupportsWidget(NS_THEME_SCROLLBAR)) {

http://searchfox.org/mozilla-central/rev/999385a5e8c2d360cc37286882508075fc2078bd/layout/xul/nsScrollbarFrame.cpp#164
While that costs an extra virtual call for platforms that supports it,
I think I'll prefer it over returning a failure from
GetMinimumWidgetSize, which feels a bit hacky.
Flags: needinfo?(matt.woodrow)
(In reply to Mats Palmgren (:mats) from comment #134)
> OK, cancelling the n-i for now since we have a lead...
> 
> So, on Android we used to depend on rv != NS_OK so that we execute
> the nsBox::GetXULMargin(aMargin) instead?
> Now that GetTheme() returns non-null we have rv == NS_OK and we don't
> reach that, and nsBox::GetXULMargin is what picks up the CSS 'margin':
> http://searchfox.org/mozilla-central/rev/
> 999385a5e8c2d360cc37286882508075fc2078bd/layout/xul/nsBox.cpp#353
> Is that correct?

Yeah, that's what I think.

> Looking at nsScrollbarFrame::GetXULMargin I tend to think that
> the right fix would be to check ThemeSupportsWidget, i.e. something like:
> 
>     nsITheme* theme = presContext->GetTheme();
>     if (theme && theme->ThemeSupportsWidget(NS_THEME_SCROLLBAR)) {
> 
> http://searchfox.org/mozilla-central/rev/
> 999385a5e8c2d360cc37286882508075fc2078bd/layout/xul/nsScrollbarFrame.cpp#164
> While that costs an extra virtual call for platforms that supports it,
> I think I'll prefer it over returning a failure from
> GetMinimumWidgetSize, which feels a bit hacky.

Ok, I will try to fix it by using ThemeSupportsWidget, then ask for review again.
Attachment #8903932 - Attachment is obsolete: true
Attachment #8903933 - Attachment is obsolete: true
Attachment #8904141 - Attachment is obsolete: true
Hi mats,
I have submitted the fixed patches, could you review again. Thansk.
The try result looks good. And the scroll bar can normally render on the Android device.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=78d8dbaf3cacca19bbbfc31b04ee8f0897c6cb14
Comment on attachment 8904270 [details]
Bug 1352238 Part 7 - Check if native theme supports the widget before using it.

https://reviewboard.mozilla.org/r/176042/#review181246
Attachment #8904270 - Flags: review?(mats) → review+
Comment on attachment 8904416 [details]
Bug 1352238 Part 8 - Make fuzzy and unexpected tests passed.

https://reviewboard.mozilla.org/r/176224/#review181248
Attachment #8904416 - Flags: review?(mats) → review+
Nice work Louis!
Rebase patches to the latest tree.

Thanks for your patience and kindly review!
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c6e7fb7bfc1f
Part 1 - Make box construction and layout for radio/checkbox elements work the same on Android as on other platforms. r=mats
https://hg.mozilla.org/integration/autoland/rev/c60cee53a363
Part 2 - Remove default styling of checkbox/radio in UA sheets. r=mats
https://hg.mozilla.org/integration/autoland/rev/d0e60b3b1b58
Part 3 - Implement a fake native theme for checkbox/radio form controls on Android. r=mats,snorp
https://hg.mozilla.org/integration/autoland/rev/68cd9f54d2b1
Part 4 - Implement PaintCheckBox/RadioControl and modify the original painting functions. r=mats
https://hg.mozilla.org/integration/autoland/rev/a6b21855bb8f
Part 5 - Remove painting related methods in nsGfxCheckbox/RadioControlFrame.cpp. r=mats
https://hg.mozilla.org/integration/autoland/rev/31d1a1111f91
Part 6 - Enable reftests for checkbox/radio form controls on Android. r=mats
https://hg.mozilla.org/integration/autoland/rev/f655236b4f52
Part 7 - Check if native theme supports the widget before using it. r=mats
https://hg.mozilla.org/integration/autoland/rev/bd7c2148cc80
Part 8 - Make fuzzy and unexpected tests passed. r=mats
Keywords: checkin-needed
I strongly doubt we want to backport this to Beta for 56 still.
Blocks: 1398520
Blocks: 1399723
Blocks: 1399776
(In reply to Ryan VanderMeulen [:RyanVM] from comment #155)
> I strongly doubt we want to backport this to Beta for 56 still.

Agreed. There's some kinks to work out.
Depends on: 1400050
No longer blocks: 1399776
Depends on: 1399776
No longer blocks: 1399723
Depends on: 1399723
Depends on: 1404770
Blocks: 1406808
See Also: → 1470867
See Also: → 1435665
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.