Closed Bug 1288255 Opened 4 years ago Closed 4 years ago

Strange spacing on list item

Categories

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

x86_64
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla51
Tracking Status
firefox48 --- wontfix
firefox49 --- wontfix
firefox-esr45 --- wontfix
firefox50 --- wontfix
firefox51 --- verified

People

(Reporter: alex.fdm, Assigned: xidorn)

References

()

Details

(Keywords: regression)

Attachments

(3 files, 2 obsolete files)

Look at the last item (<li>) of this section ("Down and dirty programming"):
http://dlang.org/overview.html#features_to_keep

The bold part is rendered with a lot of spacing between words, and even hyphenation on the last word ("programming").  This happens only if the window is larger than the page width, if I reduce the width of the window, the text is rendered fine.

The page renders fine in any size in Chrome and IE.
Even stranger, this seems to be intermittent.  

I have now one tab open with the wrong rendering:
Down          and          dirty          program-
ming.

and one with the correct rendering:
Down and dirty programming.
Alexandre, which version of Firefox are you using?

I can't reproduce the problem using Firefox 48 on Linux.
Flags: needinfo?(alex.fdm)
I am using Firefox 48 on Windows 10, but the OS should not matter since I believe this is a Layout and not a Render issue.
I just checked again now and it is still happening, but as I said it is intermittent: again I have 2 tabs open on the same link, the first shows the issue and the second does not.
I also tested with Firefox 50 (dev edition), with the same result: one tab with the issue and one seems normal.  It seems to be consistent: the first tab I open always has the spacing problem, the second (and third) do not.
Flags: needinfo?(alex.fdm)
Sorry, it is Windows 7 on this machine, not Windows 10.  But again, I don't think it should matter.
QA: is this reproducible with FF48 on Win7?
Keywords: qawanted
I can reproduce this with Firefox 48 on Win10.
I can reproduce this with Nightly 51 as well. And it seems to me it happens only when the page is not loaded from cache. I can reproduce it at the first time I load the page, but after refresh, the issue disappears, and if I shift-reload, the issue would appear again.
The patch for bug 941629 is too trivial to be suspected... probably bug 941638.
Looks like a line breaking bug. Disabling "text-align: justify" rule you would still see the line breaks after "program-".
Component: Layout → Layout: Text
Looks like it is because the advance width returned from provider.GetHyphenWidth() is different with that from hyphenTextRun->MeasureText(), and thus the text frame after the bold text is wider than it is supposed to be, consequently we decide to force a line break at the last recorded break opportunity which is between "program" and "ming".
Given comment 11, it is pretty likely to be a regression from bug 941638 then.
Blocks: 941638
Keywords: qawantedregression
I guess it is a regression of bug 941638 because we started caching the hyphen width since that bug. And the bug here is probably because the hyphen width changes when we finish loading the web font.
Assignee: nobody → xidorn+moz
Not sure whether it is testable... We probably need a test font with predictable long hyphen.
Comment on attachment 8782221 [details]
Bug 1288255 - Clear hyphen width cache when user fonts update.

https://reviewboard.mozilla.org/r/72430/#review70172
Attachment #8782221 - Flags: review?(jfkthame) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/72dd4533b66d
Clear hyphen width cache when user fonts update. r=jfkthame
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/72dd4533b66d
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Would be nice to have a reftest for this if possible.
Flags: in-testsuite?
Xidorn, do you think we should uplift that to other branches? Thanks
Flags: needinfo?(xidorn+moz)
Comment on attachment 8782221 [details]
Bug 1288255 - Clear hyphen width cache when user fonts update.

Approval Request Comment
[Feature/regressing bug #]: bug 941638
[User impact if declined]: may see weird line breaking happens when use web fonts
[Describe test coverage new/current, TreeHerder]: no test
[Risks and why]: low risk, it just ensures that we clear a cached value for correctness
[String/UUID change made/needed]: n/a

I don't think this would affect too many people, so actually I'm not very keen to uplift it. But it has low risk, and would make the behavior more reliable, so we can try.
Flags: needinfo?(xidorn+moz)
Attachment #8782221 - Flags: approval-mozilla-beta?
Attachment #8782221 - Flags: approval-mozilla-aurora?
Hello Alexandre, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(alex.fdm)
Comment on attachment 8782221 [details]
Bug 1288255 - Clear hyphen width cache when user fonts update.

I agree with the dev suggestion, this is not a recent regression and does not have a widespread impact. I think we should let this ride the 51 train to release.
Attachment #8782221 - Flags: approval-mozilla-beta?
Attachment #8782221 - Flags: approval-mozilla-beta-
Attachment #8782221 - Flags: approval-mozilla-aurora?
Attachment #8782221 - Flags: approval-mozilla-aurora-
Hello Xidorn, ESR45 is also affected. What are your thoughts about uplifting it to in time for ESR45.4 or 45.5?
Flags: needinfo?(xidorn+moz)
I don't quite understand the criteria of uplifting to ESR... Isn't that only for critical security bugs?

Again, I think the patch has low risk, and also not critical, so uplifting or not both sounds fine for whatever version. I personally don't have any preference.
Flags: needinfo?(xidorn+moz)
Attached patch Add reftest for this bug (obsolete) — Splinter Review
Attachment #8783823 - Flags: review?(jfkthame)
This is the source file (FontForge format) of the LongHyphenTest.woff2 file used in the reftest. This font includes three glyphs:
* U+0020 (whitespace) and U+0058 (letter X) are from Ahem (so they are em square)
* U+2010 (hyphen) is a straight line with 5em width
Attached patch Add reftest for this bug (obsolete) — Splinter Review
Attachment #8783823 - Attachment is obsolete: true
Attachment #8783823 - Flags: review?(jfkthame)
Attachment #8783826 - Flags: review?(jfkthame)
Criteria for uplifting to ESR should generally be at least as strict as for uplifting to Beta.
Comment on attachment 8783826 [details] [diff] [review]
Add reftest for this bug

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

Nice trick of using a media query to defer the font loading. :) Is it guaranteed, though, that we will successfully load the font and reflow the document in time, before the reftest snapshot is taken? Or should we explicitly wait/check that the font has loaded before removing the reftest-wait class? I'm a little concerned this may be fragile as it stands, but not really sure how it all interacts... can you reassure me?
It seems to me the reftest would always ensure all pending paint blockers are resolved before it takes snapshot anyway, see [1].

What I'm not very confident is that whether this test is always able to catch the issue. I use onload to start loading font. I tried to force a reflow synchronously, but that doesn't work reliably (doesn't trigger this bug). And I actually have no idea why doing so in load event can trigger this bug all the time, and whether that can be guaranteed.


[1] https://dxr.mozilla.org/mozilla-central/rev/24763f58772d45279a935790f732d80851924b46/layout/tools/reftest/reftest-content.js#607-636
(In reply to Ritu Kothari (:ritu) from comment #22)
> Hello Alexandre, could you please verify this issue is fixed as expected on
> a latest Nightly build? Thanks!

Yes, it seems to be fixed on Nightly from Aug/22.  I can easily reproduce it on 48 and 50, but I can't on 51.
Flags: needinfo?(alex.fdm)
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #31)
> It seems to me the reftest would always ensure all pending paint blockers
> are resolved before it takes snapshot anyway, see [1].

But user-font loads don't block painting, I believe. So you need to explicitly check that the load has completed before removing reftest-wait (e.g. using the CSS Font Loading API).

Trying this test locally, I'm seeing intermittent failures (using a debug+opt build, so it won't be as fast as a pure opt build) where in some cases the font doesn't load fast enough; I suspect the intermittency may be significantly worse with debug builds on VMs.

(One thing you can do to make it more likely to fail, for testing purposes, is to prefix the test with HTTP in the reftest manifest, as that will tend to add some extra latency to the font load. Running a debug build with MOZ_LOG=userfonts:5 may also help, as that will add overhead to the font-loading process as it prints out log messages.)
(In reply to Alexandre Folle de Menezes from comment #32)
> (In reply to Ritu Kothari (:ritu) from comment #22)
> > Hello Alexandre, could you please verify this issue is fixed as expected on
> > a latest Nightly build? Thanks!
> 
> Yes, it seems to be fixed on Nightly from Aug/22.  I can easily reproduce it
> on 48 and 50, but I can't on 51.

Great! Thank you for a prompt reply.
Status: RESOLVED → VERIFIED
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #25)
> I don't quite understand the criteria of uplifting to ESR... Isn't that only
> for critical security bugs?
> 
> Again, I think the patch has low risk, and also not critical, so uplifting
> or not both sounds fine for whatever version. I personally don't have any
> preference.

Yes it is only sec-crits and sec-highs + severe regressions (which is rare due to low amount of code churn every ESR release). Based on the fact that this is not something that severely impacts many end-users, let's consider this a wontfix for esr45.
Attachment #8783826 - Attachment is obsolete: true
Attachment #8783826 - Flags: review?(jfkthame)
Attachment #8785240 - Flags: review?(jfkthame)
Attachment #8785240 - Flags: review?(jfkthame) → review+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9805030e34ef
followup - Add reftest for this bug. r=jfkthame
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.