[Text Selection] UI improvement: Adjust Caret size and shadow

RESOLVED FIXED in Firefox 38, Firefox OS v2.2

Status

Firefox OS
Gaia::System::Window Mgmt
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Carol, Assigned: TYLin)

Tracking

unspecified
2.2 S6 (20feb)
x86
Mac OS X

Firefox Tracking Flags

(firefox36 wontfix, firefox37 wontfix, firefox38 fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(7 attachments, 8 obsolete attachments)

99.88 KB, image/png
Details
38.76 KB, application/zip
Details
63.73 KB, patch
TYLin
: review+
Carol
: ui-review+
Details | Diff | Splinter Review
1.19 KB, patch
automatedtester
: review+
Details | Diff | Splinter Review
3.58 KB, patch
automatedtester
: review+
jeremychen
: feedback+
Details | Diff | Splinter Review
140.20 KB, image/png
Details
139.81 KB, image/png
Details
(Reporter)

Description

3 years ago
Created attachment 8519758 [details]
screenshot_1110.png

Text selection's caret shadow need to sync with the pop-up menu's shadow.
(Reporter)

Comment 1

3 years ago
Hi Ting-yu,
Could you modify the caret shadow as the same size as pop-up menu shadow, thanks!

George is in charge of the pop-up menu.
Flags: needinfo?(tlin)
(Reporter)

Updated

3 years ago
Blocks: 1069288
(Assignee)

Comment 2

3 years ago
Hi Carol,

Currently, the carets used the png images in Text_Selection_Visual Spec.zip provided in bug 1024930. I can update the carets once you give me the latest caret images with updated shadow size.
Flags: needinfo?(tlin) → needinfo?(chuang)
(Reporter)

Comment 3

3 years ago
Created attachment 8520449 [details]
Caret_images_1111.zip

Hi Ting-Yu,
Could you help update the care images?
Please let me have UI review when you're done! Thank you!!!
Flags: needinfo?(chuang) → needinfo?(tlin)
(Assignee)

Updated

3 years ago
Flags: needinfo?(tlin)
(Assignee)

Updated

3 years ago
Assignee: nobody → tlin
Status: NEW → ASSIGNED
(Assignee)

Comment 4

3 years ago
Created attachment 8521258 [details] [diff] [review]
Adjust caret shadow.

The sizes of the images are not changed. No need to modify ua.css.
Attachment #8521258 - Flags: ui-review?(chuang)
Attachment #8521258 - Flags: review?(roc)
(Reporter)

Comment 5

3 years ago
Comment on attachment 8521258 [details] [diff] [review]
Adjust caret shadow.

thank you!
Attachment #8521258 - Flags: ui-review?(chuang) → ui-review+
Comment on attachment 8521258 [details] [diff] [review]
Adjust caret shadow.

Review of attachment 8521258 [details] [diff] [review]:
-----------------------------------------------------------------

rubber-stamp=me
Attachment #8521258 - Flags: review?(roc) → review+
(Assignee)

Comment 7

3 years ago
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b8bf2e60f117
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
Comment hidden (obsolete)
Comment hidden (obsolete)
(Reporter)

Comment 10

3 years ago
Created attachment 8526629 [details]
Caret_images_1121.zip

Hi Ting-Yu,
Please update the caret images from the attachment, thanks!!
Attachment #8520449 - Attachment is obsolete: true
Flags: needinfo?(chuang) → needinfo?(tlin)
(Assignee)

Updated

3 years ago
Flags: needinfo?(tlin)
Summary: [Text Selection] UI improvement: Adjust Caret shadow → [Text Selection] UI improvement: Adjust Caret size and shadow
(Assignee)

Comment 11

3 years ago
Created attachment 8528181 [details] [diff] [review]
Adjust caret size and shadow. r=roc (v2)

Tweak ua.css due to the size of the caret images changed.
Attachment #8521258 - Attachment is obsolete: true
Attachment #8528181 - Flags: ui-review?(chuang)
Attachment #8528181 - Flags: review?(roc)
(Assignee)

Updated

3 years ago
Attachment #8528181 - Flags: ui-review?(chuang)
Attachment #8528181 - Flags: review?(roc)
(Assignee)

Comment 12

3 years ago
Do we need still to revise the caret images?
Flags: needinfo?(ofeng)
Flags: needinfo?(chuang)
(Reporter)

Comment 13

3 years ago
Hi Ting-Yu,
Can you flash the latest version of the text caret UI look on a device? so I can check if the size is ok.
Because the UI review you gave me was all coding.
thanks
Flags: needinfo?(chuang) → needinfo?(tlin)
(Assignee)

Comment 14

3 years ago
We've discussed offline that the size of the caret still need to be revised.
Flags: needinfo?(tlin)
(Reporter)

Comment 15

3 years ago
Created attachment 8561220 [details]
Caret_images_1121.zip

Hi Ting-Yu,
Please help replace the images.
I would like to see a patch to check if the size is ok. Thank you!
Flags: needinfo?(tlin)
(Assignee)

Comment 16

3 years ago
Hi Carol, 

The size of tilt right images are smaller than normal and tilt right ones, which seem incorrect to me. Please help confirm that. Thanks.
Flags: needinfo?(tlin)
Flags: needinfo?(ofeng)
(Reporter)

Comment 17

3 years ago
Created attachment 8562626 [details]
Caret_images_0211.zip

Hi Ting-Yu,
I've revised the size of the files.
Please help update it! thanks!
Flags: needinfo?(tlin)
(Reporter)

Comment 18

3 years ago
Created attachment 8562674 [details]
Caret_images_0211.zip

Hi Ting-Yu
please update the images, thanks again!
Attachment #8562626 - Attachment is obsolete: true
(Assignee)

Comment 19

3 years ago
Created attachment 8563213 [details] [diff] [review]
Part 1 - Adjust caret size and shadow. (v2, carry roc r+)

The size of the images is enlarged by 1.5x. The CSS rules in ua.css are
changed accordingly.
Attachment #8528181 - Attachment is obsolete: true
Attachment #8563213 - Flags: ui-review?(chuang)
Attachment #8563213 - Flags: review?(roc)
(Assignee)

Updated

3 years ago
Flags: needinfo?(tlin)
Attachment #8563213 - Flags: review?(roc) → review+
(Reporter)

Updated

3 years ago
Attachment #8563213 - Flags: ui-review?(chuang) → ui-review+
(Assignee)

Comment 20

3 years ago
Created attachment 8563236 [details] [diff] [review]
Part 2 - Change tap test range in test_selectioncarets.py.
Attachment #8563236 - Flags: feedback?(jeremychen)
(Assignee)

Comment 21

3 years ago
All tests on b2g:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=611bface4a70

Marionette tests only on all platforms:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=253f79fdeaee
(Assignee)

Updated

3 years ago
Attachment #8563236 - Flags: feedback?(jeremychen)
(Assignee)

Comment 22

3 years ago
Created attachment 8563267 [details] [diff] [review]
Part 2 - Change tap test range in test_selectioncarets.py. (v2)

Eliminate the magic number 29 and make the calculation of range start
and end explicitly. It's sad we cannot retrieve the margin left from
ua.css directly. We need to copy them to the test file by hand.
Attachment #8563236 - Attachment is obsolete: true
Attachment #8563267 - Flags: review?(dburns)
Attachment #8563267 - Flags: feedback?(jeremychen)
(Assignee)

Comment 23

3 years ago
Created attachment 8563268 [details] [diff] [review]
Part 3 - Fix function redefinition in test_selectioncarets.py. (v1)
Attachment #8563268 - Flags: review?(dburns)
(Assignee)

Comment 24

3 years ago
Created attachment 8563304 [details] [diff] [review]
Part 2 - Change hit test in tilt mode. (v3)

Due to the caret images changed, we need to change this test
accordingly.

It's sufficient to perform hit tests only targeting the left edge of the
left tilted caret and the right edge of the right tilted caret. It also
reduces the running time of the tests.

It's sad that we cannot retrieve the CSS style of the carets from ua.css
directly. We need to copy them to the test file by hand.
Attachment #8563267 - Attachment is obsolete: true
Attachment #8563267 - Flags: review?(dburns)
Attachment #8563267 - Flags: feedback?(jeremychen)
Attachment #8563304 - Flags: review?(dburns)
Attachment #8563304 - Flags: feedback?(jeremychen)
Attachment #8563304 - Flags: review?(dburns) → review+
Attachment #8563268 - Flags: review?(dburns) → review+
Comment on attachment 8563304 [details] [diff] [review]
Part 2 - Change hit test in tilt mode. (v3)

Review of attachment 8563304 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. :)
Attachment #8563304 - Flags: feedback?(jeremychen) → feedback+
(Assignee)

