Fixing handling of Ligature when letter-spacing is present

Fix handling of Ligature when letter-spacing is present.

Blink is applying ligature feature when letter spacing is specified which is not correct as per the CSS
specification [1].

With this patch ligature is not applied when letter spacing is specified in CSS.

BUG=382381
TEST=fast/text/font-ligature-letter-spacing.html

1: http://dev.w3.org/csswg/css-text-3/#letter-spacing-property

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176539

[h.joshi, 2014-06-09 09:17:51]

PTAL, initial patch with test case.

[Inactive, 2014-06-09 13:00:08]

Several typos (or incorrect English) in the description:
- "letter letter"
- "updates the ligature is applied"

[h.joshi, 2014-06-09 13:09:03]

Corrected description, sorry for the typo.

[Inactive, 2014-06-09 13:09:44]

https://codereview.chromium.org/325673002/diff/1/Source/platform/fonts/FontDe...
File Source/platform/fonts/FontDescription.cpp (right):

https://codereview.chromium.org/325673002/diff/1/Source/platform/fonts/FontDe...
Source/platform/fonts/FontDescription.cpp:185: // As per CSS
(http://www.w3.org/TR/CSS2/text.html#propdef-letter-spacing),
Please always point to the latest version of the specs:
http://dev.w3.org/csswg/css-text-3/#letter-spacing-property

Specs are often updated and we wouldn't want to make an update to match an old
version of a spec if we already match the latest version.

FYI, the latest spec says: "When the effective letter-spacing between two
characters is not zero (due to either justification or non-zero computed
letter-spacing), user agents should not apply optional ligatures."

[Inactive, 2014-06-09 13:14:47]

https://codereview.chromium.org/325673002/diff/1/LayoutTests/fast/text/font-l...
File LayoutTests/fast/text/font-ligature-letter-spacing.html (right):

https://codereview.chromium.org/325673002/diff/1/LayoutTests/fast/text/font-l...
LayoutTests/fast/text/font-ligature-letter-spacing.html:38: elementWidth_1 =
document.getElementsByClassName("dligSpan")[0].getBoundingClientRect().width;
Use "var" to declare variable. Also, please use better names (e.g.
elementWithLetterSpacing, elementWithoutLetterSpacing).
assert_equals(elementWithLetterSpacing.getBoundingClientRect().width,
elementWithoutLetterSpacing.getBoundingClientRect().width);

https://codereview.chromium.org/325673002/diff/1/LayoutTests/fast/text/font-l...
LayoutTests/fast/text/font-ligature-letter-spacing.html:39: elementWidth_2 =
document.getElementsByClassName("letterSpace")[0].getBoundingClientRect().width;
var elementWidth_2

https://codereview.chromium.org/325673002/diff/1/LayoutTests/fast/text/font-l...
LayoutTests/fast/text/font-ligature-letter-spacing.html:47: 
No need for the blank line.

https://codereview.chromium.org/325673002/diff/1/LayoutTests/fast/text/font-l...
LayoutTests/fast/text/font-ligature-letter-spacing.html:57: <div id="log"></div>
Why this div?

https://codereview.chromium.org/325673002/diff/1/LayoutTests/fast/text/font-l...
LayoutTests/fast/text/font-ligature-letter-spacing.html:58: </body></html>
We usually have </html> on its own line.

[h.joshi, 2014-06-09 13:33:53]

new patch with suggested changes.

https://codereview.chromium.org/325673002/diff/1/LayoutTests/fast/text/font-l...
File LayoutTests/fast/text/font-ligature-letter-spacing.html (right):

https://codereview.chromium.org/325673002/diff/1/LayoutTests/fast/text/font-l...
LayoutTests/fast/text/font-ligature-letter-spacing.html:38: elementWidth_1 =
document.getElementsByClassName("dligSpan")[0].getBoundingClientRect().width;
Done, using new variable in separate line as using inside assert_equal was
creating very long line.

https://codereview.chromium.org/325673002/diff/1/LayoutTests/fast/text/font-l...
LayoutTests/fast/text/font-ligature-letter-spacing.html:39: elementWidth_2 =
document.getElementsByClassName("letterSpace")[0].getBoundingClientRect().width;
Changed to new name.

https://codereview.chromium.org/325673002/diff/1/LayoutTests/fast/text/font-l...
LayoutTests/fast/text/font-ligature-letter-spacing.html:47: 
Blank line removed between </head> and <body>

https://codereview.chromium.org/325673002/diff/1/LayoutTests/fast/text/font-l...
LayoutTests/fast/text/font-ligature-letter-spacing.html:57: <div id="log"></div>
This is used by testharness script to insert logs, same is used by other scripts
as well.

https://codereview.chromium.org/325673002/diff/1/LayoutTests/fast/text/font-l...
LayoutTests/fast/text/font-ligature-letter-spacing.html:58: </body></html>
On 2014/06/09 13:14:47, Chris Dumez wrote:
> We usually have </html> on its own line.

Done.

https://codereview.chromium.org/325673002/diff/1/Source/platform/fonts/FontDe...
File Source/platform/fonts/FontDescription.cpp (right):

https://codereview.chromium.org/325673002/diff/1/Source/platform/fonts/FontDe...
Source/platform/fonts/FontDescription.cpp:185: // As per CSS
(http://www.w3.org/TR/CSS2/text.html#propdef-letter-spacing),
In "https://developer.mozilla.org/en-US/docs/Web/CSS/letter-spacing" CSS 2 was
written as Recommended thats why used CSS 2 link.
Made changes and included new one as suggested in new patch.

[Inactive, 2014-06-09 13:39:45]

Please update the spec link in the CL description as well.

Once this is done and the nit is fixed, please add a knowledgeable OWNER for
this area of the code.

https://codereview.chromium.org/325673002/diff/20001/LayoutTests/fast/text/fo...
File LayoutTests/fast/text/font-ligature-letter-spacing.html (right):

https://codereview.chromium.org/325673002/diff/20001/LayoutTests/fast/text/fo...
LayoutTests/fast/text/font-ligature-letter-spacing.html:56: <div id="log"></div>
This is actually optional. testharness.js will create the div for you if it is
missing so I would suggest not explicitly adding it unless you need it to be at
a specific location in the document.

[h.joshi, 2014-06-09 13:51:59]

Done suggested changes

https://codereview.chromium.org/325673002/diff/20001/LayoutTests/fast/text/fo...
File LayoutTests/fast/text/font-ligature-letter-spacing.html (right):

https://codereview.chromium.org/325673002/diff/20001/LayoutTests/fast/text/fo...
LayoutTests/fast/text/font-ligature-letter-spacing.html:56: <div id="log"></div>
On 2014/06/09 13:39:45, Chris Dumez wrote:
> This is actually optional. testharness.js will create the div for you if it is
> missing so I would suggest not explicitly adding it unless you need it to be
at
> a specific location in the document.

Done.

[h.joshi, 2014-06-09 14:15:34]

Emil/Behdad: Pls review this patch.

[eae, 2014-06-17 18:33:36]

This is a great change, thank you!

LGTM

[behdad_google, 2014-06-17 19:16:43]

lgtm

[h.joshi, 2014-06-18 02:56:47]

@Emil/Behdad: Thank you.

[h.joshi, 2014-06-18 05:36:46]

The CQ bit was checked by h.joshi@samsung.com

[commit-bot: I haz the power, 2014-06-18 05:38:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/h.joshi@samsung.com/325673002/40001

[commit-bot: I haz the power, 2014-06-18 07:19:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-18 07:19:49]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)

[h.joshi, 2014-06-18 12:35:34]

@Emil/Behdad: Pls review new patch, this has following change in
FontDescription.cpp:
From: m_letterSpacing != 0
To: m_letterSpacing == 0
 By mistake wrong conditional check was added before Ligature application.

[eae, 2014-06-19 16:36:31]

LGTM

[h.joshi, 2014-06-19 17:52:06]

Thank you Emil.

[h.joshi, 2014-06-19 17:52:17]

The CQ bit was checked by h.joshi@samsung.com

[commit-bot: I haz the power, 2014-06-19 17:53:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/h.joshi@samsung.com/325673002/80001

[commit-bot: I haz the power, 2014-06-19 19:00:12]

Change committed as 176539