Bug 104631 – Emphasis mark is printed after inline-block with justify

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Yuki Sekiguchi]

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.

[Yuki Sekiguchi]

Created attachment 178730 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Yuki Sekiguchi]

Created attachment 178957 [details]
Update TestExpectations

[Dean Jackson]

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.

[Yuki Sekiguchi]

Created attachment 179397 [details]
Patch

[Yuki Sekiguchi]

(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.

[Yuki Sekiguchi]

Hi Dean.
Could you review this?

[WebKit Review Bot]

Comment on attachment 179397 [details]
Patch

Clearing flags on attachment: 179397

Committed r137786: <http://trac.webkit.org/changeset/137786>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yuki Sekiguchi]

Thank you for reviewing and committing!

[Roger Fong]

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

[Dean Jackson]

Roger,

I think it needs rebaseline.

[Roger Fong]

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...

[Dean Jackson]

(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.

[Yuki Sekiguchi]

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.