Closed Bug 1262755 Opened 8 years ago Closed 8 years ago

Enlarge accessible carets touch area for Fennec

Categories

(Firefox for Android Graveyard :: Text Selection, defect)

All
Android
defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: JanH, Assigned: TYLin)

References

Details

Attachments

(5 files)

Compared with the old text selection handles, the touch area of the new carets seems a bit small in the vertical direction. In multi-line text environments, where the carets will follow up-down movements as well, I find it rather difficult to select some text/drag the cursor around without my finger obscuring the view of what I'm actually trying to select - and I wouldn't consider my fingers as exceptionally large :-)
I had the same concern raised about the new caret assets are smaller and without any padding in bug 1097398 comment 90. Anthony's reply was in bug 1097398 comment 91.
I agree. To implement the floating text selection (bug 1171110) I have been selecting text all day and I had problems grabbing the handles quite often. If you miss them you immediately start to scroll the page which makes this even more annoying.
@antlam: I still have problems grabbing the handles every now and then. Especially close to the edge of the device, thanks to the protector. If you accidentally scroll the page 2-3 times then this gets really frustrating.
Flags: needinfo?(alam)
From IRC:

me: "capella, sebastian, TYLin: the visual of the caret size should stay the same there and I think. Perhaps we can just increase the touch target without increasing how big it "looks"? 

What's the size of the touch target ATM?
Flags: needinfo?(tlin)
Flags: needinfo?(jh+bugzilla)
Flags: needinfo?(alam)
The touch target is within the red box. Without any zooming, the width is the image width, which is 22px; the height is the height of the selection highlight plus the image height, which is 19px + 22px.

When zooming in, the selection highlight becomes taller, but the image size stays the same, so the touch target still becomes taller.
Flags: needinfo?(tlin)
Attached image carets touchpoint.png
Consciously thinking about it, I think I'm trying to hit the selection carets in such a way that the upper margin of my finger is approximately touching the lower border of the selection highlight. I've tried capturing my attempted touchpoint in the attachment. So the padding might need to extend approximately 75 - 100 % of the caret height downwards?

And in case it's relevant, these are my phones display specs: https://bugzilla.mozilla.org/attachment.cgi?id=8657370
Flags: needinfo?(jh+bugzilla)
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #5)
> Created attachment 8741394 [details]
> Screenshot_2016-04-14-23-33-55.png
> 
> The touch target is within the red box. Without any zooming, the width is
> the image width, which is 22px; 

"Zooming" or "pinching" shouldn't change the size of the caret. 

> the height is the height of the selection
> highlight plus the image height, which is 19px + 22px.
> 
> When zooming in, the selection highlight becomes taller, but the image size
> stays the same, so the touch target still becomes taller.

Is something like this possible? where the caret image doesn't move, but is centered in a 48px sq container (blue) that is the touch area.
Flags: needinfo?(tlin)
Enlarge the touch target of the caret to the left, bottom, and right by
59% (13px) per bug 1262755 comment 7.

