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)
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 :-)
Assignee | ||
Comment 1•8 years ago
|
||
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.
Comment 2•8 years ago
|
||
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.
Comment 3•8 years ago
|
||
@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)
Comment 4•8 years ago
|
||
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)
Assignee | ||
Comment 5•8 years ago
|
||
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)
Reporter | ||
Comment 6•8 years ago
|
||
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)
Comment 7•8 years ago
|
||
(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.
Updated•8 years ago
|
Flags: needinfo?(tlin)
Assignee | ||
Comment 8•8 years ago
|
||
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/
Assignee | ||
Comment 9•8 years ago
|
||
(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)
Assignee | ||
Comment 10•8 years ago
|
||
Jan, do you have time to test my patch in comment 8, and see if it works for you?
Flags: needinfo?(jh+bugzilla)
Reporter | ||
Comment 11•8 years ago
|
||
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)
Comment 12•8 years ago
|
||
(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)
Assignee | ||
Comment 13•8 years ago
|
||
(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)
Assignee | ||
Comment 14•8 years ago
|
||
(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 | ||
Updated•8 years ago
|
Assignee: nobody → tlin
Summary: Selection carets touch area → Enlarge accessible carets touch area for Fennec
Comment 15•8 years ago
|
||
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
Comment 16•8 years ago
|
||
(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.
Assignee | ||
Updated•8 years ago
|
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+
Comment 18•8 years ago
|
||
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)
Comment 19•8 years ago
|
||
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)
Assignee | ||
Comment 20•8 years ago
|
||
Mark, thank you for catching this bug. I can reproduce it locally. Debugging...
Flags: needinfo?(tlin)
Assignee | ||
Comment 21•8 years ago
|
||
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 22•8 years ago
|
||
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+
Assignee | ||
Comment 23•8 years ago
|
||
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/
Assignee | ||
Comment 24•8 years ago
|
||
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
Comment 26•8 years ago
|
||
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=26369786&repo=mozilla-inbound
Flags: needinfo?(tlin)
Comment 27•8 years ago
|
||
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/738b1eb6043a
Assignee | ||
Comment 28•8 years ago
|
||
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)
Assignee | ||
Comment 29•8 years ago
|
||
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/
Comment 31•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4280881aae80
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•