Closed Bug 1048829 Opened 8 years ago Closed 8 years ago

[Text Selection] wrong rect position from gecko while clicking the last letter of context

Categories

(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect, P2)

x86
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S8 (7Nov)

People

(Reporter: gduan, Assigned: mtseng)

References

Details

Attachments

(3 files, 6 obsolete files)

As title,
while clicking the last word of context, sometimes gecko will report the element's position instead of last letter position.
Priority: -- → P2
When cursor at last letter of context. The node of range would be div node instead of text node. So in [1] we'll return empty rect if node is not text node. Then the copy/paste bubble shows at error position.

[1]: http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsRange.cpp#2836
Attached patch bug1048829 (obsolete) — Splinter Review
Get the frame's rect if nsRange.GetBoundingClientRect() return empty rect.
Assignee: nobody → mtseng
Status: NEW → ASSIGNED
Attachment #8504559 - Flags: review?(roc)
Comment on attachment 8504559 [details] [diff] [review]
bug1048829

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

Can you give an example of exactly what the DOM and the Range is in the error case?
Attachment #8504559 - Flags: review?(roc)
Attached file test_range.html
Error case example.
Attached patch logSplinter Review
Logging for bounding client rect.
Hi roc,
test_range.html shows 3 different kind of error.
1. Content editable div with <br> element, if you click on the middle empty line. We'll get empty bounding box.
2. We'll get empty bounding box when you click on a empty input field.
3. Last case is if caret is in last character of textarea, the bounding box is empty as well.

