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

Canvas text #120

Merged
merged 6 commits into from Feb 6, 2015

Conversation

Projects
None yet
3 participants
@jazmit
Contributor

jazmit commented Jan 19, 2015

For discussion resolves #118

@jazmit

This comment has been minimized.

Show comment
Hide comment
@jazmit

jazmit Jan 19, 2015

Contributor

OK, I found a few minutes to write a function to parse the output of the Text module - not so hard as I thought. This way we can have discussion of any refactoring of Text separately from the Canvas Text issue. I think this is ready to be merged now, but am happy to do any changes that people think necessary. Will also do an example for the tryit editor later this week.

Contributor

jazmit commented Jan 19, 2015

OK, I found a few minutes to write a function to parse the output of the Text module - not so hard as I thought. This way we can have discussion of any refactoring of Text separately from the Canvas Text issue. I think this is ready to be merged now, but am happy to do any changes that people think necessary. Will also do an example for the tryit editor later this week.

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Jan 20, 2015

Member

How come text produces a Shape instead of a Form? Is outlined (dashed red) (text ...) meaningful?

I sort of think of Shape as an abstract thing just about geometry and that one day we can do things like diff shape1 shape2 and intersect shape1 shape2 etc.

Member

evancz commented Jan 20, 2015

How come text produces a Shape instead of a Form? Is outlined (dashed red) (text ...) meaningful?

I sort of think of Shape as an abstract thing just about geometry and that one day we can do things like diff shape1 shape2 and intersect shape1 shape2 etc.

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Jan 20, 2015

Member

Also, what happens in the case of text (bold txt1 ++ italic txt2)?

Member

evancz commented Jan 20, 2015

Also, what happens in the case of text (bold txt1 ++ italic txt2)?

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Jan 20, 2015

Member

Would it work to have the public API extended with

text : Text -> Form

And under the hood, this could use the color and font information from whatever Text object is passed in. I think this means that you can't specify different fill color and outline color for text, but is that use case important enough that we want to figure out how to do dashed outlines?

Member

evancz commented Jan 20, 2015

Would it work to have the public API extended with

text : Text -> Form

And under the hood, this could use the color and font information from whatever Text object is passed in. I think this means that you can't specify different fill color and outline color for text, but is that use case important enough that we want to figure out how to do dashed outlines?

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Jan 20, 2015

Member

Also, is it going to draw such that the position of the text is determined by the middle or the top left? Is there some way that top left is consistent with the pattern of the rest of the library?

Member

evancz commented Jan 20, 2015

Also, is it going to draw such that the position of the text is determined by the middle or the top left? Is there some way that top left is consistent with the pattern of the rest of the library?

@jazmit

This comment has been minimized.

Show comment
Hide comment
@jazmit

jazmit Jan 20, 2015

Contributor

Some really insightful & thought-provoking questions, thank you Evan..

How come text produces a Shape instead of a Form?
Would it work to have the public API extended with text : Text -> Form?

The idea is to reuse the existing library functions for styling Shapes rather than duplicating them for text objects. For example, this:

main = collage 400 400
    [ move (-200, 0)  (outlined (solid black) (text (bold         (height 40 (fromString "I am outlined black"))))) 
    , move (-200, 40) (filled   blue          (text (italic       (height 20 (fromString "I am a solid italic"))) ))
    , move (-200, 70) (gradient grad          (text (bold (italic (height 50 (fromString "I am a gradient")))) ))
    ]

Makes this screenshot:

image

I would prefer this to having gradientText, outlinedText, filledText, etc functions.
Should we disallow outlined and gradient text altogether? I think it would be nice if we could make more or less all of the canvas API available through Elm - otherwise people may start using Elm for a project only to find out.. "Oh, you can't do that with Elm" and have to switch back to something else. If you are wanting to draw text on canvas, being able to do gradients I would think would be a fairly common requirement, as would doing bordered text for titles, which requires outlined. And because we're reusing existing functions to provide this functionality, we're not adding to the conceptual load of the library.
Just my 2c, I'm happy to go with whatever you think is best.

As far as providing intersect etc, it's still possible to do this for the Polygon type, and is certainly conceptually possible across text and other future Shape subtypes (Shapes defined with arc segments may be necessary for performance one day. Fractals anyone?). It may even be possible to implement intersect across Shape types using the canvas clip or some future font data API. Also Polygon is possibly a more appropriate name for shapes defined by straight line segments than Shape.

The dashed function isn't working at the moment - I looked into the code and see that it's because the dashes are generated manually in js rather than using ctx.setLineDash, is there a reason for this? If not, deleting this code and using setLineDash instead will enable dashed with text, reduce the code footprint and probably result in a performance boost too...

Also, what happens in the case of text (bold txt1 ++ italic txt2)?

text (append (bold txt) (italic txt2)) is broken.. the implementation of append concatentates all the css together. We probably need some refactoring in the Text module to make this work. Are you happy for me to make some changes or should it just throw a helpful error message for now?

Also, is it going to draw such that the position of the text is determined by the middle or the top left? Is there some way that top left is consistent with the pattern of the rest of the library?

As the user can pass the text height, I don't think vertical centering is too controversial, I'll fix this shortly. For horizontal centering, we have lots of choices:

  1. (As present) display text with the left starting at the (x, y) position, this is not very consistent with the rest of the library.
  2. Center the text (using the canvas measureText function). However, as the width of the text isn't available to the user, it would be impossible to get the left-hand edge of the text to line up with anything.
  3. Provide text alignment. With left-alignment, the text flows right from the start (x, y) as per (1). With center, it is centered as per (2), and with right-alignment it is all to the left of the given (x, y) position. Default alignment could be center.
  4. Force the user to pass a maximum width with the text, and center on that width, wrapping if the text is longer (this could also be a separate function, eg: fixedWidthText, so we could have this simultaneously with one of the above).

We should also think about languages with other text directions...

Opinions all?

Contributor

jazmit commented Jan 20, 2015

Some really insightful & thought-provoking questions, thank you Evan..

How come text produces a Shape instead of a Form?
Would it work to have the public API extended with text : Text -> Form?

The idea is to reuse the existing library functions for styling Shapes rather than duplicating them for text objects. For example, this:

main = collage 400 400
    [ move (-200, 0)  (outlined (solid black) (text (bold         (height 40 (fromString "I am outlined black"))))) 
    , move (-200, 40) (filled   blue          (text (italic       (height 20 (fromString "I am a solid italic"))) ))
    , move (-200, 70) (gradient grad          (text (bold (italic (height 50 (fromString "I am a gradient")))) ))
    ]

Makes this screenshot:

image

I would prefer this to having gradientText, outlinedText, filledText, etc functions.
Should we disallow outlined and gradient text altogether? I think it would be nice if we could make more or less all of the canvas API available through Elm - otherwise people may start using Elm for a project only to find out.. "Oh, you can't do that with Elm" and have to switch back to something else. If you are wanting to draw text on canvas, being able to do gradients I would think would be a fairly common requirement, as would doing bordered text for titles, which requires outlined. And because we're reusing existing functions to provide this functionality, we're not adding to the conceptual load of the library.
Just my 2c, I'm happy to go with whatever you think is best.

As far as providing intersect etc, it's still possible to do this for the Polygon type, and is certainly conceptually possible across text and other future Shape subtypes (Shapes defined with arc segments may be necessary for performance one day. Fractals anyone?). It may even be possible to implement intersect across Shape types using the canvas clip or some future font data API. Also Polygon is possibly a more appropriate name for shapes defined by straight line segments than Shape.

The dashed function isn't working at the moment - I looked into the code and see that it's because the dashes are generated manually in js rather than using ctx.setLineDash, is there a reason for this? If not, deleting this code and using setLineDash instead will enable dashed with text, reduce the code footprint and probably result in a performance boost too...

Also, what happens in the case of text (bold txt1 ++ italic txt2)?

text (append (bold txt) (italic txt2)) is broken.. the implementation of append concatentates all the css together. We probably need some refactoring in the Text module to make this work. Are you happy for me to make some changes or should it just throw a helpful error message for now?

Also, is it going to draw such that the position of the text is determined by the middle or the top left? Is there some way that top left is consistent with the pattern of the rest of the library?

As the user can pass the text height, I don't think vertical centering is too controversial, I'll fix this shortly. For horizontal centering, we have lots of choices:

  1. (As present) display text with the left starting at the (x, y) position, this is not very consistent with the rest of the library.
  2. Center the text (using the canvas measureText function). However, as the width of the text isn't available to the user, it would be impossible to get the left-hand edge of the text to line up with anything.
  3. Provide text alignment. With left-alignment, the text flows right from the start (x, y) as per (1). With center, it is centered as per (2), and with right-alignment it is all to the left of the given (x, y) position. Default alignment could be center.
  4. Force the user to pass a maximum width with the text, and center on that width, wrapping if the text is longer (this could also be a separate function, eg: fixedWidthText, so we could have this simultaneously with one of the above).

We should also think about languages with other text directions...

Opinions all?

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Jan 21, 2015

Member

The dashed function isn't working at the moment - I looked into the code and see that it's because the dashes are generated manually in js rather than using ctx.setLineDash, is there a reason for this?

I don't think it existed when I was writing this library. And looking at the docs, it only shows up in IE11, so it does not really work for our user window which is IE9 and up.

Are you happy for me to make some changes or should it just throw a helpful error message for now?

Runtime errors are not acceptable. One of my goals with Elm is to give the guarantee that "you won't get runtime errors" so we are moving to make all standard library functions total and to provide errors on incomplete pattern matches. I think changing Text is much better, but that should be a separate PR.

I think the most viable path forward is to add the equivalent of this for collage such that this stuff can be implemented elsewhere. As I understand things, this API has problems as is. My personal opinion is that these are API design issues if features are not working and runtime errors are being thrown. I think they can be worked out, but I'd like to see that happening in another package so that things do not need to block on me, wait for a core release, others can try to make alternatives, etc.

Member

evancz commented Jan 21, 2015

The dashed function isn't working at the moment - I looked into the code and see that it's because the dashes are generated manually in js rather than using ctx.setLineDash, is there a reason for this?

I don't think it existed when I was writing this library. And looking at the docs, it only shows up in IE11, so it does not really work for our user window which is IE9 and up.

Are you happy for me to make some changes or should it just throw a helpful error message for now?

Runtime errors are not acceptable. One of my goals with Elm is to give the guarantee that "you won't get runtime errors" so we are moving to make all standard library functions total and to provide errors on incomplete pattern matches. I think changing Text is much better, but that should be a separate PR.

I think the most viable path forward is to add the equivalent of this for collage such that this stuff can be implemented elsewhere. As I understand things, this API has problems as is. My personal opinion is that these are API design issues if features are not working and runtime errors are being thrown. I think they can be worked out, but I'd like to see that happening in another package so that things do not need to block on me, wait for a core release, others can try to make alternatives, etc.

@jazmit

This comment has been minimized.

Show comment
Hide comment
@jazmit

jazmit Jan 21, 2015

Contributor

Agreed.
I don't think the dashed thing is a big issue, we use the custom function for when !(ctx.setLineDash) and use setLineDash otherwise. Graceful degradation - dashed outlined text will appear solid outlined in IE 9 + 10 (already rare now).
Splitting this out into a separate library looks tricky to me, Evan, can you be more specific about how you think this should be done?
I think we probably need to have a discussion about what the API should be and how it will interact with the Text API. We need to decide out how to do deal with multiple spans of differently-styled text, how to set alignment and whether we use maxWidth. There are multiple ways of doing this without throwing runtime errors. Is this the appropriate place for that?

Contributor

jazmit commented Jan 21, 2015

Agreed.
I don't think the dashed thing is a big issue, we use the custom function for when !(ctx.setLineDash) and use setLineDash otherwise. Graceful degradation - dashed outlined text will appear solid outlined in IE 9 + 10 (already rare now).
Splitting this out into a separate library looks tricky to me, Evan, can you be more specific about how you think this should be done?
I think we probably need to have a discussion about what the API should be and how it will interact with the Text API. We need to decide out how to do deal with multiple spans of differently-styled text, how to set alignment and whether we use maxWidth. There are multiple ways of doing this without throwing runtime errors. Is this the appropriate place for that?

@jazmit

This comment has been minimized.

Show comment
Hide comment
@jazmit

jazmit Jan 21, 2015

Contributor

Some updates to address Evan's concerns:

  • Text.append is supported without refactoring of Text library
  • Text is centered vertically and horizontally in keeping with the spirit of the library
  • Dashed text outlines is supported, except for IE 9 + 10 which show solid outlines
  • No runtime errors thrown
main : Element
main = collage 400 400
    -- Demonstrate centering consistent with rest of library
    [ filled grey (rect 235 30)

    -- Demonstrate multiple spans with different styling
    , filled black (text (
               height 30 (bold (fromString "BOLD"))
      `append` height 20 (italic (fromString " ITALIC"))
      `append` height 25 (fromString " normal")
      )) 

    -- Demonstrate dashed text outlines
    , move (0, 80) (outlined ({defaultLine | width <- 4, dashing <- [3,3]})
                   (text (height 100 (fromString "λ"))))
    ]

image

Any other requests? Now we've addressed these concerns, can we merge this or would you prefer it split out?

Contributor

jazmit commented Jan 21, 2015

Some updates to address Evan's concerns:

  • Text.append is supported without refactoring of Text library
  • Text is centered vertically and horizontally in keeping with the spirit of the library
  • Dashed text outlines is supported, except for IE 9 + 10 which show solid outlines
  • No runtime errors thrown
main : Element
main = collage 400 400
    -- Demonstrate centering consistent with rest of library
    [ filled grey (rect 235 30)

    -- Demonstrate multiple spans with different styling
    , filled black (text (
               height 30 (bold (fromString "BOLD"))
      `append` height 20 (italic (fromString " ITALIC"))
      `append` height 25 (fromString " normal")
      )) 

    -- Demonstrate dashed text outlines
    , move (0, 80) (outlined ({defaultLine | width <- 4, dashing <- [3,3]})
                   (text (height 100 (fromString "λ"))))
    ]

image

Any other requests? Now we've addressed these concerns, can we merge this or would you prefer it split out?

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Jan 23, 2015

Member

Holy crap, this is looking awesome!

I apologize for the delay. I have two remaining concerns:

Point 1 - I don't think the current API makes sense. I get that it'd be cool to reuse filled and outlined but I just do not think that text is a Shape. One alternative may be to have these:

text : Text -> Form  -- where colors are all determined by the Text itself
outlinedText : LineStyle -> Text -> Form -- where just the outline is drawn

From the perspective of evolving an API, I'd much rather have special functions that can be deprecated. It's not clear that this is possible with the API you suggest.

Point 2 - The way Text.append is handled is hack city. This does not block the PR necessarily, but it seems clear that the better way is to make changes to the Text module to avoid this indirection through the DOM. I don't know how slow the current way is, but that's another reason for concern.

I'm sort of okay with doing the refactor for the 2nd point as a separate PR, but the 1st one is a blocker to me.

As for how we'll make it easy to use this before the next release of core, this thread is about that.

Member

evancz commented Jan 23, 2015

Holy crap, this is looking awesome!

I apologize for the delay. I have two remaining concerns:

Point 1 - I don't think the current API makes sense. I get that it'd be cool to reuse filled and outlined but I just do not think that text is a Shape. One alternative may be to have these:

text : Text -> Form  -- where colors are all determined by the Text itself
outlinedText : LineStyle -> Text -> Form -- where just the outline is drawn

From the perspective of evolving an API, I'd much rather have special functions that can be deprecated. It's not clear that this is possible with the API you suggest.

Point 2 - The way Text.append is handled is hack city. This does not block the PR necessarily, but it seems clear that the better way is to make changes to the Text module to avoid this indirection through the DOM. I don't know how slow the current way is, but that's another reason for concern.

I'm sort of okay with doing the refactor for the 2nd point as a separate PR, but the 1st one is a blocker to me.

As for how we'll make it easy to use this before the next release of core, this thread is about that.

@jazmit

This comment has been minimized.

Show comment
Hide comment
@jazmit

jazmit Jan 25, 2015

Contributor

Thanks!

I'll hopefully have a chance to get this sorted next week. I'll leave gradient text out for now, and we can decide later whether to have a gradientText function or add something to the Text API to support this.

Contributor

jazmit commented Jan 25, 2015

Thanks!

I'll hopefully have a chance to get this sorted next week. I'll leave gradient text out for now, and we can decide later whether to have a gradientText function or add something to the Text API to support this.

@jazmit

This comment has been minimized.

Show comment
Hide comment
@jazmit

jazmit Jan 30, 2015

Contributor

I've updated this to reflect the new API suggested by Evan. An added bonus is that color of filled text is preserved when using append Example:

main = collage 400 400
    -- Demonstrate centering consistent with rest of library
    [ filled darkGrey (rect 235 30)

    -- Demonstrate multiple spans with different styling
    , (text (
               height 30 (bold (fromString "BOLD"))
      `append` height 20 (color white (italic (fromString " ITALIC")))
      `append` height 25 (color blue (fromString " normal"))
      )) 

    -- Demonstrate dashed text outlines
    , move (0, 80) (outlinedText ({defaultLine | width <- 4, dashing <- [3,3]})
                    (height 100 (fromString "λ")))
    ]

image

Contributor

jazmit commented Jan 30, 2015

I've updated this to reflect the new API suggested by Evan. An added bonus is that color of filled text is preserved when using append Example:

main = collage 400 400
    -- Demonstrate centering consistent with rest of library
    [ filled darkGrey (rect 235 30)

    -- Demonstrate multiple spans with different styling
    , (text (
               height 30 (bold (fromString "BOLD"))
      `append` height 20 (color white (italic (fromString " ITALIC")))
      `append` height 25 (color blue (fromString " normal"))
      )) 

    -- Demonstrate dashed text outlines
    , move (0, 80) (outlinedText ({defaultLine | width <- 4, dashing <- [3,3]})
                    (height 100 (fromString "λ")))
    ]

image

@rehno-lindeque

This comment has been minimized.

Show comment
Hide comment
@rehno-lindeque

rehno-lindeque Feb 5, 2015

Contributor

This is looking awesome! Does it look ready to merge?

Contributor

rehno-lindeque commented Feb 5, 2015

This is looking awesome! Does it look ready to merge?

@jazmit

This comment has been minimized.

Show comment
Hide comment
@jazmit

jazmit Feb 5, 2015

Contributor

I'm happy either for it to be merged or to do some more work on it if anyone has requests. I'll follow up with a refactoring PR for the text module later.

Contributor

jazmit commented Feb 5, 2015

I'm happy either for it to be merged or to do some more work on it if anyone has requests. I'll follow up with a refactoring PR for the text module later.

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Feb 6, 2015

Member

This is looking great! :D Awesome work!

👍 to doing the Text refactor as another PR. I want to do this review tonight, but if I don't get to it, please remind me on Monday!

Member

evancz commented Feb 6, 2015

This is looking great! :D Awesome work!

👍 to doing the Text refactor as another PR. I want to do this review tonight, but if I don't get to it, please remind me on Monday!

@jazmit

This comment has been minimized.

Show comment
Hide comment
@jazmit

jazmit Feb 6, 2015

Contributor

Will do, thanks Evan.

Contributor

jazmit commented Feb 6, 2015

Will do, thanks Evan.

@evancz evancz merged commit 3c1cb9a into elm:master Feb 6, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Feb 6, 2015

Member

Cool, I'm a fan of this, and I bet @rehno-lindeque is too :)

Next steps are:

  1. Do the Text refactor to clean things up.
  2. Make a decision about using the native dashed stroke function. Sounds like it's non-crazy to use the native version with the fallback of solid lines, but it'd be good to discuss in a separate PR.
Member

evancz commented Feb 6, 2015

Cool, I'm a fan of this, and I bet @rehno-lindeque is too :)

Next steps are:

  1. Do the Text refactor to clean things up.
  2. Make a decision about using the native dashed stroke function. Sounds like it's non-crazy to use the native version with the fallback of solid lines, but it'd be good to discuss in a separate PR.
@rehno-lindeque

This comment has been minimized.

Show comment
Hide comment
@rehno-lindeque

rehno-lindeque Feb 6, 2015

Contributor

🎉 🎆

Contributor

rehno-lindeque commented Feb 6, 2015

🎉 🎆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment