New issue
Advanced search Search tips

Issue 597311 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 2
Type: Bug-Regression

Blocking:
issue 597312



Sign in to add a comment

Text metrics miscomputed for surrogate pairs

Project Member Reported by f...@opera.com, Mar 23 2016

Issue description

With the new fancy CharacterRange computation, surrogate pairs seems to get half the advance compared to previously:

https://jsfiddle.net/0o4hbqsh/

(We "skip" the trail, so it'll never be counted.)
 

Comment 1 by f...@opera.com, Mar 23 2016

Blocking: 597312

Comment 2 by f...@opera.com, Mar 23 2016

Labels: -Type-Bug Type-Bug-Regression

Comment 3 by pdr@chromium.org, Mar 23 2016

Labels: Needs-Bisect
Lets see if a bisect will show which patch caused this (likely one of my patches).

@Testers, this testcase may be a little easier to use: http://jsbin.com/jamire

Comment 4 by f...@opera.com, Mar 23 2016

My guess would be https://codereview.chromium.org/1773403002 - that might help reduce the bisect range.
Cc: -pdr@chromium.org nyerramilli@chromium.org
Labels: -Needs-Bisect M-51 OS-Linux OS-Mac OS-Windows
Owner: pdr@chromium.org
Status: Assigned (was: Available)
Thanks for the issue & sample file

Bisect Info:
CHANGELOG URL:
  https://chromium.googlesource.com/chromium/src/+log/dbdf20c9eec45a535156daf94b94db89ace868f5..7927861fcaad41e2d4d4fb713745778bde1a2b0e

suspecting https://chromium.googlesource.com/chromium/src/+/304ec1544273ed8d62c693da6dd2c63727805cdd, 
pdr@,Could you please check the above issue & help us in finding an owner it its not yours.

Broken in M51, able to repro on Win7, Mac OSX 10.10.3, Ubuntu 14.04 on Chrome Canary/Dev #51.0.2692.0 using http://jsbin.com/jamire
Good Build:51.0.2674.0
Bad Build: 51.0.2675.0
Labels: hasbisect

Comment 7 by f...@opera.com, Mar 29 2016

Some movement in the direction of issue 593570 would likely fix this (i.e iterate glyphs and create SVGTextMetrics from them rather than doing it per character [or "per code point" even as this issues shows]. Will need to "synthesize" per-character/grapheme metrics though for the query interfaces - eventhough I don't think the spec strictly requires that granularity.)

Comment 8 by pdr@chromium.org, Mar 30 2016

@fs, wdyt about removing the synthesis of positions for ligatures in SVGTextMetricsBuilder? This would result in 'ffi' having character widths "20px", "0px", and "0px" which would look worse in some scenarios, but better for cases like this (20px is a made up font size here). Another option is to switch to the approach Gecko uses where 'fii' has character widths "20px", "20px", "20px" and the ranges overlap.

Would you be up for adding your thoughts to https://github.com/w3c/svgwg/issues/65?

Comment 9 by f...@opera.com, Mar 30 2016

Tav kind of beat me to it to some degree...

Removing the synthesis (in SVGTextMetricsBuilder) sgtm. I'm not sure if keeping zero-width entities is the the right thing to do though. We could keep the TCU and then synthesize based on graphemes[1] when needed? So store only "20px" (and maybe some metadata, we could make room for a few bits there) and then split based on grapheme count. (I think there's supposed to be data for better "split points" in fonts, but no idea how common that is.)

[1] https://svgwg.org/svg2-draft/text.html#TermFindGraphemeClusterForCharacter

Comment 10 by pdr@chromium.org, Mar 31 2016

Cc: drott@chromium.org
I walked in the office today and found Dominik at my desk! Dominik mentioned that the font data for cursor positions is available but nothing currently uses it so it may be risky to depend on.

@drott, you mentioned that we have an iterator that can disambiguate character + combining diacritic from ligatures but I wasn't able to find it in the codebase. Can you point me to that?

Comment 11 by drott@chromium.org, Mar 31 2016

> I walked in the office today and found Dominik at my desk! Dominik mentioned that the font data for cursor positions is available but nothing currently uses it so it may be risky to depend on.

Yes, we should experiment with caret positioning information from the fonts, but the next best thing atm is linear interpolation.

> @drott, you mentioned that we have an iterator that can disambiguate character + combining diacritic from ligatures but I wasn't able to find it in the codebase. Can you point me to that?

Well, it's purpose is to find grapheme cluster boundaries, a concept that can be determined without the font & style information, here is: cursorMovementIterator
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/platform/text/TextBreakIterator.h&q=cursorMovem&sq=package:chromium&l=41

See also inline countGraphemesInCluster() in ShapeResultBuffer where this is used similarly to count the number of Graphemes in a HarfBuzz cluster, in order to determine where to place emphasis marks, which is a similar thing. 
Project Member

Comment 12 by bugdroid1@chromium.org, Mar 31 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ec8e7dc866c0edc91cb9c79cd82ea40d2b5e5b58

commit ec8e7dc866c0edc91cb9c79cd82ea40d2b5e5b58
Author: pdr <pdr@chromium.org>
Date: Thu Mar 31 22:03:58 2016

Only synthesize grapheme widths once for surrogate pair characters

This patch updates the grapheme width synthesis code to not set
surrogate pairs to be half-width. With this change our rendering
more closely matches Gecko.

There are no pixel differences in svg/text layouttests as a result
of this codechange. This patch does unify the multiple copies of
highlight code which resulted in a small pixel difference in
combining-character-queries.html due to the height of the highlight
text.

BUG= 597311 

Review URL: https://codereview.chromium.org/1847763002

Cr-Commit-Position: refs/heads/master@{#384412}

[modify] https://crrev.com/ec8e7dc866c0edc91cb9c79cd82ea40d2b5e5b58/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/ec8e7dc866c0edc91cb9c79cd82ea40d2b5e5b58/third_party/WebKit/LayoutTests/platform/mac/svg/text/bidi-text-query-expected.txt
[modify] https://crrev.com/ec8e7dc866c0edc91cb9c79cd82ea40d2b5e5b58/third_party/WebKit/LayoutTests/platform/mac/svg/text/combining-character-queries-expected.png
[modify] https://crrev.com/ec8e7dc866c0edc91cb9c79cd82ea40d2b5e5b58/third_party/WebKit/LayoutTests/platform/mac/svg/text/combining-character-queries-expected.txt
[modify] https://crrev.com/ec8e7dc866c0edc91cb9c79cd82ea40d2b5e5b58/third_party/WebKit/LayoutTests/platform/mac/svg/text/ligature-queries-expected.txt
[modify] https://crrev.com/ec8e7dc866c0edc91cb9c79cd82ea40d2b5e5b58/third_party/WebKit/LayoutTests/svg/text/bidi-text-query.svg
[modify] https://crrev.com/ec8e7dc866c0edc91cb9c79cd82ea40d2b5e5b58/third_party/WebKit/LayoutTests/svg/text/combining-character-queries.html
[modify] https://crrev.com/ec8e7dc866c0edc91cb9c79cd82ea40d2b5e5b58/third_party/WebKit/LayoutTests/svg/text/ligature-queries.html
[add] https://crrev.com/ec8e7dc866c0edc91cb9c79cd82ea40d2b5e5b58/third_party/WebKit/LayoutTests/svg/text/resources/highlightGlyphs.js
[add] https://crrev.com/ec8e7dc866c0edc91cb9c79cd82ea40d2b5e5b58/third_party/WebKit/LayoutTests/svg/text/surrogate-pair-queries-expected.png
[add] https://crrev.com/ec8e7dc866c0edc91cb9c79cd82ea40d2b5e5b58/third_party/WebKit/LayoutTests/svg/text/surrogate-pair-queries-expected.txt
[add] https://crrev.com/ec8e7dc866c0edc91cb9c79cd82ea40d2b5e5b58/third_party/WebKit/LayoutTests/svg/text/surrogate-pair-queries.html
[modify] https://crrev.com/ec8e7dc866c0edc91cb9c79cd82ea40d2b5e5b58/third_party/WebKit/Source/core/layout/svg/SVGTextMetricsBuilder.cpp

Project Member

Comment 13 by bugdroid1@chromium.org, Apr 1 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/525773c8ec1801273c954b43344a75cf3aa7ff03

commit 525773c8ec1801273c954b43344a75cf3aa7ff03
Author: Rebaseline Bot <blink-rebaseline-bot@chromium.org>
Date: Fri Apr 01 01:35:20 2016

Auto-rebaseline for r384412

https://chromium.googlesource.com/chromium/src/+/ec8e7dc86

BUG= 597311 
TBR=pdr@chromium.org

Review URL: https://codereview.chromium.org/1853593002 .

Cr-Commit-Position: refs/heads/master@{#384467}

[modify] https://crrev.com/525773c8ec1801273c954b43344a75cf3aa7ff03/third_party/WebKit/LayoutTests/TestExpectations
[add] https://crrev.com/525773c8ec1801273c954b43344a75cf3aa7ff03/third_party/WebKit/LayoutTests/platform/android/svg/text/combining-character-queries-expected.png
[add] https://crrev.com/525773c8ec1801273c954b43344a75cf3aa7ff03/third_party/WebKit/LayoutTests/platform/android/svg/text/ligature-queries-expected.txt
[add] https://crrev.com/525773c8ec1801273c954b43344a75cf3aa7ff03/third_party/WebKit/LayoutTests/platform/android/svg/text/surrogate-pair-queries-expected.png
[add] https://crrev.com/525773c8ec1801273c954b43344a75cf3aa7ff03/third_party/WebKit/LayoutTests/platform/android/svg/text/surrogate-pair-queries-expected.txt
[modify] https://crrev.com/525773c8ec1801273c954b43344a75cf3aa7ff03/third_party/WebKit/LayoutTests/platform/linux/svg/text/bidi-text-query-expected.txt
[modify] https://crrev.com/525773c8ec1801273c954b43344a75cf3aa7ff03/third_party/WebKit/LayoutTests/platform/linux/svg/text/combining-character-queries-expected.png
[modify] https://crrev.com/525773c8ec1801273c954b43344a75cf3aa7ff03/third_party/WebKit/LayoutTests/platform/linux/svg/text/combining-character-queries-expected.txt
[modify] https://crrev.com/525773c8ec1801273c954b43344a75cf3aa7ff03/third_party/WebKit/LayoutTests/platform/linux/svg/text/ligature-queries-expected.txt
[add] https://crrev.com/525773c8ec1801273c954b43344a75cf3aa7ff03/third_party/WebKit/LayoutTests/platform/linux/svg/text/surrogate-pair-queries-expected.png
[add] https://crrev.com/525773c8ec1801273c954b43344a75cf3aa7ff03/third_party/WebKit/LayoutTests/platform/linux/svg/text/surrogate-pair-queries-expected.txt
[modify] https://crrev.com/525773c8ec1801273c954b43344a75cf3aa7ff03/third_party/WebKit/LayoutTests/platform/mac-mac10.10/svg/text/combining-character-queries-expected.png
[modify] https://crrev.com/525773c8ec1801273c954b43344a75cf3aa7ff03/third_party/WebKit/LayoutTests/platform/mac-mac10.10/svg/text/combining-character-queries-expected.txt
[modify] https://crrev.com/525773c8ec1801273c954b43344a75cf3aa7ff03/third_party/WebKit/LayoutTests/platform/mac-mac10.9/svg/text/combining-character-queries-expected.png
[modify] https://crrev.com/525773c8ec1801273c954b43344a75cf3aa7ff03/third_party/WebKit/LayoutTests/platform/mac-mac10.9/svg/text/combining-character-queries-expected.txt
[modify] https://crrev.com/525773c8ec1801273c954b43344a75cf3aa7ff03/third_party/WebKit/LayoutTests/platform/mac-mac10.9/svg/text/ligature-queries-expected.txt
[add] https://crrev.com/525773c8ec1801273c954b43344a75cf3aa7ff03/third_party/WebKit/LayoutTests/platform/mac-mac10.9/svg/text/surrogate-pair-queries-expected.png
[add] https://crrev.com/525773c8ec1801273c954b43344a75cf3aa7ff03/third_party/WebKit/LayoutTests/platform/mac-mac10.9/svg/text/surrogate-pair-queries-expected.txt
[modify] https://crrev.com/525773c8ec1801273c954b43344a75cf3aa7ff03/third_party/WebKit/LayoutTests/platform/win/svg/text/bidi-text-query-expected.txt
[modify] https://crrev.com/525773c8ec1801273c954b43344a75cf3aa7ff03/third_party/WebKit/LayoutTests/platform/win/svg/text/combining-character-queries-expected.png
[modify] https://crrev.com/525773c8ec1801273c954b43344a75cf3aa7ff03/third_party/WebKit/LayoutTests/platform/win/svg/text/combining-character-queries-expected.txt
[modify] https://crrev.com/525773c8ec1801273c954b43344a75cf3aa7ff03/third_party/WebKit/LayoutTests/platform/win/svg/text/ligature-queries-expected.txt
[add] https://crrev.com/525773c8ec1801273c954b43344a75cf3aa7ff03/third_party/WebKit/LayoutTests/platform/win/svg/text/surrogate-pair-queries-expected.png
[add] https://crrev.com/525773c8ec1801273c954b43344a75cf3aa7ff03/third_party/WebKit/LayoutTests/platform/win/svg/text/surrogate-pair-queries-expected.txt

Comment 14 by f...@opera.com, Apr 9 2016

Fixed?

Comment 15 by pdr@chromium.org, Apr 9 2016

Status: Fixed (was: Assigned)
Fixed!

Sign in to add a comment