STR) Open attached TC Expected: See outlines on input. Actual: No outline on input.

Hrm. This is tricky. In bug 1320732, we intentionally made it so that, on desktop, if -moz-appearance is none on checkbox or radio, that content can go hog-wild on styling the checkbox and radio button. This was requested in bug 418833 comment 123, and wworks fine on Desktop. On Fennec, there's no built-in support for -moz-appearance: checkbox or -moz-appearance: radio, so we fall back to the browser styles, which have had the !important's removed. Hey tnikkel, do you have a sense of the best way to fix this? My instinct is to add a very, very simple support for moz-appearance: checkbox and moz-appearance: radio to our Android widget layer, where it effectively draws what we used to draw for moz-appearance: none on those elements manually in nsGfxCheckboxControlFrame::BuildDisplayList and nsGfxRadioControlFrame::BuildDisplayList. Is there a better way?

https://bugzilla.mozilla.org/show_bug.cgi?id=605985#c60 feels super relevant here.

(In reply to Mike Conley (:mconley) from comment #3) > Hey tnikkel, do you have a sense of the best way to fix this? My instinct is > to add a very, very simple support for moz-appearance: checkbox and > moz-appearance: radio to our Android widget layer, where it effectively > draws what we used to draw for moz-appearance: none on those elements > manually in nsGfxCheckboxControlFrame::BuildDisplayList and > nsGfxRadioControlFrame::BuildDisplayList. Is there a better way? That plan seems reasonable to me.

Hey mats, I see you worked on bug 605985, which (as miketaylr says) does sound somewhat relevant here. Are you able to coach me on how to add -moz-appearance: checkbox and -moz-appearance: radio support just for Fennec, using the old-school -moz-appearance: none checkbox / radio drawing we used to do before bug 418833 landed? Specifically, what bits should I be poking to convince Gecko that Android can draw -moz-appearance: checkbox / radio, and where do I put the draw commands?

Take this with a grain of salt because I just poked on dxr to figure this out now. Looks like android hasn't implemented a native theme at all (ie https://dxr.mozilla.org/mozilla-central/source/widget/nsNativeTheme.cpp and what Windows does https://dxr.mozilla.org/mozilla-central/source/widget/windows/nsNativeThemeWin.cpp). So either make android implement a native theme, or maybe put a basic implementation of drawing checkbox/radiobuttons into the generic cross platform theme (that each platform theme overrides).

See widget/gtk/nsNativeThemeGTK.h/cpp as an example. This may look like a lot of code but it's not too bad if you just need to implement a couple of values. Just return true in ThemeSupportsWidget for NS_THEME_RADIO/CHECKBOX and false for the rest. Then implement the other nsITheme methods for those two values and ignore the rest. (There's also nsNativeTheme* files in the other backends, if you want to compare. They might be somewhat simpler.) Then add something like this to hook it up: http://searchfox.org/mozilla-central/source/widget/gtk/nsWidgetFactory.cpp#248,285 Drawing is done by DrawWidgetBackground IIRC, which is using the native widget SDK on the respective platform, but I guess Android doesn't have one except in Java? Maybe just leave that empty and implement nsFormControlFrame::BuildDisplayList to do some custom painting on Android when appearance != none? Search for "::Paint(nsDisplayListBuilder" for various examples of that: http://searchfox.org/mozilla-central/search?q=%3A%3APaint(nsDisplayListBuilder Make sure you take care of the various states, focused/active/checked/disabled etc, see for example GetGtkWidgetAndState for how to collect that state.

I can try to take this on, but considering how close we are to uplift time, I don't feel confident I can get this landed and uplifted to 53. How terrible would it be if this bug existed in one or max two releases?

Hmm, good question. It's hard to know the scope of the fallout, since you *should* be using -prefix-appearance: none when trying to modify form elements. But if we feel like we need it to slip I'd be much happier with just a one-release regression, rather than two.

