Closed
Bug 1301284
Opened 9 years ago
Closed 8 years ago
Update picker style to match the visual spec for <input type="time">
Categories
(Toolkit :: Themes, defect)
Toolkit
Themes
Tracking
()
RESOLVED
FIXED
mozilla52
| Tracking | Status | |
|---|---|---|
| firefox52 | --- | fixed |
People
(Reporter: scottwu, Assigned: scottwu)
References
(Blocks 1 open bug)
Details
(Whiteboard: [milestone4])
Attachments
(1 file)
No description provided.
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
Assignee | |
Comment 4•9 years ago
|
||
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 hidden (mozreview-request) |
|
||
Comment 6•9 years ago
|
||
| mozreview-review | ||
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-
|
|
||
Updated•9 years ago
|
Flags: needinfo?(mconley)
|
|
Assignee | |
Comment 7•8 years ago
|
||
| mozreview-review-reply | ||
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.
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Comment 9•8 years ago
|
||
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 10•8 years ago
|
||
| mozreview-review | ||
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+
|
|
||
Updated•8 years ago
|
Flags: needinfo?(mconley)
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Comment 12•8 years ago
|
||
Thanks again Mike! I've addressed the issues in the latest patch.
Keywords: checkin-needed
|
||
Comment 13•8 years ago
|
||
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
|
||
Comment 14•8 years ago
|
||
| bugherder | ||
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
|
||
Comment 15•8 years ago
|
||
(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)
|
|
||
Updated•8 years ago
|
Component: XUL Widgets → Themes
|
|
Assignee | |
Comment 16•8 years ago
|
||
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)
|
||
Updated•8 years ago
|
Whiteboard: [milestone4]
|
|
||
Updated•8 years ago
|
Blocks: datetime-bugs
|
|
||
Updated•8 years ago
|
No longer blocks: datetime-bugs
You need to log in
before you can comment on or make changes to this bug.
Description
•