[Text Selection] carets might be truncated by keyboard

VERIFIED FIXED in Firefox 40, Firefox OS v2.2

Status

()

P4
normal
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: u459114, Assigned: mtseng)

Tracking

unspecified
mozilla40
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, firefox38 wontfix, firefox39 wontfix, firefox40 fixed, b2g-v2.2 verified, b2g-master verified)

Details

(Whiteboard: [2.2-nexus-5-l][MGSEI-v2.2-2R] [MGSEI-v2.2-N5-2R])

Attachments

(6 attachments, 7 obsolete attachments)

114.10 KB, image/png
Details
39.28 KB, image/png
Details
1.49 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.29 KB, text/html
Details
12.87 KB, patch
roc
: review+
Details | Diff | Splinter Review
12.87 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
Created attachment 8435513 [details]
2014-06-06-12-02-49.png

1. Turn on touch caret by setting touchcaret.enabled
2. visit www.google.com
3. Click on text input
4. Touch caret is covered by keyboard.(see attached)

Since keyboard is another process, to fix this problem, we may need to create a top level layer for touch the anonymous caret frame, and compositor, in chrome process, respects "top-level" attribute and compose that layer on the top.
(Reporter)

Updated

4 years ago
OS: Linux → All
Hardware: x86_64 → All
(Reporter)

Updated

4 years ago
Blocks: 921964
(Reporter)

Updated

4 years ago
Blocks: 1023688
(Reporter)

Updated

4 years ago
No longer depends on: 924692

Comment 1

4 years ago
Hi Ting-yu, any progress on this?
Flags: needinfo?(tlin)
I haven't worked on this issue yet. The possible solution is as described in https://bugzilla.mozilla.org/show_bug.cgi?id=1021499#c0
Flags: needinfo?(tlin)

Updated

4 years ago
See Also: → bug 1057081
Duplicate of this bug: 1057081

Comment 4

4 years ago
It may be a known issue for touchcaret, lower priority.
Priority: -- → P3
[Blocking Requested - why for this release]: bug for the new caret feature.

Blocking status was requested in dupe bug 1057081.

I don't know if the issue breaks the feature, qawanted to find out.
If the feature still works, maybe we should not block because the issue looks difficult to be fixed...
blocking-b2g: --- → 2.1?
Keywords: qawanted
(In reply to Julien Wajsberg [:julienw] (PTO 08/19 -> 09/15; contact schung for SMS matters) from comment #5)
> [Blocking Requested - why for this release]: bug for the new caret feature.
> 
> Blocking status was requested in dupe bug 1057081.
> 
> I don't know if the issue breaks the feature, qawanted to find out.
> If the feature still works, maybe we should not block because the issue
> looks difficult to be fixed...

Julien - this issue does not break the feature - The caret can still be selected and dragged to the desired location. The only effect is a smaller hit box / detection area as you can only interact with the part of it that is not 'behind' the word suggestion / auto-correction bar but it is still very usable.

Tested on:
Device: Flame 2.2 Master
BuildID: 20140915091417
Gaia: 855be6ade407c26e0596e7306a44deebc3f60933
Gecko: d08c31df0c1a
Version: 35.0a1 (2.2 Master)

Repro Steps:
1) Open  the SMS app
2) Create a new message
3) Select the 'Body' text field

Firmware: V165
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0
Keywords: qawanted
Thanks, then I'm moving the blocking request to v2.2. We should definitely fix this because it's ugly, but since there is no loss in functionality it's good enough for me. UX might think otherwise though, so NI UX for this.
blocking-b2g: 2.1? → 2.2?
Flags: needinfo?(firefoxos-ux-bugzilla)

Comment 8

4 years ago
That's fine. This is not a blocker as long at it doesn't interfere with either keyboard or carat functionality.
Flags: needinfo?(firefoxos-ux-bugzilla)

Updated

4 years ago
Priority: P3 → P4
Duplicate of this bug: 1114926

Updated

4 years ago
QA Whiteboard: [textselection]

Comment 10

4 years ago
Triage: visual polish not blocker.
blocking-b2g: 2.2? → backlog

Updated

4 years ago
Blocks: 1124074
No longer blocks: 1023688

Updated

4 years ago
Duplicate of this bug: 1123232

Updated

4 years ago
Summary: [Touch Caret] Touch caret might be truncated by keyboard → [Text Selection] carets might be truncated by keyboard

Updated

4 years ago
QA Whiteboard: [textselection] → [COM=Text Selection]
Created attachment 8575611 [details]
selection handles overlaid by keyboard

I just ran into this. I found it really difficult to move the cursor. Eventually I was able, but the feature was good as broken for me.

Comment 13

4 years ago
In that case, I think Sam's experience qualifies this as a blocker: my earlier comment #8 seems not to be true. This is not polish if it interferes with functionality, which it seems to be doing. What normal user is going to try at this for as long as Sam did? As a non-engineer, will a normal user even recognize what the problem is enough to also figure out a way out of this? Nominating.
blocking-b2g: backlog → 2.2?

Updated

4 years ago
blocking-b2g: 2.2? → 2.2+
(In reply to Sam Foster [:sfoster] from comment #12)
> Created attachment 8575611 [details]
> selection handles overlaid by keyboard
> 
> I just ran into this. I found it really difficult to move the cursor.
> Eventually I was able, but the feature was good as broken for me.

Right now carets are hidden by keyboard because it is drawn by app which get lower zindex than keyboard app. It's hard to change this behavior in gecko side. The solution we may have here is to scroll up the content when the issue happens, but it doesn't work for third party apps.

In other OS, the carets jump to the top of selection when hits the bottom page, but it requires UX changes. Omega or Stephany, any thought?
Flags: needinfo?(ofeng)
Flags: needinfo?(firefoxos-ux-bugzilla)
For long term plan, we still think the carets should overlap the keyboard. If it's difficult to implement now, we can still improve the experience by using the whole caret (including the bar) as touch area. See p.14 of the UX spec for details. https://bug921965.bugzilla.mozilla.org/attachment.cgi?id=8548759#14
Flags: needinfo?(ofeng)
Although carets able to overlap keyboard, there is an issue cannot be resolved. Once our carets appear on the bottom of page, the carets would be clipped by screen. I think carets clip by screen is similar with clip by keyboard. So I think we maybe need to find out a good solution for both cases.

And enlarge the touch area for caret would cause another problem. As a user, it is hard to select or un-select the character which is nearby current selection. For example: Tilt example in p.14 of UX spec, characters "ipis" is selected. If user want to cancel selection by tapping "a" or "d" on the left side of "ipis". It is not working, we treat it as tapping on the caret. User might feel frustration about this.
After discussion with the team, we decide to use the whole caret including the bar as touch area, which can fix both the "keyboard" and "bottom page" issues. (The "cancel selection" issue that Morris mentioned is not that bad because user can tap outside to cancel the selection easily.)
@Steph, let us know if you think this solution is fine, thanks!
Flags: needinfo?(swilkes)

Updated

4 years ago
Duplicate of this bug: 1144047

Comment 19

4 years ago
This issue CAN be repro on Nexus 5_v2.2&3.0.
Reproduce rate:5/5


N5_3.0(affected):

Build ID               20150317160205
Gaia Revision          63d6639acd771f548a2613f07f3e335921e4ac87
Gaia Date              2015-03-17 16:53:50
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/e965a1a534ec
Gecko Version          39.0a1
Device Name            hammerhead
Firmware(Release)      5.0
Firmware(Incremental)  eng.cltbld.20150317.194723
Firmware Date          Tue Mar 17 19:47:37 EDT 2015
Bootloader             HHZ12d

N5_2.2(affected):

Build ID               20150317162504
Gaia Revision          306772a58335ac4cad285d27c3805090a8cc6886
Gaia Date              2015-03-17 17:12:36
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/83251e534b33
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.0
Firmware(Incremental)  eng.cltbld.20150317.195033
Firmware Date          Tue Mar 17 19:50:47 EDT 2015
Bootloader             HHZ12d
Whiteboard: [2.2-nexus-5-l]

Comment 20

4 years ago
Thanks, Omega. That's fine.
Flags: needinfo?(swilkes)
Flags: needinfo?(firefoxos-ux-bugzilla)
Switch to morris who is working on this bug.
Assignee: tlin → mtseng
Created attachment 8579807 [details] [diff] [review]
Enlarge touch/selection carets touch area.

As UX spec mentioned, we enlarge touch area for selection/touch carets.
Comment on attachment 8579807 [details] [diff] [review]
Enlarge touch/selection carets touch area.

This solution not work because APZC event region rely on size of frame. If we just enlarge hit test rect of touch/selection carets. The size of frame remain unchange. Thus, If we move selection by drag on the above of caret, the page will scroll by APZC. We want to prevent this happened, So I'll submit an patch which is enlarge the frame of caret, not the hit rect of rect.
Attachment #8579807 - Attachment is obsolete: true
Created attachment 8581411 [details] [diff] [review]
Enlarge touch/selection carets touch area.

Enlarge touch area by increase frame size of touch/selection caret.

The only concern of this patch is I added a little hack in ua.css. When dragging
carets, we don't want to scroll the page, which means APZ should not work when moving
carets. We achieve this by adding our frame of carets to a dummy touch listener.
In this bug, we want to enlarge touch area for carets. So I added padding-top
attribute to carets. But those carets would optimized to image layers eventually
because carets are consist of only background image. So the layer size exclude
the space which is added by padding-top. Hence, APZ cannot detect this extra space
and then when dragging on this extra space, page scrolling and caret movement
work at the same time which means this functionality is not usable. In the end,
I added a background-color with alpha very close to zero. So that caret frame
becomes painted layer instead of image layer. And this patined layer include
the extra space then everything works well. Do we have better way to handle issue
like this? Or is this an accepetance hack? Thanks.
Attachment #8581411 - Flags: review?(roc)
If we enlarge the touch area as described in [1], the text above the touch/selection caret will be covered. Originally, a user could click the text to change the cursor position or long press to select a word. Since there is no visual indication that a user could press the area above the touch/selection caret to drag the caret, he might feel that long pressing to select a word breaks.

[1] https://bug921965.bugzilla.mozilla.org/attachment.cgi?id=8548759#14
Created attachment 8581515 [details] [diff] [review]
Enlarge touch/selection carets touch area v2.

Add pointer-events: none to touch/selection carets so GetFrameForPoint will ignore
caret frames.
Attachment #8581411 - Attachment is obsolete: true
Attachment #8581411 - Flags: review?(roc)
Attachment #8581515 - Flags: review?(roc)
Comment on attachment 8581515 [details] [diff] [review]
Enlarge touch/selection carets touch area v2.

Add pointer-events:none would cause apz scrolling and dragging caret appears at the same time. I'll provide another patch to resolved this. So clear review flag first.
Attachment #8581515 - Flags: review?(roc)
Created attachment 8582180 [details] [review]
[gaia] mephisto41:bug1021499 > mozilla-b2g:master
Depends on: 1146753
Created attachment 8582188 [details] [review]
[gaia] mephisto41:bug1146753 > mozilla-b2g:master
Attachment #8582180 - Attachment is obsolete: true
Attachment #8582188 - Attachment is obsolete: true
Created attachment 8582217 [details] [diff] [review]
Enlarge touch/selection carets touch area v3.

Skip selection caret when build displaylist without showing caret.
Attachment #8581515 - Attachment is obsolete: true
Attachment #8582217 - Flags: review?(roc)
Created attachment 8582218 [details] [diff] [review]
Skip test if touch caret is enabled.
Attachment #8582218 - Flags: review?(roc)
since bug 1146753 has been resolved, push try again to see if b2g-desktop gij 8 still orange.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d8b55dd0e12
(In reply to Morris Tseng [:mtseng] from comment #24)
 > The only concern of this patch is I added a little hack in ua.css. When
> dragging
> carets, we don't want to scroll the page, which means APZ should not work
> when moving
> carets. We achieve this by adding our frame of carets to a dummy touch
> listener.
> In this bug, we want to enlarge touch area for carets. So I added padding-top
> attribute to carets. But those carets would optimized to image layers
> eventually
> because carets are consist of only background image. So the layer size
> exclude
> the space which is added by padding-top. Hence, APZ cannot detect this extra
> space
> and then when dragging on this extra space, page scrolling and caret movement
> work at the same time which means this functionality is not usable.

This should not be a problem and we shouldn't need a hack to fix it. The layer event region for the caret element should contain the top-padding and APZ should know not to scroll when you drag there. If you come up with a simple HTML testcase I'm sure we can get this fixed on the APZ/layers side.
Created attachment 8582869 [details]
test_apzc_event_region.html

Simple test case for apzc event region problem.

The key css attribute is "background-color:green" on the #caretfake.

If we have that css attribute, #caretfake becomes painted layer and everything works great.

But if we don't have that css attribute, #caretfake becomes image layer and drag on #caretfake would trigger apz scrolling.
Flags: needinfo?(roc)
Created attachment 8582872 [details]
test_apzc_event_region.html

Oops, previous file is wrong. Upload correct file.
Attachment #8582869 - Attachment is obsolete: true
Please file that as a separate bug and see if you can get kats or botond to help fix it.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #37)
> Please file that as a separate bug and see if you can get kats or botond to
> help fix it.

OK! Thanks.
Flags: needinfo?(roc)
Comment on attachment 8582217 [details] [diff] [review]
Enlarge touch/selection carets touch area v3.

Clear review flag first. I'll request review once apz problem is resolved.
Attachment #8582217 - Flags: review?(roc)
Depends on: 1147279
Move back to v2.
Blocks: 1023688
No longer blocks: 1124074, 921964

Updated

4 years ago
Duplicate of this bug: 1149070
Created attachment 8593152 [details] [diff] [review]
Enlarge touch/selection carets touch area v4.

Since bug 1147279 is ready to land. I remove the hack in previous patch.
Attachment #8582217 - Attachment is obsolete: true
Attachment #8593152 - Flags: review?(roc)
Comment on attachment 8593152 [details] [diff] [review]
Enlarge touch/selection carets touch area v4.

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

Great!
Attachment #8593152 - Flags: review?(roc) → review+
Whiteboard: [2.2-nexus-5-l] → [2.2-nexus-5-l][MGSEI-N5-2R]
Whiteboard: [2.2-nexus-5-l][MGSEI-N5-2R] → [2.2-nexus-5-l][MGSEI-v2.2-2R]
all set!
Status: NEW → ASSIGNED
Keywords: checkin-needed
Whiteboard: [2.2-nexus-5-l][MGSEI-v2.2-2R] → [2.2-nexus-5-l][MGSEI-v2.2-2R] [MGSEI-v2.2-N5-2R]
I think this failure should be a known issue. See bug 1155426.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ba584a128a73
https://hg.mozilla.org/mozilla-central/rev/27ab916e0e66
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Created attachment 8596383 [details] [diff] [review]
Enlarge touch/selection carets touch area (for b2g-37).
Comment on attachment 8582218 [details] [diff] [review]
Skip test if touch caret is enabled.

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 #): 
User impact if declined: Unable to drag caret if caret is truncated.
Testing completed: on master.
Risk to taking this patch (and alternatives if risky): Should be low risk.
String or UUID changes made by this patch:none
Attachment #8582218 - Flags: approval-mozilla-b2g37?
Comment on attachment 8596383 [details] [diff] [review]
Enlarge touch/selection carets touch area (for b2g-37).

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 #): 
User impact if declined: Unable to drag caret if caret is truncated.
Testing completed: on master.
Risk to taking this patch (and alternatives if risky): Should be low risk.
String or UUID changes made by this patch:none
Attachment #8596383 - Flags: approval-mozilla-b2g37?

Updated

4 years ago
Attachment #8582218 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+

Updated

4 years ago
Attachment #8596383 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/07daccc4b2b0
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/7343618cc119
status-b2g-v2.2: --- → fixed
status-b2g-master: --- → fixed
status-firefox38: --- → wontfix
status-firefox39: --- → wontfix
This issue is verified fixed on the latest Nightly Flame 3.0 and 2.2 builds.

Actual Results: The selection caret was easily dragged even when partially covered by the keyboard.

Environmental Variables:
Device: Flame 3.0
BuildID: 20150506010204
Gaia: 3e6fd1e0a478af2c95d09ce95c2c6de2de2fec14
Gecko: ba44099cbd07
Gonk: a9f3f8fb8b0844724de32426b7bcc4e6dc4fa2ed
Version: 40.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0

Environmental Variables:
Device: Flame 2.2
BuildID: 20150506002501
Gaia: 772a9491909abd02dc67278dd453746e2dd358a8
Gecko: 3af6a0a79227
Gonk: ab265fb203390c70b8f2a054f38cf4b2f2dad70a
Version: 37.0 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0
Status: RESOLVED → VERIFIED
QA Whiteboard: [COM=Text Selection] → [COM=Text Selection][QAnalyst-Triage?]
status-b2g-v2.2: fixed → verified
status-b2g-master: fixed → verified
Flags: needinfo?(ktucker)
QA Whiteboard: [COM=Text Selection][QAnalyst-Triage?] → [COM=Text Selection][QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.