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

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Comment 1

[Scott Wu [:scottwu]]

Attachment: Bug 1325922 - Add arrows svg file and style month-year button for date picker

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

Comment 2

[Scott Wu [:scottwu]]

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.

Comment 3

[Scott Wu [:scottwu]]

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!

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Sure, will look.

Comment 5

[Mike Conley (: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.

Comment 6

[Scott Wu [:scottwu]]

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 7

[Scott Wu [:scottwu]]

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

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

Comment 8

[Scott Wu [:scottwu]]

I'll go ahead and have it checked-in. Thanks Mike!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/daeecaee4447
Add arrows svg file and style month-year button for date picker r=mconley

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 11

[Scott Wu [:scottwu]]

Roxana, did you have a question for me about verification for this bug?

Comment 12

[roxleitan]

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)

Comment 13

[roxleitan]

Attachment: arrows date picker Nightly 55.png

Comment 14

[roxleitan]

Attachment: arrows date picker V 1.52.png

Comment 15

[roxleitan]

Attachment: arrows month year Nightly 55.png

Comment 16

[roxleitan]

Attachment: arrows month year V 1.52.png

Comment 17

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

The most accurate visual element should refer to the visual spec:
https://bugzilla.mozilla.org/attachment.cgi?id=8793705 available on Bug 1069609