486778 - Spell checker's wavy line is sometimes drawn through misspelled words

Status: RESOLVED FIXED

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

Description

[u88484]

Attachment: Screenshot

The new wavy line for misspelled words is sometimes drawn through the text and makes it hard to read.  Please see attached screen shot that compares shows the comment input box for the description field when filing new bugs and the input box for an attachment description.  I also see the line through the text on gmail when composing a new email.

See attached screenshots.

Comment 1

[u88484]

Attachment: Screenshot 2

Comment 2

[u88484]

Attachment: Reduced test case, set "layout.spellcheckDefault" to 2

STR:
Type asdf or any misspelled word in both inputs.  Notice the wavy line is drawn differently.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I don't think that the screenshot is unreadable. And I hope you should specify the font-family. input elements and textarea element's fonts depend on the locale of the webpage and system locale and system font settings.

Note that we have very tight space at input element. So, if you select the text and the selection rect's bottom edge is same as the wavy line bottom edge, we cannot draw the wavy line in it if we are painting the wavy line same as OOo.

Comment 4

[u88484]

It isn't unreadable just a little harder to read and not polished since there is a difference in drawing the wavy line with <input> and <textarea>.  Within <textarea>, the wavy line touches all text, whereas in <input> the wavy line only only touches the text with letters like yjq.

Profile: New profile with layout.spellcheckDefault set to "2" so that spell checker checks one line input fields
Firefox Font Settings:
Default (Times New Roman size 16)
 Advanced font settings:
   Fonts for: "Western"
   Proportional: "Serif" Size "16"
   Serif: "Times New Roman"
   Sans-serif: "Arial"
   Monospace: "Courier New" Size "13"
Minimum font size: "None"
"Checked" Allow pages to choose their own fonts
Character Encoding: Western (ISO-8859-1)

Screen Resolution: 1440*900
DPI: 60 DPI

Comment 5

[Peter van der Woude [:Peter6]]

Attachment: testcase with Times New Roman font

the wave goest through the "y" but other letters aren't touched, at least not on XP

Comment 6

[u88484]

Attachment: Testcase with default of 16px and also 22/23px

Comment 7

[u88484]

Sorry, I meant to add a comment in the last comment.  That attached test case has an <input> with a font size of 16, 22 and 23 pixels.  16px is the default.  23 pixels is when the wavy line is first drawn underneath the text and not through the bottom portion.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

We can use only descent space to draw the underline. Because input element don't have more spaces. It means we cannot draw it outside the rect of selection.

I think that we can improve only the first screenshot case. Looks like there is 1px below the wavy line. So, we can lower down the wavy line 1px.

Comment 9

[u88484]

