Closed Bug 534260 Opened 15 years ago Closed 15 years ago

Unnecessary scrollbars show on an RTL page with some float blocks, lang=fa/ar and ZWNJ's inside Arabic text

Categories

(Core :: Layout, defect, P2)

Product:
Core
Component:
Layout
Platform:
x86
macOS
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Tracking Flags:
Tracking Status
blocking2.0 --- beta1+

People

(Reporter: ehsan.akhgari, Assigned: jfkthame)

References

(
URL
)

Details

(Keywords: regression, rtl, testcase)

Attachments

(3 files)

Testcase
1.12 KB, text/html
Details
Screenshot
144.29 KB, image/png
Details
patch v1: check for bad glyph bbox returned by coregraphics
1.28 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review 
The following page shows both horizontal and vertical scrollbars, without any good reason:

https://fa.www-3.6.stage.mozilla.com/fa/firefox/3.6/whatsnew/

The only thing special about this page is that it includes a script which dynamically injects a CSS stylesheet into it.

I figured out that if I remove the following two lines from the page, the scrollbars no longer show:

    "#main-content .sub-feature { float: right; }" +
    "#personas.sub-feature { float: right; }" +

I can't figure out why these float styles have this effect.  The page shows up without scrollbars in 3.6, Safari and Chrome.
Assignee: nobody → ehsan.akhgari
Keywords: regressionwindow-wanted
Assignee: ehsan.akhgari → nobody
Attached file Testcase — 
The minimized testcase.

This is really interesting, becasue of the following:

1. If you change lang=fa on <html> to anything except fa and ar (or remove it), the problem disappears.
2. The horizontal scrollbar only shows when the h3 contains Persian or Arabic text including a ZWNJ.  It doesn't matter if the ZWNJ actually affects the letter joinings (in my testcase, it actually doesn't).
3. The more ZWNJ's you have, the longer the width of the scrollable area would be.  The number of ZWNJ's don't affect the size of the vertical scrollable area.
4. Removing the #connect div makes the vertical scrollbar go away.
Summary: Unnecessary scrollbars show on an RTL page with some float blocks → Unnecessary scrollbars show on an RTL page with some float blocks, lang=fa/ar and ZWNJ's inside Arabic text
Assignee: nobody → ehsan.akhgari
Keywords: rtl
(The Assign to Me jetpack is driving me crazy)
Assignee: ehsan.akhgari → nobody
Jonathan can probably figure this out.
See Also: → 534919
I've been unable to reproduce this locally on 10.6.2, using any of FF 3.5.6, 3.6b4, or my current trunk build.

Ehsan, if this is still an issue, could you clarify exactly what version(s) (of both the OS and browser) you're seeing it with, and perhaps post a screenshot as well?

> (The Assign to Me jetpack is driving me crazy)

So turn it off! :)
Attached image Screenshot — 
(In reply to comment #4)
> I've been unable to reproduce this locally on 10.6.2, using any of FF 3.5.6,
> 3.6b4, or my current trunk build.
> 
> Ehsan, if this is still an issue, could you clarify exactly what version(s) (of
> both the OS and browser) you're seeing it with, and perhaps post a screenshot
> as well?

Yesterday's nightly was cut off right before bug 534919 landed, so I can't test it today with a nightly, but I just tested it using my own build which includes the fix for bug 534919, and the problem still persists.  Here's a screenshot.

> > (The Assign to Me jetpack is driving me crazy)
> 
> So turn it off! :)

I wish it was that easy, but about:jetpack is broken on trunk!  I had to build my own version of Jetpack to be able to do so! :-)
BTW, the tip of my repository is 673dc988cfbd.  I'm on 10.6.2.
Aha - got it! Today I can reproduce it here too, with trunk builds.

What I'd forgotten yesterday was that my prefs were set to use the ATSUI backend, rather than the Core Text one (which is the default on trunk). If you go to about:config and set gfx.force_atsui_text to TRUE, I think that will eliminate the scrollbars for you.

