JDK-8147476 : Rendering issues with MathML token elements. #117
Conversation
| @Override public float[] getGlyphBoundingBox(int glyph) { | ||
| float[] bb = new float[4]; | ||
| bb = getFontStrike().getFontResource().getGlyphBoundingBox(glyph, font.getSize(), bb); | ||
| return new float[]{bb[0],-bb[3],bb[2],bb[3]-bb[1]}; |
There was a problem hiding this comment.
nit:
Leave a space after comma.
e.g.
new float[] { bb[0], -bb[3], bb[2], bb[3] - bb[1] };
| @@ -160,6 +160,12 @@ private FontStrike getFontStrike() | |||
| return getFontStrike().getFontResource().getAdvance(glyph, font.getSize()); | |||
| } | |||
|
|
|||
| @Override public float[] getGlyphBoundingBox(int glyph) { | |||
| float[] bb = new float[4]; | |||
There was a problem hiding this comment.
How often this method will be called from native? Is it too frequent? Too much of call to this function will leave a plenty of garbages. If it is too much, probably consider having static member similar like WCBufferedContext class.
There was a problem hiding this comment.
You are right, this method seems often used, I think that WebCore displays the mathematics words glyph by glyph due the nature of these words (frequently 1 or 2 characters) and over all because some of theses glyphs don't respect ascent and descent. But I made no test.
My purpose was simply to repair MathML rendering with a minimum of changes, not improve performances or memory usage. I think you are far more qualified than me to appreciate that. So I'm rely on you.
Do you means that, that all variables not directly used by others method of the class or not directly called could be declare private and static.
float[] bb in getGlyphBoundingBox(int glyph), for example ?
In this case, it should be interresting to test all methods ?
May be you already did that ?
Or I can not declare this variable and code like that :
return new float[]{
getFontStrike().getFontResource().getGlyphBoundingBox(glyph, font.getSize())[0],
-getFontStrike().getFontResource().getGlyphBoundingBox(glyph, font.getSize())[3],
getFontStrike().getFontResource().getGlyphBoundingBox(glyph, font.getSize())[2],
getFontStrike().getFontResource().getGlyphBoundingBox(glyph, font.getSize())[3]
- getFontStrike().getFontResource().getGlyphBoundingBox(glyph, font.getSize())[1]};
| return FloatRect(); //That is OK! platformWidthForGlyph impl is enough. | ||
| FloatRect Font::platformBoundsForGlyph(Glyph c) const | ||
| { | ||
| //return FloatRect(); //That is OK! platformWidthForGlyph impl is enough. |
|
|
||
| jfloatArray boundingBox = (jfloatArray)env->CallObjectMethod(*jFont, getGlyphBoundingBox_mID, (jint)c); | ||
|
|
||
| jfloat *bBox = env->GetFloatArrayElements(boundingBox,0); |
There was a problem hiding this comment.
You must release FloatArrayElements after reading it's contents using the following JNI function,
env->ReleaseFloatArrayElements(boundingBox, bBox, 0);
There was a problem hiding this comment.
Thanks for this advice.
There was a problem hiding this comment.
Remove env->ReleaseFloatArrayElements(boundingBox, bBox, 0);
Bad effects, I loose all BoundingBox values.
There was a problem hiding this comment.
You should add ReleaseFloatArrayElements after reading the array elements.
jfloat *bBox = env->GetFloatArrayElements(boundingBox,0);
auto bb = FloatRect { bBox[0], bBox[1], bBox[2], bBox[3] };
env->ReleaseFloatArrayElements(boundingBox, bBox, 0);
return bb;
|
|
||
| RefPtr<RQRef> jFont = m_platformData.nativeFontData(); | ||
| if (!jFont) { | ||
| return FloatRect(); |
There was a problem hiding this comment.
Use c++11 braced-init-list returns.
e.g.
if (!jFont) {
return { }
}
|
|
||
| import static test.util.Util.TIMEOUT; | ||
|
|
||
| public class MathMLRenderTest { |
There was a problem hiding this comment.
Consider writing a headless test under modules/javafx.web/src/test/java/test/javafx/scene/web/.
There was a problem hiding this comment.
You mean, I have to remove these lines ?
+ " <head>"
+ " <meta charset=\"UTF-8\">"
+ " <title>OpenJFX and MathML</title>"
+ " </head>"
There was a problem hiding this comment.
I really don't understand your request ?
This test works with all others tests when I use this command :
gradle -PFULL_TEST=true :systemTests:test
There was a problem hiding this comment.
Sorry, I'm really sorry,
I misread the message, but the place you suggest was the first place where I had put the test.
And Kevin said that I had to go into tests/system ...
I'm a bit confuse about what I have to do ?
There was a problem hiding this comment.
HeadLess Test
Under modules/javafx.web/src/test/java/test/javafx/scene/web/
- Round 1 : Ok it works with patched WebCore
- Round 2 : Ok it fails with no patched WebCore
Game Over ?
| package test.javafx.scene.web; | ||
|
|
||
| import org.junit.Assert; | ||
|
|
There was a problem hiding this comment.
nit:
Don't leave empty line between imports
| @@ -0,0 +1,87 @@ | |||
| /* | |||
| * Copyright (c) 2018, 2018, Oracle and/or its affiliates. All rights reserved. | |||
There was a problem hiding this comment.
Keep a single copy right year,
Copyright (c) 2018 Oracle and/or its affiliates. All rights reserved.
| ); | ||
|
|
||
| boolean rightRender = !(height < 2); | ||
| Assert.assertTrue("Check MathML token height : " + height + " is much smaller than the expected size." , rightRender); |
There was a problem hiding this comment.
Too cryptic, I guess the following snippet is pretty clear about the expectation.
assertTrue("MathML token height must be greater than " + height, height > 2);
|
|
||
| package test.javafx.scene.web; | ||
|
|
||
| import org.junit.Assert; |
There was a problem hiding this comment.
Do static import of necessary methods,
import static org.junit.Assert.assertTrue;
This is how we do it for other tests, keep a consistent style.
| public class MathMLRenderTest extends TestBase { | ||
|
|
||
| // Document test | ||
| static String BodyContent |
There was a problem hiding this comment.
Since these test markups(BodyContent, content) are specific to testTokenHeight, move it as a local variable inside testTokenHeight method.
Also change the variable names to meaningful one, e.g. BodyContent => quadraticFormula, content => htmlBody.
| @@ -160,6 +162,11 @@ private FontStrike getFontStrike() | |||
| return getFontStrike().getFontResource().getAdvance(glyph, font.getSize()); | |||
| } | |||
|
|
|||
| @Override public float[] getGlyphBoundingBox(int glyph) { | |||
| bb = getFontStrike().getFontResource().getGlyphBoundingBox(glyph, font.getSize(), bb); | |||
There was a problem hiding this comment.
As of now you can stick with your old approach, probably after profiling we can decide whether to go with static member approach. Sorry for the confusion.
| @@ -160,6 +162,11 @@ private FontStrike getFontStrike() | |||
| return getFontStrike().getFontResource().getAdvance(glyph, font.getSize()); | |||
| } | |||
|
|
|||
| @Override public float[] getGlyphBoundingBox(int glyph) { | |||
| bb = getFontStrike().getFontResource().getGlyphBoundingBox(glyph, font.getSize(), bb); | |||
| return new float[]{bb[0], -bb[3], bb[2], bb[3]-bb[1]}; | |||
There was a problem hiding this comment.
Still you need to give a space between operands and operators, bb[3] - bb[1]
| + " </body>" | ||
| + "</html>"; | ||
|
|
||
| @Test public void testgetTokenHeight() throws Exception { |
There was a problem hiding this comment.
Rename testgetTokenHeight => testTokenHeight
|
|
||
| env->ReleaseFloatArrayElements(boundingBox, bBox, 0); | ||
| CheckAndClearException(env); | ||
|
|
| jfloatArray boundingBox = (jfloatArray)env->CallObjectMethod(*jFont, getGlyphBoundingBox_mID, (jint)c); | ||
| jfloat *bBox = env->GetFloatArrayElements(boundingBox,0); | ||
| auto bb = FloatRect { bBox[0], bBox[1], bBox[2], bBox[3] }; | ||
|
|
| @@ -0,0 +1,85 @@ | |||
| /* | |||
There was a problem hiding this comment.
This test file's extension is missing. Did you run this?
It should be MathMLRenderTest.java right?
There was a problem hiding this comment.
Yes, the name is MathMLRenderTest.java
I ran the test but with my working copy and pasted the code directly online in the GitHub editor.
I forgot to add the extension when I enter the name.
Sorry, I have to change my way of working.
| loadContent(htmlBody); | ||
|
|
||
| int height = (int) executeScript( | ||
| "elements = document.getElementsByTagName('mo');" |
There was a problem hiding this comment.
So you are testing the height of the first mo tag which seems to be =.
Why can't use a very basic mathml markup which might be enough to test the problematic scenario? Something like below?
<body>
<math id='math'>
<mfrac>
<mn>1</mn>
<mn>2</mn>
</mfrac>
</math>
</body>
then,
final int EXPECTED_MINIUM_HEIGHT = 25;
int height = executeScript ("document.getElementById('html').clientHeight");
assertTrue("MathML token height is " + height, height > EXPECTED_MINIUM_HEIGHT);
There was a problem hiding this comment.
You could change but I will regret that :-( :-)
- I used a basic element because the problem occured for any the element, text token element include.
So the patch simply solves a character height bug, but it's difficult to see it only with a text element. In a text element the only sign was the cursor height. - Thus we don't need a formula to test it. I used this opportunity as an exercise and to show how to do to test for any others token elements.
- The quadratic formula is a reference to the first bug report in 2015 : JDK-8089878 .
- But, it's also because, if you test with complexe formula the height could be over 25 : when formula contains a fraction, the box size is not equal to 0, you must take in account the fraction line tickness. So if you've got fraction in a fraction ... the size grows up.
As I esplained in #71 the OpenJFX and WebCore are clean. I never had doubt about this.
One who wrote the code thought logically that the rendering of a simple MathML text like "Hello" is made exactly with the same logic used for same text in a html paragraph. Why it should be different ?
In fact WebCore and most of all others mathematic editor use a rendering way I hate. In their logic they can't used ascents and descents because of stretchy assembled glyphs like ( [ { ... : for example to construct a big { you need to assemble 3 characters (like bricks for a wall).
If you use the glyph ascent or descent , you'll see gap between characters, because it is the common ascent or descent for all glyph and not the ascent or descent of the glyph you're using.
I understood where was the problem when I read his comment "//That is OK! platformWidthForGlyph impl is enough." He tought that getAscent() and getDescent() was enough.
I aware of this problem because I'm building my own math editor. I prefer draw these stretchy glyphs by myself. I'm sure that is provided a better experience to users. The Swing API allows this but to use it, I'had been send several bug reports and build a patch. I sent the patch in 2012 and it has been accepted in 2017 when Sergey Bylokhov accepted to sponsor me.
You said that present WebKit version is 2.18. But I saw that some code are older. What is your project for the next WebKit version. There are many changes in MathML support, they improved RTL support and other things. Because of this PR work I haven't time to look further inside.
There was a problem hiding this comment.
Since it is a headless test, I prefer to have the test case as simple as possible. Basically the test case should able to catch the future regressions. Even though you render the quadratic formula, you end up checking only the height of '='. Then why can't just have a simple element with only '=' (or anything which you wish).
(This is my only concern right now, I request @kevinrushforth to take a look)
There was a problem hiding this comment.
All that work just for "=" :-D
As I said the quadric formula is a reference to the bug report Kevin treated in 2015 :-)
Of course we can.
Your are right a headless test doesn't need more.
I let you decide.
There was a problem hiding this comment.
According your request the test could be :
But I'm not at home and I can't test it before Friday.
/*
* Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation. Oracle designates this
* particular file as subject to the "Classpath" exception as provided
* by Oracle in the LICENSE file that accompanied this code.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/
package test.javafx.scene.web;
import static org.junit.Assert.assertTrue;
import org.junit.Test;
public class MathMLRenderTest extends TestBase {
@Test public void testTokenHeight() throws Exception {
// Document test
String htmlBody = "<!doctype html>"
+ "<html>"
+ " <body>"
+ " <p>"
+ " <math display=\"block\">"
+ " <mrow>"
+ " <mo>=</mo>"
+ " </mrow>"
+ " </math>";
+ " </p>"
+ " </body>"
+ "</html>";
loadContent(htmlBody);
int height = (int) executeScript(
"elements = document.getElementsByTagName('mo');"
+ "element = elements[0].clientHeight;"
);
assertTrue("MathML token height must be greater than " + height, height > 1);
}
}
|
Looks like this pull request is messed up with irrelevant file changes. Probably it might be due to rebasing the devlop instead of merging it. Could you please get rid of the irrelevant changes? |
|
JFXWCMathMLRenderingPatch201806241013' into WebCorePatch2018062102_work I don't understand, I don't remember having made a such request. |
scientificware
force-pushed
the
WebCorePatch2018062102_work
branch
from
9e5a033
to
241ad57
Compare
|
I cleaned my master and reverted WebCorePatch2018062102_work branch to its previous state just before my dirty commit. |
There was a problem hiding this comment.
How often this method will be called from native? Is it too frequent? Too much of call to this function will leave a plenty of garbages. If it is too much, probably consider having static member similar like WCBufferedContext class.
This have been already done ?
|
@scientificware For ease of review, to address Arun's concern, can you please squash your commits into a single commit? You should do this on your existing branch (meaning you will need to use 'git push --force' after you squash it), so that you don't need a new PR. If you are worried about losing information, you can create a new local branch to save all of your commits. Presuming you have an upstream remote pointing to javafxports/openjdk-jfx called 'upstream" the basic recipe is: and then change the 'pick' to 'squash' in your editor for all commits except the first one (leaving that first commit as 'pick') like this: |
|
Also, you have trailing white-space in two files. This will need to be eliminated. |
scientificware
force-pushed
the
WebCorePatch2018062102_work
branch
from
8814386
to
6e8f2e3
Compare
|
This PR is targeting several JBS issues. This means that either they are duplicates (or should be merged) or that this PR should be split to address each issue separately. These are:
|
|
Fails to build with : Update to 606.1 version of WebKit |
|
in ... |
Yes, good catch. I recommend using one of the bug IDs -- probably JDK-8147476 since that is the issue currently linked to this bug -- and closing the other two open issues, JDK-8090887 and JDK-8089878 as duplicates after ensuring that JDK-8089878 is fixed by this patch. (I note that JDK-8165520 is closed already). |
|
I knew JDK-8165520 report is closed but I added it in my list because it says that some tests are skipped until JDK-8165429 and JDK-8165527. In fact, JDK-8165520 mentions JDK-8165429, JDK-8165556 and JDK-8165527, I can't find these reports. Some of them have restricted access.
We should see, if some of them can be reactivated. |
The MathML patch is compatible with WebKit 606.1.I succeeded to build master and the branch with the patch (in fact a problem with ant)
|
|
@kevinrushforth Do you think this patch could be backported to previous java releases ? Becauseof this bug I don't think someone has been using this functionnality but having it without code changing could be benefit and appreciated. |
|
We have a general policy of backporting all change that touch native WebKit, so yes, this would be backported to FX 8u-dev after it is pushed to jfx-dev (for OpenJFX 11). |
Great ! Thanks ... |
|
I note that the branch referenced in this PR has not been rebased (or merged) with the latest WebKit changes from the WebKit 606.1 update. Can you please update the PR so the review can continue? |
|
Yes, but I did it in another branch (WebCorePatch2018063010_work) waiting your instructions. I wrote a comment 3 days ago, to inform you I had get a problem with the build. But after ant install, it works. |
|
Done. |
|
Thanks. The review can proceed now (likely next week). For bug tracking purposes I will close JDK-8090887 as a duplicate of JDK-8147476. As discussed with @ararunprasad yesterday, we will treat this as a bug fix, so it can go in after RDP1 of openjfx 11. Arun can comment on the DRT issue. As for the HTML issue, JDK-8089878, if that is still a bug, it can be left open, otherwise it can be closed as a duplicate. |
| @Test public void testTokenHeight() throws Exception { | ||
|
|
||
| // Document test | ||
| String mathmlContent |
There was a problem hiding this comment.
IIRC, we discussed to reduce the test mathml content. Is there any specific reason for going back to the bigger one?
There was a problem hiding this comment.
I'm not going back to the bigger one.
I posted here the shorter version you asked.
As I was not at home I couldn't test it.
But it was deleted, so I thought the discussion was closed.
| loadContent(htmlBody); | ||
|
|
||
| int height = (int) executeScript( | ||
| "elements = document.getElementsByTagName('mo');" |
There was a problem hiding this comment.
Could be simplified like executeScript("document.getElementsByTagName('mo')[0]. clientHeight");
| + "element = elements[0].clientHeight;" | ||
| ); | ||
|
|
||
| assertTrue("MathML token height must be greater than " + height, height > 1); |
There was a problem hiding this comment.
assertTrue("MathML token height is lesser than expected " + height, height > 1);
FontJava.cpp : - Completes the implementation of the platformBoundsForGlyph(Glyph c) function in FontJava.cpp. - Previous implementation simply returned a (0,0,0,0) rectangle, which explains the bad rendering height of MathML elements. - This affects only MathML elements due to specific mathematic glyphs behavior : WebCore MathML rendering can't use the font ascents and descents which causes gap into assembled glyphs. So getAscent(), getDescent() ... font methods are not appropriate. - WebCore uses bearY and height informations from the glyph bounding box. MathMLRenderTest.java : - Adds a HeadLess proper unit test for this Bug JDK-8147476. - Tests the height of the first MathML mo token element in a quadratic formula. WCFont.java : - Adds the abstract getGlyphBoundingBox() method. WCFontPerfLogger.java : - Adds the getGlyphBoundingBox() method. WCFontImpl.java : - Implements the getGlyphBoundingBox() method. - Uses and reorders (FontResource) getGlyphBoundingBox values to match perfectly with WebCore requirements. All files : - Changes copyright date and removes white space tabulation.
scientificware
force-pushed
the
WebCorePatch2018062102_work
branch
from
49fe98b
to
c98d116
Compare
|
Done, I rewrote the test as you requested and posted two runs :
|
|
lgtm |
|
Thanks @kevinrushforth. I will do it ASAP. |
|
@scientificware I would like to know your email address in order to add your name to "Contributed-by" filed while committing your contribution to jfx-dev and 8u-dev. Is it same as your jfx-dev mailing list email(guy.abossolo.foh@scientificware.com) ? |
|
@ararunprasad Thanks. |
|
This fix is now available in both jfx-dev and 8u-dev. @scientificware, Thanks for your efforts. |
|
Thanks Kevin and Arunprasad for the your help, time, suggestions, reviews, and advices. |
|
@kevinrushforth JDK-8089878 is still in Open Status. |
Yes, it is still Open. JDK-8089878 should probably be linked with issue #118. Are you still having issues with HTMLEditor rendering MathML? |
|
@kevinrushforth Yes I still having issue with HTMLEditor rendering MathML but (to be precise) none related with the problem described in the JDK-8089878, I'm afraid that linking it with #118 may produce confusions :
|
|
In that case I suggest filing a new bug in JBS to describe the bug you wish to fix with as much detail as possible. You can also create a new issue in GitHub if you like. Then once you have a solution, you can create a PR. |
|
Test with IOS 11.4
|
Hi !
This is tracked in JBS as
JDK-8147476 Rendering issues with MathML token elements.
JDK-8165520 Several mathml/ DRT tests fail
JDK-8089878 HTMLEditor messes up MathML markup.
JDK-8090887 Support MathML.
Issue #71 was submitted for this.
Please could you review this patch :
Bug JDK-8147476, commit of the following :
FontJava.cpp :
MathMLRenderTest.java :
WCFont.java :
WCFontPerfLogger.java :
WCFontImpl.java :
All files :
With last small changes (comments #71) the rendering is far better than with my previous patch. Now WebView is not so far from Safari and FireFox versus MathML display. The remaining problems are due to the missing OpenMathData table support.
A quick solution would be :
Best regards.