Summary: | REGRESSION (r160259): text-combine glyphs are not rendered | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | hro.sug1no | ||||||||||||||
Component: | Text | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | commit-queue, darin, d-r, esprehn+autocc, fmalita, glenn, gyuyoung.kim, koivisto, kondapallykalyan, pdr, schenney, sergio, webkit-bug-importer, yuki.sekiguchi | ||||||||||||||
Priority: | P1 | Keywords: | InRadar, Regression | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | OS X 10.9 | ||||||||||||||||
URL: | http://trac.webkit.org/browser/trunk/LayoutTests/fast/text/emphasis-combined-text.html | ||||||||||||||||
Attachments: |
|
Description
hro.sug1no
2014-01-20 17:27:19 PST
Created attachment 223322 [details]
Patch
Hi Antti, Darin, Could you review this patch? This may be regression of r160259. It is written by Antti and reviewed by Darin. Comment on attachment 223322 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=223322&action=review > Source/WebCore/rendering/RenderCombineText.cpp:54 > +void RenderCombineText::setTextInternal(const String& text, const bool replace) No need for const in front of bool. > Source/WebCore/rendering/RenderCombineText.h:52 > + virtual void setTextInternal(const String&, const bool repace = false) override; Same here. Also, you have a typo "replace" not "repace" > Source/WebCore/rendering/RenderText.cpp:1027 > +void RenderText::setTextInternal(const String& text, const bool replace) And here. Created attachment 224153 [details]
Removed const and fixed typo
(In reply to comment #5) > (From update of attachment 223322 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=223322&action=review > > > Source/WebCore/rendering/RenderCombineText.cpp:54 > > +void RenderCombineText::setTextInternal(const String& text, const bool replace) > > No need for const in front of bool. Fixed. > > Source/WebCore/rendering/RenderCombineText.h:52 > > + virtual void setTextInternal(const String&, const bool repace = false) override; > > Same here. Also, you have a typo "replace" not "repace" Fixed. > > Source/WebCore/rendering/RenderText.cpp:1027 > > +void RenderText::setTextInternal(const String& text, const bool replace) > > And here. Fixed. Created attachment 224467 [details]
Patch
Comment on attachment 224467 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=224467&action=review I think at minimum this needs to have a good test case. > Source/WebCore/ChangeLog:16 > + No new tests. > + Image test of fast/text/international/text-combine-image-test.html covers this. It shouldn't be hard to add a proper test for this that doesn't require running in pixel mode. Reftest (that is test.html/test-expected.html pair) might be easiest. > Source/WebCore/rendering/svg/RenderSVGInlineText.h:55 > + virtual void setTextInternal(const String&, bool replace = false) override; It would be better to add an explicit API, RenderText::overrideOriginalTextForTextCombine or similar instead of adding a confusing new parameter. (Also I don't fully understand why RenderCombineText needs this special behavior.) Hi Koivisto, Thank you for reviewing. (In reply to comment #9) > (From update of attachment 224467 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=224467&action=review > > I think at minimum this needs to have a good test case. > > > Source/WebCore/ChangeLog:16 > > + No new tests. > > + Image test of fast/text/international/text-combine-image-test.html covers this. > > It shouldn't be hard to add a proper test for this that doesn't require running in pixel mode. Reftest (that is test.html/test-expected.html pair) might be easiest. I tried to create reftest using text-orientation: upright, transform: rotate(-90deg) or writing-mode: vertical, but all of them draw horizontal text to different position from text-combine: horizontal. Do you have any idea to create reftests? > > Source/WebCore/rendering/svg/RenderSVGInlineText.h:55 > > + virtual void setTextInternal(const String&, bool replace = false) override; > > It would be better to add an explicit API, RenderText::overrideOriginalTextForTextCombine or similar instead of adding a confusing new parameter. (Also I don't fully understand why RenderCombineText needs this special behavior.) r77153 says "If a new font found, we replace the text with 0xFFFC. This is needed for a combined text block to be able to behave like a single character against text decorations." However, I don't think this replacement is not needed in the current code. I'm creating removing replacement patch. This also fix odd line break in red border block in fast/text/international/text-combine-image-test.html. I'm investigating why this bug is fixed. After the investigation is finished, I'll upload a patch and ask review. If you think this method is wrong, please correct me. Created attachment 224586 [details]
Patch
(In reply to comment #10) > Hi Koivisto, > > Thank you for reviewing. > > (In reply to comment #9) > > (From update of attachment 224467 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=224467&action=review > > > Source/WebCore/rendering/svg/RenderSVGInlineText.h:55 > > > + virtual void setTextInternal(const String&, bool replace = false) override; > > > > It would be better to add an explicit API, RenderText::overrideOriginalTextForTextCombine or similar instead of adding a confusing new parameter. (Also I don't fully understand why RenderCombineText needs this special behavior.) > > r77153 says "If a new font found, we replace the text with 0xFFFC. This is needed for a combined text block to be able to behave like a single character against text decorations." > However, I don't think this replacement is not needed in the current code. > I'm creating removing replacement patch. This also fix odd line break in red border block in fast/text/international/text-combine-image-test.html. I'm investigating why this bug is fixed. > > After the investigation is finished, I'll upload a patch and ask review. If you think this method is wrong, please correct me. I noticed that line breaking needed this replacement. If we don't replace the original text, line breaking uses the original text to break lines, and it calculates wrong width for text-combine. Added overrideOriginalTextForTextCombine() and used it instead of setTextInternal(). Created attachment 231655 [details]
patch
Comment on attachment 231655 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=231655&action=review r=me > Source/WebCore/ChangeLog:8 > + The original text gets overwriten by change that is supposed to affect rendered text only. Typo, overwritten. > LayoutTests/ChangeLog:3 > + REGRESSION (r160259?): text-combine glyphs are not rendered Why the "?" |