Closed Bug 1399310 Opened 7 years ago Closed 7 years ago

Decorations get drawn multiple times with selections

Categories

(Core :: Layout: Text and Fonts, enhancement, P3)

Product:
Core
Component:
Layout: Text and Fonts
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: Gankra, Assigned: tommykuo)

References

Details

Attachments

(7 files, 1 obsolete file)

select-a-word-1.png
16.86 KB, image/png
Details
select-a-word-2.png
22.41 KB, image/png
Details
select-a-word-3.png
23.87 KB, image/png
Details
Bug 1399310 - (Part 1) Make `nsTextFrame::DrawTextRunAndDecorations` draw only in the range of the text.
59 bytes, text/x-review-board-request
jfkthame
: review+
Details
testcase for painting text decorations in various writing-modes and directions
1.87 KB, text/html
Details
Bug 1399310 - (Part 2) Add a bunch of test cases for text selection.
59 bytes, text/x-review-board-request
jfkthame
: review+
Details
possible reftest strategy
2.00 KB, text/html
Details
If you select this element <span style="text-decoration: underline line-through overline rgba(0,0,0,0.25)">laboris nisi ut aliquip ex </span> the decorations will noticeably darken. The problem is that DrawTextRunAndDecorations doesn't factor in the range when drawing the decorations. Mostly you can't notice this because we don't apply AA to thin decorations, and most decorations are opaque.
:kuoe0, IIRC you touched some code related to text-decoration recently - would you possibly have a chance to look at this one as well?
Flags: needinfo?(kuoe0)
No problem, I'll take a look at this.
Flags: needinfo?(kuoe0)
Assignee: nobody → kuoe0
Attached image select-a-word-1.png —
Attached image select-a-word-2.png —
Attached image select-a-word-3.png —
After some investigation, we need to make `DrawTextRunAndDecorations` draw the decorations only in the range. In the attachments, we can see there are three segments in this selection text. So, we draw the entire decorations three times. That's why the decorations are more opaque than the originals.
Comment on attachment 8910689 [details] Bug 1399310 - (Part 1) Make `nsTextFrame::DrawTextRunAndDecorations` draw only in the range of the text. Hi jfkthame, could you give me some feedback for this? I have no idea how to make `PaintDecorationLine` support the range to fit the segment size. So, I think we can only draw the decorations for the first segment if they exist. For the other segments, we ignore the decorations and draw the text only.
Attachment #8910689 - Flags: feedback?(jfkthame)
Unfortunately, I don't think this approach will work. The trouble is that it results in all the decoration lines (for all the segments) getting drawn before the text of the later segments, which is the wrong result for line-through (which must be drawn after the text). Testcase: data:text/html,<span style="font-size:99px;color:red;text-decoration:line-through;text-decoration-color:green">one two three Then select the middle word. Observe how the strikethrough is painted correctly (on top of the text) only for the first segment, and is behind the text for the second and third. So I think we really need a solution that makes PaintDecorationLine limit its drawing to the range. Possibly the easiest way to do this will be by setting up clipping to the range edges, and then drawing the lines as before. (The alternative of trying to limit the actual drawing operation would arguably be more efficient, but it might be trickier to implement because of needing to maintain the "phase" of decorations such as dotted or wavy lines; we can't just arbitrarily start drawing them at different offsets, or there'll be a risk of discontinuities at the segment boundaries. Testing with "text-decoration:wavy line-through;" is probably a good way to check for this.)
Attachment #8910689 - Flags: feedback?(jfkthame)
Attachment #8910689 - Flags: review?(jfkthame)
Comment on attachment 8910689 [details] Bug 1399310 - (Part 1) Make `nsTextFrame::DrawTextRunAndDecorations` draw only in the range of the text. https://reviewboard.mozilla.org/r/182150/#review189140 I think the overall approach here should be fine. Clearing r? for now, as this needs further work to handle the reversed-direction cases as mentioned below; but it looks like it should work once this is addressed. ::: layout/generic/nsTextFrame.cpp:7440 (Diff revision 3) > + // Draw the decoration line only in the range of the text. So, we create a > + // clip for the context to limit the draw area. I think we should put this inside a conditional, so that we only do the additional measurement here (two calls to GetAdvanceWidth) and clipping in the case where aRange is *not* the whole of the textrun. That will avoid doing extra work in the commonest case. ::: layout/generic/nsTextFrame.cpp:7444 (Diff revision 3) > > + // Draw the decoration line only in the range of the text. So, we create a > + // clip for the context to limit the draw area. > + gfxFloat clipStart = mTextRun->GetAdvanceWidth(Range(0, aRange.start), > + aParams.provider); > + gfxFloat clipEnd = mTextRun->GetAdvanceWidth(aRange, aParams.provider); s/clipEnd/clipLength/, as this isn't really the 'end' coordinate, it's the 'length' of the range to be drawn. ::: layout/generic/nsTextFrame.cpp:7447 (Diff revision 3) > + if (verticalDec) { > + clipRect = gfxRect(0, (y + clipStart) / app, > + frameSize.width, clipEnd / app); > + } else { > + clipRect = gfxRect((x + clipStart) / app, 0, > + clipEnd / app, frameSize.height); > + } AFAICS this works for horizontal LTR text, and for vertical text that runs downwards (the usual case!), but the clipping will be incorrect for the cases (horizontal RTL text, and writing-mode:sideways-lr) where the inline progression of the text (i.e. the direction in which the textRun's advance is measured) is opposite to the physical coordinates being used to set up the clipRect. Examples to try: data:text/html;charset=utf-8,<div style="text-decoration: wavy line-through; text-decoration-color:red">%D8%A7%D9%84%D8%AE%D8%B7 %D8%A7%D9%84%D8%B9%D8%B1%D8%A8%D9%8A data:text/html;charset=utf-8,<div style="writing-mode:sideways-lr;text-decoration:dotted line-through;text-decoration-color:red">one two three Also check RTL text in vertical writing mode, and in sideways-{lr,rl} modes. In various of these cases, I think you'll need to deal with the reversal of the coordinates (i.e. clipStart is a measurement from the right or bottom edge of the frame, rather than the left or top).
Attachment #8910689 - Flags: review?(jfkthame)
One more thing, I think the current patch doesn't set up the clip correctly in the case where the decoration range doesn't start at the beginning of the textrun. Testcase: data:text/html,<u>one</u> two <u>three</u> four
This works well for many cases, but there are still combinations of writing-mode and directionality where it clips incorrectly. Try RTL text in sideways-lr writing mode: data:text/html;charset=utf-8,<div style="writing-mode:sideways-lr;text-decoration:red dotted line-through;">one <u>two %D8%A7%D9%84%D8%AE%D8%B7</u> %D8%A7%D9%84%D8%B9%D8%B1%D8%A8%D9%8A three Also try adding dir=rtl to various testcases: data:text/html,<div dir=rtl><u>one</u> two <u>three</u> four data:text/html,<div style="text-decoration: red wavy line-through;" dir=rtl>hello <u>world</u> I suspect the interaction between wm.IsInlineReversed() and mTextRun->IsRightToLeft() needs to be looked at again. Note that these may *both* be true in some cases. wm.IsInlineReversed is true when dir=rtl OR when writing-mode is sideways-lr (but not both, IIRC), while mTextRun->IsRightToLeft depends purely on the directionality of the textrun (which may or may not match the direction of the frame: you can have left-to-right text runs within a frame that has dir=rtl, or vice versa).
Comment on attachment 8910689 [details] Bug 1399310 - (Part 1) Make `nsTextFrame::DrawTextRunAndDecorations` draw only in the range of the text. https://reviewboard.mozilla.org/r/182150/#review192658 ::: layout/generic/nsTextFrame.cpp:6385 (Diff revision 4) > + static int dump_cnt = 0; > + char filename[100]; Don't forget to remove this from the final patch!
Attachment #8910689 - Flags: review?(jfkthame)
Here's a testcase that may be handy: this includes a bunch of writing-mode, direction, and text-run directionality combinations, to check that they all paint successfully. In any cases where the decoration code causes incorrect clipping of the text, you'll see a background "ghost" of the text that should be present.
This test case would be helpful to me. Thx!
Attachment #8910689 - Flags: review?(jfkthame)
Attachment #8917720 - Flags: review?(jfkthame)
Attachment #8910689 - Flags: review?(jfkthame)
I found there are still some failure after I ran a try.
The text-shadow would be clipped out in the following case: data:text/html,<u style="text-shadow: 0 1em 0px blue">one two</u> I'll continue to fix that.
The issue mentioned in comment 25 happens on selection.
In the try run, I found there are some problems that still happen with text-overflow.
It seems to have a wrong clip with text-combine-upright. [1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=54bec1a0936d1eacd9da4322baf29a54b461cedc
I think we cannot use the advance as the clip length when the directions of writing-mode and the text run are different.
Comment on attachment 8917720 [details] Bug 1399310 - (Part 1) Fix the indentation of nsTextFrame::DrawTextRunAndDecorations. https://reviewboard.mozilla.org/r/188650/#review194424 Although it's true the indentation is currently wrong, I'd suggest you don't bother doing this right now: the patch bit-rots very easily (e.g. it fails to apply on my current tip-of-trunk tree), which makes it unnecessarily awkward to apply and test the following patch (which is the one we really care about). I believe there is a plan to run clang-tidy over the entire codebase in the near future, which I expect should resolve formatting errors like this, so I'd prefer to leave it to be handled at that time, and just work within the existing style of the code for the actual functional patch here.
Attachment #8917720 - Flags: review?(jfkthame)
One note that may be helpful: I think you need to apply the clipping only while drawing the actual decoration lines, and not while the text itself is being drawn. This is because glyphs may quite reasonably extend beyond the origin/advance edges in the inline direction, and should not be clipped to those edges just because we're painting the run in parts. Testcase, assuming the font Impact is available (I think it's standard on both macOS and Windows): data:text/html;charset=utf-8,<u style="font:50px impact">naïve <i>test Observe how the diaersis gets clipped as you select across the ï character, and clipping of the italic glyphs in "test". To avoid this problem, I think you need to compute the approriate cliprect for decoration lines, and apply it when painting the under/overlines; then reset the clip for the rendering of the actual text (and emphasis marks, which could also extend beyond the inline edges of the base text); then apply the clip again for line-through. (And as an optimization, skip the whole operation of calculating/applying/resetting the clip in the case where the range being painted is the whole of the textrun. That is likely to be the most common case, so it's worth avoiding the extra work.) (In reply to Tommy Kuo [:kuoe0] at UTC+8 from comment #32) > I think we cannot use the advance as the clip length when the directions of > writing-mode and the text run are different. By "directions" here, I assume you mean the orientation (horizontal or vertical), not just a difference in the inline-direction (LTR vs RTL). That case occurs only with text-combine-upright, I believe. I'd be OK with ignoring that case for the time being, i.e. just don't do the clipping if verticalRun != verticalDec. That means we'd still get multiple-painting of the decoration line in a case like data:text/html,<div style="writing-mode:vertical-lr;font:36px arial;color:%238001"><u>abc<span style="text-combine-upright:all">123</span>def if you select just the "2" in the middle, but it's such an edge-case, we could leave that for a followup.
Attachment #8917720 - Attachment is obsolete: true
I'll add a new test case for text selection to prevent someone from breaking the clip region.
Yes, a test (or a set of tests) to cover the various cases here would be good to have. There are so many factors involved that it's easy to break some relatively obscure example without realizing it, because "normal" content still works as expected. You may be able to create reftests where the testcase selects a range of the content, using the ::-moz-selection pseudo-class to control what the selection highlighting looks like so that it matches color/background properties used on a corresponding <span> in the reference. (Note that the default selection highlighting varies between platforms, which is why you probably need to override it with ::-moz-selection in order to have a consistent result that can be compared to a fixed reference.)
status-firefox57: fix-optional → wontfix
Comment on attachment 8910689 [details] Bug 1399310 - (Part 1) Make `nsTextFrame::DrawTextRunAndDecorations` draw only in the range of the text. https://reviewboard.mozilla.org/r/182150/#review194876 One extra conditional suggested below, to optimize the most common case, then I think this will be ready - thanks! Please also rebase the patch to mozilla-central tip, as there have been at least a couple of recent landings in the same area of the code (my patch in bug 1403521 touched the context here - sorry! - and there's also bug 1406510). I don't think any of this will affect the design or functionality of your patch, it just needs updating so it can land cleanly. ::: layout/generic/nsTextFrame.cpp:7298 (Diff revision 9) > + gfxFloat clipLength = mTextRun->GetAdvanceWidth(aRange, aParams.provider); > + gfxRect clipRect(0, 0, > + verticalDec ? frameSize.width : clipLength / app, > + verticalDec ? clipLength / app : frameSize.height); > + > + const bool isInlineReversed = mTextRun->IsInlineReversed(); > + if (verticalDec) { > + clipRect.y = (isInlineReversed ? aTextBaselinePt.y - clipLength > + : aTextBaselinePt.y) / app; > + } else { > + clipRect.x = (isInlineReversed ? aTextBaselinePt.x - clipLength > + : aTextBaselinePt.x) / app; > + } When `skipClipping` is true, we don't need to do any of the work here; in particular, `mTextRun->GetAdvanceWidth()` is a (relatively) expensive call, so it's nice to avoid it for the common case where we're painting the whole range. So please guard this with an if `(!skipClipping)` condition. (The clipRect variable will need to be declared outside the condition, but it doesn't need to actually be computed unless we're going to use it.)
Attachment #8910689 - Flags: review?(jfkthame)
Comment on attachment 8910689 [details] Bug 1399310 - (Part 1) Make `nsTextFrame::DrawTextRunAndDecorations` draw only in the range of the text. https://reviewboard.mozilla.org/r/182150/#review195402 Looks good, thanks! Please also create the tests to land along with this, otherwise I fear it'll be easy for us to accidentally break it sometime later. ::: layout/generic/nsTextFrame.cpp:7315 (Diff revision 11) > params.decoration = NS_STYLE_TEXT_DECORATION_LINE_OVERLINE; > for (const LineDecoration& dec : Reversed(aDecorations.mOverlines)) { > paintDecorationLine(dec, &Metrics::underlineSize, &Metrics::maxAscent); > } > > + // Some glyphs and emphasises may extend outside the region, so we reset the minor comment wording fix: s/emphasises/emphasis marks/ ::: layout/generic/nsTextFrame.cpp:7338 (Diff revision 11) > // Emphasis marks > DrawEmphasisMarks(aParams.context, wm, > aTextBaselinePt, aParams.framePt, aRange, > aParams.decorationOverrideColor, aParams.provider); > > + // Re-apply the clip region when the line-through is been drawing. s/is been drawing/is being drawn/
Attachment #8910689 - Flags: review?(jfkthame) → review+
Hi jfkthame, I met some problems in the test cases with RTL and Chinese. I apply background-color to the text that I want to select. But the size of background that comes from `background-color` is different to the one that comes from the real selection. I also tried them on release build, and it looks same. How do I fix this? Thanks.
Flags: needinfo?(jfkthame)
Attachment #8921765 - Flags: feedback?(jfkthame)
Hmmm.... so I guess the main trouble you're seeing is because background-color applies to the rect of the <span> element, which has a constant height based on metrics of the first font in the font-family list, while the selection computes its rect based on the metrics of the font(s) that are actually present in each text run. Then in a case like your RTL example, if the first (default) font -- which provides the metrics that determine the default height of the spans -- doesn't include Hebrew characters, and the Hebrew letters fall back to a different font that happens to have larger ascent/descent, then the selection highlight will turn out larger than the background. One thing that may help is to apply lang="he" to the RTL example, so that we use the browser's Hebrew font prefs by default; at least on my macOS system, this results in the use of Arial (instead of Times) as the default font, and the problem doesn't seem to occur any longer. (I didn't check this across all different platforms, though.) For the vertical case that includes Chinese characters, lang="zh" may be less effective, because we have font prefs that actually list Latin fonts first in the list. It might be necessary to explicitly specify a Chinese or Japanese font in order to get more consistent behavior. We have a set of M-plus fonts in the layout/reftests/fonts/mplus directory, so using @font-face to load the mplus-1p-regular.ttf font from there might be an option.
Flags: needinfo?(jfkthame)
Another thing you could do, if it proves difficult to get background and selection areas to match reliably even with appropriate fonts selected, would be to have a testcase that sets the ::-moz-selection background to white, so that the selection is not actually visible at all (and the reference then doesn't need any corresponding spans with background). I believe this would still result in failures on current Nightly when the decoration lines use a partially-transparent color, because even though the selection isn't visible, it'll still break up the painting. Here's a proof-of-concept for a test file using this approach; the reference would be the same, but without applying any selection. On current Nightly, the decoration lines in this testcase appear too dark because of the overpainting (you can verify this by clicking in the testcase to cancel the selection ranges that are applied on load, and see how the decorations become lighter).
See Also: → 1413459
Attachment #8921765 - Flags: review?(jfkthame)
In this try run[1], there is two differences with 1 px. I think it's not a big issue and add fuzzy to it. And I also found this issue can't be fixed with this patch on WebRender, so I added fails-if(webrender) on them and filed bug 1413459 in order to track this issue on WebRender. [1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c756763e5713199e01c10877c4b78e59741f5a81
(In reply to Tommy Kuo [:kuoe0] at UTC+8 from comment #50) > In this try run[1], there is two differences with 1 px. I think it's not a > big issue and add fuzzy to it. And I also found this issue can't be fixed > with this patch on WebRender, so I added fails-if(webrender) on them and > filed bug 1413459 in order to track this issue on WebRender. Your try run was based on mozilla-central from a couple of weeks back; recent webrender changes (maybe bug 1401653) mean that the tests here don't fail with WR any longer. Here's a fresh run based on current m-c tip: https://treeherder.mozilla.org/#/jobs?repo=try&revision=048d52bd8314fa8a90672118713aba17407a9813 (I removed the fuzzy annotations from your test patch for this run, to ensure even small differences would be reported.) However, although the new tests no longer fail with webrender, this run does show up a couple of other points that we need to address somehow. First, quite a number of existing tests are failing, across several platforms, due to slight differences at the ends of decoration lines (where clipping is now being applied). This is similar to the issue you marked as fuzzy in the RTL test, but it also affects a number of pre-existing tests. Given how slight these differences are, marking them as fuzzy may still be OK, but before we do that I think we should try to figure out if something can be improved here. I wonder if rounding the length of the decoration line to whole (device) pixels would be helpful, to try and ensure that clipping happens at exact device-pixel boundaries rather that at arbitrary fractional locations, leading to extra antialiasing? The second thing that shows up in this try run is that a couple of existing tests -- trailing-space-1.html and decorations-multiple-zorder.html -- now fail with webrender. In both cases, it looks like the failure occurs because the new clipping is being applied incorrectly to the shadow of the decoration lines. I'm guessing this may also be related to the WR decoration changes in bug 1401653, so it may be worth talking to Alexis (:Gankro) about it, and filing a followup bug. I also got failures on the new tests (on some platforms) due to the selection highlight showing up where it was supposed to be invisible. I think this indicates the tests need to be annotated with needs-focus, because if the browser doesn't have focus, the specified ::-moz-selection colors are ignored and we get the inactive-selection gray highlight instead.
Comment on attachment 8921765 [details] Bug 1399310 - (Part 2) Add a bunch of test cases for text selection. https://reviewboard.mozilla.org/r/192782/#review200452 ::: layout/reftests/selection/reftest.list:51 (Diff revision 2) > +fuzzy(7,2) fails-if(webrender) == rtl-selection-with-decoration-ref.html rtl-selection-with-decoration.html # See bug 1413459 > +fails-if(webrender) == semitransparent-decoration-line-ref.html semitransparent-decoration-line.html # See bug 1413459 > +== writing-mode-ref.html writing-mode.html Please swap the reference and testcase in each line here, so that the testcase is listed first. (This makes them match the "test" and "reference" buttons when viewing failures in the reftest viewer.) I think these also want "needs-focus" annotations, otherwise they may fail due to inactive-selection highlighting; see preceding comment.
The decoration clipping issue sounds like https://github.com/servo/webrender/issues/1928 Feel free to mark the tests as fails-if(webrender).
Yes, sounds very much like that - thanks. Good to know it's on your radar already!
I applied `Round()` function to round the numbers and ran a new try[1]. It seemed most failures are gone. But the rounding issue still exists on my new test cases. So, I'll still apply the fuzzy annotation on them. And, in this try run, I found the `needs-focus` annotation seems not to work on Android and Win 10 debug. Should we add `fails-if` on for them? [1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a57701a9b8bf653f1e3815e6ddaf2afe773f180b
Flags: needinfo?(jfkthame)
BTW, should we create a new bug for the shadow failures on Bugzilla? Or, just attach the Github link on the reftest.list?
(In reply to Tommy Kuo [:kuoe0] at UTC+8 from comment #56) > BTW, should we create a new bug for the shadow failures on Bugzilla? Or, > just attach the Github link on the reftest.list? Probably best to file a bug; they're cheap! It can just mention the failing tests, and refer to the github issue; then we can note the bug number in the reftest.list manifest when adding the fails-if(webrender) annotations. (In reply to Tommy Kuo [:kuoe0] at UTC+8 from comment #55) > And, in this try run, I found the `needs-focus` annotation seems not to work > on Android and Win 10 debug. Should we add `fails-if` on for them? Hmm, that seems odd. I guess just marking them fails-if(...) is OK, we have enough test coverage across the other platforms. (For Win10-debug, it looks like it's only the non-e10s tests that fail, so I think the annotation should be something like fails-if(/^Windows\x20NT\x2010\.0/.test(http.oscpu)&&isDebugBuild&&!browserIsRemote) fails-if(Android) .... (untested).
Flags: needinfo?(jfkthame)
Comment on attachment 8921765 [details] Bug 1399310 - (Part 2) Add a bunch of test cases for text selection. https://reviewboard.mozilla.org/r/192782/#review200996 ::: layout/reftests/selection/rtl-selection-with-decoration.html:26 (Diff revision 2) > + </style> > + <script type="text/javascript" charset="utf-8"> > + function select() { > + window.getSelection().removeAllRanges(); > + var elems = document.getElementsByTagName('p'); > + for (var i in elems) { Actually, it's wrong to use `for (var i in elems)` here (sorry, I think I suggested this earlier!), because elems is an HTMLCollection, and iterating with `for ... in` will return its `item()` method, in addition to the indexes we actually want. This leads to a JS error when it gets used as a subscript in `range.setStart(...)` below (on the third time round the loop, after the two intended ranges have been successfully set). So this should be done using something like `for (var i = 0; i < elems.length; i++)` instead, as it was in an earlier version. Same applies to each of the test files.
The try run[1] seems to be ok for now. Please review the test case, thx! [1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e7ebb1105bd6e564b1a5f2c8c6a0ce642cca270
Comment on attachment 8921765 [details] Bug 1399310 - (Part 2) Add a bunch of test cases for text selection. https://reviewboard.mozilla.org/r/192782/#review201512
Attachment #8921765 - Flags: review?(jfkthame) → review+
Pushed by tokuo@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0564a1d6d2d6 (Part 1) Make `nsTextFrame::DrawTextRunAndDecorations` draw only in the range of the text. r=jfkthame https://hg.mozilla.org/integration/autoland/rev/fb90190ba5b1 (Part 2) Add a bunch of test cases for text selection. r=jfkthame
https://hg.mozilla.org/mozilla-central/rev/0564a1d6d2d6 https://hg.mozilla.org/mozilla-central/rev/fb90190ba5b1
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Depends on: 1415214
Depends on: 1415228
No longer depends on: 1415998
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: