Created attachment 178714 [details] Reproducing content There is illegal emphasis mark before "あ" or after empty inline-block in attached content. It should not be printed.
Created attachment 178730 [details] Patch
Comment on attachment 178730 [details] Patch Attachment 178730 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15278019 New failing tests: fast/inline/justify-emphasis-inline-box.html
Comment on attachment 178730 [details] Patch Attachment 178730 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15257580 New failing tests: fast/inline/justify-emphasis-inline-box.html
Created attachment 178957 [details] Update TestExpectations
Comment on attachment 178957 [details] Update TestExpectations View in context: https://bugs.webkit.org/attachment.cgi?id=178957&action=review Some grammar changes. This is my first review of text layout, so I'm quite nervous and I hope I don't break anything :( > Source/WebCore/ChangeLog:8 > + Fix emphasis mark is printed after inline-block with justify. -> "Do not print an emphasis mark after an inline-block when justified." > Source/WebCore/ChangeLog:14 > + If an inline-block is expanded, a space is added after inline-block. > + Emphasis mark is drawn over the space for the expand. > + > + When an emphasis mark is drawn, check the charactor is the space. > + If so, don't draw emphasis mark. -> "If an inline-block is expanded, it has a space appended to it. This space should not have any emphasis marks drawn." > Source/WebCore/platform/graphics/WidthIterator.cpp:228 > + glyphBuffer->add(0, fontData, m_expansionPerOpportunity); I wonder if we should have a special name for a null glyph, or a glyph we don't care about? > LayoutTests/ChangeLog:8 > + Test that draw emphasis mark to justified inline box and text. -> Test that emphasis marks are not drawn incorrectly in justified text with inline boxes.
Created attachment 179397 [details] Patch
(In reply to comment #5) Thank you for reviewing. > > Source/WebCore/ChangeLog:8 > > + Fix emphasis mark is printed after inline-block with justify. > > -> "Do not print an emphasis mark after an inline-block when justified." Fixed. > > Source/WebCore/ChangeLog:14 > > + If an inline-block is expanded, a space is added after inline-block. > > + Emphasis mark is drawn over the space for the expand. > > + > > + When an emphasis mark is drawn, check the charactor is the space. > > + If so, don't draw emphasis mark. > > -> "If an inline-block is expanded, it has a space appended to it. This space should not have any emphasis marks drawn." Fixed. > > Source/WebCore/platform/graphics/WidthIterator.cpp:228 > > + glyphBuffer->add(0, fontData, m_expansionPerOpportunity); > > I wonder if we should have a special name for a null glyph, or a glyph we don't care about? I think zero width space is better than a null glyph. Therefore, I change the fix to use zero width space glyph. > > LayoutTests/ChangeLog:8 > > + Test that draw emphasis mark to justified inline box and text. > > -> Test that emphasis marks are not drawn incorrectly in justified text with inline boxes. Fixed.
Hi Dean. Could you review this?
Comment on attachment 179397 [details] Patch Clearing flags on attachment: 179397 Committed r137786: <http://trac.webkit.org/changeset/137786>
All reviewed patches have been landed. Closing bug.
Thank you for reviewing and committing!
Hello, fast/inline/justify-emphasis-inline-box.html fails on Win7. Results here: http://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r137786%20(30937)/fast/inline/justify-emphasis-inline-box-pretty-diff.html Similar results on the EFL port. The results were rebaselined there. Are these results expected on these ports? Thx
Roger, I think it needs rebaseline.
Aight sure, just concerned that the difference in the diffs is 185 pixels and that there was no explanation about why that was the right thing to do on the EFL port...
(In reply to comment #14) > Aight sure, just concerned that the difference in the diffs is 185 pixels and that there was no explanation about why that was the right thing to do on the EFL port... I think the change looks bigger than it really is because there is enough extra space now to make sure the emphasis mark is on the next line.
Hmm... I watched test results on Mac, Windows and Linux. The justification is occurred on Mac, but it isn't occurred on Windows and Linux. This behavior is changed by Font::canExpandAroundIdeographsInComplexText() implementation. If your port returns true from canExpandAroundIdeographsInComplexText(), this bug is reproduced and the result width is 200px. If your port returns false, this bug is NOT reproduced and the result width is about 18px(depend on Japanese font). Safari for Windows should be about 18px, because win/FontWin.cpp returns false. I don't know which directory ELF port use. If it use harfbuzz/FontHarfBuzz.cpp, it returns false, so the width should be about 18px.