1146754 - Selection highlight does not cover the last space at the end of contenteditable

Status: VERIFIED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

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

Attachment: Select "123 ".png

Steps to reproduce:
1. Paste this line to address bar: data:text/html,<div contenteditable="true">123 </div>
2. Click the text, and Ctrl-a to select all the text "123 "

Actual results:
The selection highlight covers only "123" without the last space.

Expected results:
The selection highlight covers "123 " including the last space.

When selection-carets enabled, this introduce visual inconsistency between the selection highlight and the position of the right selection-caret. The position of right selection-caret is at the last space which is correct. See the attached screenshot.

Note that we can copy and paste "123 " (including the last space) correctly.

Comment 1

[Mats Palmgren (inactive)]

Does it work if you pass !IsSelected() instead of true here:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrame.cpp?rev=3c4b353f5daf#6125

Comment 2

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

No, it doesn't work correctly. However, it does have slight difference. After selecting "123" and dragging the selection toward the space at the end, I see the width of the highlight extended for about 1px.

Comment 3

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

Mats, do you have other ideas?

Comment 4

[Mats Palmgren (inactive)]

I don't have any other ideas from the top of my head.

Perhaps Jonathan or Simon has?

Comment 5

[Jonathan Kew [:jfkthame]]

It's not enough to tell the PropertyProvider to avoid trimming the trailing space when painting, because the nsDisplayText item clips its painting to its bounding rect, which it gets from the text frame's visual overflow region. That is based on the metrics returned by gfxTextRun::BreakAndMeasureText, AFAICS.

I suspect we should probably hack BreakAndMeasureText to include trailing space in the bounding box to ensure it gets included in the dirtyRect when painting, even when it's being trimmed from the advance width for line-breaking purposes.

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: patch, include trailing space when highlighting selected text

Here's an experimental patch that I think fixes the issue here. We'll want to look at the changed behavior carefully, and (assuming the result is as desired) adjust tests accordingly, as I suspect this will break some existing selection-highlighting tests.

In particular, with this patch you'll get trailing spaces highlighted when selecting normal line-wrapped text in HTML content. This is a change from our current behavior, where the highlight does not include the space where line-wrap occurred. Personally, I think it's an OK change - even an improvement - but perhaps there were good reasons motivating the existing behavior.

Let's see what try thinks of it:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0daf07f4865a

Comment 7

