search

wordpress-mobile / AztecEditor-Android

A reusable native Android rich text editor component.
Mozilla Public License 2.0
675 stars 112 forks source link

Issue/201 image blocks #202

Closed 0nko closed 7 years ago

0nko commented 7 years ago

Resolves #201.

theck13 commented 7 years ago

I don't think we should make images block elements. Here are a few unwanted side effects to doing that.

For those reasons, I would keep images inline.

0nko commented 7 years ago

I made the images inline again and they behave and look more or less like in visual editor. There is a bunch of improvements and tweaks to make images work, so this branch is still relevant, even though the original issue was about making images block elements.

I had to, for example, change the way the spans are rendered because removing the placeholder and adding the actual image was causing text overlaps within posts. Now the span stays the same but the drawable inside is replaced.

As a consequence of these changes, #204 is not relevant anymore.

There is an associated PR in wpandroid, which fixes image loading in the WordPress app. So to test the whole thing, please use that branch.

0nko commented 7 years ago

The images are stretched so that their dimension in px matches the value in dp (so a 300px by 300px image is stretched to 300dp by 300dp). This is what the visual editor does.

theck13 commented 7 years ago

I'm not sure stretching the images from 300px to 300dp is good. Images smaller than the screen are stretched and look pixellated.

@maxme and @aforcier, do you know why that approach was taken with the visual editor?

aforcier commented 7 years ago

The images are stretched so that their dimension in px matches the value in dp (so a 300px by 300px image is stretched to 300dp by 300dp). This is what the visual editor does.

I'm not sure stretching the images from 300px to 300dp is good. Images smaller than the screen look pixellated. @maxme and @aforcier, do you know why that approach was taken with the visual editor?

I'm not sure if I'm missing something, but the visual editor should not be stretching images. The visual editor will display images at their natural size, scaled down to fit the screen if the width is larger than the screen's width. Otherwise, they're displayed at their natural size, which may appear larger or smaller on different screens with different densities (but should never appear stretched).

This handling is done by the WebView - all the editor does explicitly is limit the maximum allowed width the size of the screen.

The only case of stretching would be for images smaller than 30px x 30px, which are scaled up to that size so that the edit overlay is big enough to be selectable.

0nko commented 7 years ago

Based on the observation of the visual editor I figured the images are strechted. The cat image in Elisa's intro post is 300px wide. The native resolution on the Pixel is 788px.

This is what it looks like in the visual editor:

screenshot_1485431215

This is what the image looks like in Aztec in its native resolution (300px):

screenshot_1485431038

This is what the image looks like in Aztec, when it's stretched to 300dp (788px native):

screenshot_1485452210

khaykov commented 7 years ago

I noticed that images inserted in the line breaks it, like this image is (72x72): break

Nexus 5X, 6.0.0

aforcier commented 7 years ago

@0nko I think you meant that the native screen with for a Pixel is 1080px.

I see what you mean, but I wouldn't call that stretched, the WebView is displaying the image at its natural size by correcting for the device's pixel density. The Chrome app does the same thing (and that way it also looks the same as it does on desktop, web browser or not).

In which cases are images smaller than the screen appearing pixelated? @theck13

theck13 commented 7 years ago

The visible part of this icon is 67 pixels square. In the PX screenshot, it's 67 pixels. In the DP screenshot, it's 175 pixels (i.e. ~2.6 times PX).

PX

px

DP

dp

0nko commented 7 years ago

Right, converting the native dimensions (67px) to its dp (175px) = correcting for the device's pixel density, which is what the visual editor does and what this PR did before.

khaykov commented 7 years ago

I fixed little issue that caused crash when adding image to unordered lists. The code looks good, but there is one big issue - adding images into block elements.

In this commit we removed the line splitting, which is fine when adding images into inline elements, but we still need to split block elements, and without newlines they get messed up.

I worked a bit on adding images into block elements, and as always it has gazilion of edge cases, like adding image to empty element in list, empty line in quote (logic is different from the list), adding image between block elements and so on. Will probably take another day or so, so should we merge this PR and add the support for inserting images into block elements a bit later?

0nko commented 7 years ago

Yeah, let's do that. I'll create an issue for it and merge this.