Comment 26

3 years ago
Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fbefb718dc2
(Assignee)

Comment 27

3 years ago
Created attachment 8563865 [details]
Screenshot for old caret 29x31
(Assignee)

Comment 28

3 years ago
Created attachment 8563866 [details]
Screenshot for new caret 44x47
(Assignee)

Updated

3 years ago
Attachment #8563865 - Attachment description: Screenshot for old caret 29x31.png → Screenshot for old caret 29x31
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Comment 29

3 years ago
Comment on attachment 8563213 [details] [diff] [review]
Part 1 - Adjust caret size and shadow. (v2, carry roc r+)

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): None. Revise image resource by UX team request.
User impact if declined: The caret will be smaller and harder to drag.
Testing completed: Pass marionette tests on try.
Risk to taking this patch (and alternatives if risky): No risk. Simple image and CSS changes.
String or UUID changes made by this patch: None.
Attachment #8563213 - Flags: approval-mozilla-b2g37?
(Assignee)

Comment 30

3 years ago
Patch 2 and 3 modified the test case written in bug 1130951. They won't be uplifted unless patches in bug 1130951 are also uplifted.
(Assignee)

Updated

3 years ago
Attachment #8526629 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8561220 - Attachment is obsolete: true
https://hg.mozilla.org/integration/b2g-inbound/rev/f0f528da4d3b
https://hg.mozilla.org/integration/b2g-inbound/rev/eb7e4b1b9cf8
https://hg.mozilla.org/integration/b2g-inbound/rev/3563d3cfba5c
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f0f528da4d3b
https://hg.mozilla.org/mozilla-central/rev/eb7e4b1b9cf8
https://hg.mozilla.org/mozilla-central/rev/3563d3cfba5c
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED

Updated

3 years ago
Attachment #8563213 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/089444e5358e
status-b2g-v2.2: --- → fixed
status-b2g-master: --- → fixed
status-firefox36: --- → wontfix
status-firefox37: --- → wontfix
Target Milestone: --- → 2.2 S6 (20feb)
You need to log in before you can comment on or make changes to this bug.