So it's a problem with the Core Text backend. I'll look into it.
Setting gfx.force_atsui_text to true doesn't change anything for me here.
(In reply to comment #8)
> Setting gfx.force_atsui_text to true doesn't change anything for me here.

Did you restart the browser? Sorry, should have mentioned that it only takes effect on restart.
Oh, right, should have figured that out myself.  Yes, with ATSUI, the problem does not happen.
This seems to be caused by a bug in the Al Bayan font, which gets used as fallback for the ZWNJ when your testcase is using Geeza Pro (our default, which doesn't have ZWNJ as noted in bug 534919).

If you explicitly set the font family of the testcase to Baghdad, for example, the problem goes away; but if you explicitly set it to Al Bayan, it returns. And with Al Bayan, ZWJ will exhibit the same problem.

The root of the problem is that the ZWNJ glyph in Al Bayan has incorrect bounding box information in the TrueType file. From a ttx dump:

    <TTGlyph name="u200C.zwnj" xMin="32767" yMin="32767" xMax="-32767" yMax="-32767">
      <component glyphName=".null" x="0" y="0" flags="0x204"/>
    </TTGlyph>

The xMin/yMin/xMax/yMax values are obviously nonsensical, an artifact of how the glyph was constructed (with a reference to the empty glyph ".null" in the "u200C.zwnj" slot). The font-creating tool then initialized values to opposite extremes, intending to iterate over the components looking for min/max values; but the only component was blank, with no bbox of its own. (So the true root cause, arguably, is a bug in Fontographer, which looks like the tool used to create this font.)

Compare a similar blank glyph from Baghdad:

    <TTGlyph name="u200C.zwnj"/><!-- contains no outline data -->

Note that here, there is no bounding box and no outline data or reference; the glyph is truly empty (as it should be, and as the ".null" glyph in Al Bayan is).

I guess ATSUI must delete the "invisible" control chars from the final glyph array, which would explain why we don't see this problem with the ATSUI text backend. (I'll double-check that tomorrow.)

I'll file a Radar bug about the Al Bayan font, and begin to look for ways we can work around the issue. Given that the bbox rect in Al Bayan clearly represents (if anything) an entirely empty rectangle, with xMin>xMax and yMin>yMax, it would be good for Cairo to detect this anomaly when fetching extents, and return zeroes for the text extents. Apparently it doesn't do this at the moment.
(In reply to comment #11)
> I guess ATSUI must delete the "invisible" control chars from the final glyph
> array, which would explain why we don't see this problem with the ATSUI text
> backend. (I'll double-check that tomorrow.)

Just to correct this: it doesn't delete the zwnj glyphs. But we have an ATSUI-specific override gfxAtsuiFont::SetupGlyphExtents that uses ATSUI APIs instead of going through Cairo to get the glyph extents. Apparently this path ignores the bad glyph bbox in the font.

The Core Text backend relies on gfxFont::SetupGlyphExtents, which uses cairo_glyph_extents; this gets confused by the invalid bbox in the font.
Ok, this is a Core Graphics bug combined with a font problem. The bad glyph bbox data in the Al Bayan font is described above. When we ask Cairo for the extents of this glyph, it ends up calling the Core Graphics function CGFontGetGlyphBBoxesPtr() at http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-quartz-font.c#420. This returns the supposed bounding box of the glyph:

(gdb) p bbox
$13 = {
  origin = {
    x = -32767, 
    y = -32767
  }, 
  size = {
    width = 65534, 
    height = 65534
  }
}

It's easy to see how this is derived from the glyph data, but unfortunately the min/max values that indicated a nonexistent rect have apparently been "regularized" so that this now looks like a valid (though surprisingly large) box.

This means that by the time Cairo gets its hands on the box, it's too late to reliably recognize the broken situation. However, I think we could special-case this particular CG result, on the basis that it's extremely unlikely to arise as a correct bbox in a real-world font. An ugly hack, but it's hard to see how else to handle this short of actually pulling the raw glyph data from the font and checking it, which would be even more painful.
Filed rdar://7484029 about the Al Bayan glyph bbox errors.
Filed rdar://7484042 about the incorrect result from CGFontGetGlyphBoundingBoxes.
This patch for Cairo resolves the problem by checking for the crazy glyph bbox result from Core Graphics, and replacing it with an empty rect.

Flagging Jeff for review, as this is a hack within Cairo. It would be harder to detect reliably later on (at the Thebes level), because by that time the values have been scaled and become floating-point. And it's possible other Cairo clients could encounter related problems, so IMO this is a patch that would be worth considering upstream as well.
Assignee: nobody → jfkthame
Attachment #418351 - Flags: review?(jmuizelaar)
Requesting blocking-1.9.3: we should ensure this is fixed for FF 3.7, as it regresses important Persian pages such as "what's new in firefox". (It's not a problem for 1.9.2 / 3.6, as we're still using ATSUI there.)
blocking2.0: --- → ?
(In reply to comment #17)
> Requesting blocking-1.9.3: we should ensure this is fixed for FF 3.7, as it
> regresses important Persian pages such as "what's new in firefox". (It's not a
> problem for 1.9.2 / 3.6, as we're still using ATSUI there.)

I agree that this should block, because it affects *all* Persian (and possibly some Arabic) pages written with the default system font on Mac and 1.9.3.
Tryserver build with this patch is available at
http://build.mozilla.org/tryserver-builds/jkew@mozilla.com-try-f6805466464a
I can confirm that the problem is fixed with the tryserver build.
Johnathan,

Do you have an explanation as to why this doesn't affect other applications like Safari?
(In reply to comment #21)
> Do you have an explanation as to why this doesn't affect other applications
> like Safari?

Not a proven one, but I can speculate! Note that it would only affect applications that rely on glyph bounding boxes obtained via the CGFontGetGlyphBoundingBoxes API (which is what cairo uses), and then make layout decisions - such as whether to make the view scrollable - based on these results. We'd have to trawl through the webkit source to be sure, but presumably that's not how they go about measuring text extents.

We do know, for example, that ATSUI copes with the anomaly in the font, and so masks it for applications that use those APIs rather than going directly to the CGFont level (which is why we don't see the problem on 3.5 or 3.6, only on trunk where we've switched text backends). It seems likely that Core Text does something similar, and Safari is probably working via those APIs. But Cairo deals with the glyphs directly at the CG level, so we need to handle the problem there.
Comment on attachment 418351 [details] [diff] [review]
patch v1: check for bad glyph bbox returned by coregraphics

For the record, the only place that webkit uses CGFontGetGlyphBBoxes is in SimpleFontDataCGWin.cpp to get an xheight.
Attachment #418351 - Flags: review?(jmuizelaar) → review+
Pushed http://hg.mozilla.org/mozilla-central/rev/f86ce3afaba7
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Also added a patch file to the gfx/cairo directory, and noted in README there.

Jeff, do you think this should go upstream to Cairo as well? And if so, will you take care of that or do you want me to pursue it?
It should probably go upstream.

I want to sync with upstream cairo in the new year, but if you want to pursue upstreaming it I wouldn't complain either :)
Johnathan, can this patch land?
(In reply to comment #27)
> Johnathan, can this patch land?

It landed on trunk, see comment 24. Are you still seeing the problem with a current build?
Oh, I don't know why I thought this bug was not resolved.  :/

But I can confirm that the fix worked!  Marking verified.  Thanks!
Status: RESOLVED → VERIFIED
blocking2.0: ? → beta1
Priority: -- → P2
Issue is resolved - clearing old keywords - qa-wanted clean-up
Keywords: regressionwindow-wanted
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: