Closed Bug 1301284 Opened 8 years ago Closed 8 years ago

Update picker style to match the visual spec for <input type="time">

Categories

(Toolkit :: Themes, defect)

Product:
Toolkit
Component:
Themes
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: scottwu, Assigned: scottwu)

References

(Blocks 1 open bug)

Details

(Whiteboard: [milestone4])

Attachments

(1 file)

Bug 1301284 - Update time picker style to match visual spec;
58 bytes, text/x-review-board-request
mconley
: review+
Details 
      No description provided.
Hello Mike,

It's me again! I know you are super swamped with review requests and ni, so if you know anyone else who might be able to help me review this patch, please let me know. The good news is this is a much smaller patch than bug 1283384.

This bug aligns the style of time picker with the visual spec: https://bugzilla.mozilla.org/show_bug.cgi?id=1069609#c60.

What's in this patch:
- Style is adjusted to match visual spec
- Changes units to em and rem so it could be resized in bug 1310898
- Spinner height and last child margin-bottom are set dynamically so that viewport size could be customized
- Hide highlight and hover state when spinner is spinning
- Emulates button:active style by adding an "active" class on button when clicked

Thanks!
Flags: needinfo?(mconley)
Comment on attachment 8801662 [details]
Bug 1301284 - Update time picker style to match visual spec;

https://reviewboard.mozilla.org/r/86340/#review88522

Thanks scottwu, see below.

