1443143 - Date picker needs tweaking with arrow panel theming

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Description

[Tim Nguyen :ntim]

Attachment: Screen Shot 2018-03-05 at 2.27.49 PM.png

Comment 1

[Vivek]

Attachment: Bug 1443143 - Date picker needs tweaking with arrow panel theming

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

Comment 2

[Vivek]

Comment on attachment 8961025 [details]
Bug 1443143 - Date picker needs tweaking with arrow panel theming

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

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8961025 [details]
Bug 1443143 - Date picker needs tweaking with arrow panel theming

https://reviewboard.mozilla.org/r/229746/#review235614

This is mostly fine. I thought about asking for you to include :-moz-lwtheme in the selector but I don't see the benefit of having potentially different styling for the datepicker whether a theme is enabled or not, especially if we're not using the theme colors here.

Also, this should be moved to /browser/themes/shared/browser.inc.css since it is theming (style/skin/color) related and not functional.

::: browser/base/content/browser.css:706
(Diff revision 2)
>  
>  #DateTimePickerPanel[active="true"] {
>    -moz-binding: url("chrome://global/content/bindings/datetimepopup.xml#datetime-popup");
>  }
>  
> +/*Force background for datepicker popup to white so themes don't override it*/

Please put a space after /* and before */

::: browser/base/content/browser.css:709
(Diff revision 2)
>  }
>  
> +/*Force background for datepicker popup to white so themes don't override it*/
> +#DateTimePickerPanel[active="true"] > .panel-arrowcontainer > .panel-arrowbox,
> +#DateTimePickerPanel[active="true"] > .panel-arrowcontainer > .panel-arrowcontent {
> +  --arrowpanel-background: #FFFFFF;

--arrowpanel-background: #fff;

Comment 4

[Vivek]

Comment on attachment 8961025 [details]
Bug 1443143 - Date picker needs tweaking with arrow panel theming

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

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8961025 [details]
Bug 1443143 - Date picker needs tweaking with arrow panel theming

https://reviewboard.mozilla.org/r/229746/#review235640

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c502e0be067e
Date picker needs tweaking with arrow panel theming r=jaws

Comment 7

[Tim Nguyen :ntim]

Comment on attachment 8961025 [details]
Bug 1443143 - Date picker needs tweaking with arrow panel theming

https://reviewboard.mozilla.org/r/229746/#review235708

::: browser/themes/shared/browser.inc.css:135
(Diff revision 3)
> +#DateTimePickerPanel[active="true"] > .panel-arrowcontainer > .panel-arrowbox,
> +#DateTimePickerPanel[active="true"] > .panel-arrowcontainer > .panel-arrowcontent {

It would have been better to just use `#DateTimePickerPanel` as selector.

Comment 8

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/c502e0be067e

Comment 9

[Dão Gottwald [:dao]]

This patch looks rather wrong to me. Has somebody tested this with, say, dark high contrast themes?

Comment 10

[Tim Nguyen :ntim]

(In reply to Dão Gottwald [::dao] from comment #9)
> This patch looks rather wrong to me. Has somebody tested this with, say,
> dark high contrast themes?

Considering the date picker panel only uses hardcoded colors (not platform colors), a white background should be appropriate.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Dão Gottwald [::dao] from comment #9)
> This patch looks rather wrong to me. Has somebody tested this with, say,
> dark high contrast themes?

The date picker never handled dark high contrast themes well.

Screenshots with the patch reverted, no lightweight theme applied, and dark high contrast theme enabled:
Opened popup: https://www.screencast.com/t/qki4wHmh2l
Month/year picker opened: https://www.screencast.com/t/av4KtQHH5

Screenshots with the patch applied, no lightweight theme applied, and dark high contrast theme enabled:
Opened popup: https://www.screencast.com/t/xbKagw5DHc3Y
Month/year picker opened: https://www.screencast.com/t/QVy8WiZ0

Screenshots with the patch applied, dark lightweight theme applied, and dark high contrast theme enabled:
Opened popup: https://www.screencast.com/t/pixkMccSApyj
Month/year picker opened: https://www.screencast.com/t/MDRnyrgM2pv

I don't think this warrants us backing this patch out, but I think we should file a new bug to get this fixed for high contrast themes. The picker uses three colors (and shades within): red, white, and black. I'm not sure what system color we can use to replace the red, though as a hack we could use -moz-activehyperlinktext to get a red color (by default) on non-high contrast mode and then when high contrast mode is enabled the color will use the high contrast text color.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

This is actually on file already at bug 1317581.