Image resizer now cleans view after resize is committed/cancelled #122

mlewand · 2020-04-17T11:12:40Z

Suggested merge commit message (convention)

Fix: Image resize now cleans up temporary view width style changes. Closes ckeditor/ckeditor5#6060.

Additional information

Reverting should be handled in _cleanup method as it guarantees it to happen whether resize process was committed or cancelled.

…gering options.onCommit.

…age resize.

…riginal view value.

coveralls · 2020-04-17T11:17:29Z

Coverage remained the same at 100.0% when pulling de2922f on i/6060-cleanup into 6273527 on master.

tests/widgetresize.js

Co-Authored-By: Aleksander Nowodzinski <[email protected]>

jodator

I've found one UX issue - it flickers (dunno if related to this issue).

src/widgetresize.js

src/widgetresize/resizer.js

jodator · 2020-04-20T09:56:22Z

src/widgetresize/resizer.js

 		this._cleanup();
+		this._options.onCommit( newValue );


I wonder if we need to comment on this? Why it is better to commit after cleanup? It looks like it is better to cleanup the view before making changes to it from the model change. Is that it?

jodator · 2020-04-20T10:08:19Z

src/widgetresize/resizer.js

+
+		const editingView = this._options.editor.editing.view;
+
+		editingView.change( writer => {


I wonder if this is the reason of:

Found on Firefox but also visible on Chrome: after ending resizing the image border of the original size flickers. It happens on every drag (not GIF drops some frames and is not always visible).

What I can think of is a single commit block (if doable) - i.e. wrapping clean-up and commit in a single view change block.

Yeah, this is probably because things are done in two batches and the browser gets a chance to re-pain in between. I don't know how to address this unless I spend some serious time digging into the code.

Doh 🤦 @jodator is right. I was a little overconfident with the change and did it only by writing unit test and making a fix. Without realizing there might be some rendering artifacts 😃 Fixed.

Co-Authored-By: Maciej <[email protected]>

mlewand · 2020-04-21T07:23:02Z

@ckeditor/qa-team Guys can I ask you to double check this little change whether there are no visible regressions vs master?

mlewand · 2020-04-21T07:26:01Z

Tests are passing locally for me, 100% CC. Don't know what's up with the CI. @jodator can you confirm?

jodator · 2020-04-21T07:44:48Z

@mlewand I can confirm that the test don't work 😜. Merged with master to fix ;)

Could you also add a test to ensure that this change is executed in one view.change() block? Dunno if we have events for that as we can test for similar thing in the model.

FilipTokarski · 2020-04-21T09:13:07Z

Tested in all browsers, looks good.

mlewand · 2020-04-21T09:57:55Z

@mlewand I can confirm that the test don't work 😜. Merged with master to fix ;)

Seems somebody had outdated masters on other repos 🤔

@jodator TBH I'd rather skip on making this test - there's gonna be a major effort to make this TC while this issue was meant to be a quick one.

Reinmar · 2020-04-21T21:00:44Z

@jodator TBH I'd rather skip on making this test - there's gonna be a major effort to make this TC while this issue was meant to be a quick one.

This is a pretty simple test to write, AFAICT. You just need to listen to the View#render event and check if it was fired once.

As far as I can see from your discussion, this PR proposed an incomplete (buggy) fix, so we should definitely have a test for this scenario. If the mistake could have been done once it can easily be repeated.

As requested in #122 (comment).

mlewand · 2020-04-22T10:55:48Z

Added the test, I referenced the proper comment to give at least some clarity for a person that faces it in the future.

Reinmar · 2020-04-22T17:26:29Z

I think that this should be a separate test as we're testing for the number of renders. Also, why 3? That's an odd number.

I'll fix these issues.

Reinmar · 2020-04-22T18:07:18Z

BTW, when I started digging into this, it turned out that this render count check passes before the issue that it was supposed to cover was fixed. When adding a test, you should verify that it's failing without the thing that you test (like in TDD, but to me it can also be done afterwards).

Reinmar · 2020-04-22T21:55:22Z

I forgot to push the fixed test before merging this PR 🤦. I pushed it to master: edd37d8

As requested in ckeditor/ckeditor5-widget#122 (comment).

mlewand added 5 commits April 17, 2020 11:04

Internal: Dirty PoC to revert original image view's width before trig…

868ce4d

…gering options.onCommit.

Tests: Unit test for restoring original view width after canceling im…

8065df4

…age resize.

Docs: Added API docs, turned initialViewWidth into a private variable.

e09b2cb

Tests: Added unit test checking if graceful commit also reverts the o…

9a2d01b

…riginal view value.

Internal: Extracted common constant.

7cd57d7

mlewand mentioned this pull request Apr 17, 2020

Revert view element's original width after resize #114

Closed

jodator self-requested a review April 20, 2020 08:49

jodator self-assigned this Apr 20, 2020

oleq reviewed Apr 20, 2020

View reviewed changes

tests/widgetresize.js Outdated Show resolved Hide resolved

Update tests/widgetresize.js

b664d9d

Co-Authored-By: Aleksander Nowodzinski <[email protected]>

jodator reviewed Apr 20, 2020

View reviewed changes

jodator and others added 4 commits April 20, 2020 12:13

Add missing inheritDoc annotations.

401af93

Docs: API docs adjustment.

eb5db23

Co-Authored-By: Maciej <[email protected]>

Docs: API docs adjustment.

3f105c7

Co-Authored-By: Maciej <[email protected]>

Internal: Ensure that commit is executed in a single view change batch.

32e18a4

mlewand requested a review from jodator April 21, 2020 07:23

Merge branch 'master' into i/6060-cleanup

5991e03

Tests: Added test for render count.

de2922f

As requested in #122 (comment).

Reinmar merged commit 92226f9 into master Apr 22, 2020

Reinmar deleted the i/6060-cleanup branch April 22, 2020 21:29

mlewand added a commit to ckeditor/ckeditor5 that referenced this pull request May 1, 2020

Tests: Added test for render count.

e3e15a5

As requested in ckeditor/ckeditor5-widget#122 (comment).

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Image resizer now cleans view after resize is committed/cancelled #122

Image resizer now cleans view after resize is committed/cancelled #122

mlewand commented Apr 17, 2020

coveralls commented Apr 17, 2020 •

edited

Loading

jodator left a comment

jodator Apr 20, 2020

jodator Apr 20, 2020 •

edited

Loading

oleq Apr 20, 2020

mlewand Apr 21, 2020 •

edited

Loading

mlewand commented Apr 21, 2020

mlewand commented Apr 21, 2020 •

edited

Loading

jodator commented Apr 21, 2020

FilipTokarski commented Apr 21, 2020

mlewand commented Apr 21, 2020

Reinmar commented Apr 21, 2020 •

edited

Loading

mlewand commented Apr 22, 2020

Reinmar commented Apr 22, 2020

Reinmar commented Apr 22, 2020

Reinmar commented Apr 22, 2020


		const editingView = this._options.editor.editing.view;

		editingView.change( writer => {

Image resizer now cleans view after resize is committed/cancelled #122

Image resizer now cleans view after resize is committed/cancelled #122

Conversation

mlewand commented Apr 17, 2020

Suggested merge commit message (convention)

Additional information

coveralls commented Apr 17, 2020 • edited Loading

jodator left a comment

Choose a reason for hiding this comment

jodator Apr 20, 2020

Choose a reason for hiding this comment

jodator Apr 20, 2020 • edited Loading

Choose a reason for hiding this comment

oleq Apr 20, 2020

Choose a reason for hiding this comment

mlewand Apr 21, 2020 • edited Loading

Choose a reason for hiding this comment

mlewand commented Apr 21, 2020

mlewand commented Apr 21, 2020 • edited Loading

jodator commented Apr 21, 2020

FilipTokarski commented Apr 21, 2020

mlewand commented Apr 21, 2020

Reinmar commented Apr 21, 2020 • edited Loading

mlewand commented Apr 22, 2020

Reinmar commented Apr 22, 2020

Reinmar commented Apr 22, 2020

Reinmar commented Apr 22, 2020

coveralls commented Apr 17, 2020 •

edited

Loading

jodator Apr 20, 2020 •

edited

Loading

mlewand Apr 21, 2020 •

edited

Loading

mlewand commented Apr 21, 2020 •

edited

Loading

Reinmar commented Apr 21, 2020 •

edited

Loading