957128 - Soft hyphen not visible in container styled with word-wrap:break-word

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[filipnohe]

Attachment: Test case

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131205075310

Steps to reproduce:

A paragraph has the CSS property "word-wrap: break-word" set, or has the property "word-wrap: break-word" inherited from a parent element.

A word that is too long to fit the paragraph has a manual soft hyphen (­ or Unicode U+00AD) added.


Actual results:

The word is broken where the soft hyphen is, but the hyphen glyph is not displayed.


Expected results:

The word should be broken where the soft hyphen is, and the hyphen glyph should be displayed.

Comment 1

[sjw]

Workaround: use hyphens instead of word-wrap and ­
https://developer.mozilla.org/en-US/docs/Web/CSS/hyphens

Comment 2

[Igor Clukas]

Attachment: Workaround

Comment 3

[Igor Clukas]

A more SEO compatible workaround is to put the another ­ somewhere before the one where the word should be split. There has to be at least one “normal” character between the two ­s.

Comment 4

[Mats Palmgren (inactive)]

(In reply to bugzilla from comment #2)
> Created attachment 8357099 [details]
> Workaround

Interesting, thanks!

That workaround illuminates the bug quite clearly...
http://hg.mozilla.org/mozilla-central/annotate/e89afc241513/gfx/thebes/gfxFont.cpp#l6309
which is that the assignment to 'lastBreakUsedHyphenation' is not using 'hyphenation',
whereas '*aBreakPriority' is.  After setting "*aBreakPriority" to eNormalBreak, the
remaining iterations in the loop will set 'wordWrapping' to false (line 6294) because
*aBreakPriority <= eWordWrapBreak is false, so any &shy; after the first will set
'lastBreakUsedHyphenation' to true.

Comment 5

[Mats Palmgren (inactive)]

Attachment: fix+reftests

Comment 6

[Mats Palmgren (inactive)]

Attachment: optional part 2

This is an idempotent change to make the code slightly clearer.
I think it should be at least as fast as the previous version but
I haven't benchmarked it.  I'll be happy to do a microbenchmark
on request.

https://tbpl.mozilla.org/?tree=Try&rev=b4bf4585dd61

Comment 7

[Simon Montagu :smontagu]

Comment on attachment 8358854 [details] [diff] [review]
optional part 2

Review of attachment 8358854 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, this improves clarity a great deal. I think that this code is sufficiently on a performance-critical path that you need to do the micro-benchmark for due diligence, though.

Comment 9

[Mats Palmgren (inactive)]

I micro-benchmarked the if-block that I changed:
https://hg.mozilla.org/integration/mozilla-inbound/file/0c8fa25eafe9/gfx/thebes/gfxFont.cpp#l6394
and that block became ~5% faster with the change, in Linux64 and OSX Opt builds.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ef6bc080fa3
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c8fa25eafe9

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3ef6bc080fa3
https://hg.mozilla.org/mozilla-central/rev/0c8fa25eafe9