Patch proposal

Ignore spaces when determining the length of text to treat as a first-letter pseudo-element.

Spaces are are neither in the Ps, Pe, Pi, Pf and Po sections of UNICODE, so they should not be
considered when determining the length of the first-letter pseudo-element. This change will make
Blink inconsistent with WebKit (which consider spaces, without it being specified by the CSS spec.
It will make Blink consistent with other engines such Trident (IE11) and Opera.

R=eae@chromium.org, tkent@chromium.org
BUG=340688
TEST=Update existing test to treat spaces according to this change, and move it to use reftests.

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

[mario.prada, 2014-03-17 17:56:45]

Uploaded patch proposal to fix issue 340688 by removing support for white spaces
in first-letter pseudo-elements.

Also, replaced platform-specific expectations with a reftest.

Please review, thanks!

[eae, 2014-03-17 22:36:04]

LGTM

[tkent, 2014-03-17 23:13:00]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-03-17 23:13:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mario.prada@samsung.com/198013006/1

[commit-bot: I haz the power, 2014-03-17 23:14:33]

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

[commit-bot: I haz the power, 2014-03-17 23:14:34]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_compile_dbg

[tkent, 2014-03-17 23:17:39]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-03-17 23:17:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mario.prada@samsung.com/198013006/1

[commit-bot: I haz the power, 2014-03-18 00:02:55]

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

[commit-bot: I haz the power, 2014-03-18 00:02:56]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[mario.prada, 2014-03-18 14:41:10]

The CQ bit was checked by mario.prada@samsung.com

[commit-bot: I haz the power, 2014-03-18 14:41:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mario.prada@samsung.com/198013006/1

[commit-bot: I haz the power, 2014-03-18 15:13:59]

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

[commit-bot: I haz the power, 2014-03-18 15:14:01]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[mario.prada, 2014-03-18 17:39:01]

Wait, I just realized there's an actual error in my patch and not that the
extend-by-word test needed updating. Thus marking my own patch as not lgtm even
if it's weird.

Will upload a new patch soon.

[mario.prada, 2014-03-18 18:32:17]

On 2014/03/18 17:39:01, mario.prada wrote:
> Wait, I just realized there's an actual error in my patch and not that the
> extend-by-word test needed updating. Thus marking my own patch as not lgtm
even
> if it's weird.
> 
> Will upload a new patch soon.

Just uploaded a new patch that fixes a problem with the previous version: that
other patch would recognize a punctuation character alone as a valid
first-letter pseudo element, which is wrong since, according to the spec,
"Punctuation that precedes or follows the first letter should be included",
meaning that you need at least a letter to define a first-letter element.

As an example, this is what the previous patch would identify as the
first-letter pseudoelement for the following strings:

  "% F" -> "%"
  "%F" -> "%F"
  "%F %" -> "%F"
  "%F%" -> "%F%"

...and that was the root reason of extend-by-word-002 to fail, because it was
making fun with offsets due to that.

However, this new patch would identify this as the first-letter pseudoelement
for the same strings:

  "% F" -> "" (nothing, because "%" alone can't be considered a "letter" and the
space in between prevents it to reach the "F")
  "%F" -> "%F"
  "%F %" -> "%F"
  "%F%" -> "%F%"


Last, worth mentioning that this matches now the behaviour implemented by
Firefox, although is slightly different than what Opera and IE11 do (they would
allow "%" to be a first-letter too), which is what misled me in the first place.
However, after re-reading the spec carefully I think that the behaviour in FF is
the right one here, so that's why I proposed this new patch.

Please review again. Thanks!

[eae, 2014-03-18 18:34:27]

On 2014/03/18 18:32:17, mario.prada wrote:
> On 2014/03/18 17:39:01, mario.prada wrote:
> > Wait, I just realized there's an actual error in my patch and not that the
> > extend-by-word test needed updating. Thus marking my own patch as not lgtm
> even
> > if it's weird.
> > 
> > Will upload a new patch soon.
> 
> Just uploaded a new patch that fixes a problem with the previous version: that
> other patch would recognize a punctuation character alone as a valid
> first-letter pseudo element, which is wrong since, according to the spec,
> "Punctuation that precedes or follows the first letter should be included",
> meaning that you need at least a letter to define a first-letter element.
> 
> As an example, this is what the previous patch would identify as the
> first-letter pseudoelement for the following strings:
> 
>   "% F" -> "%"
>   "%F" -> "%F"
>   "%F %" -> "%F"
>   "%F%" -> "%F%"
> 
> ...and that was the root reason of extend-by-word-002 to fail, because it was
> making fun with offsets due to that.
> 
> However, this new patch would identify this as the first-letter pseudoelement
> for the same strings:
> 
>   "% F" -> "" (nothing, because "%" alone can't be considered a "letter" and
the
> space in between prevents it to reach the "F")
>   "%F" -> "%F"
>   "%F %" -> "%F"
>   "%F%" -> "%F%"
> 
> 
> Last, worth mentioning that this matches now the behaviour implemented by
> Firefox, although is slightly different than what Opera and IE11 do (they
would
> allow "%" to be a first-letter too), which is what misled me in the first
place.
> However, after re-reading the spec carefully I think that the behaviour in FF
is
> the right one here, so that's why I proposed this new patch.

Ok, that makes sense. Could you update the test to include some of these edge
cases?

[eae, 2014-03-18 18:49:38]

Please add a couple of more punctuation and white-space cases to
fast/css/first-letter-punctuation.html and feel free to submit when you have.

LGTM

[mario.prada, 2014-03-19 10:19:46]

The CQ bit was checked by mario.prada@samsung.com

[commit-bot: I haz the power, 2014-03-19 10:19:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mario.prada@samsung.com/198013006/60001

[commit-bot: I haz the power, 2014-03-19 11:28:13]

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

[commit-bot: I haz the power, 2014-03-19 11:28:14]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[mario.prada, 2014-03-19 12:07:25]

The CQ bit was checked by mario.prada@samsung.com

[commit-bot: I haz the power, 2014-03-19 12:07:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mario.prada@samsung.com/198013006/60001

[commit-bot: I haz the power, 2014-03-19 13:15:54]

Change committed as 169547