1021499 - [Text Selection] carets might be truncated by keyboard

Status: VERIFIED FIXED

(Core :: DOM: Selection, defect, P4)

Description

[u459114]

Attachment: 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.

Comment 1

[howie [:howie]]

Hi Ting-yu, any progress on this?

Comment 2

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

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

Comment 4

[Peter Chang[:pchang]]

It may be a known issue for touchcaret, lower priority.

Comment 5

[Julien Wajsberg [:julienw]]

[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...

Comment 6

[Joshua Mitchell (Inactive)]

(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

Comment 7

[Julien Wajsberg [:julienw]]

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.

Comment 8

[Stephany Wilkes]

That's fine. This is not a blocker as long at it doesn't interfere with either keyboard or carat functionality.

Comment 10

[howie [:howie]]

Triage: visual polish not blocker.

Comment 12

[Sam Foster [:sfoster] (he/him)]

Attachment: 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

[Stephany Wilkes]

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.

Comment 14

[Peter Chang[:pchang]]

(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?

Comment 15

[Omega Feng [:Omega] [:馮於懋]]

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

Comment 16

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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.

Comment 17

[Omega Feng [:Omega] [:馮於懋]]

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!

Comment 19

[Coler]

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

Comment 20

[Stephany Wilkes]

Thanks, Omega. That's fine.

Comment 21

[Peter Chang[:pchang]]

Switch to morris who is working on this bug.

Comment 22

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Enlarge touch/selection carets touch area.

As UX spec mentioned, we enlarge touch area for selection/touch carets.

Comment 23

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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.

Comment 24

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: 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.

Comment 25

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

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

Comment 26

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Enlarge touch/selection carets touch area v2.

Add pointer-events: none to touch/selection carets so GetFrameForPoint will ignore
caret frames.

Comment 27

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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.

Comment 28

[GitHub Autolander Bot]

Attachment: [gaia] mephisto41:bug1021499 > mozilla-b2g:master

Comment 29

[GitHub Autolander Bot]

Attachment: [gaia] mephisto41:bug1146753 > mozilla-b2g:master

Comment 30

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Enlarge touch/selection carets touch area v3.

Skip selection caret when build displaylist without showing caret.

Comment 31

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Skip test if touch caret is enabled.

Comment 32

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

try https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e08b9e9df5d

Comment 33

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 35

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: 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.

Comment 36

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: test_apzc_event_region.html

Oops, previous file is wrong. Upload correct file.

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Please file that as a separate bug and see if you can get kats or botond to help fix it.

Comment 38

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(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.

Comment 39

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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.

Comment 40

[Peter Chang[:pchang]]

Move back to v2.

Comment 42

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Enlarge touch/selection carets touch area v4.

Since bug 1147279 is ready to land. I remove the hack in previous patch.

Comment 43

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e93538d1ae70

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8593152 [details] [diff] [review]
Enlarge touch/selection carets touch area v4.

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

Great!

Comment 45

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

all set!

Comment 46

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/59c97d681f50
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ef9825777a2

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/59c97d681f50
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ef9825777a2

Comment 48

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/048b67897b20
https://hg.mozilla.org/integration/mozilla-inbound/rev/f41b98598f78

Comment 49

[Carsten Book [:Tomcat]]

had to back this out since this have caused https://treeherder.mozilla.org/logviewer.html#?job_id=8976090&repo=mozilla-inbound

Comment 50

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

I think this failure should be a known issue. See bug 1155426.

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/ba584a128a73
https://hg.mozilla.org/integration/b2g-inbound/rev/27ab916e0e66

Comment 52

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ba584a128a73
https://hg.mozilla.org/mozilla-central/rev/27ab916e0e66

Comment 53

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Enlarge touch/selection carets touch area (for b2g-37).

Comment 54

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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

Comment 55

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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

Comment 56

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/07daccc4b2b0
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/7343618cc119

Comment 57

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

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