(In reply to comment #8)
> We can use only descent space to draw the underline. Because input element
> don't have more spaces. It means we cannot draw it outside the rect of
> selection.
> 
> I think that we can improve only the first screenshot case. Looks like there is
> 1px below the wavy line. So, we can lower down the wavy line 1px.
I'm assuming the rect is what is blue when you highlight text?  The same height as the caret?  If that is the case then like you say, I don't think it could be fixed but moving the wavy line down one pixel would help a lot.

Just curious on why once the text is 23 pixels that there isn't a problem.  Wouldn't the rect be the same scale no matter the text size?

I don't think the spellchecker is used much for input fields so I don't think this is really a big deal though but the 1 pixel down would definitely look nicer.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #9)
> (In reply to comment #8)
> > We can use only descent space to draw the underline. Because input element
> > don't have more spaces. It means we cannot draw it outside the rect of
> > selection.
> > 
> > I think that we can improve only the first screenshot case. Looks like there is
> > 1px below the wavy line. So, we can lower down the wavy line 1px.
> I'm assuming the rect is what is blue when you highlight text?

Yes.

> The same height as the caret?

I'm not sure what is used for deciding caret height.

> Just curious on why once the text is 23 pixels that there isn't a problem. 
> Wouldn't the rect be the same scale no matter the text size?

Currently, we honor the underline offset of font metrics. At wavy line case, my implementation lifts up the wavy line half of its height (it means the top most of the wavy line to the bottom most). I.e., the underline offset means middle of the wavy line on my implementation. Therefore, we can think that when it's 22px case and when it's 23px case, the font metrics have different underline offset.

Comment 12

[u88484]

OS -> All, since dupe was Windows 7 and I filed as Vista.  How does this look on Mac and Linux?

Comment 13

[Sylvain Pasche]

Attachment: Linux screenshot. Includes a comparison with Pango

There is the same issue on Linux. The issue should be cross-platform as the underline is painted directly by Gecko.

By the way, I've added a comparison with Pango. I think the Pango way of drawing the wavy underline looks more readable. What about getting some inspiration from that?

The code that draws the wavy underline (called error underline in Pango naming) is in http://svn.gnome.org/viewvc/pango/trunk/pango/pango-renderer.c?revision=2814&view=markup#l897

Comment 14

[Sylvain Pasche]

(note that the part the computes the position of the underline, which is what matters most here, may be in another file like pango-layout.c or something).

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.0

Slides down the wavy line if it's possible. I want the feedback for this patch. I'll post this patch to tryserver.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

And I have an idea. The current selection underline is painted *over* the text, so, if we draw the selection underlines to *below* the text, it's better for the readability.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://build.mozilla.org/tryserver-builds/2009-04-07_12:[e-mail]-attachment371503 [review]/

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v2.0

This looks good for me.

This patch has previous patch and changing the painting stack.

This paints as:

Top
  - text-decoration: line-through;
  - IME selection underline
  - Text
  - Spellchecker underline
  - Selection background
  - text-decoration: underline overline;
Bottom

This changes the quirks mode's painting stack, but the result is same as standards mode. I'll post this patch to tryserver.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

You can check the behavior:

data:text/html,<!DOCTYPE html><textarea style="text-shadow: green 3px -3px; text-decoration: underline overline line-through;font-size:64px;">asdfqjpy</textarea>

and

data:text/html,<textarea style="text-shadow: green 3px -3px; text-decoration: underline overline line-through;font-size:64px;">asdfqjpy</textarea>

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: screenshot of Patch v2.0

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

patched builds:

https://build.mozilla.org/tryserver-builds/2009-04-07_23:[e-mail]-attachment371625 [review]/

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

roc and dbaron:

I posted following comment to wrong bug, please recheck my post here.

>   -------  Comment #59 From  Masayuki Nakano (Mozilla Japan)   2009-04-08 04:36:42 PDT   (-) [reply] -------
> 
> roc or dbaron:
> 
> The reftests has testing the text-decoration stacking test in quirks mode. This
> patch makes UNEXPECTED-PASS error.
> 
>> REFTEST TEST-PASS | file:///e:/builds/moz2_slave/sendchange-win32-unittest/mozilla/layout/reftests/text-decoration/text-decoration-zorder-1-standards.html |
>> REFTEST TEST-UNEXPECTED-PASS | file:///e:/builds/moz2_slave/sendchange-win32-unittest/mozilla/layout/reftests/text-decoration/text-decoration-zorder-1-quirks.html |
>> REFTEST   IMAGE: data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAyAAAAPoCAYAAAAmy5qxAAAfg0lEQVR4nO3d227jOBZA0fr/n655GiCNVBxdqC2KWgvwS9u6ODDDs92Z6T9/AQAAIn/uvgEAAOA9BAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJARIAAAQEaAAAAAGQECAABkBAgAAJCZMkD+/Pn5AW/3aX1YIwDA7KYbVwxW8DPxAQA83XQji8EKfiY+AICnm25sMVjBv4kPAGAFU40uBiv4mfgAAFYw1fhisIKfiQ8AYAVTjTAGK/g38QEArGKaMcZgBQAA6zPuAwAAGQECAABkBAhM7rf/+OAKf7Y48j2t+PMBgJVMuTWvNDCMHIIMVO8kQM6dZ4WfDwCsZMqteaWhYdQQZKB6py3xscJn4soAAQDmMt32vNrgcEWA8B4C5J7zAADXmW57Xm14ECActSc+nv7ZECAA8B5Tbc8rDg8j3stqPxO2ESDnz/HknwkArGqq7XnFAWJ0gPAen9bDamvlqgABAOYzzRa94re6f/8KEI75bT2stl4ECAC8xzRb9GoD1f+N/pt23kGA3HMOAOB6U2zRv/1d+5MHiZEBwntsWQd3rZUr1unZc632ewMAVjbFFi1Arjue59m6Dkaul7PXu/vfXqz0OwMAVjfFNn3VUDODkUMZ7zBjgOyJjyP3IUAA4D1u36a/DguzBcjowa48lufa8/kvAuRofOy5FwECAO9x+za9d9A5e50t577qm90z98w77P3MjVorZyPjzgAZ/bsCALjWrdv0lYP/p+uN+rZ37zWP3ivv8YQAOXLskeufuXcAYF5TBcjXf3bVt5qjB6w91zt6n7zD0c/ZiPVyVUSceQ9n7h0AmNdtW/WV36Zuue7ecwsQrjZzgNzxHs5cEwCY13QB8vW5JwTInsHqyD3yHgJk/zWv+B0BAFzr1gD59NxVEVIPeQKELc5+1u8+/uy5Rt43ADC3W7brswP/HQHy6ditxxy5R97h7oAQIABA5bEBcnTQuCJAtg5We++P97g7IAQIAFDJt+sRw74AYSWjPuP1Z3vkuUbeMwAwt2kD5OtrBQgr2xLbZx9n7mHk+9lzzJHrAADzS7fsOwarPfdwxbFHz897CBABAgBvIkAECDcq4mPLZ+ruNSZAAOA9si37imHmyDkFCDMRID8fc+QaAMD8pg+Qr8euHCCGqPcp4+NIAJRrbESAAADPsESA7DmvAGEWKwbI0XMIEAB4j2TbPjsgzB4gW48Z8TrWMeLzPOq8o9aYAAEAfvOIAPl6jrsGpE/Hbj3mzGtYy6igHnXuEevrzPEjYgkAeIbLt+1qqLrjG1oBwlFXxcfR829ZXz8df1W8jHgtADCfLEBGnuuqb2mPHLfnemdew1qeHCB7H0fvd8RrAYD5XLp1jx4Srg6QvUPPiAAxRL3PiIF99DUECABQuWzrHj1cjRp4jpxj1GB15HnWc3V8HLnOb6+9Mj5+Ov+e9wQAPMfwrXvkcHLFN69bXn/VYHXkedbzxADZ8roz7+PM+gUAnuV12/fZQevM9fY8x5pGD+6jrjUq5Efe65nXAQDzet32XQ1//7oezKpeF1uuDwCs6XXbvACB7wQIAFB53TYvQOA7AQIAVF63zQsQ+E6AAACV8dv8n7kfHweti6837LwwmAABACoCRIBQmmANzLAutlx/6DUAgGmM35onGKZmGrQECP8xwRqYYV1suf7QawAA0xi/NU8wTM00aAkQ/mOCNTDDuthy/aHXAACmMX5rnmCYOjJkXTVsCRD+Y4J1sHdt3HX9odcAAKbxmq15S3xc8T9+9T+q5T8miA0BAgDcydYM3M7/CxYAvIdtHgAAyAgQAAAgI0AAAICMAAEAADICBAAAyAgQAAAgI0AAAICMAAEAADICBAAAyAgQAAAgI0AAAICMAAEAADICBAAAyAgQAAAgI0AAAICMAAEAADICBAAAyAgQAAAgI0AAAICMAAEAADICBAAAyAgQAAAgI0AAAICMAAEAADICBAAAyAgQAAAgI0AAAICMAAEAADICBAAAyAgQAAAgI0AAAICMAAEAADICBAAAyAgQAAAgI0AAAICMAAEAADICBAAAyAgQAAAgI0AAAICMAAEAADICBAAAyAgQAAAgMz5A/nhc/uC57v7svPUBAExj/NY8YFj48+Fx+yAzw4Pnunh9WCM/PACAaYzfmi8crm4fYmZ58FwXrg9r5MMDAJjG+K35ogHr9gFmpgfPddH6sEZ+eQAA0xi/NV8wYN0+vMz24LkGrw1rZOMDAJjGVFvzPwcr4O/fvx/iAwDgQaYaXwxW8DPxAQCsYKoRxmAF/yY+AIBVTDPGGKwAAGB9xn0AACAjQAAAgIwAgcn99h8fXOHPFke+pxV/PgCwkim35pUGhpFDkIHqnQTIufOs8PMBgJVMuTWvNDSMGoIMVO+0JT5W+ExcGSAAwFym255XGxyuCBDeQ4Dccx4A4DrTbc+rDQ8ChKP2xMfTPxsCBADeY6rtecXhYcR7We1nwjYC5Pw5nvwzAYBVTbU9rzhAjA4Q3uPTelhtrVwVIADAfKbZolf8VvfvXwHCMb+th9XWiwABgPeYZotebaD6v9F/0847CJB7zgEAXG+KLfq3v2t/8iAxMkB4jy3r4K61csU6PXuu1X5vAMDKptiiBch1x/M8W9fByPVy9np3/9uLlX5nAMDqptimrxpqZjByKOMdZgyQPfFx5D4ECAC8x+3b9NdhYbYAGT3YlcfyXHs+/0WAHI2PPfciQADgPW7fpvcOOmevs+XcV32ze+aeeYe9n7lRa+VsZNwZIKN/VwAA17p1m75y8P90vVHf9u695tF75T2eECBHjj1y/TP3DgDMa6oA+frPrvpWc/SAted6R++Tdzj6ORuxXq6KiDPv4cy9AwDzum2rvvLb1C3X3XtuAcLVZg6QO97DmWsCAPOaLkC+PveEANkzWB25R95DgOy/5hW/IwCAa90aIJ+euypC6iFPgLDF2c/63cefPdfI+wYA5nbLdn124L8jQD4du/WYI/fIO9wdEAIEAKg8NkCODhpXBMjWwWrv/fEedweEAAEAKvl2PWLYFyCsZNRnvP5sjzzXyHsGAOY2bYB8fa0AYWVbYvvs48w9jHw/e445ch0AYH7pln3HYLXnHq449uj5eQ8BIkAA4E0EiADhRkV8bPlM3b3GBAgAvEe2ZV8xzBw5pwBhJgLk52OOXAMAmN/0AfL12JUDxBD1PmV8HAmAco2NCBAA4BmWCJA95xUgzGLFADl6DgECAO+RbNtnB4TZA2TrMSNexzpGfJ5HnXfUGhMgAMBvHhEgX89x14D06ditx5x5DWsZFdSjzj1ifZ05fkQsAQDPcPm2XQ1Vd3xDK0A46qr4OHr+Levrp+OvipcRrwUA5pMFyMhzXfUt7ZHj9lzvzGtYy5MDZO/j6P2OeC0AMJ9Lt+7RQ8LVAbJ36BkRIIao9xkxsI++hgABACqXbd2jh6tRA8+Rc4warI48z3qujo8j1/nttVfGx0/n3/OeAIDnGL51jxxOrvjmdcvrrxqsjjzPep4YIFted+Z9nFm/AMCzvG77PjtonbnenudY0+jBfdS1RoX8yHs98zoAYF6v276r4e9f14NZ1etiy/UBgDW9bpsXIPCdAAEAKq/b5gUIfCdAAIDK67Z5AQLfCRAAoPK6bV6AwHcCBACovG6bFyDwnQABACqv2+YFCHwnQACAyuu2eQEC3wkQAKDyqm3+05B11dBjoOIJBAgAUHnNNr8lPq4YfAxUPIEAAQAqtnngdgIEAN7DNg8AAGQECAAAkBEgAABARoAAAAAZAQIAAGQECAAAkBEgAABARoAAAAAZAQIAAGQECAAAkBEgAABARoAAAAAZAQIAAGQECAAAkBEgAABARoAAAAAZAQIAAGQECAAAkBEgAABARoAAAAAZAQIAAGQECAAAkBEgAABARoAAAAAZAQIAAGQECAAAkBEgAABARoAAAAAZAQIAAGQECAAAkBEgAABARoAAAAAZAQIAAGQECAAAkBEgAABARoAAAAAZAQIAAGQECAAAkBEgAABARoAAAAAZAQIAAGQECAAAkBEgAABAZsoA+fPn5we83af1YY0AALObblwxWMHPxAcA8HTTjSwGK/iZ+AAAnm66scVgBf8mPgCAFUw1uhis4GfiAwBYwVTji8EKfiY+AIAVTDXCGKzg38QHALCKacYYgxUAAKzPuA8AAGQECAAAkBEgMLnf/uODK/zZ4sj3tOLPBwBWMuXWvNLAMHIIMlC9kwA5d54Vfj4AsJIpt+aVhoZRQ5CB6p22xMcKn4krAwQAmMt02/Nqg8MVAcJ7CJB7zgMAXGe67Xm14UGAcNSe+Hj6Z0OAAMB7TLU9rzg8jHgvq/1M2EaAnD/Hk38mALCqqbbnFQeI0QHCe3xaD6utlasCBACYzzRb9Irf6v79K0A45rf1sNp6ESAA8B7TbNGrDVT/N/pv2nkHAXLPOQCA602xRf/2d+1PHiRGBgjvsWUd3LVWrlinZ8+12u8NAFjZFFu0ALnueJ5n6zoYuV7OXu/uf3ux0u8MAFjdFNv0VUPNDEYOZbzDjAGyJz6O3IcAAYD3uH2b/joszBYgowe78liea8/nvwiQo/Gx514ECAC8x+3b9N5B5+x1tpz7qm92z9wz77D3MzdqrZyNjDsDZPTvCgDgWrdu01cO/p+uN+rb3r3XPHqvvMcTAuTIsUeuf+beAYB5TRUgX//ZVd9qjh6w9lzv6H3yDkc/ZyPWy1URceY9nLl3AGBet23VV36buuW6e88tQLjazAFyx3s4c00AYF7TBcjX554QIHsGqyP3yHsIkP3XvOJ3BABwrVsD5NNzV0VIPeQJELY4+1m/+/iz5xp53wDA3G7Zrs8O/HcEyKdjtx5z5B55h7sDQoAAAJXHBsjRQeOKANk6WO29P97j7oAQIABAJd+uRwz7AoSVjPqM15/tkecaec8AwNzGb9d/PC5/sJQtsX32ceYeRr6fPcccuQ4AML/xW/bdw/kbHjzX3Z+dtz4AgGmkW/Md3+zuuYcrjj16fhZ19yD+1gcAMI1sa74iGI6cU4Awk+LPr7Z8pu6O/FH3CwDMb/oA+XrsygFiiHqfMj6OBEC5xkYECADwDEsEyJ7zChBmsWKAHD2HAAGA90i27bMDwuwBsvWYEa9jHSM+z6POO2qNCRAA4DePCJCv57hrQPp07NZjzryGtYwK6lHnHrG+zhw/IpYAgGe4fNuuhqo7vqEVIBx1VXwcPf+W9fXT8VfFy4jXAgDzyQJk5Lmu+pb2yHF7rnfmNazlyQGy93H0fke8FgCYz6Vb9+gh4eoA2Tv0jAgQQ9T7jBjYR19DgAAAlcu27tHD1aiB58g5Rg1WR55nPVfHx5Hr/PbaK+Pjp/PveU8AwHMM37pHDidXfPO65fVXDVZHnmc9TwyQLa878z7OrF8A4Flet32fHbTOXG/Pc6xp9OA+6lqjQn7kvZ55HQAwr9dt39Xw96/rwazqdbHl+gDAml63zQsQ+E6AAACV123zAgS+EyAAQOV127wAge8ECABQed02L0DgOwECAFRet80LEPhOgAAAlddt8wIEvhMgAEDlddu8AIHvBAgAUHnVNv9pyLpq6DFQ8QQCBACovGab3xIfVww+BiqeQIAAABXbPHA7AQIA72GbBwAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgIwAAQAAMgIEAADICBAAACAjQAAAgMz/APfhpl7HzoZfAAAAAElFTkSuQmCC
> 
> Do you know why we are using different stacking between standards mode and
> quirks mode? I think that the stacking should not be different between both
> modes excepting the backward compatibility.
> 
> http://www.w3.org/TR/CSS21/zindex.html#painting-order
> CSS2.1 says that underline and overline should be drawn below the text.
>

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

We implement text-decoration completely differently in standards mode and quirks mode because what *used* to be specified in CSS2.1 was incompatible with the Web.  CSS 2.1 has now changed such that what it specifies is *probably* compatible with the Web, but we haven't had a chance to change our implementation to try it out.  See bug 403524 and many other bugs.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

dbaron:

So, do you like the patch which changes the painting stacking of quirks mode to same as our standards mode?

Currently, we are painting by following stacking in quirks mode:

- <selection underlines>
- text-decoration: line-through;
- text-decoration: underline;
- text-decoration: overline;
- <text>
- <selection background color>
- <background>

but the patch changes to:

- text-decoration: line-through;
- <IME selection underlines>
- <text>
- <spellchecker underline>
- text-decoration: underline;
- text-decoration: overline;
- <selection background>
- <background>

So, spellchecker underline and text-decoration: underline overline; are moved to below the text. By this change, the spellchecker underline should not cause the unreadability. But it should be over the decoration lines of CSS for readability of the spellchecker underline. This stacking is same as CSS2.1's.

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v2.1

I think that we should use same painting stacking between both modes and it should be same as CSS2.1 recommended.

This patch should improves the readability of the words which is misspelled words. The descent glyph is drawn over the spellchecker underlines which is drawn over the text-decoration's underline. And also, IME selection underlines should be kept to drawn *over* the text, because the decoration lines are important and should not be hidden by the *inputting* characters.

The patched builds are here:
https://build.mozilla.org/tryserver-builds/2009-04-12_19:[e-mail]-spellchecker-underline-z-order-2/

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oops, I forgot to comment the changes in nsCSSRendering.cpp.

Our current implementation is lifting up half height of the decoration rect the selection underline if it is wavy line. Because I was thinking that it suppresses that we paint wavy line at too far position from baseline with some fonts. However, it helps to create this bug, therefore, I just removes the special code.

Comment 27

[u88484]

Attachment: Comparison screenshot of Trunk and w/patch v. 2.1

Looks a lot better!

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Kurt:

Thank you for your testing. And I want to know your impressions of descent space using characters, e.g., 'y', 'g' and 'j'. They are painted *under* the spellchecker underline on current trunk. However, the patch paints them *over* the spellchecker underline. Do you feel better the new behavior?

Comment 29

[Sylvain Pasche]

I think that's an improvement. IMHO it would be even more readable if the underline didn't touch the baseline. For instance if there was always at least one pixel gap between the baseline and the top of the wavy line.

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Yeah, if the font has enough descent space, the wavy line doesn't touch to baseline. But if not so (it's most cases), we need to lift up the wavy line for input element. We need to paint the selection line in smaller space than Office applications, unfortunately.

Comment 31

[u88484]

Attachment: Comparing line placement and drawing line under or over on trunk and w/patch 2.1

I think drawing the line over looks better.  With patch 2.1 it kind of looks like random pixels and with the trunk you can barely make out that it is all one wavy line.  Just my two cents.

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 372366 [details] [diff] [review]
Patch v2.1

Kurt:

OK, your opinion may be true for some other people. I'm restart to think the better way.

Comment 33

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v3.0

ok, this removes the changing of decoration line z-order. I think the way may be good way, but I'm not sure whether that is safe.

This patch gets down the underline if it's possible. This patch uses line-height value if that is larger than 1 or normal. If we can know that there is line gap below the descent space, this patch also uses there.
# But this patch doesn't check vertical-align value, but it can be problem on HTML editor in rare cases, probably. The lift up code is needed for input element, so, we should not have the tight space problem on HTML editor.

And this patch removes some special code from nsCSSRendering::GetTextDecorationRectInternal. Now, this patch makes simpler.

Comment 34

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

new test builds:
https://build.mozilla.org/tryserver-builds/2009-04-17_02:[e-mail]-attachment373285 [review]/

Comment 35

[u88484]

(In reply to comment #34)
> new test builds:
> https://build.mozilla.org/tryserver-builds/2009-04-17_02:[e-mail]-attachment373285 [review]/
(In reply to comment #33)
> Created an attachment (id=373285) [details]
> Patch v3.0
> ok, this removes the changing of decoration line z-order. I think the way may
> be good way, but I'm not sure whether that is safe.
> This patch gets down the underline if it's possible. This patch uses
> line-height value if that is larger than 1 or normal. If we can know that there
> is line gap below the descent space, this patch also uses there.
> # But this patch doesn't check vertical-align value, but it can be problem on
> HTML editor in rare cases, probably. The lift up code is needed for input
> element, so, we should not have the tight space problem on HTML editor.
> And this patch removes some special code from
> nsCSSRendering::GetTextDecorationRectInternal. Now, this patch makes simpler.

I haven't test the latest tryserver build yet but will tonight but what exactly should I test for?  I don't understand the above comment

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The latest patch just gets down the wavy line as far as possible. Current trunk build and Patch v2.0 and former only uses maxDescent space of the font, however, v3.0 also uses the gap between lines. This helps to render CJK fonts which don't have enough internal/external leading. I have written some special code for wavy line positioning for such CJK fonts. But this patch can remove them by using line-gap spaces.

I hope that some people use the test build on some sites. And please feed back the readability of the new patch.

If this patch cannot improve the readability, I don't have more ideas excepting the underline z-order changing.

Comment 37

[u88484]

What's the status on this bug?  I haven't had any issues anywhere and it looks a lot better then what it does on the trunk, although not perfect but it appears there is nothing more you can do to fix this.

Comment 38

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I'm still waiting the feedbacks of the latest patch :-p

Comment 39

[u88484]

Do the tryserver builds disappear after a certain amount of time?  If so, I never got to test a build with patch 3.0.

Comment 40

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Yes, currently, the builds are removed 14days later. I'll recreate the builds ASAP.

Comment 41

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v3.1

Updated for latest trunk. The test builds are here:
https://build.mozilla.org/tryserver-builds/[e-mail]-better-spellchecker-underline/

Comment 42

[Ria Klaassen (not reading all bugmail)]

That is weird, after mistyping a few words and then pressing enter, the underlines seem to disappear, at least on this Bugzilla text field.

Comment 43

[Ria Klaassen (not reading all bugmail)]

Attachment: screenshot underlines gone

Comment 44

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #42)
> That is weird, after mistyping a few words and then pressing enter, the
> underlines seem to disappear, at least on this Bugzilla text field.

Is that a regression of my patch??

Comment 45

[Ria Klaassen (not reading all bugmail)]

I assumed so. I could repeat it with default settings and in safe-mode, but not anymore now. Weird.

Comment 46

[Ria Klaassen (not reading all bugmail)]

I tested 5 different forum text fields with default font settings and three of them were slightly better than before (one pixel lower) and the other two were not changed. So I think the overall result is better than before.
I have not seen the issue of comment 43 anymore; no idea why that happened.

Comment 47

[Ria Klaassen (not reading all bugmail)]

(In reply to comment #42)
> That is weird, after mistyping a few words and then pressing enter, the
> underlines seem to disappear, at least on this Bugzilla text field.

This is what I did: first I ran the tryserver build with my default profile and there I saw this bug. To verify that it was not caused by an extension I copied the profile to my desktop and deleted prefs.js and the folder extensions. And weird, it happened again. The only remarkable thing in my set-up is a very large, nearly maximum size persdict.dat file (193 kB).
But I can't reproduce it anymore.

Comment 48

[u88484]

Patch 3.1 looks great and I haven't seen any regressions.  Great job!

Comment 49

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 383714 [details] [diff] [review]
Patch v3.1

ok, let's go to the review process.

This patch removes the special lifting up code for wavy line.

But this patch lifts up the wavy line when it overflows below the descent space. Then, the wavy line is lifted up to the bottom of its baseline even if the wavy line cannot be in the descent space.

Comment 50

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

+  return aFontMetrics.maxDescent + (lineHeight - aFontMetrics.maxHeight) / 2;

Why / 2?

Isn't this assuming that the text is going to be baseline-aligned? What about other values of 'vertical-align'?

Comment 51

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #50)
> +  return aFontMetrics.maxDescent + (lineHeight - aFontMetrics.maxHeight) / 2;
> 
> Why / 2?

The leading should be used at the upper of the line and the bottom. I.e., |(lineHeight - aFontMetrics.maxHeight) / 2| means the half leading.

> Isn't this assuming that the text is going to be baseline-aligned? What about
> other values of 'vertical-align'?

No, I don't assume that the other vertical-align values. I think that the checking of the limitation of the descent space is needed only when the textframe is in input element or textarea element. In other cases, i.e., when the textframe is in HTML editor and the textframe is in complex inline layout, there should be enough non-clipped space in most cases. Therefore, ComputeDescentLimitForSelectionUnderline returns non-problem values but they can be incorrect, maybe. How do you think?

Comment 52

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

Comment on attachment 383714 [details] [diff] [review]
Patch v3.1

I think you're right.

Comment 53

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/mozilla-central/rev/e3ec5794180b

Comment 54

[-fullmetaljacket-]

(how) can we nominate this for 3.5.1?

Comment 55

[u88484]

(In reply to comment #54)
> (how) can we nominate this for 3.5.1?
Like so for now but I really, really doubt it will be checked in there.

Comment 56

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

bug 338209 is not fixed on gecko 1.9.1, so the patch isn't needed on 1.9.1 branch.

Comment 57

[-fullmetaljacket-]

(In reply to comment #56)
> bug 338209 is not fixed on gecko 1.9.1, so the patch isn't needed on 1.9.1
> branch.

i forgot to look that bug. :)