Hey tn, So I've posted a WIP that essentially brings the manual drawing of the checkbox and radio buttons back from the dead (essentially reverting the main patch from bug 418833, but behind ANDROID ifdef's). I haven't gone the route of creating an Android-specific Theme and doing the drawing there. Not 100% sure I need to do that. First of all, given what I'm trying to do (see comment 3 and comment 6), am I going about it the right way? If so, the remaining challenge is, I believe, colouring the box around the checkmark and the circle around the radio dot. Right now I'm using the color attribute, and I'm not sure that's correct. We probably want to use border-color's, and let Fennec's user agent theme CSS define what colour the border around these controls will be. Right? I think I can do that for the checkbox. Reading in the border colors and stroking 4 lines sounds not too bad. The circle around the radio dot though is harder. I've been looking at AppendRoundedRectToPath as a potential solution, but I'm not 100% sure it's what I want or need, and before I learn it, I want to know if I'm even going down the right path. So, uh, does the above make sense, or should I pull the chute and try a different approach?

I suspect this may also be related to webcompat issue #4893, where giving a checkbox or radio width:auto collapses them only on Android, like they have no intrinsic width anymore. A simple test-case: <!DOCTYPE html> <html> <body> With width:auto <input type="checkbox" style="width:auto"> <input type="radio" style="width:auto"> Without width:auto <input type="checkbox"> <input type="radio"> </body> </html>

Talked to mconley on irc. I think he got the information he needs from me.

(In reply to Thomas Wisniewski from comment #14) > I suspect this may also be related to webcompat issue #4893, where giving a > checkbox or radio width:auto collapses them only on Android, like they have > no intrinsic width anymore. A simple test-case: It seems like that is because the size of the radiobutton/checkbox goes to zero. I don't think mconley's patch will change the size of these form controls, only what we paint inside their box. So that sounds like a seperate issue.

Thanks! I've spun off bug 1344395 to deal with it.

Hey Thomas, I know you've been tracking this work since it first landed. Is it appropriate for me to ask if you can see if my latest patch addresses the issue (and doesn't open new web compat problems)? Locally, this patch does the job for me, fwiw. Or is that something I should point at QA?

Try build coming in here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a67bc7d52a22

Comment on attachment 8842630 [details] Bug 1340661 - Manually draw checkbox and radio frames on Android. https://reviewboard.mozilla.org/r/116404/#review119394 lgtm, but definitely should get a layout review.

I can confirm that this patch does make the elements visible again, though they're small and subtle in the testcase attached to the bug (you may have to zoom in to see them). As a bonus, they now also show up in bug 1344395 as well (though still not quite correctly).

Comment on attachment 8842630 [details] Bug 1340661 - Manually draw checkbox and radio frames on Android. https://reviewboard.mozilla.org/r/116404/#review119426 ::: layout/forms/nsGfxCheckboxControlFrame.h:25 (Diff revision 3) > virtual nsresult GetFrameName(nsAString& aResult) const override { > return MakeFrameName(NS_LITERAL_STRING("CheckboxControl"), aResult); > } > #endif > > +#ifdef ANDROID Can we add a comment to both these classes to explain the presence of this Android only code? ::: layout/forms/nsGfxCheckboxControlFrame.cpp:104 (Diff revision 3) > + for (int32_t polyIndex = 1; polyIndex < checkNumPoints; polyIndex++) { > + p = paintCenter + nsPoint(checkPolygonX[polyIndex] * paintScale, > + checkPolygonY[polyIndex] * paintScale); > + builder->LineTo(NSPointToPoint(p, appUnitsPerDevPixel)); > + } > + ColorPattern color(ToDeviceColor(Color(0.21f, 0.23f, 0.25f))); Can we factor the color out into a (static if possible) variable with a descriptive name? And same for the other colors.

Comment on attachment 8842630 [details] Bug 1340661 - Manually draw checkbox and radio frames on Android. https://reviewboard.mozilla.org/r/116404/#review119426 Thanks for the review! Is something like this what you had in mind? Interdiff: https://reviewboard.mozilla.org/r/116404/diff/3-4/

ni? to tn for comment 25.