::: toolkit/content/widgets/datetimepopup.xml:27
(Diff revision 4)
>          <parameter name="detail"/>
>          <body><![CDATA[
>            this.hidden = false;
>            this.type = type;
>            this.pickerState = {};
> +          this.style.fontSize = "10px";

This looks like it should probably be set via CSS. Why is it necessary for this to be scripted?

::: toolkit/content/widgets/spinner.js:74
(Diff revision 4)
>          up: spinnerElement.querySelector(".up"),
>          down: spinnerElement.querySelector(".down"),
>          itemsViewElements: []
>        };
>  
> +      this.elements.spinner.style.height = (ITEM_HEIGHT_PX * viewportSize) / 10 + "rem";

As an aside, we should definitely work to find all of the hard-coded dimensions that we're establishing them, and move as many to CSS as possible.

::: toolkit/content/widgets/spinner.js:76
(Diff revision 4)
> +      if (hideButtons) {
> +        this.elements.up.style.visibility = "hidden";
> +        this.elements.down.style.visibility = "hidden";
> +      }

This is another thing that'd probably be better to manage with CSS. Instead of setting the styles directly, it'd probably be better to set a class or attribute on the spinner-container, and then use CSS to hide the buttons.

::: toolkit/content/widgets/spinner.js:223
(Diff revision 4)
> -        for (let i = 0; i < count; i++) {
> +        // Remove margin bottom on the last element before appending
> +        if (parent.lastChild) {
> +          parent.lastChild.style.marginBottom = "";
> +        }

Another thing that can / should be done with CSS via the :last-child pseudoselector: https://developer.mozilla.org/en-US/docs/Web/CSS/:last-child

::: toolkit/content/widgets/spinner.js:228
(Diff revision 4)
> -        for (let i = 0; i < count; i++) {
> +        // Remove margin bottom on the last element before appending
> +        if (parent.lastChild) {
> +          parent.lastChild.style.marginBottom = "";
> +        }
> +
> +        for (let i = 0; i < Math.abs(diff); i++) {

We know that diff is positive (line 219). Why is Math.abs necessary?

::: toolkit/content/widgets/spinner.js:242
(Diff revision 4)
> +      parent.lastChild.style.marginBottom =
> +        ITEM_HEIGHT_PX * this.props.viewportTopOffset + ITEM_SPACING_PX + "px";

Another thing we should try to avoid scripting if we can.
Attachment #8801662 - Flags: review-
Flags: needinfo?(mconley)
Comment on attachment 8801662 [details]
Bug 1301284 - Update time picker style to match visual spec;

https://reviewboard.mozilla.org/r/86340/#review88522

Hello Mike, I've replied to your concerns under each issue, and fixed some of the issues. Again, thanks a lot for your help!

> This looks like it should probably be set via CSS. Why is it necessary for this to be scripted?

I'm setting the font size here because I plan on supporting zooming in/out in the future. The picker should be bigger/smaller depending on the content zoom level, as they do for other widgets like combo boxes.

> As an aside, we should definitely work to find all of the hard-coded dimensions that we're establishing them, and move as many to CSS as possible.

The reason I am setting the height of the spinner here is because the viewport size is configurable. Here the time spinner shows 7 items, but in date picker the spinner will only show 5 items. I could create specific CSS rules for both 5 and 7 items, but then the spinner would only work for the pre-defined cases.

> This is another thing that'd probably be better to manage with CSS. Instead of setting the styles directly, it'd probably be better to set a class or attribute on the spinner-container, and then use CSS to hide the buttons.

Good idea, thanks!

> Another thing that can / should be done with CSS via the :last-child pseudoselector: https://developer.mozilla.org/en-US/docs/Web/CSS/:last-child

Here the margin bottom is actually dependent on the viewport size, which is configurable. The more number of items the spinner shows, the bigger marginBottom is needed for the last item. Ideally I would use last-child but I don't think it's possible in this scenario.

> We know that diff is positive (line 219). Why is Math.abs necessary?

You're right. Math.abs is not necessary here.
Hi Mike, I've updated the patch, but couldn't set you as the reviewer. Please take a look whenever you are free, no pressure!

Just NI'ing you here so you can keep track. Thanks!
Flags: needinfo?(mconley)
Comment on attachment 8801662 [details]
Bug 1301284 - Update time picker style to match visual spec;

https://reviewboard.mozilla.org/r/86340/#review90954

Thanks!

::: toolkit/content/widgets/spinner.js:41
(Diff revision 5)
>       *           {Function} getDisplayString: Takes a value, and output it
>       *             as localized strings.
> -     *           {Number} itemHeight [optional]: Item height in pixels.
>       *           {Number} viewportSize [optional]: Number of items in a
>       *             viewport.
> +     *           {Boolean} hideButtons [optional]: Hide up & down buttons

You've added a new `rootFontSize` property - should document that above.

::: toolkit/themes/shared/timepicker.css:28
(Diff revision 5)
> +
> +  --button-font-color: #858585;
> +  --button-font-color-hover: #4D4D4D;
> +  --button-font-color-active: #191919;
> +
> +

Why the extra newline here?

::: toolkit/themes/shared/timepicker.css:63
(Diff revision 5)
> -.spinner-container .stack {
> -  position: relative;
> -  height: var(--spinner-height);
> +.spinner-container button:hover {
> +  background-color: var(--button-font-color-hover);
> +}
> +
> +.spinner-container button.active {
> +  background-color: var(--button-font-color-active);
> +}
> +
> +.spinner-container button.up {
> +  mask: url("chrome://global/skin/icons/find-arrows.svg#glyph-find-previous") no-repeat 50% 50%;
> +}
> +
> +.spinner-container button.down {
> +  mask: url("chrome://global/skin/icons/find-arrows.svg#glyph-find-next") no-repeat 50% 50%;
> +}
> +
> +.spinner-container.hide-buttons button {
> +  visibility: hidden;

The buttons are direct descendants of the .spinner-container, so maybe best to use the child selector as opposed to the descendant selector for these.

::: toolkit/themes/shared/timepicker.css:83
(Diff revision 5)
> +
> +.spinner-container.hide-buttons button {
> +  visibility: hidden;
>  }
>  
>  .spinner-container .spinner {

Same as above.

Basically, anywhere where you have `.spinner-container .spinner`.

::: toolkit/themes/shared/timepicker.css:93
(Diff revision 5)
>    overflow-y: scroll;
>    scroll-snap-type: mandatory;
>    scroll-snap-points-y: repeat(100%);
>  }
>  
>  .spinner-container .spinner > div {

Same as above.
Attachment #8801662 - Flags: review+
Flags: needinfo?(mconley)
Thanks again Mike! I've addressed the issues in the latest patch.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2133a6ec73c4
Update time picker style to match visual spec; r=mconley
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2133a6ec73c4
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(In reply to Mike Conley (:mconley) (PTO - Nov 11 - Nov 14) from comment #10)
> > +  --button-font-color: #858585;
> > +  --button-font-color-hover: #4D4D4D;
> > +  --button-font-color-active: #191919;

Have you tested if / how these colors work in high contrast mode? If you don't know about high contrast mode, see https://mail.mozilla.org/pipermail/firefox-dev/2016-October/004713.html for some background info.

> ::: toolkit/themes/shared/timepicker.css:63

> > +.spinner-container button.up {
> > +  mask: url("chrome://global/skin/icons/find-arrows.svg#glyph-find-previous") no-repeat 50% 50%;
> > +}
> > +
> > +.spinner-container button.down {
> > +  mask: url("chrome://global/skin/icons/find-arrows.svg#glyph-find-next") no-repeat 50% 50%;
> > +}

timepicker.css shouldn't depend on resources that are clearly and explicitly there for the find bar and might change with future updates to the find bar widget. Can you please file a followup bug on removing this dependency?
Flags: needinfo?(scwwu)
Component: XUL Widgets → Themes
I've tried it on high contrast mode, and your concern was right, it doesn't work well at all there. I've created a bug that will fix that problem, as well as removing dependency for find-arrows.svg: Bug 1317581,
Bug 1317600.

Thanks for pointing them out Dão!
Flags: needinfo?(scwwu)
Whiteboard: [milestone4]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: