Closed Bug 1507661 Opened 7 years ago Closed 7 years ago

hyphens:auto renders two hyphens in words like e-commerce

Categories

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

Product:
Core
Component:
Layout: Text and Fonts
Version:
63 Branch
Platform:
Unspecified
All
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla67
Tracking Flags:
Tracking Status
firefox67 --- fixed

People

(Reporter: mnater, Assigned: jfkthame)

References

Details

Attachments

(3 files)

example-issue.html
846 bytes, text/html
Details
Reftest for spurious soft-hyphenation occuring at an explicit hyphen after initial letter of word
1.75 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
Don't treat an explicit hyphen as though it could be a soft-hyphenation position
2.64 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
Attached file example-issue.html —
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_1) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/12.0.1 Safari/605.1.15 Steps to reproduce: See attached test case (you may resize the window to see hyphenation): use hyphens:auto and make a word like e-commerce to break. Actual results: Firefox adds an additional hyphen if the break happens after the explicit hyphen. e-- commerce Expected results: No additional hyphen should be added. There are similar issues: https://bugzilla.mozilla.org/show_bug.cgi?id=1482371 and https://bugzilla.mozilla.org/show_bug.cgi?id=1478574 but they don't cover this case.
Component: Untriaged → Layout: Text and Fonts
Product: Firefox → Core
I can reproduce this on Nightly65.0a1 Windows10.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Unspecified → All
Huh, interesting. So the problem appears only if the first part of the compound word (before the "real" hyphen) is a single letter, as in "e-commerce"; when it is longer, as in "cost-effective" or "UV-Schutzbekleidung", no extra hyphen appears.
Priority: -- → P3
This is a minimized reftest based on the example here (which fails with current trunk code).
Attachment #9041862 - Flags: review?(jwatt)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
And this fixes the problem: we shouldn't be treating an explicit hyphen like a soft-hyphen break here.
Attachment #9041863 - Flags: review?(jwatt)
Comment on attachment 9041862 [details] [diff] [review] Reftest for spurious soft-hyphenation occuring at an explicit hyphen after initial letter of word Review of attachment 9041862 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/reftests/text/1507661-1-ref.html @@ +1,2 @@ > +<!DOCTYPE html> > +<div lang="en">e-mail-<br>ing</div> Maybe make this more explicit with an extra <br>, as in: e-<br>mail-<br>ing
Attachment #9041862 - Flags: review?(jwatt) → review+
Attachment #9041863 - Flags: review?(jwatt) → review+
Comment on attachment 9041862 [details] [diff] [review] Reftest for spurious soft-hyphenation occuring at an explicit hyphen after initial letter of word Review of attachment 9041862 [details] [diff] [review]: ----------------------------------------------------------------- Rather than have this under some generic and undiscoverable name like 1507661-1.html, it would be better to have this in a relevant directory.

(In reply to Jonathan Watt [:jwatt] from comment #6)

Comment on attachment 9041862 [details] [diff] [review]
Reftest for spurious soft-hyphenation occuring at an explicit hyphen after
initial letter of word

Review of attachment 9041862 [details] [diff] [review]:

::: layout/reftests/text/1507661-1-ref.html
@@ +1,2 @@

+<!DOCTYPE html>
+<div lang="en">e-mail-<br>ing</div>

Maybe make this more explicit with an extra <br>, as in: e-<br>mail-<br>ing

Actually, the expected result is that we don't break after the first hyphen (because the nsLineBreaker code doesn't consider a hyphen usable as a line-break if there's only one letter before or after it in the word). So we get

e-mail-
ing

The bug here is that although nsLineBreaker doesn't mark the hyphen as a line-break, the code in BreakAndMeasureText still goes ahead and uses it as if it were an optional hyphen position, so we currently end up with

e--
mail-
ing

(ugh).

Rather than have this under some generic and undiscoverable name like 1507661-1.html, it would be better to have this in a relevant directory.

True. Other hyphenation tests are also in layout/reftests/text/, so that seems the appropriate dir, but I can certainly make the name more useful!

(In reply to Jonathan Kew (:jfkthame) from comment #8)

Actually, the expected result is that we don't break after the first hyphen

Ah, that makes more sense. Thanks. :)

Pushed by jkew@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/235a41977032 Reftest for spurious soft-hyphenation occuring at an explicit hyphen after initial letter of word. r=jwatt https://hg.mozilla.org/integration/mozilla-inbound/rev/70b03f41e1bc Don't treat an explicit hyphen as though it could be a soft-hyphenation position. r=jwatt

https://hg.mozilla.org/mozilla-central/rev/235a41977032
https://hg.mozilla.org/mozilla-central/rev/70b03f41e1bc

Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox67: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

Thanks for your awesome work!

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: