[Webkit-unassigned] [Bug 16131] ZWNJ - Display non-printing, invisible character

Mon Apr 19 09:33:52 PDT 2010

https://bugs.webkit.org/show_bug.cgi?id=16131

Joseph Pecoraro <joepeck at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #51638|review?, commit-queue?      |review-
               Flag|                            |

--- Comment #22 from Joseph Pecoraro <joepeck at webkit.org>  2010-04-19 09:33:51 PST ---
(From update of attachment 51638)
> +2010-03-25  David Yonge-Mallo  <davinci at chromium.org>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Added tests for the handling of ZWJ and ZWNJ characters in simple font
> +        path.  These characters have zero width but may change the widths of text 
> +        around them.  Furthermore, their glyphs should not be displayed in
> +        simple static text.
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=16131
> +
> +        * fast/text/format-control.html: Added.
> +        * fast/text/zero-width-characters.html: modified to test ZWSP, ZWJ, and ZWNJ.
> +        * platform/mac/fast/text/format-control-expected.checksum: Added.
> +        * platform/mac/fast/text/format-control-expected.png: Added.
> +        * platform/mac/fast/text/format-control-expected.txt: Added.

NIT: Since I'm going to recommend some minor changes, could you clean up the
ChangeLog a little. The bug title "ZWNJ - Display non-printing, invisible
character" is missing, and the bug link should go above the comments.
Otherwise, this is a great comment and a good ChangeLog!

The normal order (produced by prepare-ChangeLog) is:

  Reviewed By ...

  Bug Title
  https://bugs.webkit.org/show_bug.cgi?id=...

  Comments...

  * files and comments...

> +<html><head>
> +<meta charset="utf-8"> 
> +</head>
> +<body>
> +This tests the ZWJ and ZWNJ format control characters on basic Latin text.
> +
> +<p>fi fl ff ffi ffl</p> 
> +<p>fâi fâl fâf fâfâi fâfâl</p> 
> +<p>fâi fâl fâf fâfâi fâfâl</p> 
> +<textarea rows="5" cols="80">fi fl ff ffi ffl
> +fâi fâl fâf fâfâi fâfâl
> +fâi fâl fâf fâfâi fâfâl</textarea>
> +</body>
> +</html>

This test is currently passing on Mac platforms. It would be needlessly
difficult to make it fail by default on all possible platforms, but I think
making it fail by default on mac platforms as well would help it to be caught
on build bots. On Mac you can use a font like "Times New Roman". I would
recommend duplicating the <p> and <textarea> down below but forcing that font:

  <div style="font-family: Times New Roman">
  <p>fi fl ff ffi ffl</p> 
  <p>f‌i f‌l f‌f f‌f‌i f‌f‌l</p> 
  <p>f‍i f‍l f‍f f‍f‍i f‍f‍l</p> 
  <textarea rows="5" cols="80">fi fl ff ffi ffl
  f‌i f‌l f‌f f‌f‌i f‌f‌l
  f‍i f‍l f‍f f‍f‍i f‍f‍l</textarea>
  </div>

This would require updated test results.

> +2010-03-25  David Yonge-Mallo  <davinci at chromium.org>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Fix the (non)display of glyphs for ZWJ and ZWNJ in simple font code path.
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=16131
> +
> +        Tests: fast/text/format-control.html
> +               fast/text/zero-width-characters.html
> +
> +        * platform/graphics/Font.h:
> +        (WebCore::Font::operator!=):
> +        (WebCore::Font::treatAsZeroWidthSpace): treat ZWNJ and ZWJ as ZWSP.
> +        * platform/graphics/GlyphPageTreeNode.cpp:
> +        (WebCore::GlyphPageTreeNode::initializePage): added ZWNJ and ZWJ.
> +        * platform/text/CharacterNames.h: added ZWNJ and ZWJ.
> +

NIT: The same ChangeLog comments that I had above.

The patch itself looks good to me! Just update the test so a regression could
surely be caught!

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.