You can apply "log" patch to show the bounding box information when selection changed.
Flags: needinfo?(roc)
(In reply to Morris Tseng [:mtseng] from comment #6)
> test_range.html shows 3 different kind of error.
> 1. Content editable div with <br> element, if you click on the middle empty
> line. We'll get empty bounding box.
> 2. We'll get empty bounding box when you click on a empty input field.
> 3. Last case is if caret is in last character of textarea, the bounding box
> is empty as well.

What exactly are begin and end points of the Range in each of these case?
Flags: needinfo?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> (In reply to Morris Tseng [:mtseng] from comment #6)
> > test_range.html shows 3 different kind of error.
> > 1. Content editable div with <br> element, if you click on the middle empty
> > line. We'll get empty bounding box.
> > 2. We'll get empty bounding box when you click on a empty input field.
> > 3. Last case is if caret is in last character of textarea, the bounding box
> > is empty as well.
> 
> What exactly are begin and end points of the Range in each of these case?

here are begin and end points of the Range for 3 cases. Because 3 cases are all collapsed case, so begin and end point are the same, begin offset and end offset are the same as well.

1.content:13a7c6880 offset:2
2.content:13a7c6ce0 offset:0
3.content:13a7c6e20 offset:1 

Below is complete frame tree:
=============
webshell=0x130ee51a8
Viewport(-1)@13aad4458 [view=13a1a94a0] {0,0,36600,22980} [state=0000062000002220] [sc=13aad40f8:-moz-viewport]<
  HTMLScroll(html)(-1)@11de3ca20 {0,0,36600,22980} [state=0000020000000010] [content=126690140] [sc=11de3c020:-moz-viewport-scroll]<
    ScrollbarFrame(scrollbar)(-1)@11de3d218 next=130e8f6d8 {0,22980,36600,0} [state=0000100080c80000] [content=13f4fd040] [sc=13f5e58e8]<
      SliderFrame(slider)(-1)@11de3dcc8 {0,0,36600,960} [state=0000160080c00000] [content=13f4fd3a0] [sc=11de3d6a0]<
        ButtonBoxFrame(thumb)(0)@130e8f218 {60,120,36480,780} [state=2000160080400000] [content=13f4fd430] [sc=130e901e8]<>
      >
    >
    ScrollbarFrame(scrollbar)(-1)@130e8f6d8 next=130e90a30 {36600,0,0,22980} [state=0000100080880000] [content=13f4fd0d0] [sc=13f5e59a8]<
      SliderFrame(slider)(-1)@130e90048 {0,0,960,22980} [state=0000160080800000] [content=13f4fd8b0] [sc=130e8fac0]<
        ButtonBoxFrame(thumb)(0)@130e90488 {120,60,780,22860} [state=2000160080000000] [content=13f4fd940] [sc=13f5e0e00]<>
      >
    >
    Box(scrollcorner)(-1)@130e90a30 next=11de3c710 {36600,22980,0,0} [state=0000100080c00200] [content=13f4fd160] [sc=130e90680]<>
    Canvas(html)(-1)@11de3c710 {0,0,36600,22980} [state=0000006000000200] [content=126690140] [sc=130e90bf0:-moz-scrolled-canvas]<
      Block(html)(-1)@11de2f0e0 next=11de30160 {0,0,36600,13098} [state=0000100000d00210] [content=126690140] [sc=11de2f020,parent=0]<
        line 11de2f7c8: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x48] bm=480 {480,480,35640,12138} vis-overflow=360,420,35820,12258 scr-overflow=480,480,35640,12138 <
          Block(body)(1)@11de2f730 {480,480,35640,12138} vis-overflow=-120,-60,35820,12258 scr-overflow=0,0,35640,12138 [state=0000120000100210] [content=13a7c6560] [sc=11de2f518]<
            line 11de30e68: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x300] {0,0,0,0} <
              Text(0)"\n"@11de30df8 next=11de30720 {0,0,0,0} [state=0000000008600000] [content=13f4fdca0] [sc=11de30d50:-moz-non-element] [run=0][0,1,T]
            >
            line 11de308f0: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x148] {0,0,35640,3576} vis-overflow=-60,-60,35760,3696 scr-overflow=0,0,35640,3576 <
              Block(div)(1)@11de30720 next=13f5df8d0 {0,0,35640,3576} vis-overflow=-60,-60,35760,3696 [state=0000124000100210] [content=13a7c6880] [sc=13f5e6040]<
                line 11de30aa8: count=2 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:linebr[0x8100] {60,60,10070,1152} vis-overflow=30,60,10130,1152 scr-overflow=60,60,10070,1152 <
                  Text(0)"this is content editable div"@11de30a38 next=11de30ba0 {60,156,10070,960} vis-overflow=-30,0,10130,960 scr-overflow=0,0,10070,960 [state=00000040b0600010] [content=13f4fdd30] [sc=13f5e5a68:-moz-non-element] [run=11929a040][0,28,T]
                  Frame(br)(1)@11de30ba0 next=11de30bf0 {10130,876,0,0} [content=13a7c6920] [sc=13f5e5590]
                >
                line 11de30c40: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:linebr[0x8100] {60,1212,1,1152} <
                  Frame(br)(2)@11de30bf0 next=11de30c90 {60,1212,1,1152} [state=0000004000000000] [content=13a7c69c0] [sc=13f5e5590]
                >
                line 11de30d00: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x100] {60,2364,4555,1152} vis-overflow=30,2364,4615,1152 scr-overflow=60,2364,4555,1152 <
                  Text(3)"another line"@11de30c90 {60,2460,4555,960} vis-overflow=-30,0,4615,960 scr-overflow=0,0,4555,960 [state=00000040b0600000] [content=13f4fddc0] [sc=13f5e5a68:-moz-non-element] [run=1151de3d0][0,12,T]
                >
              >
            >
            line 13f5e8058: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:linebr[0x8140] {0,3576,1,1152} <
              Frame(br)(3)@13f5df8d0 next=13f5df9c0 {0,3576,1,1152} [content=13a7c6a60] [sc=11de30eb8]
            >
            line 13f5e80a8: count=2 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:linebr[0x8140] {0,4728,8880,1290} vis-overflow=-120,4608,9120,1530 scr-overflow=0,4728,8880,1290 <
              nsTextControlFrame@13f5df9c0 next=13f5e0a20 {0,4728,8880,1290} vis-overflow=-120,-120,9120,1530 scr-overflow=0,0,8880,1290 [state=0000004000004230] [content=13a25b220] [sc=13f5df0f8]<
                HTMLScroll(div)(-1)@13f5e00e0 {180,180,8520,930} [state=0000000000084018] [content=13a7c6ce0] [sc=13f5e8448]<
                  Block(div)(-1)@13f5e06d8 {0,0,8520,930} [state=0000100000d04210] [content=13a7c6ce0] [sc=13f5e8ac0:-moz-scrolled-content]<
                    line 13f5e09d0: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x100] {60,60,0,810} vis-overflow=60,720,0,0 scr-overflow=60,720,0,0 <
                      Text(0)""@13f5e0960 {60,720,0,0} [state=0000000000404000] [content=11de33590] [sc=13f5e8dd0:-moz-non-element,parent=13f5e8448] [run=0][0,0,T]
                    >
                  >
                >
              >
              Frame(br)(7)@13f5e0a20 next=13f5e0b10 {8880,5628,0,0} [content=13a7c6b00] [sc=11de30eb8]
            >
            line 13f5e80f8: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x140] {0,6018,6000,6120} vis-overflow=-120,5958,6240,6240 scr-overflow=0,6018,6000,6120 <
              nsTextControlFrame@13f5e0b10 {0,6078,6000,6000} vis-overflow=-120,-120,6240,6240 scr-overflow=0,0,6000,6000 [state=000000c000004210] [content=13a4da6e0] [sc=13f5df570]<
                HTMLScroll(div)(-1)@13f5e5270 {60,60,5880,5880} [state=0000008000084018] [content=13a7c6e20] [sc=13f5dfcf8]<
                  ScrollbarFrame(scrollbar)(-1)@13f5e5830 next=13f5e5ec8 {0,5880,5880,0} [state=0000100080c84000] [content=11de33620] [sc=11de3d988]<
                    SliderFrame(slider)(-1)@13f5e5b28 {0,0,5880,960} [state=0000160080c04000] [content=11de33a10] [sc=13f5e9268]<
                      ButtonBoxFrame(thumb)(0)@13f5e5cb0 {60,120,5760,780} [state=2000160080404000] [content=11de33aa0] [sc=13f5e5f80]<>
                    >
                  >
                  ScrollbarFrame(scrollbar)(-1)@13f5e5ec8 next=13f5e6b10 {5880,0,0,5880} [state=0000100080884000] [content=11de336b0] [sc=130e8fda8]<
                    SliderFrame(slider)(-1)@13f5e61c0 {0,0,960,5880} [state=0000160080804000] [content=11de33f20] [sc=13f5e5bf0]<
                      ButtonBoxFrame(thumb)(0)@13f5e6348 {120,60,780,5760} [state=2000160080004000] [content=11de33fb0] [sc=11de3def8]<>
                    >
                  >
                  Box(resizer)(-1)@13f5e6b10 next=13f5e6bd8 {4920,4920,960,960} [state=2000104080c04000] [content=11de33740] [sc=13f5e5d98]<>
                  Box(scrollcorner)(-1)@13f5e6bd8 next=13f5e6f50 {5880,5880,0,0} [state=0000100080c04200] [content=11de337d0] [sc=13f5e6760]<>
                  Block(div)(-1)@13f5e6f50 {0,0,5880,5880} [state=0000100000d04210] [content=13a7c6e20] [sc=13f5e6430:-moz-scrolled-content]<
                    line 13f5e8240: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:linebr[0x8100] {60,0,1872,810} vis-overflow=30,0,1932,810 scr-overflow=60,0,1872,810 <
                      Text(0)"this\n"@13f5e8330 next=13f5f6d70 next-in-flow=13f5f6d70 {60,0,1872,810} vis-overflow=-30,0,1932,810 scr-overflow=0,0,1872,810 [state=80000040a0604000] [content=11de343a0] [sc=13f5e6100:-moz-non-element,parent=13f5dfcf8] [run=1152c7820][0,5,F]
                    >
                    line 13f5f6de8: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:linebr[0x8100] {60,810,936,810} vis-overflow=30,810,996,810 scr-overflow=60,810,936,810 <
                      Text(0)"is\n"@13f5f6d70 next=13f5f6e38 prev-in-flow=13f5e8330 next-in-flow=13f5f6e38 {60,810,936,810} vis-overflow=-30,0,996,810 scr-overflow=0,0,936,810 [state=00000040a0604004] [content=11de343a0] [sc=13f5e6100:-moz-non-element,parent=13f5dfcf8] [run=1152c78b0][5,3,F]
                    >
                    line 13f5f6eb0: count=2 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:linebr[0x8100] {60,1620,3744,810} vis-overflow=30,1620,3804,810 scr-overflow=60,1620,3744,810 <
                      Text(0)"textarea"@13f5f6e38 next=13f5e81f0 prev-in-flow=13f5f6d70 {60,1620,3744,810} vis-overflow=-30,0,3804,810 scr-overflow=0,0,3744,810 [state=00000040a0604004] [content=11de343a0] [sc=13f5e6100:-moz-non-element,parent=13f5dfcf8] [run=1151a12a0][8,8,T]
                      Frame(br)(1)@13f5e81f0 {3804,2220,0,0} [state=0000000000004000] [content=13f5ea200] [sc=13f5e8148,parent=13f5dfcf8]
                    >
                  >
                >
              >
            >
          >
        >
      >
      Placeholder(div)(-1)@11de30160 next=11de30500 {0,0,0,0} [state=0000000000200402] [content=13a7c6600] [sc=11de2fd50:-moz-non-element,parent=0] outOfFlowFrame=Block(div)(-1)@11de2fcb0
      Placeholder(div)(-1)@11de30500 {0,0,0,0} [state=0000000000200402] [content=13a7c66a0] [sc=11de30260:-moz-non-element,parent=0] outOfFlowFrame=Block(div)(-1)@11de301c0
    >
    AbsoluteList 139f9c080 <
      Block(div)(-1)@11de2fcb0 next=11de301c0 {0,0,0,0} [state=000010a000d00308] [content=13a7c6600] [sc=11de2f5c0,parent=0]<
      >
      Block(div)(-1)@11de301c0 {0,0,0,0} [state=000010a000d00308] [content=13a7c66a0] [sc=11de2f680,parent=0]<
      >
    >
  >
