'deltaY', superscript, subscript etc. #4177

denim2x · 2017-08-07T14:06:14Z

added deltaY to _styleProperties;
updated getSvgSpanStyles for deltaY -> baseline-shift;
updated _renderChar to honor deltaY by adjusting top;
added functionality for 'superscript' and 'subscript';
fixed some bugs 🐞

asturur · 2017-08-07T14:22:40Z

src/mixins/itext.svg_export.js

@@ -131,7 +131,7 @@
          timeToRender = this._hasStyleChanged(actualStyle, nextStyle);
        }
        if (timeToRender) {
-          style = this._getStyleDeclaration(lineIndex, i) || { };
+          style = this.getStyleDeclaration(lineIndex, i) || { };


we cannot rename this as part of the PR. _getStyleDeclaration will stay named like this for now.

asturur · 2017-08-07T14:34:52Z

a couple of suggestions, leave refactors of properties array and renaming out. Is easier to grasp meaningfull changes.
If you have some ideas about them open a PR later.

Let's concentrate on deltaY support in toSVG and rendering as first step.

asturur · 2017-08-07T14:36:58Z

src/shapes/text.class.js

+     * @type Number
+     * @default
+     */
+    deltaY:               0,


i m uncertain about this. We should not offer a global deltaY property to the developer.

There is a probably a way to manage style without having the parent property set.

Added 'deltaY' to '_styleProperties' and updated 'getSvgSpanStyles' for 'deltaY' -> 'baseline-shift'

asturur · 2017-08-08T07:59:18Z

src/shapes/text.class.js

@@ -908,7 +913,7 @@
      var box = {
        width: width,
        left: 0,
-        height: charStyle.fontSize,
+        height: style.fontSize,
        kernedWidth: kernedWidth,
      };


would be nice to have the deltay in the graphmebox information, even if for now we are not using it in the grapheme box context.

i think we can just return deltay: deltay in the box object, is more than fine for now.

asturur · 2017-08-08T08:01:06Z

src/shapes/text.class.js

     */
    getHeightOfLine: function(lineIndex) {
      if (this.__lineHeights[lineIndex]) {
        return this.__lineHeights[lineIndex];
      }

      var line = this._textLines[lineIndex],
-          maxHeight = this.getHeightOfChar(lineIndex, 0);
+          maxHeight = Number.NEGATIVE_INFINITY,


In this way you are loosing the maxHeight of the first line.
Other than that, text can be in an empty state situation, where there is no text. I would not start from negative_infinity with the risk of getting it as result.

the idea was that we need to take dy into account - and the only way was to start with -Infinity

asturur · 2017-08-08T08:04:35Z

I'm unsure about the lineHieght changes.
Moving up a char, should not make the line taller.
We should just need to worried about the first and last line to do not go out of the bounding boxes, but setting the proper values of dy to do not make the text look bad is a developer task, not the library to allow everything to look good. is just too hard.

As a first step would be nice just to see the letters moved up and and down in the render, even if they overlap with others, and the same effect respected in toSVG.

That would be enough small to be readable/reviewable and would be enough working to be mergeable.

asturur · 2017-08-08T08:09:03Z

this is a screen from google docs. They limit the displacement of apix and pedix to little so that they do not cross line and lineHeight stays the same.

Since we are implementing this property as a tool to get apix and pedix, is ok leave lineHeight calculation out.

Since later i want to introduce per char rotation as svg, i think the measurement logic must be revised useing the boxes of each char. Do not worry now.

asturur · 2017-08-08T08:10:27Z

they actually nearly overlap

denim2x · 2017-08-08T09:19:23Z

then may I calculate deltaY to fit lineHeight?

asturur · 2017-08-08T09:26:13Z

you can go by trying, do not need to get it perfect.
Over the UPPERCASE letters there is like 0.15 space of font size or 0.12. It looks like they are using a 0.5 or 0.4 multiplier.

asturur · 2017-08-08T10:44:57Z

src/shapes/text.class.js

+          deltaY = Math.max(deltaY, lineHeight * LINE_PADDING);
+        }
+        top += deltaY;
+      }


what is this doing? why we cannot have just top += deltay? i do not understand the math purpose.

well the math ensures there's no overlap; there could be some flag (e.g. allowOverlap) for disabling this behavior (defaults to false)

asturur · 2017-08-08T17:21:59Z

no let the overlap free.
we need to be free with rendering and svg overlaps too if it needs it.

The method to set sub or super script can take in consideration overlap, but rendering just excutes the values that find in style.

denim2x · 2017-08-09T17:25:02Z

🔗 plunker

asturur · 2017-08-09T19:38:13Z

src/shapes/text.class.js

@@ -1092,7 +1106,7 @@
     * @param {Number} top Top coordinate
     * @param {Number} lineHeight Height of the line
     */
-    _renderChar: function(method, ctx, lineIndex, charIndex, _char, left, top) {
+    _renderChar: function(method, ctx, lineIndex, charIndex, _char, left, top, lineHeight) {


do we still need that lineHeight? looks like we do not need now.

asturur · 2017-08-09T19:44:04Z

src/shapes/text.class.js

+      var decl = this._getStyleDeclaration(line, char) || {};
+      decl[key] = value;
+      this._setStyleDeclaration(line, char, decl);
+      return value;


I do not think we should return a value with the setter. We can return this and make it chainable

asturur · 2017-08-09T19:47:32Z

I think is everything ok. I left a pair of comments and i have some comment to write about the scale function that is too much general for what we have to do. Let me think a bit about it.

asturur · 2017-08-09T20:04:03Z

src/mixins/object.svg_export.js

@@ -60,7 +60,6 @@

    /**
     * Returns styles-string for svg-export
-     * @param {Boolean} skipShadow a boolean to skip shadow filter output


would you mind correcting the JSDOC here?

@param {Object} style object to getthe style-string from

asturur · 2017-08-10T07:45:43Z

I would prefere something like this for both sup and sub script

   setSuperscript: function(line, char) {
         var currentFontSize = this.getPropertyAt(line, char, 'fontSize');
         var currentBaseline = this.getPropertyAt(line, char, 'baseline');
         setPropertyAt(line, char, 'fontSize', currentFontSize * this.superscript.fontSize);
         setPropertyAt(line, char, 'deltaY', baseline  - currentFontSize * this.superscript.baseline);
         return this;
   },

Is shorter than the general one and we do not have any general case to apply this for.
Any reason why you think the scale function is necessary?

I m really trying to just save space and code to mantain. Fabric is a big library full of features that maybe should not be introduced at all at the time.

I'm sorry if it looks like i'm spoon feeding you.

denim2x · 2017-08-10T07:47:38Z

fine by me

asturur · 2017-08-10T07:50:20Z

take care of this ^^^^

if you cloned the repository, you should have a command line
npm run lint that would allow you to check this beforehand.

Also try to add a small test for setSubscript and setSuperScript so that it verifies that after calling the method the style is mutated.

i can provide a snippet for you.

I would merge this then.

After this is merged we would need some more work to displace the cursor that i believe does not follow the superscript. Probably it automatically gets the size but not the baseline.

Do not worry for now do not mix it here.

asturur · 2017-08-10T07:56:00Z

test example

  test('text superScript', function() {
    var text = new fabric.Text('xxx', { fontSize: 30 });
    text.styles = { 0: { 0: { fontSize: 20, fill: 'blue' }, 1:  { fill: 'blue' }}};
    ok(typeof text.setSuperScript === 'function');
    equal(text.styles[0][0].baseline, undefined, 'the baseline is not set');
    equal(text.styles[0][1].baseline, undefined, 'the baseline is not set');
    text.setSuperScript(0, 0);
    text.setSuperScript(0, 1);
    equal(text.styles[0][0].fontSize,....., 'the fontSize has been.... add description');
    equal(text.styles[0][0].baseline, ...., 'baseline is now a portion of ...');
// then verify for the char 1 that has no fontSize in the style
    equal(text.styles[0][1].fontSize,....., 'the fontSize has been.... add description');
    equal(text.styles[0][1].baseline, ...., 'baseline is now a portion of ...');
  });

asturur · 2017-08-10T07:59:52Z

I was also thinking that while the fontsize is multiplied the baseline is added. This is ok for 1 usage of superscript.

The functionality is generic enough so that i can set superscript twice, but i need to add the baseline to the current one.
Since deltaY is not a property of the text prototype and probably should not be, i think we need a special handling when we get is as undefined to interpret as 0.

denim2x · 2017-08-10T08:06:00Z

when do we need 'deltaY == 0'? I always check for the presence of 'deltaY' in my code - when rendering and converting to SVG

asturur · 2017-08-10T08:09:53Z

I made the ipothesis of calling setSuperScript twice.

The font mulitplication works. Because it gets multiplies by some 0.x twice. So the char gets smaller and smaller.

The second time you call it, baseline would be erased and recalculated by the current fontSize that is the one of the first call of setSuperScript, while thinking of making the apix of the apix, you would need to displace the character UP by what it was before plus a portion of the currentFontsize.

This bring in the need of knowing the current baseline. the current baseline may be undefined.

denim2x · 2017-08-10T08:10:52Z

I'll check it

asturur · 2017-08-10T08:13:23Z

#4177 (comment)

in this comment i was adding it in that small function example

denim2x · 2017-08-10T10:04:32Z

well what do you know - it works! I just had to do dy + size * ...

Honors 'deltaY' during rendering; provides functionality for 'superscript' & 'subscript'; some bug-fixing

asturur · 2017-08-12T05:18:15Z

i think we are over engineering the solution.
all the schema thing is way overcomplicated for me, the plain function is the best in my opinion.

we need tests to pass. are you able to run tests locally on your end?

denim2x · 2017-08-12T07:59:52Z

first off - when you said scaleChar should go - well, it's gone;
secondly, well - there's absolutely no point in writing the same code twice (DRY anyone?);
and last (but not least), have you checked the plunker I posted?

asturur · 2017-08-12T08:12:27Z

what you say is true in general, in this case is more than necessary. in this case the general code is bigger than twice the simple one and it involves modifying our class creation code. other than that being an open project mantained by volunteers, getting a grasp of what a function does at first glance is very important. i ll show you later what i mean.

…

On Aug 12, 2017 9:59 AM, "Cristian Tudorache" ***@***.***> wrote: first off - when you said scaleChar should go - well, it's gone; secondly, well - there's absolutely *no point* in writing the *same* code *twice* (*DRY* anyone?); and last (but *not* least), have you checked the plunker I posted? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#4177 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABI4QCgTQGzv-zm96nZPHlY9Nzxb4Mqrks5sXVt6gaJpZM4Ove7S> .

asturur · 2017-08-12T08:13:05Z

no i did not open the plunker. i saw a link with no introduction no explanations and does not work from mobile.

…

On Aug 12, 2017 9:59 AM, "Cristian Tudorache" ***@***.***> wrote: first off - when you said scaleChar should go - well, it's gone; secondly, well - there's absolutely *no point* in writing the *same* code *twice* (*DRY* anyone?); and last (but *not* least), have you checked the plunker I posted? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#4177 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABI4QCgTQGzv-zm96nZPHlY9Nzxb4Mqrks5sXVt6gaJpZM4Ove7S> .

asturur · 2017-08-12T11:46:40Z

so the feature is working, tests do not pass but i think i can fix them.
I m going to merge this branch, fix the tests in a PR.

We should try to fix the cursor look that is not in the right position during deltaY displacement.

denim2x · 2017-08-12T11:53:57Z

ok

* 'deltaY', superscript, subscript etc. (#4177)

* 'deltaY', superscript, subscript etc. (fabricjs#4177)

asturur reviewed Aug 7, 2017

View reviewed changes

Add 'deltaY' attribute

e4432c8

Added 'deltaY' to '_styleProperties' and updated 'getSvgSpanStyles' for 'deltaY' -> 'baseline-shift'

denim2x force-pushed the baseline-shift branch 2 times, most recently from 94dfe97 to a0a410c Compare August 8, 2017 06:43

asturur reviewed Aug 8, 2017

View reviewed changes

denim2x force-pushed the baseline-shift branch from a0a410c to ac8cfa6 Compare August 8, 2017 10:10

asturur reviewed Aug 8, 2017

View reviewed changes

denim2x force-pushed the baseline-shift branch from ac8cfa6 to 4f6b6ff Compare August 9, 2017 12:29

denim2x changed the title ~~Add 'deltaY' attribute~~ 'deltaY', superscript, subscript etc. Aug 9, 2017

denim2x force-pushed the baseline-shift branch from 4f6b6ff to d644d17 Compare August 9, 2017 16:30

denim2x force-pushed the baseline-shift branch from 498a86a to 7b84dd4 Compare August 9, 2017 17:55

asturur reviewed Aug 9, 2017

View reviewed changes

denim2x force-pushed the baseline-shift branch from 7b84dd4 to 0d8d3fe Compare August 10, 2017 06:49

denim2x force-pushed the baseline-shift branch 5 times, most recently from 60021f3 to 045fe04 Compare August 10, 2017 18:34

'deltaY', superscript, subscript etc.

d7a9b80

Honors 'deltaY' during rendering; provides functionality for 'superscript' & 'subscript'; some bug-fixing

denim2x force-pushed the baseline-shift branch from 045fe04 to d7a9b80 Compare August 11, 2017 21:11

asturur merged commit ffc5cd7 into fabricjs:baseline-shift Aug 12, 2017

asturur added a commit that referenced this pull request Mar 1, 2018

Add BaselineShift property to fabricJS (#4765)

aa9b8a6

* 'deltaY', superscript, subscript etc. (#4177)

thiagocunha pushed a commit to thiagocunha/fabric.js that referenced this pull request Nov 18, 2019

Add BaselineShift property to fabricJS (fabricjs#4765)

8d7abd5

* 'deltaY', superscript, subscript etc. (fabricjs#4177)

'deltaY', superscript, subscript etc. #4177

'deltaY', superscript, subscript etc. #4177

Conversation

denim2x commented Aug 7, 2017 • edited Loading

Choose a reason for hiding this comment

asturur commented Aug 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asturur commented Aug 8, 2017

asturur commented Aug 8, 2017

asturur commented Aug 8, 2017

denim2x commented Aug 8, 2017

asturur commented Aug 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asturur commented Aug 8, 2017

denim2x commented Aug 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asturur commented Aug 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asturur commented Aug 10, 2017 • edited Loading

denim2x commented Aug 10, 2017

asturur commented Aug 10, 2017

asturur commented Aug 10, 2017 • edited Loading

asturur commented Aug 10, 2017

denim2x commented Aug 10, 2017

asturur commented Aug 10, 2017

denim2x commented Aug 10, 2017

asturur commented Aug 10, 2017

denim2x commented Aug 10, 2017

asturur commented Aug 12, 2017

denim2x commented Aug 12, 2017

asturur commented Aug 12, 2017 via email

asturur commented Aug 12, 2017 via email

asturur commented Aug 12, 2017

denim2x commented Aug 12, 2017

denim2x commented Aug 7, 2017 •

edited

Loading

asturur commented Aug 7, 2017 •

edited

Loading

asturur commented Aug 10, 2017 •

edited

Loading

asturur commented Aug 10, 2017 •

edited

Loading