1152172 - [Text Selection] Left caret disappeared in view email

Status: VERIFIED FIXED

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

Description

[Teri Wen [:twen]]

Attachment: Left caret disappeared

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

### Actual
Selection only has right caret.

### Expected
Selection with both carets.

### Version
Gaia-Rev        485322797a069e5b185931e87d15d4e921e25b64
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/f766e5052b39
Build-ID        20150406162505
Version         37.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150406.200227
FW-Date         Mon Apr  6 20:02:39 EDT 2015
Bootloader      L1TC000118D0

Comment 1

[Teri Wen [:twen]]

Mark blocking 2.2 because feature broken.

Comment 2

[Peter Chang[:pchang]]

Ting-Yu, please help to check where is the left caret.

Comment 3

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

Attachment: Left selection-caret.png

After reproducing this issue by the steps in description, I set "left: 32em" to the div with id="cardContainer" in WebIDE, and see the left caret as in attached screenshot.

Comment 4

[howie [:howie]]

Hi Andrew, can you help to check why this happens like the picture in Comment 3 ? And is this something email app can fix? Thanks.

Comment 5

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

It seems like the selection logic is choosing a weird root common element for select all.  What text is being long pressed on to trigger the select all?

Comment 6

[Teri Wen [:twen]]

In the example of the screenshot, I long pressed the message "b14285331236035016" and tapped select all.

Comment 7

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

I found that after select all, the range would be divided by 3 ranges by nsRange::ExcludeNonSelectableNodes. Second and third range is text of subject and content. That's ok. The problem is first range. The first range contains an input which is search bar of mail. But the content of input has independent selection which means input or textarea should be excluded. So I write a patch which ignores content with independent selection. Does it make any sense?

Comment 8

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

Attachment: Ignore content with independent selection when excluding non-selectable nodes.

Comment 9

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

I think in general the behaviour of "select all" isn't particularly useful outside of input fields, so this is a somewhat odd issue.  I've tried to start a discussion on that at https://groups.google.com/forum/#!topic/mozilla.dev.gaia/3tBQxCN5INQ.

But for this bug, it seems like:

- There may be a bug in the selection code, and :mtseng's patch may address that

- But fixing that bug doesn't actually fix the flaw in email, which is that email's marking elements as selectable is not impacted by whether they are on an (in)active card.  However, fixing that bug may hide the only instance of this being a problem in the email app since we have a very limited set of cases we mark things as selectable and most of the time the only sane things to try and select are the contents of input elements.

  Probably the least horrible selector strategy is to leave the "* { -moz-user-select: none; }" as our one horrible wildcard selector.  And then in the cases where we re-enable selection, we do it by prepending a ".center" descendant selector to the existing selector.  So ".tng-signature-text { -moz-user-select: text; }" becomes ".center .tng-signature-text { -moz-user-select: text; }".  Alternately we could do ".before *, .after * { -moz-user-select: none !important; }" (or further increase the specificity), but I worry about the cost of that selector.

Adding a needinfo to :jrburke for his thoughts on altering our scoping strategy to activate selection.

Comment 10

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

I think "-ms-user-select:element" behavior in IE would help in this case. See this [1]. Basically, this css attribute will bound selection range to element. So we don't select entire document if we select all. Unfortunatelly, we don't support it yet.

[1]: http://blogs.msdn.com/b/ie/archive/2012/01/11/controlling-selection-with-css-user-select.aspx

Comment 11

[Mats Palmgren (inactive)]

Comment on attachment 8591513 [details] [diff] [review]
Ignore content with independent selection when excluding non-selectable nodes.

My gut feeling is that this is wrong, but I haven't looked in detail yet.
I'd like to understand the problem better.  Can you make a small testcase
that demonstrates the problem?  I gather from the comments that there is
an <input> in some "hidden" place that is selectable and thus affects
the Selection range, but that seems like the correct behavior to me.

Comment 12

[Mats Palmgren (inactive)]

Why do "inactive cards" have CSS boxes at all?  It seems better if they
had display:none which would avoid this problem (and would be better
for performance, memory footprint etc).

Comment 13

[James Burke [:jrburke]]

:asuth - As to the email doing a .center scoped selector, that makes sense, and I will work up an email patch just to have it available, just in case that ends up being the fix. Hopefully just adding the .center is enough and not the extra !important ones for .before and .after.

:mats - on the question about inactive cards not using display:none in the DOM: we can have some cards that overlap or show together on a screen, so we cannot do that as a generic solution, and so I think we would still need the scoped css selector fix. 

I am also a bit concerned if there is a perf hit for turning off display none just before triggering a css transform animation between cards. Our DOM structures are normally not very deep either. However, it is definitely worth it for me to do an experiment around using display: none with options to not do it for overlapping cases. I will take that as a research item to try longer term for email, and for this bug, for an email-targeted fix, do the more targeted .center CSS selection.

Comment 14

[GitHub Autolander Bot]

Attachment: [gaia] jrburke:bug1152172-email-select-all > mozilla-b2g:master

Comment 15

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

(In reply to James Burke [:jrburke] from comment #13)
> I am also a bit concerned if there is a perf hit for turning off display
> none just before triggering a css transform animation between cards.

I just want to second this concern.  It may sound superstitious, but it's been difficult to keep the card transition animations working.  And a bigger complication for us now is that since we no longer develop on devices that will ship, only on Flame, "it works on Flame" isn't really confidence-building.  (Unless we know that all the devices shipping are as/more-powerful than Flame with APZ cranked up to the same levels, etc., which I don't think is the case.)

Comment 16

[James Burke [:jrburke]]

Attachment: Email select all with gaia/email patch applied

Autolander added the pull request that has the gaia/email change to scope the text selection to .center active cards. This screenshot shows what it looks like with that patch applied.

Note that the title ends up selected, because of the text selection CSS behavior described in comment 9, so I do not believe the email app can control that behavior given the set of CSS properties we have available.

The left caret is visible though with this change.

I will hold off on asking for review until there is more consensus that this should be the way to fix the issue for now.

Comment 17

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

Comment on attachment 8591929 [details] [review]
[gaia] jrburke:bug1152172-email-select-all > mozilla-b2g:master

This looks good to me, and I suspect we're going to need it, so speculatively r+'ing.

Comment 18

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

Assigned to James since him provided a workable gaia patch. I'll create a follow-up bug for gecko part once I finish the small testcase which Mats ask for.

Comment 19

[James Burke [:jrburke]]

Merged in gaia master:
https://github.com/mozilla-b2g/gaia/commit/4ba5ce17a076d8d585d73de7c6955fa329bc73cc

Manually merged given the component does not match, although in retrospect I should have switched that to gaia email component and will do that now

Comment 20

[James Burke [:jrburke]]

Comment on attachment 8591929 [details] [review]
[gaia] jrburke:bug1152172-email-select-all > mozilla-b2g:master

[Approval Request Comment]
Asking for 2.2 inclusion since I believe text selection is part of the 2.2 release, this was flagged as blocking-b2g: 2.2? and the fix is very simple.

[Bug caused by] (feature/regressing bug #):
The main text selection work done for email in bug 1092440.

[User impact] if declined:
Select all text when done on a text email body will not get what is expected.

[Testing completed]:
On flame device, see screenshot in comment 9.

[Risk to taking this patch] (and alternatives if risky):
Super low. Just a more specific CSS selector than what was used before, and since the more specific part is to scope it just to the visible card, should be very safe.

[String changes made]:
none

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[Teri Wen [:twen]]

Verified fixed on v2.2. Verification on v3.0 is blocked by bug 1154604.

v2.2
Gaia-Rev        16e948bfaaa15dbc0200135d52f16257b4eab193
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/0eec28e78eb1
Build-ID        20150414162502
Version         37.0
Device-Name     hammerhead
FW-Release      5.1
FW-Incremental  eng.cltbld.20150414.201650
FW-Date         Tue Apr 14 20:17:04 EDT 2015
Bootloader      HHZ12d

Comment 23

[Teri Wen [:twen]]

Verified fixed on v3.0.

Gaia-Rev        777d01f4a2c7b41c4b02e3cf87715714ccc0590b
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/53ceefb0e1c8
Build-ID        20150415160205
Version         40.0a1
Device-Name     hammerhead
FW-Release      5.1
FW-Incremental  eng.cltbld.20150415.192113
FW-Date         Wed Apr 15 19:21:33 EDT 2015
Bootloader      HHZ12d