Closed Bug 1146754 Opened 10 years ago Closed 10 years ago

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

Categories

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

Product:
Core
Component:
Layout: Text and Fonts
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla40
Tracking Flags:
Tracking Status
firefox40 --- verified

People

(Reporter: TYLin, Assigned: jfkthame)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [testday-20150724])

Attachments

(4 files, 1 obsolete file)

Select "123 ".png
(deleted), image/png
Details
Reftest for selection highlighting of trailing space
(deleted), patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
Fix reftests for selected shadowed text to avoid trailing-space issues
(deleted), patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
Show selection highlighting for trailing space
(deleted), patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
Attached image Select "123 ".png (deleted) —
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.
Blocks: AccessibleCaret
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
Component: Selection → Layout: Text
Flags: needinfo?(tlin)
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.
Flags: needinfo?(mats)
Flags: needinfo?(tlin)
Flags: needinfo?(mats)
Mats, do you have other ideas?
Flags: needinfo?(mats)
I don't have any other ideas from the top of my head. Perhaps Jonathan or Simon has?
Flags: needinfo?(mats)
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.
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
Here's a simple reftest for selection highlighting of a trailing space; currently fails.
Attachment #8584423 - Flags: review?(mats)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Existing reftests for text-shadow on selected text are dependent on this; let's eliminate their trailing spaces to avoid problems.
Attachment #8584424 - Flags: review?(mats)
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.
Attachment #8584426 - Flags: review?(mats)
Attachment #8583900 - Attachment is obsolete: true
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.
Flags: needinfo?(roc)
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?
Flags: needinfo?(jwatt)
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>
Attachment #8584423 - Flags: review?(mats) → review+
Attachment #8584424 - Flags: review?(mats) → review+
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.
Attachment #8584426 - Flags: review?(mats) → review+
(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?
Flags: needinfo?(jwatt)
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?
Flags: needinfo?(roc)
(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.
(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
Depends on: 1149519
(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.
QA Whiteboard: [good first verify]
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]
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!
Status: RESOLVED → VERIFIED
QA Whiteboard: [good first verify] → [good first verify][testday-20150724]
status-firefox40: fixed → verified
Whiteboard: [testday-20150724]
Depends on: 1328055
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: