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)
Tracking
(firefox54-, firefox55 wontfix, firefox56+ wontfix, firefox57 fixed)
RESOLVED
FIXED
mozilla57
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).
Reporter | ||
Comment 1•7 years ago
|
||
[Tracking Requested - why for this release]: need this for web compatibility
tracking-firefox54:
--- → ?
Comment 2•7 years ago
|
||
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)
Comment 3•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment 5•7 years ago
|
||
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)
Reporter | ||
Comment 6•7 years ago
|
||
> 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...
Reporter | ||
Comment 7•7 years ago
|
||
Comment 8•7 years ago
|
||
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.
Reporter | ||
Comment 9•7 years ago
|
||
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.
Reporter | ||
Comment 10•7 years ago
|
||
(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.
Comment 11•7 years ago
|
||
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)
Reporter | ||
Comment 12•7 years ago
|
||
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)
Comment 13•7 years ago
|
||
(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)
Reporter | ||
Comment 14•7 years ago
|
||
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)
Comment 15•7 years ago
|
||
(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)
Reporter | ||
Comment 16•7 years ago
|
||
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)
Comment 17•7 years ago
|
||
(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)
Comment 18•7 years ago
|
||
(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?
Reporter | ||
Comment 19•7 years ago
|
||
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)
Comment 20•7 years ago
|
||
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)
Comment 22•7 years ago
|
||
(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)
Comment 23•7 years ago
|
||
(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)
Comment 24•7 years ago
|
||
(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
Comment 25•7 years ago
|
||
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.
See Also: → https://webcompat.com/issues/6195
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/6194
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/6254
Updated•7 years ago
|
Comment 26•7 years ago
|
||
Track 54- as we are in the beta cycle and it's likely we won't take this in 54.
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/6412
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/6464
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/6577
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/6576
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/6725
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/6731
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/6755
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/6791
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/6806
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/6835
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/6912
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/6973
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/7070
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/7300
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/7425
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/7429
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/7445
[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.
tracking-firefox56:
--- → ?
status-firefox56:
--- → affected
Updated•7 years ago
|
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/7565
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/7514
Comment 28•7 years ago
|
||
Louis, let's work it out based on the WIP patch from Mike.
Assignee: nobody → lochang
Status: NEW → ASSIGNED
Whiteboard: [webcompat]
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/7587
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/7803
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/7749
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/7795
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/7797
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/7869
Assignee | ||
Comment 29•7 years ago
|
||
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)
Reporter | ||
Comment 30•7 years ago
|
||
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)
Assignee | ||
Comment 31•7 years ago
|
||
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)
Reporter | ||
Comment 32•7 years ago
|
||
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)
Reporter | ||
Comment 33•7 years ago
|
||
FYI, this is the frame code for radio controls: http://searchfox.org/mozilla-central/source/layout/forms/nsGfxRadioControlFrame.cpp#74
Assignee | ||
Comment 34•7 years ago
|
||
(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!
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/8117
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8884137 -
Attachment is obsolete: true
Assignee | ||
Comment 38•7 years ago
|
||
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)
Assignee | ||
Comment 39•7 years ago
|
||
checkbox and check mark painted with native theme on fennec.
Comment 40•7 years ago
|
||
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+
Assignee | ||
Comment 41•7 years ago
|
||
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)
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/7002
Updated•7 years ago
|
Flags: needinfo?(snorp)
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/8443
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/8011
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/8009
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/8005
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/7832
Comment 42•7 years ago
|
||
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&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.
See Also: → https://webcompat.com/issues/8451
Comment 43•7 years ago
|
||
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)
Comment 44•7 years ago
|
||
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
Comment 45•7 years ago
|
||
(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 47•7 years ago
|
||
mozreview-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/#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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8886067 -
Flags: review?(mats)
Assignee | ||
Updated•7 years ago
|
Attachment #8886068 -
Flags: review?(mats)
Assignee | ||
Updated•7 years ago
|
Attachment #8886069 -
Flags: review?(mats)
Assignee | ||
Comment 52•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=67250a1ea228700f969b10010462fdfff0bab889
Assignee | ||
Comment 53•7 years ago
|
||
(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
Assignee | ||
Comment 54•7 years ago
|
||
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
Assignee | ||
Comment 55•7 years ago
|
||
(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?
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 60•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=685ee6bf0463a43d3acfa85dc64d8333f4cd0e72
Assignee | ||
Updated•7 years ago
|
Attachment #8886069 -
Flags: review?(mats)
Assignee | ||
Updated•7 years ago
|
Attachment #8886068 -
Flags: review?(mats)
Assignee | ||
Updated•7 years ago
|
Attachment #8886067 -
Flags: review?(mats)
Assignee | ||
Comment 61•7 years ago
|
||
(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.
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/8599
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/8618
Updated•7 years ago
|
Assignee | ||
Comment 62•7 years ago
|
||
(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.
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/6982
Reporter | ||
Comment 63•7 years ago
|
||
mozreview-review |
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+
Reporter | ||
Comment 64•7 years ago
|
||
mozreview-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-
Reporter | ||
Comment 65•7 years ago
|
||
mozreview-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-
Reporter | ||
Comment 66•7 years ago
|
||
The review comments hopefully answers the pending needinfo questions. Please ask again if not.
Flags: needinfo?(mats)
Assignee | ||
Comment 67•7 years ago
|
||
mozreview-review-reply |
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?
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
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.
Assignee | ||
Updated•7 years ago
|
Attachment #8886086 -
Attachment is obsolete: true
Assignee | ||
Comment 74•7 years ago
|
||
mozreview-review-reply |
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
Reporter | ||
Comment 75•7 years ago
|
||
(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.
Reporter | ||
Comment 76•7 years ago
|
||
mozreview-review |
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-
Reporter | ||
Comment 77•7 years ago
|
||
(Quoted the wrong word above, I meant: s/aRect 'here'/'aRect' here/)
Reporter | ||
Comment 78•7 years ago
|
||
mozreview-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/#review176992 LGTM. r=mats
Attachment #8886068 -
Flags: review?(mats) → review+
Reporter | ||
Comment 79•7 years ago
|
||
(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.
Reporter | ||
Comment 80•7 years ago
|
||
mozreview-review |
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+
Reporter | ||
Comment 81•7 years ago
|
||
mozreview-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+
Reporter | ||
Comment 82•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 83•7 years ago
|
||
(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?
Assignee | ||
Comment 84•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fec580794aacb573ec1472d8e21d13f4ff7a247b
Assignee | ||
Comment 85•7 years ago
|
||
(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)
Reporter | ||
Comment 86•7 years ago
|
||
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)
Assignee | ||
Comment 87•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b271503a3a49467a6a99153ae0c94b9901ec09da
Assignee | ||
Comment 88•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c73190db69233e329a9c5899a29065ace03ce0f
Assignee | ||
Comment 89•7 years ago
|
||
(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)
Reporter | ||
Comment 90•7 years ago
|
||
> 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)
Assignee | ||
Comment 91•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c4c540f63839f3f06401d44ef4b855202a06808
Assignee | ||
Comment 92•7 years ago
|
||
try result without any pathces: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c465a7aaa04b02456de72dcda7af100288c2bff
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 99•7 years ago
|
||
(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
Reporter | ||
Comment 100•7 years ago
|
||
> 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.
Reporter | ||
Comment 101•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 102•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e73242ae20643bac51fd9f722e10b65416fdc33
Assignee | ||
Comment 103•7 years ago
|
||
(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)
Reporter | ||
Comment 104•7 years ago
|
||
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)
Reporter | ||
Comment 105•7 years ago
|
||
> 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)
Reporter | ||
Comment 106•7 years ago
|
||
(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.
Assignee | ||
Comment 107•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cdd06cee08c529a91bad9e7ef9db377e1abfab0f
Assignee | ||
Comment 108•7 years ago
|
||
(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)
Reporter | ||
Comment 109•7 years ago
|
||
(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)
Assignee | ||
Comment 110•7 years ago
|
||
(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.
Assignee | ||
Comment 111•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2edbf020a1c3510f38c07bbf6908940ff185775
Assignee | ||
Comment 112•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0de6b8f89a9a05f2e282674e0fca728997ac87d8
Assignee | ||
Comment 113•7 years ago
|
||
(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)
Reporter | ||
Comment 114•7 years ago
|
||
(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)
Comment 115•7 years ago
|
||
(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)
Assignee | ||
Comment 116•7 years ago
|
||
Assignee | ||
Comment 117•7 years ago
|
||
Assignee | ||
Comment 118•7 years ago
|
||
(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)
Reporter | ||
Comment 119•7 years ago
|
||
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?
Assignee | ||
Comment 120•7 years ago
|
||
(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?
Assignee | ||
Comment 121•7 years ago
|
||
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
Assignee | ||
Comment 122•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb336ad6e74b95e1d977b8d158facc9e10a629d9
Reporter | ||
Comment 123•7 years ago
|
||
(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.
Reporter | ||
Comment 124•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 132•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=46d18068c8003a173a5bf963cac5daa7c7b9e7a5
Assignee | ||
Comment 133•7 years ago
|
||
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
Reporter | ||
Comment 134•7 years ago
|
||
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)
Assignee | ||
Comment 135•7 years ago
|
||
(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.
Assignee | ||
Comment 136•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9accee967431ecc5edae96625f642ab9ad2f808
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8903932 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8903933 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8904141 -
Attachment is obsolete: true
Assignee | ||
Comment 140•7 years ago
|
||
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
Reporter | ||
Comment 141•7 years ago
|
||
mozreview-review |
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+
Reporter | ||
Comment 142•7 years ago
|
||
mozreview-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+
Reporter | ||
Comment 143•7 years ago
|
||
Nice work Louis!
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 152•7 years ago
|
||
Rebase patches to the latest tree. Thanks for your patience and kindly review!
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 153•7 years ago
|
||
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
Comment 154•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c6e7fb7bfc1f https://hg.mozilla.org/mozilla-central/rev/c60cee53a363 https://hg.mozilla.org/mozilla-central/rev/d0e60b3b1b58 https://hg.mozilla.org/mozilla-central/rev/68cd9f54d2b1 https://hg.mozilla.org/mozilla-central/rev/a6b21855bb8f https://hg.mozilla.org/mozilla-central/rev/31d1a1111f91 https://hg.mozilla.org/mozilla-central/rev/f655236b4f52 https://hg.mozilla.org/mozilla-central/rev/bd7c2148cc80
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 155•7 years ago
|
||
I strongly doubt we want to backport this to Beta for 56 still.
status-firefox55:
--- → wontfix
Comment 157•7 years ago
|
||
(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.
Updated•7 years ago
|
Updated•7 years ago
|
Updated•6 years ago
|
See Also: → https://webcompat.com/issues/7225
Updated•6 years ago
|
See Also: → https://webcompat.com/issues/7283
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•