Closed Bug 1443143 Opened 3 years ago Closed 2 years ago

Date picker needs tweaking with arrow panel theming

Categories

(Toolkit :: Themes, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: ntim, Assigned: vivek3zero)

References

Details

Attachments

(2 files)

No description provided.
Blocks: 1408121
Assignee: nobody → ntim.bugs
Assignee: ntim.bugs → nobody
Assignee: nobody → vivek3zero
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;
Attachment #8961025 - Flags: review?(jaws) → review-
Comment on attachment 8961025 [details]
Bug 1443143 - Date picker needs tweaking with arrow panel theming

https://reviewboard.mozilla.org/r/229746/#review235640
Attachment #8961025 - Flags: review+
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c502e0be067e
Date picker needs tweaking with arrow panel theming r=jaws
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.
https://hg.mozilla.org/mozilla-central/rev/c502e0be067e
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
This patch looks rather wrong to me. Has somebody tested this with, say, dark high contrast themes?
Flags: needinfo?(vivek3zero)
Flags: needinfo?(jaws)
(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.
(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.
Flags: needinfo?(vivek3zero)
Flags: needinfo?(jaws)
This is actually on file already at bug 1317581.
You need to log in before you can comment on or make changes to this bug.