1140625 - [Messages][Text Selection] Carets appear above the selection of a vcf file's text in the messages app

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Derek Harris [:DerekH] - Dont need info. Last day 6/5/15]

Description:
The carats for text selection appear above the selected text when selecting a vcf file within a message thread


Repro Steps:
1) Update a Flame to 20150306010207
2) Open Messages app> Open a message thread, or start a new one
3) Send a message with an attached vcf file (contact info)
4) Long press on the sent vcf file
5) Tap "Select text"


Actual:
The carats appear above the selected text


Expected:
Carats appear below the selected text and are indicating where the selection ends at the tip of the carat

Environmental Variables:
Device: Flame 3.0 (319mb)(Kitkat)(Full Flash)
Build ID: 20150306010207
Gaia: 7a91c16bfa348be8b25e09719178efa051512988
Gecko: 0189941a3fd5
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 39.0a1 (Master)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0

Repro frequency: 10/10
See attached: Logcat, Video - http://youtu.be/czX7GHOTFEs

Comment 1

[Derek Harris [:DerekH] - Dont need info. Last day 6/5/15]

Adding QAwanted for a logcat
_________________________________________________________________________

This issue DOES occur on Flame 2.2

The carats appear above the selected text

Device: Flame 2.2 (319mb)(Kitkat)(Full Flash)
Build ID: 20150305002528
Gaia: 89af288bad6751248ff84504fa898206fee127fe
Gecko: 6d8d294aa8f3
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0 (Master)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0


This issue does NOT occur on Flame 2.1

The option to share a contact via messages was no yet implemented

Device: Flame 2.1 (319mb)(Kitkat)(Full Flash)
Build ID: 20150302001220
Gaia: 5d3479fdd438412adee4452720856b6b771fe5cd
Gecko: 9bf4c663241f
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 34.0 (2.1)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Comment 2

[KTucker [:KTucker][Inactive 3/4/2016]]

Gerry, can you take a look at this please?

Comment 3

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

Attachment: Text selection locat

Requested logcat for qawanted.

Comment 4

[Julien Wajsberg [:julienw]]

QA: can you check if this happens with other attachment types, please ?

Comment 5

[Shine(Leave from Mozilla)]

Attachment: video

This problem can be repro on latest build of Flame 2.2/Flame 3.0 with attachments of video/music/gallery/camera/vcf:
1. Long press the message with video/music/gallery/camera/vcf file attachment to select text, the start and end carats appear above the selected text.
2. Long press the message both with video/music/gallery/camera attachment and body text to select text, only the start carat appears above the selected text.
3. After selecting the message, there's an unwanted blue block displayed on the attachment icon.
See attachment: Flame3.0_video.MP4 & logcat_flame3.0_1135.txt
Occurrence time: 11:35
Rate: 5/5

Flame 2.2 build:
Build ID               20150308002503
Gaia Revision          166491b92278dc9e648f8d49ab02d9ca00d74421
Gaia Date              2015-03-06 18:26:27
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/a48af0b5a6e4
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150308.052515
Firmware Date          Sun Mar  8 05:25:25 EDT 2015
Bootloader             L1TC000118D0

Flame 3.0 build:
Build ID               20150308160204
Gaia Revision          fea83511df9ccba64259346bc02ebf2c417a12c2
Gaia Date              2015-03-08 06:36:28
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/eab4a81e4457
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150308.192120
Firmware Date          Sun Mar  8 19:21:31 EDT 2015
Bootloader             L1TC000118D0

Comment 6

[Shine(Leave from Mozilla)]

Attachment: logcat

Comment 7

[Peter Chang[:pchang]]

Jeremy, please help to check the calculation of end caret's position. It looks like the range is correct but the position to draw end caret is wrong.

Comment 8

[Steve Chung [:steveck]]

For the starting caret position, please see bug 1127648 that we might have no plan to fix it in the near future because of bug 1127648 comment 8 and bug 1127648 comment 9. But there is another serious issue that vcard's file name should not be selectable. Will file another bug for this.

Comment 9

[Jeremy Chen ([e-mail])]

It looks like when sending a message with attachments inside, an extra <span> node is created as text node's parent. The extra frame level leads the mis-calculation for end caret position. Both endFrame and endOffset seem wrong in this case.

Comment 10

