1149070 - [Text Selection] Right caret disappeared in email select all

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::E-Mail, defect)

Description

[Teri Wen [:twen]]

### STR
0. Prerequisite: Set up an email account.
1. Open email app.
2. Tap pencil icon to compose a new email.
3. Enter some text.
4. Long press on the text to bring out copy/paste selection menu.
5. Tap select all icon

### Actual
Selection only has left caret.

### Expected
Selection with both carets.

### Version
Gaia-Rev        473cd63f53c855299b719285d9b95e3f2910782f
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/4b13c4254e2f
Build-ID        20150329162502
Version         37.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150329.200030
FW-Date         Sun Mar 29 20:00:41 EDT 2015
Bootloader      L1TC000118D0

Comment 1

[Teri Wen [:twen]]

Nominate for blocking 2.2 because broke text selection function.

Comment 2

[Teri Wen [:twen]]

Attachment: Screenshot after select all

Comment 3

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Add padding-bottom to cmp-body-text.png

In this select-all case, the right selection-caret is out of the view port, so it cannot be seen. Likewise, if the user long press any word at the last line, both carets cannot be seen.

I've done a experiment to add a line "padding-bottom: 1em;" to .cmp-body-text at [1] so that the email body can be scrolled up to reveal the selection-caret.

If email app UI doesn't change, there's no way that selection-caret can be seen. This might be related to bug 1021499.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/email/style/compose_cards.css#L119

Comment 4

[howie [:howie]]

ni UX Juwei, what do you think the we make the change as mentioned in Comment 3? Thanks.

Comment 5

[Juwei Huang[:juwei]]

Solution on comment 3 seems like a good idea :)
However, I guess it will also happen in other app, like message or any app that requires input field.
It would be better to fix in general.
Thanks

Comment 6

[howie [:howie]]

Blocking as feature broken.

Hi Dylan, can you find the right person for this as Comment 3 suggested? Thank you.

Comment 7

[Dylan Oliver [:doliver]]

This seems like a bigger issue than email -- we could hack something like comment #3 but we can't assume that every app will have a scrollable area under the text so we can make the carets visible. I think the solution is making the carets better aware of where they are on the screen and having them choose a more appropriate location. 

Does this possibly dupe to or at least overlap somewhat with the work happening in bug 1021499?

Comment 8

[Teri Wen [:twen]]

I think this is the same issue: left caret disappeared in selection when you try to view an email and tap select all.  
Impact to user: Unable to drag selection from the left. 
There's no keyboard involved though.

Comment 9

[Teri Wen [:twen]]

Attachment: Left caret disappeared

Comment 10

[Peter Chang[:pchang]]

