1152263 - [Messages][Text Selection] Hard to drag the caret when there is an attachment in the beginning of a message

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Jeremy Chen ([e-mail])]

Description:
After selecting text in a message with an attachment in the beginning, it is hard to drag the start caret.

Repro Steps:
1) Open Messages app> Open a message thread, or start a new one
2) Send a message with an attached file
message is like this:
  
  ------------
  |Attachment|
  ------------
   some text

3) Long press on the sent message
4) Tap "Select text"
5) Drag the start caret

Actual:
Hard to drag the carat

Expected:
Carat should be dragable

Environmental Variables:
Device: Flame 3.0
Build ID               20150408115226
Gaia Revision          edb97837286128eb705f852533e593bf0ec435f4
Gaia Date              2015-04-07 21:44:54
Gecko Revision         n/a
Gecko Version          40.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150406.194015
Firmware Date          Mon Apr  6 19:40:27 EDT 2015
Bootloader             L1TC10011880

Repro frequency: always
See attached: Video - http://youtu.be/y4WDJnwYNF4

Comment 1

[howie [:howie]]

ni Peter to check the possible root cause.

Comment 2

[Peter Chang[:pchang]]

Jeremy, please help to evaluate the effort to fix this bug first.

howie, do we need to consider the selection of the attachments in message?

Comment 3

[howie [:howie]]

Triage: not blocking but put as priority bug.

Hi Julien, is that possible we make the attachment non-selectable?

Comment 4

[Julien Wajsberg [:julienw]]

As I understand it, it's already non-selectable in CSS.

NI Steve who knows better about this.

Comment 5

[Steve Chung [:steveck]]

No we didn't :(

I gave a patch to a bug that remove the attachment selection few months ago, but calling selectAllChildren API will still select the element even it set to select none already. Since we agree that selectable attachment is fine in 2.2, the patch is not applied yet. But I can give a patch to set the attachment to select: none for further testing.

Comment 6

[Julien Wajsberg [:julienw]]

It could be a good idea Steve. Can you investigate a little further here?

Comment 7

[Steve Chung [:steveck]]

Attachment: 0001-Disable-attachment-selection.patch

Hi Jeremy, this is a simple patch that set the attachment user-select to none, but the use experience is still not pleasant because the cursor still not stick to finger's position. The only benifit is the attachment will not be selected again when trying to select attachment again. Could you please verify the root cause of it?

I'm alo wondering if it's possible to "skip" the element which is already set user-select to none while calling selectAllChildren API. Is it possible/reasonable to perform this automatically?

Comment 8

[Jeremy Chen ([e-mail])]

(In reply to Steve Chung [:steveck] from comment #7)
> Created attachment 8591545 [details] [diff] [review]
> 0001-Disable-attachment-selection.patch
> 
> Hi Jeremy, this is a simple patch that set the attachment user-select to
> none, but the use experience is still not pleasant because the cursor still
> not stick to finger's position. The only benifit is the attachment will not
> be selected again when trying to select attachment again. Could you please
> verify the root cause of it?

Looks good to me.
I just found the root cause of this bug, and I'll upload a patch very soon.

> I'm alo wondering if it's possible to "skip" the element which is already
> set user-select to none while calling selectAllChildren API. Is it
> possible/reasonable to perform this automatically?

According to Bug 1130297 comment 1, I think skipping element with user-select: none while calling selectAllChildren API is not possible for now. Maybe sending an event to gecko and let gecko finish the selection could be one approach, but I'm not sure if it's a good idea.

Comment 9

[Jeremy Chen ([e-mail])]

Attachment: Ensure Matrix4x4::ProjectRectBounds being functional for Rect with zero width/height. r=mattwoodrow

Rect with zero width/height would make max value equal to min value in ProjectRectBounds, so a pure zero Rect, Rect(0, 0, 0, 0), would be early returned.

In this bug, we are trying to drag the caret on a "\n" element, which is used very often in apps for typesetting. A Rect with zero width and non-zero height is sent into ProjectRectBounds, and the height of rect is forced to be zero.  It is not reasonable to set its height to be zero here. I check the blam history, but I can't get the idea why we should take the equal condition into consideration. So, I remove the equal condition.

Hi Matt, would you have some availability to review this patch, as a solution to this bug?

Comment 10

[Matt Woodrow (:mattwoodrow)]

How are we getting a rect with a width of 0?

ProjectRectBounds is used for converting a rect in 'screen-space' back into a rect in the coordinate space of the element.

If the screen space has a width of 0, then the object won't be drawn, so why do we need this to work?

Comment 11

[Jeremy Chen ([e-mail])]

(In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> How are we getting a rect with a width of 0?

In cut/copy/paste, we'll calculate frame rect to determine where to place/drag the selection carets. Since many pure newline text nodes ("\n") have been used in apps for typesetting, it would be easy to get a rect with zero width.

> ProjectRectBounds is used for converting a rect in 'screen-space' back into
> a rect in the coordinate space of the element.
> 
> If the screen space has a width of 0, then the object won't be drawn, so why
> do we need this to work?

Would it be better to let ProjectRectBounds only be responsible to the calculation of coordinate projection, and let the caller of ProjectRectBounds take care of whether to draw the object or not?

Comment 12

[Jeremy Chen ([e-mail])]

try result looks positive:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7fdffc5ada9

Comment 13

[Matt Woodrow (:mattwoodrow)]

(In reply to Jeremy Chen [:jeremychen] from comment #11)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> > How are we getting a rect with a width of 0?
> 
> In cut/copy/paste, we'll calculate frame rect to determine where to
> place/drag the selection carets. Since many pure newline text nodes ("\n")
> have been used in apps for typesetting, it would be easy to get a rect with
> zero width.

Ok, that much makes sense.

What I don't understand is why we're using ProjectRectBounds on these empty text nodes, and why we would want to get a non-0 answer back?

If they have a width of 0, then they can't be clicked on, so it shouldn't matter.

It sounds like (from comment 9) that you care about the height, even though the width is 0 (before and after ProjectRectBounds). What is that used for?

> 
> Would it be better to let ProjectRectBounds only be responsible to the
> calculation of coordinate projection, and let the caller of
> ProjectRectBounds take care of whether to draw the object or not?

This is the case already.

Comment 14

[Jeremy Chen ([e-mail])]

(In reply to Matt Woodrow (:mattwoodrow) from comment #13)
> (In reply to Jeremy Chen [:jeremychen] from comment #11)
> > (In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> > > How are we getting a rect with a width of 0?
> > 
> > In cut/copy/paste, we'll calculate frame rect to determine where to
> > place/drag the selection carets. Since many pure newline text nodes ("\n")
> > have been used in apps for typesetting, it would be easy to get a rect with
> > zero width.
> 
> Ok, that much makes sense.
> 
> What I don't understand is why we're using ProjectRectBounds on these empty
> text nodes, and why we would want to get a non-0 answer back?
> 
> If they have a width of 0, then they can't be clicked on, so it shouldn't
> matter.
> 
> It sounds like (from comment 9) that you care about the height, even though
> the width is 0 (before and after ProjectRectBounds). What is that used for?

 -----------------
 | example text  |
 | ^          ^  |
 -----------------
It can be seen from the graph above, selection carets are a bit under the selected text with a shift. To achieve dragging caret to change selection range feature, we simulate the shift+click action like desktop browser does. It means that when we drag a caret to a specific position, we calculate the caret vertical shift (see [1]) first. Then, according to the shift, we can simulate a shift+click action on the right position on the selected text to change the selection range. 

According to the original early return design in [2], we'll get a frameRect with pure zero Rect(0,0,0,0) instead of a Rect(pointed_text_x, pointed_text_y, 0, pointed_text_height) in [3]. This leads to a mis-calculation for caret vertical shift and trigger a shift+click on a wrong and non-selectable position (the root cause of this bug). That is why I suggest to fix the condition in the uploaded patch. When zero width/height appears, min=max will hold, even we don't early return pure zero Rect, we can still get zero width/height in the end of projection [4].

It seems that we can't 100% avoid starting to drag caret on an "\n" text node for now. So I write this patch to fix this bug and try not to affect the original functionality of ProjectRectBounds in the meantime.

[1] https://dxr.mozilla.org/mozilla-central/source/layout/base/SelectionCarets.cpp#219
[2] https://dxr.mozilla.org/mozilla-central/source/gfx/2d/Matrix.cpp#223
[3] https://dxr.mozilla.org/mozilla-central/source/layout/base/SelectionCarets.cpp#865
[4] https://dxr.mozilla.org/mozilla-central/source/gfx/2d/Matrix.cpp#227 

> > Would it be better to let ProjectRectBounds only be responsible to the
> > calculation of coordinate projection, and let the caller of
> > ProjectRectBounds take care of whether to draw the object or not?
> 
> This is the case already.

Comment 15

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8591583 [details] [diff] [review]
Ensure Matrix4x4::ProjectRectBounds being functional for Rect with zero width/height. r=mattwoodrow

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

Ok

Comment 16

[Jeremy Chen ([e-mail])]

The gaia patch has been landed in Bug 1152502 already, so please only land the gecko patch. thx! :)

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/e76d8a5c8a18

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e76d8a5c8a18