Last Comment Bug 537230 - Modifying spellchecked text may lead to discontinuity in the misspelling marker
: Modifying spellchecked text may lead to discontinuity in the misspelling marker
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla16
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: 59109 518122
  Show dependency treegraph
 
Reported: 2009-12-30 05:11 PST by Olli Pettay [:smaug]
Modified: 2012-07-04 06:37 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (290 bytes, text/html)
2009-12-30 05:11 PST, Olli Pettay [:smaug]
no flags Details
screenshot (24.11 KB, image/png)
2009-12-31 00:52 PST, Olli Pettay [:smaug]
no flags Details
Patch v0.1 (WIP) (13.51 KB, patch)
2011-04-01 02:33 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Patch v0.2 (WIP) (13.99 KB, patch)
2011-04-01 03:30 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
part.1 Paint connection of complex style decoration lines smoothly (33.59 KB, patch)
2012-07-03 01:53 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
roc: review+
Details | Diff | Splinter Review
part.2 Remake the test for selection underlines with CSS3 text-decoration (61.63 KB, patch)
2012-07-03 02:04 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
part.2 Remake the test for selection underlines with CSS3 text-decoration (60.96 KB, patch)
2012-07-03 18:38 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
roc: review+
Details | Diff | Splinter Review

Description Olli Pettay [:smaug] 2009-12-30 05:11:49 PST
Created attachment 419559 [details]
testcase

See the testcase.

Not yet sure if this is a layout/painting issue or a problem in spellchecker.
Comment 1 Olli Pettay [:smaug] 2009-12-30 05:17:20 PST
Doesn't seem to be spellchecker problem, since it recognizes two text nodes 
as one word.

Painting seems to start at the beginning of a textframe/node. It should probably
check if the previous frame has the misspelling marker.
Comment 2 Olli Pettay [:smaug] 2009-12-30 05:26:01 PST
(Not that it matters, but other browsers seem to have the same problem.)
Comment 3 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-12-30 13:43:17 PST
What exactly are the steps to reproduce? If I insert text into that testcase, then first the spellcheck marker goes away, and then if I click outside the text, the whole word is marked again.
Comment 4 Olli Pettay [:smaug] 2009-12-31 00:52:34 PST
Created attachment 419662 [details]
screenshot

Just load the testcase. You may need to zoom in a bit to see the problem.
Comment 5 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-12-31 03:14:45 PST
Ah, so this is because there are two text nodes and we restart painting of the decoration.

Pretty minor bug :-)
Comment 6 David Baron :dbaron: ⌚️UTC-10 2009-12-31 05:12:27 PST
Maybe we want to make the waviness of wavy text decorations relative to the origin of the window?
Comment 7 Olli Pettay [:smaug] 2009-12-31 05:15:09 PST
Spellchecking marker is different on other platforms. On OSX it is dots.
But still, the bug is visible there too.
Comment 8 Masayuki Nakano [:masayuki] (Mozilla Japan) 2009-12-31 19:40:11 PST
(In reply to comment #6)
> Maybe we want to make the waviness of wavy text decorations relative to the
> origin of the window?

I think that it's better especially when we implement CSS3 text-decoration-style. And also dotted and dashed style.
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-12-31 21:10:21 PST
Making it relative to the origin of the window wouldn't work well for scrolling. Maybe relative to the origin of the nearest enclosing block.
Comment 10 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-04-01 02:33:25 PDT
Created attachment 523543 [details] [diff] [review]
Patch v0.1 (WIP)

Works fine for me for normal rendering, however, its shadow isn't rendered as I expected. I'll check the text-shadow rendering code.
Comment 11 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-04-01 02:36:20 PDT
Oops, it doesn't work fine with the testcase...
Comment 12 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-04-01 03:30:39 PDT
Created attachment 523548 [details] [diff] [review]
Patch v0.2 (WIP)

looks fine for me, I'll add some tests.
Comment 13 a4010956 2011-04-26 16:56:30 PDT
(In reply to comment #12)
> Created attachment 523548 [details] [diff] [review]
> Patch v0.2 (WIP)
> 
> looks fine for me, I'll add some tests.

is there a try-server build available with this patch landed?
Comment 14 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-07-03 01:53:49 PDT
Created attachment 638624 [details] [diff] [review]
part.1 Paint connection of complex style decoration lines smoothly

This patch paints decoration lines relative to the nearest ancestor block frame.

And also, at painting decoration lines in text-shadow or relative positioned box, this patch paints them with the offset from original (static) position.
Comment 15 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-07-03 02:04:26 PDT
Created attachment 638626 [details] [diff] [review]
part.2 Remake the test for selection underlines with CSS3 text-decoration

CSS3 text-decoration has been implemented and same methods are used for layouting/painting the CSS3 decoration lines and selection underlines.

And also, the selection underlines test uses canvas and paints with ported methods in decoration_line_rendering.js from nsCSSRendering. So, when we change the code ins nsCSSRendering, we need to change decoration_line_rendering.js too. I think that it doesn't make sense.

Additionally, it works only on Windows with GDI rendering.

So, we should now remake the tests with CSS3 text-decoration. However, unfortunately, there are some difference between selection underline and CSS3 text-decoration. If font doesn't have enough descender for painting underline, CSS3 text-decoration may overlap it on the text. However, selection underline may overflow from the descender. Therefore, this patch may fail on Windows and Mac at testing "double" or "wavy". However, fortunately, all tests pass on Linux. So, the new tests can check new regressions perfectly by running on all platforms.
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-03 17:27:52 PDT
Why do the tests pass on Linux but not Windows or Mac?
Comment 17 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-07-03 17:38:07 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> Why do the tests pass on Linux but not Windows or Mac?

On Mac, all tests with mplus passed. And on Windows, all tests for double and wavy are not passed on tryserver but all tests with mplus passed on my machine.

According to these facts, I guess that it depends on how to compute the descender height. If higher descender font is included in the font group, it can expand the descender. Then, CSS text-decoration and selection underline become same result. And also the difference of native font APIs may cause it too.
Comment 18 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-03 17:52:33 PDT
I don't think we should have a test that depends on the way the descender height is computed. We might want to change Linux to match the other platforms.
Comment 19 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-07-03 17:59:35 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #18)
> I don't think we should have a test that depends on the way the descender
> height is computed. We might want to change Linux to match the other
> platforms.

Hmm, okay.
Comment 20 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-07-03 18:38:35 PDT
Created attachment 638935 [details] [diff] [review]
part.2 Remake the test for selection underlines with CSS3 text-decoration
Comment 21 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-07-03 18:48:38 PDT
Er, kIsWin and kIsMac are not necessary, please ignore them.

Note You need to log in before you can comment on or make changes to this bug.