[Peter Chang[:pchang]]

Set as 2.2 blocker because of it's related to copypaste feature

Comment 11

[Jeremy Chen ([e-mail])]

Attachment: Add recursive call in nsSelection.cpp to find the right caret frame for node offset. (wip v1)

I try to use DONT_DO_THIS_YET code in [1]. It seems that the function was initially designed to be called recursively if the node contains children. This patch can fix this bug, but few fails occur on try server (please see try results in [2]).

[1] https://dxr.mozilla.org/mozilla-central/source/layout/generic/nsSelection.cpp#1858
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=440b868ff2cd

Comment 12

[Jeremy Chen ([e-mail])]

Attachment: Add iterative call in SelectionCarets.cpp to find the right caret frame for node offset. (wip v2)

Comment 13

[Jeremy Chen ([e-mail])]

Comment on attachment 8577853 [details] [diff] [review]
Add recursive call in nsSelection.cpp to find the right caret frame for node offset. (wip v1)

I can keep working on wip-v1.patch and try to fix the try errors. An alternative would be patching SelectionCarets.cpp only, like wip-v2.path does, to avoid the possibility to affect other features in Gecko.

Roc, can you take a look at this please? I'm not sure which way would be more appropriate.

Comment 14

[Jeremy Chen ([e-mail])]

Comment on attachment 8577853 [details] [diff] [review]
Add recursive call in nsSelection.cpp to find the right caret frame for node offset. (wip v1)

I can keep working on wip-v1.patch and try to fix the try errors. An alternative would be patching SelectionCarets.cpp only, like wip-v2.path does, to avoid the possibility to affect other features in Gecko.

roc, can you take a look at this please? I'm not sure which way would be more appropriate.

Comment 15

[Jeremy Chen ([e-mail])]

try result for wip-v2.patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2d68a8e8d7c

Comment 16

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

Jeremy, have you got a simple testcase for this bug?

Comment 17

[Jeremy Chen ([e-mail])]

An simple testcase is in https://jsfiddle.net/eqx37fps/8/. Enable caret and refresh the test web page. It can be seen that both cases can select text with right range. But caret in the case with an extra <span> node level would be wrongly positioned.

Comment 18

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

I think the primary issue here is that in nsFrameSelection::GetFrameForNodeOffset, for the lastRange, the last child of the <p> element is a TextNode with no frame, which causes nsFrameSelection::GetFrameForNodeOffset to fall back to code with the comment
          // If we're at a collapsed whitespace content node (which
          // does not have a primary frame), just use the original node
          // to get the frame on which we should put the caret.
which then completely fails.

The TextNode has no frame because as an optimization, we avoid creating frames for whitespace-only textnodes at the end of a block, when they can't have any layout or rendering effect. If that optimization was disabled I think the current nsFrameSelection::GetFrameForNodeOffset code would work, because we'd place the caret based on the location of that whitespace-only text node, which is at the end of the block.

There are a couple of things we could do here. One is to do something like what GetTextFrameForContent does in nsRange: disable the textnode optimization for the specific textnode we care about, and ensure a textframe gets created for that node. That's a bit tricky in SelectionCarets since flushing layout means all the nsIFrame* pointers we already have could be invalid, so call callers to nsFrameSelection::GetFrameForNodeOffset would have to be carefully audited to handle flushing.

It's probably better to fix nsFrameSelection::GetFrameForNodeOffset to compute the right results without flushing. Instead of the patches here, I think the right thing to do is fix the case where textNode has no frame. In that case I think we should recursively call GetFrameForNodeOffset on the previous child node, passing in its child count as the offset to find its end. Or, if we're searching forward for the start range and we find a missing text frame, recursively call GetFrameForNodeOffset on the next child after the text node, passing in 0 as the offset.

While we're here, the call to mPresShell->FlushPendingNotifications(Flush_Layout) in SelectionCarets::UpdateSelectionCarets is in the wrong place. It has to be before we get rootFrame, since it could destroy rootFrame and replace it with some other object. Please fix that.

Comment 19

[Jeremy Chen ([e-mail])]

Hi roc, I'm a bit confused with what you said in comment 18.

