Upper canvas retina scaling

[cabaret]

@asturur, as requested in #5931, I've moved the retina scaling code from #5746 to a separate branch.

Currently, the pointer offset is incorrect due to the retina scaling. Is this something I should fix in getPointer (canvas.class.js:1310) or would you suggest another place? I'll do the work if you guide me a bit ;)

Thanks.

[cabaret]

In canvas.class.js, if I use the following code, everything seems to work. Not sure if this is the correct approach though:

      if (boundsWidth === 0 || boundsHeight === 0) {
        // If bounds are not available (i.e. not visible), do not apply scale.
        cssScale = { width: 1, height: 1 };
      }
      else {
        cssScale = {
          width: upperCanvasEl.width / boundsWidth,
          height: upperCanvasEl.height / boundsHeight
        };
      }
      if (upperRetina) {
        cssScale = {
          width: cssScale.width / 2,
          height: cssScale.height / 2,
        };
      }

[asturur]

Going to look at it, i have a working example somewhere, i ll dig it up. I prefer to keep the one i have, so i'm sure it works already.

[asturur]

I have to introduce some breaking changes soon.
Skip the configuration option here, if retina scaling is enabled in general it will be for both canvases.

No need for extra config.

[asturur]

So this is my personal implementation in an app

this is the getPointer method of canvas.

  getPointer(e, ignoreZoom, upperCanvasEl) {
    const p = this.callSuper('getPointer', e, ignoreZoom, upperCanvasEl);
    const retinaScaling = this.getRetinaScaling();
    if (retinaScaling !== 1) {
      p.x /= retinaScaling;
      p.y /= retinaScaling;
    }
    return p;
  },

those lines can be added in the original function directly.

[asturur]

urgh i commented on the wrong lines.
Before pushing up a commit, revert the changes to fabric.js file

to work and test, use the npm run build:watch, it will rebuild every file save.

Before committing use git checkout master dist to avoid sending up a built version of fabric.js

[cabaret]

Updated the PR, reverted previous dist mess and fixed your remarks.

[asturur]

wondering why this scaling is not already working correctly:

https://github.com/fabricjs/fabric.js/pull/5938/files#diff-9dc132ed9a0787568f773999ba577e2bR1341

Does it feels like working to you?
Can you try with also a container that is css stretched?

[cabaret]

Just tried this build in our project and it seems to work properly.

Getting two failed tests apparently:

• number is halved because of retina scaling, I suppose? (57/2 ~= 28)

fabric.IText: hiddenTextarea does not move DOM
    ---
        actual: >
            28
        expected: >
            57

• not sure about this one:

 iText click interaction: click on editing itext make selection:changed fire
    ---
        actual: >
            1
        expected: >
            2
        stack: >
                at http://localhost:8080/3638/test/unit/itext_click_behaviour.js:145:14
        message: >
            Itext set the selectionStart
        Log: |

[asturur]

yes we need to modify tests to be sure retina enabled and disabled is covered.

[cabaret]

@asturur I've started fixing the tests (differentiating between retina disabled and enabled) and they work fine locally but I can't seem to get them to work on the CI. Seems like it always returns the non-retina values. Any pointers? Thanks!

[cabaret]

Hey @asturur, did you have any time yet to take a look at this? Thanks!

[asturur]

Did not have time, i ll try today/tomorrow. Can you put me as contributor on your fork so i can push up changes?

[cabaret]

@asturur No problem. I've added you as a collaborator. Let me know where I can help!

[cabaret]

Hey @asturur, anything I can help with to move this forward? Thanks!

[asturur]

I'm sorry really under a pile of things to do.
Tomorrow i start an holiday, i have the notebook with me, i m sure i ll find time in the evenings to do something.

[asturur]

i m not expert with QUnit, can you explain me this change?

[cabaret]

I don't remember perfectly but I split up those tests in three blocks: independent of retina, retinaEnabled false and retinaEnabled true. I needed a way to specify the afterEach block for all tests and then the beforeEach calls for the nested modules. I found it this way in the QUnit docs but perhaps there are other ways. I think the reason I changed the line you commented on is to make the hooks calls the same throughout the file. Not quite sure anymore, sorry!

[asturur]

I just want to read the changes to tests because the diffs are so large i want to understand if is just indentation or there is more. I ll merge the next time i m on the computer.

[asturur]

Support for languages that may use composition is broken right now.
The hiddentextArea does not get positioned correctly.

[asturur]

Shadow in free drawing is broken

[asturur]

Things starts to be complicated. Retina scaling in Node is always 1, so some tests may be ineffective.
Anyway pencil brush is fixed and there is no way i can write a test that catch the regression for now.
We need to fix the textbox position.

[cabaret]

@asturur Can you give me some pointers on the problem with the textbox? I'm not sure I understand the issue. I'll gladly have a go at fixing it once I know what to look for ;)

[asturur]

in the function called _calcTextareaPosition there is some adjustment to do.
To be honest i did not expect it.
Probably we need to multiply or divide by the retina factor.

[cabaret]

What did you mean by Support for languages that may use composition is broken right now.? I'm not exactly sure what the hidden textbox is supposed to do or how to reproduce this. I'll take a look at the _calcTextareaPosition method.

[asturur]

you need to set your input language in japanese for example and type things.
You will notice that the overlay dropdown is displaced, like in my gif

[cabaret]

This is what I'm getting with Japanese: https://cloud.cupofco.de/qGuz2ZgB

[asturur]

that seems fine, but wasn't what i was getting at all.

[asturur]

this is what i get with latest branch running locally

[cabaret]

Hey @asturur, any idea on how I can reproduce this or next steps to get this merged eventually? Thanks.

[asturur]

ok let's merge!