(In reply to Mike Conley (:mconley) (Catching up on reviews and needinfos) from comment #25) > Thanks for the review! Is something like this what you had in mind? Yes! That's even better than what I had in mind (just a local variables in the files that need it).

Pushed by mconley@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8bd27accc2a0 Manually draw checkbox and radio frames on Android. r=snorp,tnikkel

Backed out for mass-failing on Android at gfx/thebes/gfxPrefs.h:377: https://hg.mozilla.org/integration/autoland/rev/dd92d0734a266dab534af530c8ae35670aedd282 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=8bd27accc2a065e12cd5f91bbfbe901bcffd787e&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=82249484&repo=autoland [task 2017-03-07T20:15:52.705903Z] 20:15:52 INFO - 03-07 12:13:15.245 W/EGL_emulation( 769): eglSurfaceAttrib not implemented [task 2017-03-07T20:15:52.706114Z] 20:15:52 INFO - 03-07 12:13:15.255 D/OpenGLRenderer( 769): Enabling debug mode 0 [task 2017-03-07T20:15:52.706313Z] 20:15:52 INFO - 03-07 12:13:15.335 D/GeckoToolbar( 769): onTabChanged: RESTORED [task 2017-03-07T20:15:52.706549Z] 20:15:52 INFO - 03-07 12:13:15.405 I/Choreographer( 769): Skipped 186 frames! The application may be doing too much work on its main thread. [task 2017-03-07T20:15:52.706760Z] 20:15:52 INFO - 03-07 12:13:16.965 I/ActivityManager( 278): Displayed org.mozilla.fennec/org.mozilla.gecko.BrowserApp: +8s966ms [task 2017-03-07T20:15:52.706941Z] 20:15:52 INFO - 03-07 12:13:16.975 D/AndroidRuntime( 759): Shutting down VM [task 2017-03-07T20:15:52.707163Z] 20:15:52 INFO - 03-07 12:13:17.055 I/Choreographer( 769): Skipped 98 frames! The application may be doing too much work on its main thread. [task 2017-03-07T20:15:52.707453Z] 20:15:52 INFO - 03-07 12:13:38.165 W/GeckoLinker( 769): /data/app/org.mozilla.fennec-1.apk!/assets/armeabi-v7a/libxul.so: unhandled flags #8 not handled [task 2017-03-07T20:15:52.707678Z] 20:15:52 INFO - 03-07 12:13:38.255 W/GeckoLinker( 769): /data/app/org.mozilla.fennec-1.apk!/assets/armeabi-v7a/liblgpllibs.so: unhandled flags #8 not handled [task 2017-03-07T20:15:52.707928Z] 20:15:52 INFO - 03-07 12:13:38.365 I/Gecko ( 769): [769] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /home/worker/workspace/build/src/xpcom/base/nsTraceRefcnt.cpp, line 172 [task 2017-03-07T20:15:52.708153Z] 20:15:52 INFO - 03-07 12:13:38.365 F/MOZ_Assert( 769): Assertion failure: SingletonExists(), at /home/worker/workspace/build/src/gfx/thebes/gfxPrefs.h:377 [task 2017-03-07T20:15:52.708380Z] 20:15:52 INFO - 03-07 12:13:38.365 F/libc ( 769): Fatal signal 11 (SIGSEGV) at 0x00000000 (code=1), thread 788 (Gecko) [task 2017-03-07T20:15:52.708601Z] 20:15:52 INFO - 03-07 12:13:38.477 I/DEBUG ( 35): *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** [task 2017-03-07T20:15:52.708842Z] 20:15:52 INFO - 03-07 12:13:38.477 I/DEBUG ( 35): Build fingerprint: 'generic/sdk/generic:4.3.1/JLS36I/eng.gbrown.20150308.182649:eng/test-keys' [task 2017-03-07T20:15:52.709022Z] 20:15:52 INFO - 03-07 12:13:38.477 I/DEBUG ( 35): Revision: '0' [task 2017-03-07T20:15:52.709277Z] 20:15:52 INFO - 03-07 12:13:38.477 I/DEBUG ( 35): pid: 769, tid: 788, name: UNKNOWN >>> org.mozilla.fennec <<< [task 2017-03-07T20:15:52.709544Z] 20:15:52 INFO - 03-07 12:13:38.477 I/DEBUG ( 35): signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 00000000 [task 2017-03-07T20:15:52.709748Z] 20:15:52 INFO - 03-07 12:13:38.795 I/DEBUG ( 35): r0 00000071 r1 599abaf2 r2 599abaf2 r3 599abaf2 [task 2017-03-07T20:15:52.709954Z] 20:15:52 INFO - 03-07 12:13:38.795 I/DEBUG ( 35): r4 00000000 r5 52968958 r6 52968a3c r7 52c051d7 [task 2017-03-07T20:15:52.710154Z] 20:15:52 INFO - 03-07 12:13:38.805 I/DEBUG ( 35): r8 52c08068 r9 03e55f3c sl 4ec6a0b4 fp 52c05150 [task 2017-03-07T20:15:52.710422Z] 20:15:52 INFO - 03-07 12:13:38.805 I/DEBUG ( 35): ip 00000003 sp 52968928 lr 5a8fdc8b pc 5a90007a cpsr 60000030 [task 2017-03-07T20:15:52.710621Z] 20:15:52 INFO - 03-07 12:13:38.805 I/DEBUG ( 35): d0 0000000000000000 d1 0000000000000000 [task 2017-03-07T20:15:52.710824Z] 20:15:52 INFO - 03-07 12:13:38.805 I/DEBUG ( 35): d2 0000000000000000 d3 7e37e43c8800759c [task 2017-03-07T20:15:52.711026Z] 20:15:52 INFO - 03-07 12:13:38.805 I/DEBUG ( 35): d4 8000000000000000 d5 3f40000042400000 [task 2017-03-07T20:15:52.711264Z] 20:15:52 INFO - 03-07 12:13:38.805 I/DEBUG ( 35): d6 3fc999999999999a d7 0000000000000000 [task 2017-03-07T20:15:52.711518Z] 20:15:52 INFO - 03-07 12:13:38.815 I/DEBUG ( 35): d8 0000000000000000 d9 0000000000000000 [task 2017-03-07T20:15:52.711727Z] 20:15:52 INFO - 03-07 12:13:38.815 I/DEBUG ( 35): d10 0000000000000000 d11 0000000000000000 [task 2017-03-07T20:15:52.711929Z] 20:15:52 INFO - 03-07 12:13:38.815 I/DEBUG ( 35): d12 0000000000000000 d13 0000000000000000 [task 2017-03-07T20:15:52.712140Z] 20:15:52 INFO - 03-07 12:13:38.815 I/DEBUG ( 35): d14 0000000000000000 d15 0000000000000000 [task 2017-03-07T20:15:52.712331Z] 20:15:52 INFO - 03-07 12:13:38.815 I/DEBUG ( 35): scr 60000010 [task 2017-03-07T20:15:52.712516Z] 20:15:52 INFO - 03-07 12:13:38.835 I/DEBUG ( 35): [task 2017-03-07T20:15:52.712704Z] 20:15:52 INFO - 03-07 12:13:38.835 I/DEBUG ( 35): backtrace: [task 2017-03-07T20:15:52.712935Z] 20:15:52 INFO - 03-07 12:13:38.835 I/DEBUG ( 35): #00 pc 00a5f07a /data/data/org.mozilla.fennec/cache/libxul.so [task 2017-03-07T20:15:52.713210Z] 20:15:52 INFO - 03-07 12:13:38.835 I/DEBUG ( 35): #01 pc 00a87d37 /data/data/org.mozilla.fennec/cache/libxul.so [task 2017-03-07T20:15:52.713430Z] 20:15:52 INFO - 03-07 12:13:38.835 I/DEBUG ( 35): #02 pc 017c9167 /data/data/org.mozilla.fennec/cache/libxul.so [task 2017-03-07T20:15:52.713645Z] 20:15:52 INFO - 03-07 12:13:38.835 I/DEBUG ( 35): #03 pc 0001c7e5 /data/app-lib/org.mozilla.fennec-1/libmozglue.so [task 2017-03-07T20:15:52.713865Z] 20:15:52 INFO - 03-07 12:13:38.835 I/DEBUG ( 35): #04 pc 0001d725 /data/app-lib/org.mozilla.fennec-1/libmozglue.so [task 2017-03-07T20:15:52.714074Z] 20:15:52 INFO - 03-07 12:13:38.845 I/DEBUG ( 35): #05 pc 0001f6e7 /data/app-lib/org.mozilla.fennec-1/libmozglue.so [task 2017-03-07T20:15:52.714311Z] 20:15:52 INFO - 03-07 12:13:38.845 I/DEBUG ( 35): #06 pc 0001f7d1 /data/app-lib/org.mozilla.fennec-1/libmozglue.so [task 2017-03-07T20:15:52.714527Z] 20:15:52 INFO - 03-07 12:13:38.845 I/DEBUG ( 35): #07 pc 00032e27 /data/app-lib/org.mozilla.fennec-1/libmozglue.so [task 2017-03-07T20:15:52.714739Z] 20:15:52 INFO - 03-07 12:13:38.845 I/DEBUG ( 35): #08 pc 00025489 /data/app-lib/org.mozilla.fennec-1/libmozglue.so [task 2017-03-07T20:15:52.714990Z] 20:15:52 INFO - 03-07 12:13:38.845 I/DEBUG ( 35): #09 pc 000258e9 /data/app-lib/org.mozilla.fennec-1/libmozglue.so (Java_org_mozilla_gecko_mozglue_GeckoLoader_loadGeckoLibsNative+32) [task 2017-03-07T20:15:52.715208Z] 20:15:52 INFO - 03-07 12:13:38.845 I/DEBUG ( 35): #10 pc 0001dc4c /system/lib/libdvm.so (dvmPlatformInvoke+112) [task 2017-03-07T20:15:52.715450Z] 20:15:52 INFO - 03-07 12:13:38.855 I/DEBUG ( 35): #11 pc 0004dcab /system/lib/libdvm.so (dvmCallJNIMethod(unsigned int const*, JValue*, Method const*, Thread*)+394) [task 2017-03-07T20:15:52.715680Z] 20:15:52 INFO - 03-07 12:13:38.855 I/DEBUG ( 35): #12 pc 000385e1 /system/lib/libdvm.so (dvmCheckCallJNIMethod(unsigned int const*, JValue*, Method const*, Thread*)+8) [task 2017-03-07T20:15:52.715915Z] 20:15:52 INFO - 03-07 12:13:38.855 I/DEBUG ( 35): #13 pc 0004f699 /system/lib/libdvm.so (dvmResolveNativeMethod(unsigned int const*, JValue*, Method const*, Thread*)+184) [task 2017-03-07T20:15:52.716119Z] 20:15:52 INFO - 03-07 12:13:38.855 I/DEBUG ( 35): #14 pc 00027060 /system/lib/libdvm.so [task 2017-03-07T20:15:52.716369Z] 20:15:52 INFO - 03-07 12:13:38.855 I/DEBUG ( 35): #15 pc 0002b580 /system/lib/libdvm.so (dvmInterpret(Thread*, Method const*, JValue*)+184) [task 2017-03-07T20:15:52.716615Z] 20:15:52 INFO - 03-07 12:13:38.855 I/DEBUG ( 35): #16 pc 0005fcbd /system/lib/libdvm.so (dvmCallMethodV(Thread*, Method const*, Object*, bool, JValue*, std::__va_list)+292) [task 2017-03-07T20:15:52.716838Z] 20:15:52 INFO - 03-07 12:13:38.865 I/DEBUG ( 35): #17 pc 0005fce7 /system/lib/libdvm.so (dvmCallMethod(Thread*, Method const*, Object*, JValue*, ...)+20) [task 2017-03-07T20:15:52.717025Z] 20:15:52 INFO - 03-07 12:13:38.865 I/DEBUG ( 35): #18 pc 00054a6f /system/lib/libdvm.so [task 2017-03-07T20:15:52.717247Z] 20:15:52 INFO - 03-07 12:13:38.865 I/DEBUG ( 35): #19 pc 0000ca58 /system/lib/libc.so (__thread_entry+72) [task 2017-03-07T20:15:52.717508Z] 20:15:52 INFO - 03-07 12:13:38.865 I/DEBUG ( 35): #20 pc 0000cbd4 /system/lib/libc.so (pthread_create+208)

Pushed by mconley@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/00daf2c5342a Manually draw checkbox and radio frames on Android. r=snorp,tnikkel

Backed out in https://hg.mozilla.org/integration/autoland/rev/5c13b85e8b07 for https://treeherder.mozilla.org/logviewer.html#?job_id=82283428&repo=autoland

(In reply to Mike Conley (:mconley) (Catching up on reviews and needinfos) from comment #12) > I haven't gone the route of creating an Android-specific Theme and doing the > drawing there. Not 100% sure I need to do that. This is a good start, but I don't think it's enough to be web-compatible. We really need to add a primitive Android nsITheme that supports checkbox/radio controls. The reason is that when authors specify (-prefix-)appearance:none it's expected to trigger all sorts of special layout behavior (creating a zero-sized inline box that paints nothing except what the author specified). So we really can't have -moz-appearance:none in the UA style sheet like we have now, it should be -moz-appearance:checkbox/radio. I suppose that can be done as a follow-up bug though. It should remove the #ifdef Android in these files (except the painting stuff you're adding here) and in: http://searchfox.org/mozilla-central/rev/d4adaabd6d2f88be0d0b151e022c06f6554909da/layout/base/nsCSSFrameConstructor.cpp#3726 and then remove most of the style in layout/style/res/forms.css and mobile/android/themes/core/content.css for Android checkbox/radio controls (as we did in bug 605985 for other platforms). We should then be able to remove some of the "fails-if(Android)" in: layout/reftests/forms/input/checkbox/reftest.list layout/reftests/forms/input/radio/reftest.list

Okay, my patch has bounced twice for reftest failures, and I'm starting to think that I'm going to need to go down the nsITheme path again as mats suggested, which... is going to take a little work. Considering that this regression is now on beta, I'm going into risk-mitigation mode on this one. Hey snorp - in bug 1320732, I removed the !important from the border stroke so that web authors could style the checkboxes on Fennec. I propose that we back that change out on beta and aurora until we get this one fixed. This should revert us to currently shipping behaviour. Does that sound reasonable to you? I'd ask miketaylr to weigh in too, but he's on PTO until the 19th...

> Does that sound reasonable to you? I'd ask miketaylr to weigh in too, but he's on PTO until the 19th... (My PTO starts at noon tomorrow, so here comes some weight...) I think that's the safest way forward, in terms of not breaking sites for Fennec users. Also, it sucks to have to back this out, but it one or two releases isn't that long in web-scale units™. Also, thanks a ton for the work you've done so far to try to get this fixed!

(In reply to Mike Conley (:mconley) (Catching up on reviews and needinfos) from comment #34) > Okay, my patch has bounced twice for reftest failures, and I'm starting to > think that I'm going to need to go down the nsITheme path again as mats > suggested, which... is going to take a little work. Did you look at the latest reftest failure? It's just a fuzz difference that looks the same to the eye. And the test is already fuzzed. Increasing the fuzz limit seems reasonable here.

(In reply to Timothy Nikkel (:tnikkel) from comment #36) > Did you look at the latest reftest failure? It's just a fuzz difference that > looks the same to the eye. And the test is already fuzzed. Increasing the > fuzz limit seems reasonable here. Yeah, I looked at it. Part of the reason it's failing is because my patch draws another stroke within the defined border of the radio and checkbox. I think there might be some blending going on there that's causing the reference and test images to be slightly different. I also don't think it's worth it, at this point, to try to get this uplifted to beta (which is now affected). I'm going to go the same course and request a back out of bug 1320732 on all channels until we can get this fixed properly.

> I also don't think it's worth it, at this point, to try to get this uplifted > to beta (which is now affected). I'm going to go the same course and request > a back out of bug 1320732 on all channels until we can get this fixed > properly. So if that's backed out, this is no longer a regression. Up to you if it should be duped or closed or kept open. Backed out of 53 and 54 in https://bugzilla.mozilla.org/show_bug.cgi?id=1345956#c7

one case with height: auto https://webcompat.com/issues/5115

(In reply to Karl Dubost :karlcow from comment #39) > one case with height: auto > https://webcompat.com/issues/5115 That sounds more like bug 1344395 than this bug.

(I filed bug 1352238 on implementing a native Android theme for checkbox/radio)