Review commit: https://reviewboard.mozilla.org/r/46693/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46693/
(In reply to Anthony Lam (:antlam) from comment #7)
> Is something like this possible? where the caret image doesn't move, but is
> centered in a 48px sq container (blue) that is the touch area.

I can retain the caret at the same position, and extend the touch area by 13px (59% of 22px) of the caret to the left, bottom, and right. But I need upper touch area to be aligned to the selection highlight in order to make the position correct in all zoom scales. See the image for the new touch target.
Flags: needinfo?(tlin)
Jan, do you have time to test my patch in comment 8, and see if it works for you?
Flags: needinfo?(jh+bugzilla)
It's better, although personally I was even happier with something like
> padding-left: 42%;
> padding-bottom: 75%;
> padding-right: 42%;
> left: -42%;

Okay, so there's no real basis to the 42% left/right padding I've chosen here :-) - there's probably no harm in having a square-ish padding either, but I don't think the difficulty is in hitting the caret per se, it's that I'm moving my finger down so it doesn't obscure the line of text I'm trying to select. So for me it's basically only the padding-bottom that really needs increasing, and I think 59% is not quite enough - 75% works much better.
Flags: needinfo?(jh+bugzilla)
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #9)
> Created attachment 8741704 [details]
> Enlarge touch target by 59%.png
> 
> (In reply to Anthony Lam (:antlam) from comment #7)
> > Is something like this possible? where the caret image doesn't move, but is
> > centered in a 48px sq container (blue) that is the touch area.
> 
> I can retain the caret at the same position, and extend the touch area by
> 13px (59% of 22px) of the caret to the left, bottom, and right. But I need
> upper touch area to be aligned to the selection highlight in order to make
> the position correct in all zoom scales. See the image for the new touch
> target.

I see what you mean. Will this affect the carets when they're at the edge of the screen?

Would it be possible to get an .APK to test this? thanks!
Flags: needinfo?(tlin)
(In reply to Anthony Lam (:antlam) from comment #12)
> I see what you mean. Will this affect the carets when they're at the edge of
> the screen?

No. The caret position and the touch area do not affect by whether they're at the edge of the screen or not. One use cases I had encounter on B2G is that the caret are below the bottom edge of the screen, so the touch area beside the selection highlight could still be dragged to change the selection range.

> Would it be possible to get an .APK to test this? thanks!

Yes. I had push my patch to try.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eee3dc9888a3&selectedJob=19533560

The apk is here.
http://archive.mozilla.org/pub/mobile/try-builds/tlin@mozilla.com-eee3dc9888a33ef0a43d580025d63e0ec43481d6/try-android-api-15/fennec-48.0a1.en-US.android-arm.apk
Flags: needinfo?(tlin) → needinfo?(alam)
(In reply to Jan Henning [:JanH] from comment #11)
> It's better, although personally I was even happier with something like
> > padding-left: 42%;
> > padding-bottom: 75%;
> > padding-right: 42%;
> > left: -42%;
> 
> Okay, so there's no real basis to the 42% left/right padding I've chosen
> here :-) - there's probably no harm in having a square-ish padding either,
> but I don't think the difficulty is in hitting the caret per se, it's that
> I'm moving my finger down so it doesn't obscure the line of text I'm trying
> to select. So for me it's basically only the padding-bottom that really
> needs increasing, and I think 59% is not quite enough - 75% works much
> better.

Jan, thank you for the feedback. I don't have a fat finger, so the padding-bottom 59% works for me. And I guess my opinion shouldn't count because I know too well about where the touch area is ;) But I'm confident that Anthony will take your feedback into consideration.
Assignee: nobody → tlin
Summary: Selection carets touch area → Enlarge accessible carets touch area for Fennec
Yeah, let's not design for our finders in particular. :)

If helpful here, the guidelines suggest touch targets to be at least 48x48dp:
https://www.google.com/design/spec/layout/metrics-keylines.html#metrics-keylines-touch-target-size
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #13)
> (In reply to Anthony Lam (:antlam) from comment #12)
> > I see what you mean. Will this affect the carets when they're at the edge of
> > the screen?
> 
> No. The caret position and the touch area do not affect by whether they're
> at the edge of the screen or not. One use cases I had encounter on B2G is
> that the caret are below the bottom edge of the screen, so the touch area
> beside the selection highlight could still be dragged to change the
> selection range.
> 
> > Would it be possible to get an .APK to test this? thanks!
> 
> Yes. I had push my patch to try.
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=eee3dc9888a3&selectedJob=19533560
> 
> The apk is here.
> http://archive.mozilla.org/pub/mobile/try-builds/tlin@mozilla.com-
> eee3dc9888a33ef0a43d580025d63e0ec43481d6/try-android-api-15/fennec-48.0a1.en-
> US.android-arm.apk

Thanks TYLin!

I haven't had a chance to test this on my device yet because I don't want to replace my "Nightly" build but it sounds like it makes sense to me. I'll test this tomorrow when I'm back in the office on another device.
Attachment #8741702 - Flags: review?(snorp)
Comment on attachment 8741702 [details]
MozReview Request: Bug 1262755 - Enlarge caret touch target by 59% for Fennec.

https://reviewboard.mozilla.org/r/46693/#review44457
Attachment #8741702 - Flags: review?(snorp) → review+
Just tested this on my N5 on a Wikipedia page and it does seem to be easier to grab without any side effects. 

Thanks TYLin!
Flags: needinfo?(alam)
I happened to have this patch in my queue while running local testAccessibleCarets robocop test and I'm seeing an error I can't explain ... pointing to [0]

[object HTMLInputElement] === [object HTMLTextAreaElement] ] Focused element should match expected. - See following stack:
 0:34.16 LOG: None INFO testAccessibleCarets.js | JS frame :: testAccessibleCarets.js :: testAccessibleCarets :: line 197


[0] http://mxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/robocop/testAccessibleCarets.js?rev=dbb14513cb83&mark=197-197#182

You might want to quick verify? I thought maybe it was related to the issue we have with bug 1265750, but the fail occurs while testing with or without that patch.
Flags: needinfo?(tlin)
Mark, thank you for catching this bug. I can reproduce it locally. Debugging...
Flags: needinfo?(tlin)
Comment on attachment 8741702 [details]
MozReview Request: Bug 1262755 - Enlarge caret touch target by 59% for Fennec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46693/diff/1-2/
Attachment #8741702 - Flags: review?(markcapella)
Comment on attachment 8741702 [details]
MozReview Request: Bug 1262755 - Enlarge caret touch target by 59% for Fennec.

https://reviewboard.mozilla.org/r/46693/#review44783

First time r+ for reviewboard ... crosses fingers
Attachment #8741702 - Flags: review?(markcapella) → review+
Comment on attachment 8741702 [details]
MozReview Request: Bug 1262755 - Enlarge caret touch target by 59% for Fennec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46693/diff/2-3/
Adding one <br> to the html fixed the test fail in comment 19 locally, but it still fails on try. By adding two <br>s, the try is happy.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=142571755b8f
Backout due to the same symptom like comment 19 because Bug 1265750 adds more test cases. Keep adding <br> to separate the elements. All look green.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ae06ef6e09b
Flags: needinfo?(tlin)
Comment on attachment 8741702 [details]
MozReview Request: Bug 1262755 - Enlarge caret touch target by 59% for Fennec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46693/diff/3-4/
Blocks: 1266715
https://hg.mozilla.org/mozilla-central/rev/4280881aae80
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.