(In reply to Teri Wen [:twen] from comment #8)
> I think this is the same issue: left caret disappeared in selection when you
> try to view an email and tap select all.  
> Impact to user: Unable to drag selection from the left. 
> There's no keyboard involved though.

(In reply to Teri Wen [:twen] from comment #9)
> Created attachment 8588991 [details]
> Left caret disappeared

I think this should be fired as another bug because this is not related to keyboard.

Comment 11

[Peter Chang[:pchang]]

(In reply to Dylan Oliver [:doliver] from comment #7)
> This seems like a bigger issue than email -- we could hack something like
> comment #3 but we can't assume that every app will have a scrollable area
Yes, you are right. We still have problem for other apps, actually we have more problem for outside apps(like google map in bug 1124566). From my perspective, we can make our own in-house app more user-friendly for CopyPaste. Dylan, does it make sense to you?

> under the text so we can make the carets visible. I think the solution is
> making the carets better aware of where they are on the screen and having
> them choose a more appropriate location. 
To choose a more appropriate location for carets, I think we need UX's input for this.
Omega, any comment?

In my opinion, another idea is to move carets more closely to the begin/end of selection
> 
> Does this possibly dupe to or at least overlap somewhat with the work
> happening in bug 1021499?

Comment 12

[Teri Wen [:twen]]

(In reply to peter chang[:pchang][:peter] from comment #10)
> (In reply to Teri Wen [:twen] from comment #8)
> > I think this is the same issue: left caret disappeared in selection when you
> > try to view an email and tap select all.  
> > Impact to user: Unable to drag selection from the left. 
> > There's no keyboard involved though.
> 
> (In reply to Teri Wen [:twen] from comment #9)
> > Created attachment 8588991 [details]
> > Left caret disappeared
> 
> I think this should be fired as another bug because this is not related to
> keyboard.

Bug 1152172 opened for this issue.

Comment 13

[Dylan Oliver [:doliver]]

(In reply to peter chang[:pchang][:peter] from comment #11)
>  From my
> perspective, we can make our own in-house app more user-friendly for
> CopyPaste. Dylan, does it make sense to you?

Not really -- I think we should make CopyPaste more user-friendly for all apps. Adding whitespace in the email app is a small band-aid for the larger problem.

Comment 14

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

It would help if we render the two bars above the two carets.

Comment 15

[Peter Chang[:pchang]]

(In reply to Omega Feng [:Omega] [:馮於懋] (please ni?) from comment #14)
> It would help if we render the two bars above the two carets.
Omega, I still don't understand why we need these two bars since we already have the carets.
In my opinion, it's kind of workaround to avoid the caret visibility problem.

Comment 16

[Peter Chang[:pchang]]

(In reply to Dylan Oliver [:doliver] from comment #13)
> (In reply to peter chang[:pchang][:peter] from comment #11)
> >  From my
> > perspective, we can make our own in-house app more user-friendly for
> > CopyPaste. Dylan, does it make sense to you?
> 
> Not really -- I think we should make CopyPaste more user-friendly for all
> apps. Adding whitespace in the email app is a small band-aid for the larger
> problem.

So far, I don't have good solution to fix this problem from gecko side without changing the content dom tree.

Comment 18

[GitHub Autolander Bot]

Attachment: [gaia] jrburke:bug1149070-text-selection-email-compose > mozilla-b2g:master

Comment 19

[James Burke [:jrburke]]

Comment on attachment 8591085 [details] [review]
[gaia] jrburke:bug1149070-text-selection-email-compose > mozilla-b2g:master

I agree with :doliver that it would be good to get a generic fix for this issue. However, if time constraints do not allow for that, then this pull request can be used to modify email to help give some space to see the second carat. However I will hold off on asking for review to give the generic fix more time.

Comment 20

[Teri Wen [:twen]]

Bug 1021499 is fixed, but this email issue still exist. 

V3.0 
Gaia-Rev        b4c949cdc780893897c9b45c1adea46e2eb694ff
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/37d60e3b8be6
Build-ID        20150426160201
Version         40.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150426.193323
FW-Date         Sun Apr 26 19:33:34 EDT 2015
Bootloader      L1TC000118D0

V2.2
Gaia-Rev        265ca0bc9408c21fc4b25a259fcee7fb642cd06b
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/1908685d798d
Build-ID        20150426162504
Version         37.0
Device-Name     hammerhead
FW-Release      5.1
FW-Incremental  eng.cltbld.20150426.195739
FW-Date         Sun Apr 26 19:57:56 EDT 2015
Bootloader      HHZ12d


Hi James, please advise. Thanks.

Comment 21

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

Actually bug 1021499 doesn't resolved overlap issue. It increases touch area of caret so that even if caret is truncated by anything such as keyboard, user still able to drag caret. For more info, please refer bug 1021499 or p.14 of UX spec [1] for more details.

[1] UX spec: https://bug921965.bugzilla.mozilla.org/attachment.cgi?id=8548759#14

Comment 22

[James Burke [:jrburke]]

:twen - it sounds like there is some confusion then about what bug 1021499 addresses. Comment 17 indicates this issue would be solved by the fix where comment 21 does not, and given the observed behavior, comment 21 seems to be correct. So probably unmarking this bug as a duplicate would make sense.

As to getting a general fix for the caret when selections are close to the keyboard for 2.2, someone familiar with the work in Core::Selection would need to comment more. 

If a 2.2 fix seems unlikely, then I can ask for review on the email-specific fix for 2.2 and land that fix, but it would be good to at least have a bug number to point to for a more general fix long term.

Comment 23

[Teri Wen [:twen]]

Reopen bug.

Comment 24

[Andrew Sutherland [:asuth] (he/him)]

If doing the hack-fix in email, .cmp-body-text is the wrong place to apply padding.  It will make things look broken when the user replies to an HTML message.  The padding needs to go on the bottom of .cmp-body-html or in its own distinct hacky DOM node below .cmp-body-html.  (Specifically, we try and make the editable text area flow directly into the non-editable HTML area.  Having whitespace there that the user cannot edit and does not actually exist would likely lead the user to believe there will be one or more newlines between the two pieces of content when in fact none will exist.)

Comment 25

[howie [:howie]]

Hi James, the general fix is unlikely in 2.2, can you ask review for the email-specific fix patch?

Hi Peter, can you open another bug that is for general fix?

Thank you all!

Comment 26

[James Burke [:jrburke]]

Comment on attachment 8591085 [details] [review]
[gaia] jrburke:bug1149070-text-selection-email-compose > mozilla-b2g:master

Adjusted and rebased workaround in email app. Adds the padding to the .cmp-body-html element instead of the cmp-body-text.

Comment 27

[GitHub Autolander Bot]

Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.

Comment 28

[James Burke [:jrburke]]

Temporarily moving to email component to pass autolander checks.

Comment 29

[Peter Chang[:pchang]]

(In reply to howie [:howie] from comment #25)
> Hi James, the general fix is unlikely in 2.2, can you ask review for the
> email-specific fix patch?
> 
> Hi Peter, can you open another bug that is for general fix?
> 
> Thank you all!
Bug 1159550 is created.

Comment 30

[GitHub Autolander Bot]

https://github.com/mozilla-b2g/gaia/pull/29462

Autolander could not land the pull request due to not having collaborator rights. This is possibly due to a tree closure. Please check the tree status and request checkin again once the tree is open.

Comment 31

[GitHub Autolander Bot]

http://docs.taskcluster.net/tools/task-graph-inspector/#4kC8OrlSTYafEUH6TamOyA

The pull request failed to pass integration tests. It could not be landed, please try again.

Comment 32

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/abb3a635dbfe9162c9923d0318d59f06abfda25f

Comment 33

[James Burke [:jrburke]]

Comment on attachment 8591085 [details] [review]
[gaia] jrburke:bug1149070-text-selection-email-compose > mozilla-b2g:master

[Approval Request Comment]

[Bug caused by] (feature/regressing bug #):
Mitigation for the second selection caret to be seen since it cannot draw on top of the keyboard. The mitigation is adding some space below the text entry area.

[User impact] if declined:
Unable to see the second caret of the selection.

[Testing completed]:
On device, with a new compose and a reply to an HTML message.

[Risk to taking this patch] (and alternatives if risky):
Very low. Just a bit of padding added to the bottom

[String changes made]:
none

Comment 34

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/209bf4d6fcb16ea6834b8bd86976c012e5914fe6

Comment 35

[Brogan Zumwalt [Inactive]]

Issue still reproduces on Flame 2.2 and 3.0

Highlighting all text in email body by tapping select all has no right caret visible. No added white space visible above keyboard.

Device: Flame 2.2
Build ID: 20150505002501
Gaia: 772a9491909abd02dc67278dd453746e2dd358a8
Gecko: 2df83538ae20
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

Device: Flame 3.0 Master
Build ID: 20150505010204
Gaia: 70077825aab2c7a79611befb40a5fe7e610d5443
Gecko: 102d0e9aa9e1
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

Comment 36

[James Burke [:jrburke]]

To clarify what the fix did: it just added space to the bottom of the text entry, so that the compose area can be scrolled to show part of second caret. It is not initially visible though if typing one line of text, selecting all, with the default signature in place. The user will need to scroll the area to be able to see part of the second caret though.

The issue is similar if there are 5 lines of text typed, user scrolls all the way up and then does select all with the keyboard showing: the full selection is not visible on the screen, the user needs to scroll down to see the end of the selection. With just one line of text with the default signature just happens to fit perfectly above the keyboard, so the extra space is not visible until a scroll is done.

Comment 37

[Brogan Zumwalt [Inactive]]

Apologies. With clarification from comment 36, issue is verified fixed on Flame 2.2 and 3.0 (see same environmental variables from comment 35)

Highlighting all text in email body by tapping select all then scrolling down shows both left and right text selection carets.