[Jonathan Kew [:jfkthame]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7d3f399bbaa

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: Reftest for selection highlighting of trailing space

Here's a simple reftest for selection highlighting of a trailing space; currently fails.

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: Fix reftests for selected shadowed text to avoid trailing-space issues

Existing reftests for text-shadow on selected text are dependent on this; let's eliminate their trailing spaces to avoid problems.

Comment 10

[Jonathan Kew [:jfkthame]]

Attachment: Show selection highlighting for trailing space

Patch that makes us highlight trailing spaces. Note that this currently causes a couple of reftest failures on *some* Windows systems; see https://treeherder.mozilla.org/#/jobs?repo=try&revision=c42f5c995036. I'm still considering what to do about those, but in any case we should decide if this is a change we're happy to make.

Comment 11

[Jonathan Kew [:jfkthame]]

One of the failing tests here occurs on Windows XP *only* (not on Win7 unaccelerated):

REFTEST TEST-UNEXPECTED-FAIL | file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/bugs/1013054-1.html | image comparison (==), max difference: 255, number of differing pixels: 7 

:roc, this is the test that you marked fuzzy (1 px) in bug 1013054 comment 8. It looks to me like the patch here is causing the error to become larger because it extends the overflow-area of the text frame, and hence enlarges the dirtyRect that we end up painting. But I have no idea what's causing the black line (of which previously only a single pixel showed up) in the first place. Some kind of painting glitch when the transform changes?

We could presumably modify the test file to eliminate the trailing space in the (currently unclosed) <body>, and thus make the failure go away, but that just hides the problem; it doesn't explain what causes the unexpected black line. Should we just add more WinXP-only fuzz and move on? I can't see myself debugging this any time soon.

Comment 12

[Jonathan Kew [:jfkthame]]

The other issue is a pair of SVG clipPath failures:

REFTEST TEST-UNEXPECTED-FAIL | file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/svg/svg-integration/clipPath-html-06.xhtml | image comparison (==), max difference: 255, number of differing pixels: 30
REFTEST TEST-UNEXPECTED-FAIL | file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/svg/svg-integration/clipPath-html-06-extref.xhtml | image comparison (==), max difference: 255, number of differing pixels: 30 

These occur on WinXP and Win7/8 Unaccelerated, but not under D2D or on any other platform. It appears that a trailing space in the <span style="clip-path: url(#c1);"> is being included in its objectBoundingBox used as the basis for the clip size. This makes sense inasmuch as the patch here deliberately causes trailing spaces, even when trimmed for text-measurement purposes, to be included in the frame's visual overflow; but I don't understand why it is happening only on unaccelerated Windows and not on other platforms.

:jwatt, do you have any idea why the SVG clip path would be behaving in this unexpectedly platform-dependent way?

Comment 13

[Mats Palmgren (inactive)]

Comment on attachment 8584423 [details] [diff] [review]
Reftest for selection highlighting of trailing space

I'd like this to include tests for 'text-decoration' and 'text-shadow' too.
Something like:

    <span id="test">
      <div>123 </div>
      <div style="text-shadow:5px 3px 0 black;">123 </div>
      <div style="text-decoration:underline">123 </div>
      <div style="text-decoration:underline; text-shadow:5px 3px 0 black;">123 </div>
    </span>

Comment 14

[Mats Palmgren (inactive)]

Comment on attachment 8584426 [details] [diff] [review]
Show selection highlighting for trailing space

>layout/generic/nsTextFrame.cpp
>nsTextFrame::PaintText
>   // Trim trailing whitespace
>-  provider.InitializeForDisplay(true);
>+  provider.InitializeForDisplay(!IsSelected());

Please update the code comment (and append a period).

>nsTextFrame::RecomputeOverflow
>-  provider.InitializeForDisplay(true);
>+  provider.InitializeForDisplay(false); // don't trim trailing space,
>+                                        // in case we need to highlight it

You might as well move that comment up to a separate line before
the call since you use two lines anyway.
Also, s/highlight it/paint it as selected/ is probably clearer.

Comment 15

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Kew (:jfkthame) from comment #12)
> :jwatt, do you have any idea why the SVG clip path would be behaving in this
> unexpectedly platform-dependent way?

No idea. You're sure it's not the text measurement that has the platform dependent behavior?

Comment 16

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

I'm not sure we should be highlighting selected trailing spaces in general. That's a pretty big behavior change.

(In reply to Jonathan Kew (:jfkthame) from comment #11)
> One of the failing tests here occurs on Windows XP *only* (not on Win7
> unaccelerated):
> 
> REFTEST TEST-UNEXPECTED-FAIL |
> file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/bugs/1013054-
> 1.html | image comparison (==), max difference: 255, number of differing
> pixels: 7 
> 
> :roc, this is the test that you marked fuzzy (1 px) in bug 1013054 comment
> 8. It looks to me like the patch here is causing the error to become larger
> because it extends the overflow-area of the text frame, and hence enlarges
> the dirtyRect that we end up painting. But I have no idea what's causing the
> black line (of which previously only a single pixel showed up) in the first
> place. Some kind of painting glitch when the transform changes?
> 
> We could presumably modify the test file to eliminate the trailing space in
> the (currently unclosed) <body>, and thus make the failure go away, but that
> just hides the problem; it doesn't explain what causes the unexpected black
> line. Should we just add more WinXP-only fuzz and move on? I can't see
> myself debugging this any time soon.

How about just fixing the test instead of adding fuzz, and open a new bug with a testcase for this problem?

Comment 17

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> I'm not sure we should be highlighting selected trailing spaces in general.
> That's a pretty big behavior change.

I guess I'm OK with it.

Comment 18

[Jonathan Kew [:jfkthame]]

(In reply to Jonathan Watt [:jwatt] from comment #15)
> (In reply to Jonathan Kew (:jfkthame) from comment #12)
> > :jwatt, do you have any idea why the SVG clip path would be behaving in this
> > unexpectedly platform-dependent way?
> 
> No idea. You're sure it's not the text measurement that has the platform
> dependent behavior?

Apparently it is...

Dumping the frames involved, I can see that on WinXP, the line that contains the "unit" <span> is ending up with a visual overflow that includes the trailing space; see Text(6), from which it propagates to the Inline(span) and to the line:

  line 110ac128: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x100] {0,13200,6000,1200} {0,13200,6000,1200;cw=48000} vis-overflow=0,13200,6360,1200 scr-overflow=0,13200,6000,1200 <
    Inline(span)(1)@eb0cae0 next=eb0cb30 IBSplitPrevSibling=eb0ca10 {0,13230,6000,1140} vis-overflow=0,0,6360,1140 scr-overflow=0,0,6000,1140 [state=0000100000008200] [content=1108be20] [sc=eb0bab0]<
      Text(4)"\n    "@eb0c648 next=eb0c698 {0,900,0,0} [state=4000000028200000] [content=eb0d0b0] [sc=eb0be18:-moz-non-element] [run=eb0d830][0,5,T]
      Block(span)(5)@eb0c698 next=eb0c6f8 {0,300,6000,600} [state=0000168000d00200] [content=eb0d100] [sc=eb0bed0]<
      >
      Text(6)"\n  "@eb0c6f8 {6000,0,0,1140} vis-overflow=-60,0,420,1140 scr-overflow=0,0,0,1140 [state=00000000a0400000] [content=eb0d150] [sc=eb0be18:-moz-non-element] [run=eb0d880][0,3,T]
    >
  >

On Win7, in contrast, we don't get this vis-overflow:

  line 13cb6128: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x100] {0,13200,6000,1200} {0,13200,6000,1200;cw=48000} <
    Inline(span)(1)@13c7bae0 next=13c7bb30 IBSplitPrevSibling=13c7ba10 {0,13230,6000,1140} [state=0002100000008200] [content=13c5ac90] [sc=13c7aab0]<
      Text(4)"\n    "@13c7b648 next=13c7b698 {0,900,0,0} [state=4001000028200000] [content=13c5aec0] [sc=13c7ae18:-moz-non-element] [run=13ca86f0][0,5,T]
      Block(span)(5)@13c7b698 next=13c7b6f8 {0,300,6000,600} [state=0000168000d00200] [content=13c5af10] [sc=13c7aed0]<
      >
      Text(6)"\n  "@13c7b6f8 {6000,0,0,1140} [state=40010000a0400000] [content=13c5af60] [sc=13c7ae18:-moz-non-element] [run=13ca8740][0,3,T]
    >
  >

So yes, the difference seems to be somewhere in the text or frame measurement, rather than on the SVG side, but currently I have no idea why it behaves differently.

Nor am I quite sure which result should be regarded as correct; a trawl through the spec left me unsure exactly what the "objectBoundingBox" should refer to, but it's not clear to me that basing it on the frame's visualOverflow region is correct. The text at [1] seems pretty clear that the "bounding box" need not necessarily include all pixels painted by the object.... it's more like a "geometry bounding box" than an "ink bounding box".

[1] http://www.w3.org/TR/SVG/coords.html#ObjectBoundingBoxUnits

Comment 19

[Jonathan Kew [:jfkthame]]

(In reply to Jonathan Kew (:jfkthame) from comment #18)
> (In reply to Jonathan Watt [:jwatt] from comment #15)
> > No idea. You're sure it's not the text measurement that has the platform
> > dependent behavior?
> 
> Apparently it is...
> 
> Dumping the frames involved, I can see that on WinXP, the line that contains
> the "unit" <span> is ending up with a visual overflow that includes the
> trailing space
...
> 
> On Win7, in contrast, we don't get this vis-overflow:

OK, I think I figured this out; it's a GDI vs DirectWrite/D2D difference (I thought I'd checked Win7 without D2D, but must have missed that). The unexpected result arises because the GDI font backend does not return an empty bounding box for the <space> glyph, as we would expect. This seems to be a "feature" of the GetGlyphOutline API that cairo-win32-font uses to get glyph extents.

I've filed bug 1149519 about this.

Comment 20

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5583bde552e3
https://hg.mozilla.org/integration/mozilla-inbound/rev/28c7f733035e
https://hg.mozilla.org/integration/mozilla-inbound/rev/66945c5c4a40

Comment 21

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/38b412b7e200

Comment 22

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

https://hg.mozilla.org/mozilla-central/rev/5583bde552e3
https://hg.mozilla.org/mozilla-central/rev/28c7f733035e
https://hg.mozilla.org/mozilla-central/rev/66945c5c4a40
https://hg.mozilla.org/mozilla-central/rev/38b412b7e200

Comment 23

[Mohammad Maruf Islam]

I have reproduced this bug with Firefox 39.0a1 (Build:20150323030203) on windows 8.1 pro 64-bit as instructed in comment 0.

Verified as fixed with Latest Firefox beta 40.0b7 (Build:20150723165742)

Mozilla/5.0 (Windows NT 6.3; WOW64; rv:40.0) Gecko/20100101 Firefox/40.0

[testday-20150724]

Comment 24

[Nazir Ahmed Sabbir [:NaSb]]

Reproduced the bug in Nightly 39.0a1 (2015-03-23) (Build ID: 20150323030203) on Linux x64 following the comment 0's instruction!

This Bug is now verified as fixed on Latest Firefox Beta 40.0b7

Build ID: 20150723165742
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:40.0) Gecko/20100101 Firefox/40.0
As it is also verified on Windows (Comment 23), Marking it as verified!