In the simple testcase given in comment 17, two cases are given, the abnormal one on top and the normal one on bottom. In the abnormal case, the last child of <p> is SpanNode, not TextNode. Since SpanNode is not a TextNode, it won't even pass the isTextNode condition to get to the whitespace-only textnode case as you mentioned.

According to the dumped frame tree, it looks like nsFrameSelection::GetFrameForNodeOffset would return SpanNode frame, not the TextNode frame, since TextNode is no longer a child of <p>. Therefore, I think the problem is that the unexpectedly inserted SpanNode changes the relationship between <p> and TextNode (from parent-child to grandparent-grandchild), which causes miscalculation for frame and offset.

Comment 20

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

(In reply to Jeremy Chen [:jeremychen] from comment #19)
> In the simple testcase given in comment 17, two cases are given, the
> abnormal one on top and the normal one on bottom. In the abnormal case, the
> last child of <p> is SpanNode, not TextNode.

No, it's not. After the <span>, there is a text node containing exactly one whitespace character.

If you delete that text node, so the HTML is "</span></p>", you'll see that the test works.

Comment 21

[Jeremy Chen ([e-mail])]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> (In reply to Jeremy Chen [:jeremychen] from comment #19)
> > In the simple testcase given in comment 17, two cases are given, the
> > abnormal one on top and the normal one on bottom. In the abnormal case, the
> > last child of <p> is SpanNode, not TextNode.
> 
> No, it's not. After the <span>, there is a text node containing exactly one
> whitespace character.
> 
> If you delete that text node, so the HTML is "</span></p>", you'll see that
> the test works.

I made a new test as you said, please see https://jsfiddle.net/eqx37fps/12/ The position of end caret is still wrong.

Comment 22

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

(In reply to Jeremy Chen [:jeremychen] from comment #21)
> I made a new test as you said, please see https://jsfiddle.net/eqx37fps/12/
> The position of end caret is still wrong.

Oh, you're quite right. Yes, we'll need to recursively descend into the span in that case.

Comment 23

[Jeremy Chen ([e-mail])]

Attachment: Part1: Add recursive call in GetFrameForNodeOffset (wip)

With this wip, the test in [1] can be passed, and the bug seems to be fixed on my local device. I've pushed this patch to try (see [2]). Some tests could be failed, so I'll keep working on fixing them.

[1] simple test https://jsfiddle.net/eqx37fps/12/
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=5047382569aa

Comment 24

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

Attachment: Part2: Recursive call GetFrameForNodeOffset if text node has no frame.

Comment 25

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

Comment on attachment 8579928 [details] [diff] [review]
Part2: Recursive call GetFrameForNodeOffset if text node has no frame.

Hi roc,
This patch handle the issue which is text node without text frame. As you mentioned, If text node without frame, get previous or next node and call GetFrameForNodeOffset again.

Here is try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5eb1cfaeb4e

Comment 26

[Jeremy Chen ([e-mail])]

Attachment: Part1: Add recursive call in GetFrameForNodeOffset if node has children (wip)

Comment 27

[Jeremy Chen ([e-mail])]

Attachment: PartX: Fix scroll_selection_into_view test to make its function remain.

After applying Part1 patch, which we let GetFrameForNodeOffset recursively descend into the leaf node, the test_scroll_selection_into_view would fail. It can be seen from the dom structure in [1], if we want to make the test functional, we need to ensure only scroll the element we want (in this test, the <span> node). Could I just remove the text node decended to the <span> node?

[1] https://dxr.mozilla.org/mozilla-central/source/layout/base/tests/chrome/scroll_selection_into_view_window.html

Comment 28

[Jeremy Chen ([e-mail])]

Attachment: Part1: (v4) Add recursive call in GetFrameForNodeOffset.

Comment 29

[Jeremy Chen ([e-mail])]

Attachment: Part3: Fix a bug in bug414526 so we can reopen the test.

Looks like our part1 patch can fix a bug in bug414526, so we can reopen the test.

Comment 30

[Jeremy Chen ([e-mail])]

Attachment: Part4: Fix scroll_selection_into_view test to make its function remain.

For testing scroll_into_view, we shall make sure only scroll the element we want. We can ensure this by removing the text inside the scrolled element.

Comment 31

[Jeremy Chen ([e-mail])]

try result for part1~4: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e107694e090

Comment 32

[Jeremy Chen ([e-mail])]

Attachment: Part5: Move Flush_Layout ahead in SelectionCarets::UpdateSelectionCarets.

Move mPresShell->FlushPendingNotifications(Flush_Layout) before we get rootFrame, as comment 18 said.

Comment 33

[Jeremy Chen ([e-mail])]

try result for part1~5: https://treeherder.mozilla.org/#/jobs?repo=try&revision=096e5435b0f4

Comment 34

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

Comment on attachment 8582261 [details] [diff] [review]
Part4: Fix scroll_selection_into_view test to make its function remain.

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

It's not really clear to me why we need to do this. The test looks correct as it's currently written. Can you explain in more detail?

Comment 35

[Jeremy Chen ([e-mail])]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #34)
> Comment on attachment 8582261 [details] [diff] [review]
> Part4: Fix scroll_selection_into_view test to make its function remain.
> 
> Review of attachment 8582261 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It's not really clear to me why we need to do this. The test looks correct
> as it's currently written. Can you explain in more detail?

With the part1 patch, we let GetFrameForNodeOffset recursively descend into the leaf node. It can be seen from the dom structure in [1], if we want to make the test functional, we need to ensure only scroll the element we want (in this test, the <span> node, not the <text> node). So I just remove the text node, the decendant of the <span> node.

[1] https://dxr.mozilla.org/mozilla-central/source/layout/base/tests/chrome/scroll_selection_into_view_window.html

Comment 36

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

Comment on attachment 8582261 [details] [diff] [review]
Part4: Fix scroll_selection_into_view test to make its function remain.

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

Yeah OK. This changes behavior of scroll_selection_into_view but I guess in some cases the new behavior will be better.

Comment 37

[Jeremy Chen ([e-mail])]

try looks good except a known issue M12 for B2G ICS Emulator debug.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=438ce1eca7e7

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/1b76bac87d82
https://hg.mozilla.org/integration/b2g-inbound/rev/65d48bfeb654
https://hg.mozilla.org/integration/b2g-inbound/rev/7b4e1bd6c8aa
https://hg.mozilla.org/integration/b2g-inbound/rev/ce21be83c48b
https://hg.mozilla.org/integration/b2g-inbound/rev/8df7f01875e8

Comment 39

[Mats Palmgren (inactive)]

This doesn't look safe to me. 'startFrame' might be destroyed after the flush:

nsIFrame* startFrame = FindFirstNodeWithFrame...
...
mPresShell->FlushPendingNotifications(Flush_Layout);
...
nsLayoutUtils::FindNearestCommonAncestorFrame(startFrame, ...

Comment 40

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1b76bac87d82
https://hg.mozilla.org/mozilla-central/rev/65d48bfeb654
https://hg.mozilla.org/mozilla-central/rev/7b4e1bd6c8aa
https://hg.mozilla.org/mozilla-central/rev/ce21be83c48b
https://hg.mozilla.org/mozilla-central/rev/8df7f01875e8

Comment 41

[Jeremy Chen ([e-mail])]

Comment on attachment 8582257 [details] [diff] [review]
Part1: (v4) Add recursive call in GetFrameForNodeOffset.

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: Selection carets may remain functional but look weird. (wrong position) 
Testing completed: on master
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: not available

Comment 42

[Jeremy Chen ([e-mail])]

Comment on attachment 8579928 [details] [diff] [review]
Part2: Recursive call GetFrameForNodeOffset if text node has no frame.

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: Selection carets may remain functional but look weird. (wrong position) 
Testing completed: on master
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: not available

Comment 43

[Jeremy Chen ([e-mail])]

Comment on attachment 8582336 [details] [diff] [review]
Part5: Move Flush_Layout ahead in SelectionCarets::UpdateSelectionCarets.

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: not obvious, but this could prevent selection from potential bugs.
Testing completed: on master
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: not available

Comment 44

[Jeremy Chen ([e-mail])]

Comment on attachment 8582260 [details] [diff] [review]
Part3: Fix a bug in bug414526 so we can reopen the test.

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: none
Testing completed: on master
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: not available

Comment 45

[Jeremy Chen ([e-mail])]

Comment on attachment 8582261 [details] [diff] [review]
Part4: Fix scroll_selection_into_view test to make its function remain.

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: none
Testing completed: on master
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: not available

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/f779c655b539
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/55f671fc665d
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/610745ee33ed
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/5973128bd3f7
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/f043d69285d1

Comment 47

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

Attachment: left-side-caret.png

Comment 48

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

This issue is NOT fixed on the latest Nightly Flame KK  3.0 and 2.2 builds.

Actual Results: The text selection carets appear above selected text or icon on the left side of a shared contact still.

The right side appears correctly.  See above screenshot.

Environmental Variables:
Device: Flame 3.0 KK (319MB) (Full Flash)
BuildID: 20150401010204
Gaia: 03164bd160809747e6a198e0dba1b7c3ee7789f5
Gecko: 18a8ea7c2c62
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
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 KK (319MB) (Full Flash)
BuildID: 20150401002624
Gaia: 8b3086ad3963f1707e2bee9094baccafffe161c4
Gecko: 20b67213a047
Gonk: ebad7da532429a6f5efadc00bf6ad8a41288a429
Version: 37.0 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Comment 49

[Julien Wajsberg [:julienw]]

Hey Jeremy, maybe you could help with what QA found in previous comment?

Comment 50

[Jeremy Chen ([e-mail])]

Sorry, I can't reproduce it. I tested on the latest Nightly from pvt, and the left-caret looked good to me. See screenshot below.

Environmental Variables:
Gaia-Rev        4bb3a933bd805e8df1e11827cb247754c3565b0b
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/37ddc5e2eb72
Build-ID        20150401180920
Version         40.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150401.213521
FW-Date         Wed Apr  1 21:35:31 EDT 2015
Bootloader      L1TC10011880

Comment 51

[Jeremy Chen ([e-mail])]

Attachment: left-side-caret-2.png

Comment 52

[Mats Palmgren (inactive)]

Did you file a follow-up bug on comment 39?  If not, please do so.

Comment 53

[Jeremy Chen ([e-mail])]

(In reply to Mats Palmgren (:mats) from comment #52)
> Did you file a follow-up bug on comment 39?  If not, please do so.

Hi Mats, by applying Part5 patch, we move mPresShell->FlushPendingNotifications(Flush_Layout) before we get rootFrame and startFrame. Therefore, we should already avoid destroying rootFrame and startFrame. I'm not sure what follow-up bug you want me to file. Could you provide more information?

Comment 54

[Julien Wajsberg [:julienw]]

The placement of the caret does not look especially wrong. However, I can't really move the carets when there is an attachment in a message... It works well when there is only text though.

Comment 55

[Jeremy Chen ([e-mail])]

Hi Julien, with an attachment in a text message, moving carets in the text part works find. I think
you mean moving caret on the attachment and its file name, right? Please see the video below.

According to commment 8, the file name should not be selectable in the first place. So, I'm not sure if we should file another bug to deal with this.

Comment 56

[Jeremy Chen ([e-mail])]

Attachment: move carets video

Comment 57

[Julien Wajsberg [:julienw]]

I tested with a message that is like this:

  ------------
  |Attachment|
  ------------
  some text

Then when I press "select text", it's very difficult to move the left caret. At least for me. Maybe the issue is me :)

Comment 58

[Jeremy Chen ([e-mail])]

(In reply to Julien Wajsberg [:julienw] (PTO April 6th) from comment #57)
> I tested with a message that is like this:
> 
>   ------------
>   |Attachment|
>   ------------
>   some text
> 
> Then when I press "select text", it's very difficult to move the left caret.
> At least for me. Maybe the issue is me :)

I dug a little deeper and found you're right. I've filed another bug (Bug 1152263). Thanks for reporting this. :)

Comment 59

[Julien Wajsberg [:julienw]]

Thanks to you !

Comment 60

[Mats Palmgren (inactive)]

(In reply to Jeremy Chen [:jeremychen] from comment #53)
> (In reply to Mats Palmgren (:mats) from comment #52)
> > Did you file a follow-up bug on comment 39?  If not, please do so.
> 
> Hi Mats, by applying Part5 patch, we move
> mPresShell->FlushPendingNotifications(Flush_Layout) before we get rootFrame
> and startFrame. Therefore, we should already avoid destroying rootFrame and
> startFrame.

OK, looks good so far, but I think we need to move it all they way to the start
of the method, since mPresShell and |this| might be destroyed after the flush.
I filed bug 1152432.