>
Flags: needinfo?(roc)
Comment on attachment 8504559 [details] [diff] [review]
bug1048829

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

OK, thanks for that.

I think we should be calling nsCaret::GetGeometry here. That will return the rectangle where we would draw the caret.
Attached patch bug1048829 v2 (obsolete) — Splinter Review
Fixing rect using nsCaret::GetGeometry().
Attachment #8504559 - Attachment is obsolete: true
Flags: needinfo?(roc)
Attachment #8506794 - Flags: review?(roc)
Comment on attachment 8506794 [details] [diff] [review]
bug1048829 v2

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

::: dom/base/nsGlobalWindow.cpp
@@ +9311,5 @@
>        nsCOMPtr<nsIDOMRange> range;
>        nsresult rv = aSel->GetRangeAt(0, getter_AddRefs(range));
>        if (NS_SUCCEEDED(rv) && range) {
>          nsRefPtr<nsRange> nsrange = static_cast<nsRange*>(range.get());
>          init.mBoundingClientRect = nsrange->GetBoundingClientRect(true, false);

Test whether the selection is collapsed. If it is, call nsCaret::GetGeometry without calling GetBoundingClientRect. Otherwise, get the first range and call GetBoundingClientRect on it.
Attachment #8506794 - Flags: review?(roc) → review-
Attached patch bug1048829 v3 (obsolete) — Splinter Review
Fix addressed comment.
Attachment #8506794 - Attachment is obsolete: true
Attachment #8507598 - Flags: review?(roc)
Attached patch bug1048829 v4 (obsolete) — Splinter Review
call Selection::Stringify to get selection text instead of nsRange->ToString()
Attachment #8507598 - Attachment is obsolete: true
Attachment #8507598 - Flags: review?(roc)
Attachment #8507624 - Flags: review?(roc)
Attached patch bug1048829 v5 (obsolete) — Splinter Review
Fix try failure.

try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=463a16e82c41
Attachment #8507624 - Attachment is obsolete: true
Attachment #8507624 - Flags: review?(roc)
Attachment #8507678 - Flags: review?(roc)
Comment on attachment 8507678 [details] [diff] [review]
bug1048829 v5

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

::: dom/base/nsGlobalWindow.cpp
@@ +9313,5 @@
> +      nsIPresShell* presShell = mDoc->GetShell();
> +      // Bounding client rect may be empty after calling GetBoundingClientRect
> +      // when range is collapsed. So we get caret's rect when range is
> +      // collapsed.
> +      if (selection->IsCollapsed() && !presShell->IsPaintingSuppressed()) {

Why are you checking IsPaintingSuppressed here? You shouldn't need to.
Attachment #8507678 - Flags: review?(roc)
Because calling nsCaret::GetGeometry may hit assertion like this.

[56681] ###!!! ASSERTION: frame must not be dirty: '!NS_SUBTREE_DIRTY(this)', file /Users/Morris/mozilla/mozilla-central/layout/generic/nsFrame.cpp, line 1339
#01: nsFrame::GetLogicalBaseline(mozilla::WritingMode) const (nsFrame.cpp:1338, in XUL)
#02: nsIFrame::GetCaretBaseline() const (nsIFrame.h:986, in XUL)
#03: nsCaret::GetGeometryForFrame(nsIFrame*, int, int*) (nsCaret.cpp:297, in XUL)
#04: nsCaret::GetGeometry(nsISelection*, nsRect*) (nsCaret.cpp:394, in XUL)
#05: nsGlobalWindow::UpdateCommands(nsAString_internal const&, nsISelection*, short) (nsGlobalWindow.cpp:9319, in XUL)

Does it have another way to avoid this assertion?
Flags: needinfo?(roc)
Yes. A dirty frame means that something has changed that requires a reflow, but we haven't actually done the reflow. You should trigger the reflow by calling presShell->FlushPendingNotifications(Flush_Layout) before calling GetGeometry.

Also, please break your new code out into a helper function.
Flags: needinfo?(roc)
Attached patch bug1048829 v6 (obsolete) — Splinter Review
Update to addressed comment.

new try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=813a90dfefd2
Attachment #8507678 - Attachment is obsolete: true
Attachment #8515719 - Flags: review?(roc)
Comment on attachment 8515719 [details] [diff] [review]
bug1048829 v6

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

::: dom/base/nsGlobalWindow.cpp
@@ +9332,5 @@
>    }
>  }
>  
> +static void
> +GetSelectionBoundingRect(Selection* aSel, nsIPresShell* aShell, nsRect& aRect)

Just return aRect directly instead of making it an out-parameter.
Attachment #8515719 - Flags: review?(roc) → review-
Attached patch bug1048829 v7Splinter Review
Return aRect directly.

try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4319afc2b1c7
Attachment #8515719 - Attachment is obsolete: true
Attachment #8515738 - Flags: review?(roc)
Accidentally import wrong patches on previous try run.

so, here is new try:
 https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6865a715e07a
https://hg.mozilla.org/mozilla-central/rev/886fba0d95e5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S8 (7Nov)
You need to log in before you can comment on or make changes to this bug.