[LayoutNG] DCHECK for text-transform: uppercase and ß. |
|||||||||
Issue descriptionChromium at 9aaa9887153703218ffdc5e89532c189fd609f52 with layoutng on (does not reproduce with layoutng off). Discovered at http://unifraktur.sourceforge.net/testcases/small-caps/ . The minimized test case is <span style="text-transform: uppercase">ß</span> which DCHECKS like [1:1:1029/150837.365747:FATAL:position.cc(137)] Check failed: static_cast<unsigned>(offset) <= ToCharacterData(anchor_node_)->length() (2 vs. 1)#text "\u00DF"
,
Oct 29
Note that this may be related to issue 852470 . Not sure if that needs to be fixed to fix this particular issue though.
,
Nov 10
,
Nov 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/abcd4da5aa44fea262bf8a450bcb14e4dd910541 commit abcd4da5aa44fea262bf8a450bcb14e4dd910541 Author: Emil A Eklund <eae@chromium.org> Date: Sat Nov 10 05:46:26 2018 [LayoutNG] Fix text-transform for 8-bit text Fix a bug in CaseMappingHarfBuzzBufferFiller where 8-bit strings weren't up-converted to 16-bit in cases were the original and case mapped string differed in length which hit a CHECK in the String::Characters16 method. Bug: 899876 , 899868 No-Try: true Change-Id: I5de8fbaabbc08932904846ffa4674129263f50aa Reviewed-on: https://chromium-review.googlesource.com/c/1330723 Commit-Queue: Emil A Eklund <eae@chromium.org> Reviewed-by: Christian Biesinger <cbiesinger@chromium.org> Reviewed-by: Stefan Zager <szager@chromium.org> Cr-Commit-Position: refs/heads/master@{#607106} [add] https://crrev.com/abcd4da5aa44fea262bf8a450bcb14e4dd910541/third_party/WebKit/LayoutTests/fast/text/8-bit-text-transform-crash-expected.txt [add] https://crrev.com/abcd4da5aa44fea262bf8a450bcb14e4dd910541/third_party/WebKit/LayoutTests/fast/text/8-bit-text-transform-crash.html [modify] https://crrev.com/abcd4da5aa44fea262bf8a450bcb14e4dd910541/third_party/blink/renderer/platform/fonts/shaping/case_mapping_harfbuzz_buffer_filler.cc
,
Nov 10
,
Nov 12
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8e40418fa31e14332698753e479e87c06369f64e commit 8e40418fa31e14332698753e479e87c06369f64e Author: Adam Rice <ricea@chromium.org> Date: Mon Nov 12 06:32:05 2018 Revert "[LayoutNG] Fix text-transform for 8-bit text" This reverts commit abcd4da5aa44fea262bf8a450bcb14e4dd910541. Reason for revert: Causing failures on Linux debug bot: https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20Tests%20%28dbg%29%281%29/75629 Original change's description: > [LayoutNG] Fix text-transform for 8-bit text > > Fix a bug in CaseMappingHarfBuzzBufferFiller where 8-bit strings weren't > up-converted to 16-bit in cases were the original and case mapped string > differed in length which hit a CHECK in the String::Characters16 method. > > Bug: 899876 , 899868 > No-Try: true > Change-Id: I5de8fbaabbc08932904846ffa4674129263f50aa > Reviewed-on: https://chromium-review.googlesource.com/c/1330723 > Commit-Queue: Emil A Eklund <eae@chromium.org> > Reviewed-by: Christian Biesinger <cbiesinger@chromium.org> > Reviewed-by: Stefan Zager <szager@chromium.org> > Cr-Commit-Position: refs/heads/master@{#607106} TBR=szager@chromium.org,cbiesinger@chromium.org,eae@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 899876 , 899868 Change-Id: I0dbcc4d3c64cfcc9f074b6255dca09885043eed7 Reviewed-on: https://chromium-review.googlesource.com/c/1331096 Reviewed-by: Adam Rice <ricea@chromium.org> Commit-Queue: Adam Rice <ricea@chromium.org> Cr-Commit-Position: refs/heads/master@{#607156} [delete] https://crrev.com/d934948431f2afd0e40130c91f9ca4cb37bb6941/third_party/WebKit/LayoutTests/fast/text/8-bit-text-transform-crash-expected.txt [delete] https://crrev.com/d934948431f2afd0e40130c91f9ca4cb37bb6941/third_party/WebKit/LayoutTests/fast/text/8-bit-text-transform-crash.html [modify] https://crrev.com/8e40418fa31e14332698753e479e87c06369f64e/third_party/blink/renderer/platform/fonts/shaping/case_mapping_harfbuzz_buffer_filler.cc
,
Nov 12
Unfortunately, layoutng is reporting that the minimal test case in the original report turns into 'S' when it should be 'SS'. See https://chromium-review.googlesource.com/c/chromium/src/+/1331138 .
,
Nov 12
Well, it is 'SS' in the non-layoutng case. It is unclear if the output here should be the pre or post text-transform. It appears layoutng is correct(?) here in producing the actual text of the node, but the existing code seems to produce the text after the transform? Seems we need separate expectations in that case.
,
Nov 12
No wait... in that case (getting the original text and not the transformed text) one should get 'ß'. So the layoutng case is incorrect. It should produce either 'SS' or 'ß' depending on if the pre or post text-transform content is expected.
,
Nov 28
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ec8ad11b2e541a3e41a847e762b5a289937a3919 commit ec8ad11b2e541a3e41a847e762b5a289937a3919 Author: Ben Wagner <bungeman@chromium.org> Date: Wed Nov 28 16:11:56 2018 Reland "[LayoutNG] Fix text-transform for 8-bit text" This is a reland of abcd4da5aa44fea262bf8a450bcb14e4dd910541 Original change's description: > [LayoutNG] Fix text-transform for 8-bit text > > Fix a bug in CaseMappingHarfBuzzBufferFiller where 8-bit strings weren't > up-converted to 16-bit in cases were the original and case mapped string > differed in length which hit a CHECK in the String::Characters16 method. > > Bug: 899876 , 899868 > No-Try: true > Change-Id: I5de8fbaabbc08932904846ffa4674129263f50aa > Reviewed-on: https://chromium-review.googlesource.com/c/1330723 > Commit-Queue: Emil A Eklund <eae@chromium.org> > Reviewed-by: Christian Biesinger <cbiesinger@chromium.org> > Reviewed-by: Stefan Zager <szager@chromium.org> > Cr-Commit-Position: refs/heads/master@{#607106} Bug: 899876 , 899868 Change-Id: I6b35a8335ab77fc700040a37f23278b67ec4eda5 Reviewed-on: https://chromium-review.googlesource.com/c/1331138 Commit-Queue: Ben Wagner <bungeman@chromium.org> Reviewed-by: Christian Biesinger <cbiesinger@chromium.org> Reviewed-by: Emil A Eklund <eae@chromium.org> Cr-Commit-Position: refs/heads/master@{#611716} [modify] https://crrev.com/ec8ad11b2e541a3e41a847e762b5a289937a3919/third_party/blink/renderer/platform/fonts/shaping/case_mapping_harfbuzz_buffer_filler.cc [add] https://crrev.com/ec8ad11b2e541a3e41a847e762b5a289937a3919/third_party/blink/web_tests/fast/text/8-bit-text-transform-crash-expected.html [add] https://crrev.com/ec8ad11b2e541a3e41a847e762b5a289937a3919/third_party/blink/web_tests/fast/text/8-bit-text-transform-crash.html
,
Nov 28
,
Nov 28
The small-caps variant is fixed, but this text-transform issue still exists. I'm not sure why fast/text/8-bit-text-transform-crash.html passed on linux_layout_tests_layout_ng , but it still dchecks in my local build of Chromium at 1a85e52504e9b381e0c the same way as in the original report.
,
Nov 28
Interesting, I'll take a look. Thanks!
,
Nov 29
Hits an assert in LayoutText::PositionForCaretOffset (and crashes in PositionTemplate<>::PositionTemplate without the assert). Xiaocheng, could you take a look please?
,
Dec 3
We currently don't have proper support for text-transform in NGOffsetMapping, causing out-of-range offsets. kojii@: As discussed before, we are not going to work around this issue before using ICU to handle text-transform directly in LayoutNG. Do we still stick to this plan, or are we going to introduce some workarounds to unblock LayoutNG shipping?
,
Dec 4
I think handling text-transform in NG is still the plan. Minor point but since this is a known deferred feature, I think it's better to fail more silently and gracefully if possible. By DCHECK failure, it's not easy to know if, for instance, we crash or show bunch of garbages on the next line when DCHECK is not enabled. What about converting DCHECK to TODO for known cases such as when 'text-transform' is applied?
,
Dec 4
We either need to fall back on legacy in these cases or add support for it in NG, crashing on real world content isn't really an option.
,
Dec 7
Sorry for the delay. I'm leaning towards working around it and turing DCHECKs into silent failures, and track these workarounds with issue 7500990.
,
Dec 13
Sounds like a good plan.
,
Jan 18
(4 days ago)
|
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by bunge...@chromium.org
, Oct 29