drawing failure with stretchy horizontal parenthesis when no fonts are installed

RESOLVED FIXED in mozilla32

Status

()

Core
MathML
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: beta, Assigned: anujagarwal464)

Tracking

22 Branch
mozilla32
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 6 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 722159 [details]
Screenshot from 2013-03-07 11:48:28.png

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:22.0) Gecko/20130306 Firefox/22.0
Build ID: 20130306031012

Steps to reproduce:

Visit https://developer.mozilla.org/en-US/docs/Mozilla_MathML_Project/Fonts then the test page link from that, https://developer.mozilla.org/en-US/docs/Mozilla_MathML_Project/Fonts/Test


Actual results:

The page does not render, instead the previous page still appears.

If I visit the URL https://developer.mozilla.org/en-US/docs/Mozilla_MathML_Project/Fonts/Test directly from a new tab, nothing is rendered at all. If I tab into the test page from this bug report, I still see the bug report form…


Expected results:

The MathML test page should have appeared.

This issue affects Nightly (22) and Aurora (21).
(Reporter)

Comment 1

5 years ago
Installing the STIX fonts fixes the issue with the tab not rendering at all, but for users without it, the tab not rendering is the problem ‐ the lack of fonts for the test is obviously the point of the test :)

http://www.mozilla.org/projects/mathml/demo/texvsmml.html appears as it should, with it looking better with the font installed.
Yes, that's the point of the test to check font installation. If the fonts are not install, the grid will just not appear.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → INVALID
Reopening as I did not understand the issue.
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: INVALID → ---
Created attachment 722176 [details]
testcase

Here is the content of the test page.
Component: Untriaged → MathML
Product: Firefox → Core
(Reporter)

Comment 5

5 years ago
http://www.youtube.com/watch?v=WRUcVCL2wMg is the behaviour I see in Firefox 19, same behaviour as described above.
Summary: MDN MathML test page fails to render → Hang with stretchy horizontal parenthesis when no fonts are installed
Created attachment 722184 [details]
testcase

So here is a reduced testcase.

I suspect it has something to do with the scale transform fallback (without fonts, this horizontal parenthesis has always looked horrible and badly stretched for me). It would be good to check on a debug build if that triggers an exception, for example with box size computation.

I'm not able to reproduce the problem. Karl, do you see the issue?
Attachment #722176 - Attachment is obsolete: true
(Reporter)

Comment 7

5 years ago
Created attachment 722185 [details]
minimal testcase
John confirmed that the bug is due to the scale transform. He said that the scale factor is about 38 for the testcase and the issue disappears when the space has smaller sizes.
FT_Render_Glyph is failing at
http://git.savannah.gnu.org/cgit/freetype/freetype2.git/tree/src/smooth/ftgrays.c?id=9ea55c7c333e47345de2dff0e613e3e23dc8fdd3#n491
and cairo propagates this error through its drawing so that nothing draws.

Don't know exactly what is going wrong here, but I assume it has something to do with the size or distortion of the glyph.

Breakpoint 18, gray_record_cell (worker=0x2cee340)
    at /var/tmp/portage/media-libs/freetype-2.4.11/work/freetype-2.4.11/src/smooth/ftgrays.c:507
(gdb) p worker->num_cells
$46 = 435
(gdb) p worker->max_cells
$47 = 435

#12 0x00007f59e9411d78 in _render_glyph_outline (face=0x452de50, 
    font_options=0x44b81a0, surface=0x7fff161fbc58)
    at /home/karl/moz/dev/gfx/cairo/cairo/src/cairo-ft-font.c:1382
(gdb) p width
$49 = 458
(gdb) p height
$50 = 4
Summary: Hang with stretchy horizontal parenthesis when no fonts are installed → drawing failure with stretchy horizontal parenthesis when no fonts are installed
Depends on: 960115
(Assignee)

Comment 10

4 years ago
Created attachment 8419319 [details] [diff] [review]
bug848725.diff
Assignee: nobody → anujagarwal464
Attachment #8419319 - Flags: feedback?(fred.wang)
Comment on attachment 8419319 [details] [diff] [review]
bug848725.diff

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

I think we actually only want to limit the scale for the vertical and horizontal stretchy operators: http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLChar.cpp#1641. The case of large operators can be ignored, as the scale correction is likely to be small.

Could you make attachment 722184 [details] a crashtest (you can test vertical stretching too)? I think these are good candidate to force the scale to happen even with math fonts installed:

↠ 	↠ 	rightwards two headed arrow
↡ 	↡ 	downwards two headed arrow
Attachment #8419319 - Flags: feedback?(fred.wang) → feedback+
(Assignee)

Comment 12

4 years ago
Created attachment 8419588 [details] [diff] [review]
bug848725.diff
Attachment #8419319 - Attachment is obsolete: true
Attachment #8419588 - Flags: feedback?(fred.wang)
Comment on attachment 8419588 [details] [diff] [review]
bug848725.diff

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

Be sure to avoid whitespace change (remove space at the end of lines)

::: layout/mathml/crashtests/848725-2.html
@@ +9,5 @@
> +      <math> 
> +        <mover>
> +          <mo>&#x21A1;</mo>
> +          <mspace height="500px"></mspace>
> +        </mover>

If you want it to stretch vertically, replace the <mover> with an <mrow>

::: layout/mathml/nsMathMLChar.cpp
@@ +1503,5 @@
>        scale = (largeopFactor *
>                 (initialSize.ascent + initialSize.descent)) /
>          (aDesiredStretchSize.ascent + aDesiredStretchSize.descent);
> +      if(scale > kMaxScaleFactor) {
> +        scale = kMaxScaleFactor; 

This is still one for large operator.

The two desired one for vertical/horizontal stretchy op are located before the comments:

// make the character match the desired height.

// make the character match the desired width.
Attachment #8419588 - Flags: feedback?(fred.wang) → feedback+
(Assignee)

Comment 14

4 years ago
Created attachment 8419625 [details] [diff] [review]
bug848725.diff

@fredw: Thanks!
Attachment #8419588 - Attachment is obsolete: true
Attachment #8419625 - Flags: review?(fred.wang)
Comment on attachment 8419625 [details] [diff] [review]
bug848725.diff

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

::: layout/mathml/nsMathMLChar.cpp
@@ +32,5 @@
>  
>  //#define NOISY_SEARCH 1
>  
>  // -----------------------------------------------------------------------------
> +static const float kMaxScaleFactor = 20.0;

Could you add a comment that refers to this bug to explain why kMaxScaleFactor is necessary?

@@ +1448,5 @@
>          float(aContainerSize.ascent + aContainerSize.descent) /
>          (aDesiredStretchSize.ascent + aDesiredStretchSize.descent);
>        if (!largeop || scale > 1.0) {
>          // make the character match the desired height.
> +        if(scale > kMaxScaleFactor) {

missing space between after the "if".

I don't think that won't change much, but I would do that just after the scale is computed. You could then write

float scale =
  std::min(kMaxScaleFactor,
           ...)
(Assignee)

Comment 16

4 years ago
Created attachment 8419898 [details] [diff] [review]
bug848725.diff
Attachment #8419625 - Attachment is obsolete: true
Attachment #8419625 - Flags: review?(fred.wang)
Attachment #8419898 - Flags: feedback?(fred.wang)
Attachment #8419898 - Flags: feedback?(fred.wang) → feedback+
I wanted to send that to the Try server, but the patch does not seem to apply with the latest mozilla-central source.
Flags: needinfo?(anujagarwal464)
(Assignee)

Comment 18

4 years ago
Created attachment 8420489 [details] [diff] [review]
bug848725.diff
Attachment #8419898 - Attachment is obsolete: true
(Assignee)

Comment 19

4 years ago
Uploaded revised patch.
Flags: needinfo?(anujagarwal464)
The results look good now.
@Anuj: could you prepare the patch (unbirot if necessary, update the commit message) for checkin?
Flags: needinfo?(anujagarwal464)
(Assignee)

Comment 23

4 years ago
Created attachment 8420824 [details] [diff] [review]
bug848725.diff  r=fredw.

@fredw: Thanks!
Attachment #8420489 - Attachment is obsolete: true
Flags: needinfo?(anujagarwal464)
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/eb7008a07746
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.