887110 - [Clock] Fix clock face tick marks and hands to adhere to spec

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Clock, defect)

Description

[Erik Vold [:erikvold] (please needinfo? me)]

Why does the FirefoxOS make the 10min marks prominent and the quarter hour marks nonexistent? this is very odd imo.

Comment 1

[Evelyn Eastmond [:evhan55]]

Waiting on an SVG version of the new clock face design

Comment 2

[Peter La]

Hi,

I've sent the relevant files to Evelyn.

Comment 3

[Evelyn Eastmond [:evhan55]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/12342

- Analog clock face changes
    - Removed .png files for marks at 1, 1.5x, 2.0x
    - Inserted new analog clock face SVG into clock's index.html
    - Positioned current hands to fit this clock face, but those hands will change to new SVG paths
    - Separated height/width declarations in CSS
- Tests
    - Tested this new face manually in browser and on Unagi
    - Tested the resizing by adding/removing alarms to cause the alarm list to grow/shrink and face to resize
    - Wasn't sure how to test the removal of images for 1.5x and 2.0x scale devices
- Issues
    - Found a new bug: clock doesn't resize to largest size when all alarms are deleted from a list, but that can maybe be taken care of with this bug later: https://bugzilla.mozilla.org/show_bug.cgi?id=910854
    - CSS file was not refactored in its entirety, perhaps for a later bug?
    - SVG for analog clock face is massive, is there a better way to include that in the html?

Comment 4

[Evelyn Eastmond [:evhan55]]

Comment on attachment 807936 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12342

Clock hands position hack (5 pixels from top) will probably get fixed when this ticket is addressed: https://bugzilla.mozilla.org/show_bug.cgi?id=909361

Comment 5

[Mike Pennisi [:jugglinmike]]

Comment on attachment 807936 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12342

I've left a suggestion about file organization and markup structure on the GitHub pull request.

I'm happy to see this is finally being addressed!

Comment 6

[Evelyn Eastmond [:evhan55]]

Comment on attachment 807936 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12342

Resubmitting for review.

- Moved SVG to a new file
- Removed analog-clock-container tag and made equivalent changes to digital-clock
- Clock hands don't snap 100% correctly at all clock sizes, but there is another bug out there to change the clock hands anyway, I'll update that bug to report this
- Clock face still doesn't resize to biggest size when all alarms are deleted from list, but there is another bug out there to fix clock face resizing anyway, I'll update that bug to report this

Comment 7

[Evelyn Eastmond [:evhan55]]

I meant to add that the whole 'marks1/2/3/4' resize system is on the horizon to change with: https://bugzilla.mozilla.org/show_bug.cgi?id=910854

Comment 8

[Mike Pennisi [:jugglinmike]]

Comment on attachment 807936 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12342

Hey Evelyn,

I agree about the clock resizing--since the bug is already present in `master`, we should use the existing bug to track that fix.

As for the hands being out of alignment, this is behavior that does not exist in `master`, so we should avoid introducing a regression in this patch. It's true that bug 909361 also concerns the clock hands, but we can't make any assumptions about when that patch will land. We discussed this in person, but I think this may be a problem with the new clock face asset itself (in which case, we can't resolve it through changes to the clock hands alone).

Comment 9

[Evelyn Eastmond [:evhan55]]

Comment on attachment 807936 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12342

Great, thanks.  I will attempt a fix at the asset/alignment myself and flag you and Peter for review.

Comment 10

[Evelyn Eastmond [:evhan55]]

Comment on attachment 807936 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12342

Resubmitting for review.

I figured out the alignment issue, I think.  The clock face SVG file is indeed taller than it is wide, but that is because the 12o'clock mark is taller than all the rest of them.  The clock face itself is still a circle.  So, I had to add an individual margin to the top of the svg-hands container (analog-clock-hands), per size.

To test, I run the analog clock and observe it for one minute for each of the following cases:
- no alarms in the list
- 1 alarm in the list
- 2 alarms in the list
- 3+ alarms in the list

Alignment seems okay on my end, please let me know.
No need to tag Peter for review.

Comment 11

[Evelyn Eastmond [:evhan55]]

Comment on attachment 807936 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12342

Oh yeah, I also tried a percentage based on the difference at the large clock size, and also noticed that it didn't work.  That's why I did the individual margins.  However, I was probably being lazy in not calculating the exact percentage at the large size.  I will revisit all those numbers and try for a cleaner margin.

Comment 12

[Mike Pennisi [:jugglinmike]]

Comment on attachment 807936 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12342

It looks like you've already got to work incorporating my feedback on GitHub--you're certainly on top of your game! I'll unset the review flag so that you have a way to tell me when you're done.

Comment 13

[Evelyn Eastmond [:evhan55]]

Comment on attachment 807936 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12342

Resubmitting for review.

The padding-top idea worked extremely well and I tried to leave comments in the CSS to point to the technique.  The new clock face sits well at the four old sizes, I think (both on device and on desktop).

Comment 14

[Evelyn Eastmond [:evhan55]]

Comment on attachment 807936 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12342

Resubmitting for review.

The padding-top idea worked extremely well and I tried to leave comments in the CSS to point to the technique.  The new clock face sits well at the four old sizes, I think (both on device and on desktop).

Comment 15

[Evelyn Eastmond [:evhan55]]

Comment on attachment 807936 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12342

Resubmitting for review.

Mike Pennisi's padding-top idea worked well, except it *has* to be 3.9% to correctly show the entire clock face background.
3.9% is too big for the clock hands to snap correctly though, so they have their own margin-top fix to account for that.

The clock face background seems properly replaced now (best tested on a device directly).  Let me know if the alignment still seems off. There are other clock face resizing issues to address, but I think that's for other patches.

Comment 16

[Mike Pennisi [:jugglinmike]]

Requesting "koi+" because this change was identified as "high priority for version 1.2" by the UX team and because the patch will be entirely HTML & CSS.

Comment 18

[Evelyn Eastmond [:evhan55]]

Combining this with 909361 so bringing over a comment that was relevant from there:

From Peter La:

" it would be good if you could make sure they are pixel snapped by setting the center coordinate correctly for each clock size:

Large
x, y = 159.5, 211.5

Medium
x, y = 159.5, 181.5

Small
x, y = 159.5, 136.0

Let me know if this doesn't make sense.  Essentially, all this amounts to is that the small sized clock should be .5 pixels y offset from the other sizes so that the 3 o'clock and 9 o'clock hour markers pixel snap correctly."

Comment 19

[Evelyn Eastmond [:evhan55]]

The spec for clock v1.2 hands:
https://mozilla.app.box.com/applications/1/1092493904/9862006998/1

The current hands do not taper, are too long, have a blue circle, etc.  This all needs to be updated to reflect the spec.

Comment 20

[Evelyn Eastmond [:evhan55]]

Update the PR with the new changes for the new SVG clock hands.
Submitting for review.

Comment 21

[Mike Pennisi [:jugglinmike]]

Comment on attachment 807936 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12342

Hey Evelyn,

Thanks for updating this patch to include the latest spec for the clock hands! Unfortunately, this means we have one more step: we have to update the ClockView unit tests. Let me know when you've had a chance to address this (or if you need help getting them to pass).

Comment 22

[Evelyn Eastmond [:evhan55]]

(In reply to Mike Pennisi [:jugglinmike] from comment #21)
> this means we have one more step: we have to update
> the ClockView unit tests. Let me know when you've had a chance to address
> this (or if you need help getting them to pass).

Oh I'm sorry! I didn't run tests because for some reason I thought the Clock had nonexistent integration tests for the clock hands, my fault.  I don't know why I was thinking that and submitted without trying tests.  Will fix and resubmit Thursday, hopefully!

Comment 23

[Preeti Raghunath(:Preeti)]

Erroneous feature implementation of 1.2 feature. Hence koi+

Comment 24

[Evelyn Eastmond [:evhan55]]

Comment on attachment 807936 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12342

Resubmitting for review
New PR comments:

- JS changes
    - Removed the angle offset for the clock hands since they now all start at 0 degrees
- HTML/CSS changes
    - Removed .png files for marks at 1, 1.5x, 2.0x
    - Inserted new analog clock face/hands divs (with SVG backgrounds) into clock's index.html
    - Positioned new hands to fit new clock face with CSS
    - Some format cleanup in `clock.css`
- Unit Tests
    - Updated the unit tests to test he clock hands against the new angles (not offset)
- Manual Tests
    - Tested new face in browser and on Unagi
    - Tested resizing by adding/removing alarms to cause the alarm list to grow/shrink and face to resize
- Issues
    - Wasn't sure how to test the removal of images for 1.5x and 2.0x scale devices
    - Found a new bug: clock doesn't resize to largest size when all alarms are deleted from a list, but that can maybe be taken care of with this bug later: https://bugzilla.mozilla.org/show_bug.cgi?id=910854
    - `clock.css` file was not refactored in its entirety, perhaps for a later bug?

Comment 25

[Mike Pennisi [:jugglinmike]]

Comment on attachment 807936 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12342

I noticed the resizing issue, as well, but since it already exists on `master`, I agree that we should resolve it elsewhere.

This patch is great! I'm happy to land it for you :)

Comment 26

[Mike Pennisi [:jugglinmike]]

master: https://github.com/mozilla-b2g/gaia/commit/f3a8a0ef8b674606d202ced2b2fe1dd1090ddb77

Nice work, Evelyn!

Comment 27

[Evelyn Eastmond [:evhan55]]

(In reply to Mike Pennisi [:jugglinmike] from comment #26)
> master:
> https://github.com/mozilla-b2g/gaia/commit/
> f3a8a0ef8b674606d202ced2b2fe1dd1090ddb77
> 
> Nice work, Evelyn!

Go team! \o/
Thank you!

Comment 28

[Ed Morley [:emorley]]

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

Comment 29

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted f3a8a0ef8b674606d202ced2b2fe1dd1090ddb77 to:
v1.2: 44713d06dc40491553c1def5bc46d1eb6af2459d

Comment 30

[William Hsu [:whsu]]

Thanks for all your help!

Verified it.

* Test Build: (V1.2)
 - Gaia:     04ee9e4430b25ba2c38752d3897f0ee5e2a6ab80
 - Gecko:    http://hg.mozilla.org/releases/mozilla-aurora/rev/52f24889dccc
 - BuildID   20131027004003
 - Version   26.0a2

Attaching the screenshot.

Comment 31

[William Hsu [:whsu]]

Attachment: New Clock UI

Comment 32

[Montoya Clemmons (MClemmons)]

Verified quarter hour marks displayed prominently on below environmental variables for v 1.2

BuildID: 20131107004003
Gaia: 590eb598aacf1e2136b2b6aca5c3124557a365ca
Gecko: 26f1e160e696
Version: 26.0