Closed Bug 1325922 Opened 5 years ago Closed 5 years ago

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

Categories

(Toolkit :: XUL Widgets, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: scottwu, Assigned: scottwu)

References

(Blocks 1 open bug)

Details

(Whiteboard: [milestone4])

Attachments

(5 files)

No description provided.
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)
Attachment #8822004 - Flags: review?(mconley)
Attachment #8822004 - Flags: review?(scwwu)
Whiteboard: [milestone4]
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 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+
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.
I'll go ahead and have it checked-in. Thanks Mike!
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/daeecaee4447
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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)
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.