[DateTimePicker] Add arrows svg file and style month-year button for date picker

RESOLVED FIXED in Firefox 53

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: scottwu, Assigned: scottwu)

Tracking

(Blocks: 1 bug)

unspecified
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [milestone4])

Attachments

(5 attachments)

Comment hidden (empty)
Comment hidden (mozreview-request)
(Assignee)

Comment 2

2 years ago
mozreview-review
Comment on attachment 8822004 [details]
Bug 1325922 - Add arrows svg file and style month-year button for date picker

https://reviewboard.mozilla.org/r/101076/#review102170

::: toolkit/content/widgets/datepicker.js:177
(Diff revision 1)
>      /**
>       * Attach event listeners
>       */
>      _attachEventListeners() {
>        window.addEventListener("message", this);
> -      document.addEventListener("click", this);
> +      document.addEventListener("mouseup", this, { passive: true });

Switched to listening to mouseup/mousedown because I need to style the active state when buttons are pressed, but can't use :active selector because the focus always stays on the input box.
Attachment #8822004 - Flags: review?(scwwu)
(Assignee)

Updated

2 years ago
Attachment #8822004 - Flags: review?(mconley)
(Assignee)

Updated

2 years ago
Attachment #8822004 - Flags: review?(scwwu)
Whiteboard: [milestone4]
(Assignee)

Comment 3

2 years ago
This patch aligns the top navigation and month/year button with the visual spec. It's pretty straight forward, hope you could take a look. Thanks Mike!
Flags: needinfo?(mconley)
Sure, will look.
Flags: needinfo?(mconley)

Comment 5

2 years ago
mozreview-review
Comment on attachment 8822004 [details]
Bug 1325922 - Add arrows svg file and style month-year button for date picker

https://reviewboard.mozilla.org/r/101076/#review102540

This looks okay to me - just confused about part of how this works with the "active" class state. See below.

::: toolkit/content/widgets/datepicker.js:178
(Diff revision 1)
>       * Attach event listeners
>       */
>      _attachEventListeners() {
>        window.addEventListener("message", this);
> -      document.addEventListener("click", this);
> +      document.addEventListener("mouseup", this, { passive: true });
> +      document.addEventListener("mousedown", this);

Should this one be passive too? Or might we preventDefault somewhere?

::: toolkit/content/widgets/datepicker.js:205
(Diff revision 1)
> +          if (event.target == this.context.buttonLeft || event.target == this.context.buttonRight) {
> +            event.target.classList.remove("active");
> +          }

It seems to me like we could get stuck with this active state on the target if we click, hold, drag away from the target, and mouseup outside of the target. And yet I can't seem to cause that bug to occur manually. Do you know how that's working? Is it because of setProps clearing the class?

::: toolkit/themes/shared/datetimeinputpickers.css:100
(Diff revision 1)
>    z-index: 10;
>  }
>  
> +button.month-year {
> +  font-size: 1.3rem;
> +  border: 0.1rem solid #D6D6D6;

Might be worth extracting this colour out to a CSS variable.

::: toolkit/themes/shared/datetimeinputpickers.css:106
(Diff revision 1)
> +  border-radius: 0.3rem;
> +  padding: 0.2rem 2.6rem 0.2rem 1.2rem;
> +}
> +
> +button.month-year:hover {
> +  background: #EBEBEB;

Might be worth extracting this colour out to a CSS variable.

::: toolkit/themes/shared/datetimeinputpickers.css:110
(Diff revision 1)
> +  border-color: #B1B1B1;
> +  background: #D4D4D4;

Might be worth extracting these colours out to some CSS variables.
Attachment #8822004 - Flags: review?(mconley) → review+
(Assignee)

Comment 6

2 years ago
mozreview-review-reply
Comment on attachment 8822004 [details]
Bug 1325922 - Add arrows svg file and style month-year button for date picker

https://reviewboard.mozilla.org/r/101076/#review102540

> Should this one be passive too? Or might we preventDefault somewhere?

I'm adding a preventDefault here. See issue below.

> It seems to me like we could get stuck with this active state on the target if we click, hold, drag away from the target, and mouseup outside of the target. And yet I can't seem to cause that bug to occur manually. Do you know how that's working? Is it because of setProps clearing the class?

Yes you are right that it shouldn't have worked correctly. The reason it works is because the spinner component listens to the document and does a preventDefault and setCapture there. But this is weird so I'm moving the preventDefault and setCapture to the picker level (timepicker/datepicker) instead of at the spinner level.
Comment hidden (mozreview-request)
(Assignee)

Comment 8

2 years ago
I'll go ahead and have it checked-in. Thanks Mike!
Keywords: checkin-needed

Comment 9

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/daeecaee4447
Add arrows svg file and style month-year button for date picker r=mconley
Keywords: checkin-needed

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/daeecaee4447
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Assignee)

Comment 11

2 years ago
Roxana, did you have a question for me about verification for this bug?
Flags: needinfo?(roxana.leitan)
Tested on the latest Nightly 55.0a1(2017-03-16) on Windows, Mac OS X and Ubuntu.

The arrows and style for month/year section from the top of the date picker and for month/year button are different from the UX specs V 1.52:https://mozilla.invisionapp.com/share/GU7VBIB4D. (please see attached photos)
Flags: needinfo?(roxana.leitan)
Created attachment 8848458 [details]
arrows date picker Nightly 55.png
Created attachment 8848460 [details]
arrows date picker V 1.52.png
Created attachment 8848461 [details]
arrows month year Nightly 55.png
Created attachment 8848462 [details]
arrows month year V 1.52.png
The most accurate visual element should refer to the visual spec:
https://bugzilla.mozilla.org/attachment.cgi?id=8793705 available on Bug 1069609
You need to log in before you can comment on or make changes to this bug.