Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Text fixes #156

Closed
wants to merge 4 commits into from
Closed

Text fixes #156

wants to merge 4 commits into from

Conversation

rFlex
Copy link

@rFlex rFlex commented Sep 6, 2017

  • Using bottom as default baseline. I also think the current calculation for the baseline might be a little off, will look a little more in detail when I have the time.
  • font-size attributes can end with "px", this change allows that.

@kayuri
Copy link
Collaborator

kayuri commented Sep 11, 2017

  • Changing baseline default value looks too risky at this point.
  • Allowing px looks good though. If you don't mind I will extract this into a separate PR and merge it?

@kayuri kayuri closed this Sep 11, 2017
@rFlex
Copy link
Author

rFlex commented Sep 11, 2017

Please do not close and do the changes yourself. In open source you should allow external contributors to get proper attribution on their work.

Do you have an SVG example where baseline top should be the default? In my test SVGs Macaw doesn't render the text at the right location at all compared to Chrome or even SVGKit. Changing the baseline fixes it.

@kayuri
Copy link
Collaborator

kayuri commented Sep 11, 2017

@rFlex my apologies, sure. Well, my understanding of the baseline change was it was too generic and might break some existing code. What do you think if we move it to the SVG parser? Would that work?

@kayuri kayuri reopened this Sep 11, 2017
@rFlex
Copy link
Author

rFlex commented Sep 20, 2017

@kayuri I agree that it might break existing code, not sure how to work around that because the problem is that the current behavior seems incorrect.

Example SVG:

<svg
    xmlns="http://www.w3.org/2000/svg"
    xmlns:xlink="http://www.w3.org/1999/xlink"
    xmlns:jfreesvg="http://www.jfree.org/jfreesvg/svg" viewBox="0 0 500 500" shape-rendering="auto">
    <rect x="0" y="0" width="500" height="500" style="fill: rgb(147,220,254); fill-opacity: 1.0" transform="matrix(1,0,0,1,0,0)"/>
    <text x="120" y="250" style="fill: rgb(255,255,255); fill-opacity: 1.0; font-family: Comic Sans MS; font-size: 40px;">This is a test</text>
    <rect x="120" y="253" width="250" height="2" style="fill: yellow; fill-opacity: 1.0" transform="matrix(1,0,0,1,0,0)"/>    
</svg>

How Chrome renders it:
screen shot 2017-09-20 at 10 12 05 am

How Macaw renders it:
screen shot 2017-09-20 at 10 11 51 am

@kayuri
Copy link
Collaborator

kayuri commented Sep 21, 2017

@rFlex I think we can probably just change the way our parser works, e.g. in SVGParser change parseSimpleText method and create Text with baseline:.bottom by default. This will work for your case. Would be better of course to properly parse baseline and make it work for all cases, but at least this would fix the default case. I can prepare this quick fix if you want.

@kayuri
Copy link
Collaborator

kayuri commented Sep 21, 2017

@rFlex I took a liberty of making a quick fix for text baseline. Works fine for SVG from your comment. Could you please update your PR?

@rFlex
Copy link
Author

rFlex commented Sep 21, 2017

got it @kayuri , updated!

@ystrot
Copy link
Member

ystrot commented Nov 27, 2017

Hi Simon,

Sorry for delay from Macaw Team. I'm going to apply all your patches in the nearest time. Hope you'll be available for little fixes if necessary. Regarding this issue, could you please create pull request with only this commit which related to this issue?

Other changes related to implementing CustomStringConvertible and Equatable protocols also seems useful, but they need to be consistent (not only Rect and Point, but other classes should be affected) and independent (there should be another pull request with only these changes).

f3dm76 added a commit to f3dm76/Macaw that referenced this pull request Apr 24, 2018
ystrot added a commit that referenced this pull request Apr 24, 2018
Parse units in font sizes (Tribute to #156 Text fixes)
@ystrot ystrot self-assigned this May 3, 2018
@ystrot ystrot added this to the 0.9.2 milestone May 3, 2018
@ystrot
Copy link
Member

ystrot commented May 3, 2018

These issues were fixed as part of the SVG parser improvements (see f3dm76@843cac6)

@ystrot ystrot closed this May 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants