Address review: cropper mime/zoom/error fixes, harden browser test
- Output a canvas-encodable mime (jpeg/png/webp, else png) and keep the File's name/extension in sync, so non-encodable input (gif/svg/heic) no longer ships PNG bytes mislabeled as the original type. - Clamp zoom to a maximum (8x cover) so scrolling in can't collapse the crop to a sub-pixel region. - Handle undecodable/zero-dimension images (@error + naturalWidth guard) with a crop_error message instead of a permanently-disabled Save. - Replace the str_contains(static::class) Vite heuristic with a dedicated BrowserTestCase ($fakesVite = false). - The browser test now decodes the dispatched blob and asserts a 512x512 image, and uses route(..., absolute: false) instead of a hardcoded path. - Remove tests/Browser/ProbeTest.php (committed debug scratch).
This commit is contained in:
Paulo Castellano
parent
c6369a6ddb
commit
2549a82e9b
22 changed files with 103 additions and 68 deletions
|
|
@ -26,6 +26,7 @@
|
|||
'crop_hint' => 'اسحب لإعادة التموضع',
|
||||
'crop_save' => 'حفظ',
|
||||
'crop_cancel' => 'إلغاء',
|
||||
'crop_error' => 'تعذّر تحميل هذه الصورة. جرّب ملفًا آخر.',
|
||||
],
|
||||
|
||||
'timezone' => [
|
||||
|
|
|
|||
|
|
@ -26,6 +26,7 @@
|
|||
'crop_hint' => 'Zum Verschieben ziehen',
|
||||
'crop_save' => 'Speichern',
|
||||
'crop_cancel' => 'Abbrechen',
|
||||
'crop_error' => 'Dieses Bild konnte nicht geladen werden. Versuchen Sie eine andere Datei.',
|
||||
],
|
||||
|
||||
'timezone' => [
|
||||
|
|
|
|||
|
|
@ -26,6 +26,7 @@
|
|||
'crop_hint' => 'Σύρετε για επανατοποθέτηση',
|
||||
'crop_save' => 'Αποθήκευση',
|
||||
'crop_cancel' => 'Άκυρο',
|
||||
'crop_error' => 'Δεν ήταν δυνατή η φόρτωση αυτής της εικόνας. Δοκιμάστε άλλο αρχείο.',
|
||||
],
|
||||
|
||||
'timezone' => [
|
||||
|
|
|
|||
|
|
@ -26,6 +26,7 @@
|
|||
'crop_hint' => 'Drag to reposition',
|
||||
'crop_save' => 'Save',
|
||||
'crop_cancel' => 'Cancel',
|
||||
'crop_error' => 'Couldn\'t load this image. Try another file.',
|
||||
],
|
||||
|
||||
'timezone' => [
|
||||
|
|
|
|||
|
|
@ -26,6 +26,7 @@
|
|||
'crop_hint' => 'Arrastra para reposicionar',
|
||||
'crop_save' => 'Guardar',
|
||||
'crop_cancel' => 'Cancelar',
|
||||
'crop_error' => 'No se pudo cargar esta imagen. Prueba con otro archivo.',
|
||||
],
|
||||
|
||||
'timezone' => [
|
||||
|
|
|
|||
|
|
@ -26,6 +26,7 @@
|
|||
'crop_hint' => 'Faites glisser pour repositionner',
|
||||
'crop_save' => 'Enregistrer',
|
||||
'crop_cancel' => 'Annuler',
|
||||
'crop_error' => 'Impossible de charger cette image. Essayez un autre fichier.',
|
||||
],
|
||||
|
||||
'timezone' => [
|
||||
|
|
|
|||
|
|
@ -26,6 +26,7 @@
|
|||
'crop_hint' => 'Trascina per riposizionare',
|
||||
'crop_save' => 'Salva',
|
||||
'crop_cancel' => 'Annulla',
|
||||
'crop_error' => 'Impossibile caricare questa immagine. Prova un altro file.',
|
||||
],
|
||||
|
||||
'timezone' => [
|
||||
|
|
|
|||
|
|
@ -26,6 +26,7 @@
|
|||
'crop_hint' => 'ドラッグして移動',
|
||||
'crop_save' => '保存',
|
||||
'crop_cancel' => 'キャンセル',
|
||||
'crop_error' => 'この画像を読み込めませんでした。別のファイルをお試しください。',
|
||||
],
|
||||
|
||||
'timezone' => [
|
||||
|
|
|
|||
|
|
@ -26,6 +26,7 @@
|
|||
'crop_hint' => '드래그하여 위치 조정',
|
||||
'crop_save' => '저장',
|
||||
'crop_cancel' => '취소',
|
||||
'crop_error' => '이 이미지를 불러올 수 없습니다. 다른 파일을 사용해 보세요.',
|
||||
],
|
||||
|
||||
'timezone' => [
|
||||
|
|
|
|||
|
|
@ -26,6 +26,7 @@
|
|||
'crop_hint' => 'Sleep om te verplaatsen',
|
||||
'crop_save' => 'Opslaan',
|
||||
'crop_cancel' => 'Annuleren',
|
||||
'crop_error' => 'Kan deze afbeelding niet laden. Probeer een ander bestand.',
|
||||
],
|
||||
|
||||
'timezone' => [
|
||||
|
|
|
|||
|
|
@ -26,6 +26,7 @@
|
|||
'crop_hint' => 'Przeciągnij, aby zmienić położenie',
|
||||
'crop_save' => 'Zapisz',
|
||||
'crop_cancel' => 'Anuluj',
|
||||
'crop_error' => 'Nie można załadować tego obrazu. Spróbuj innego pliku.',
|
||||
],
|
||||
|
||||
'timezone' => [
|
||||
|
|
|
|||
|
|
@ -26,6 +26,7 @@
|
|||
'crop_hint' => 'Arraste para reposicionar',
|
||||
'crop_save' => 'Salvar',
|
||||
'crop_cancel' => 'Cancelar',
|
||||
'crop_error' => 'Não foi possível carregar esta imagem. Tente outro arquivo.',
|
||||
],
|
||||
|
||||
'timezone' => [
|
||||
|
|
|
|||
|
|
@ -26,6 +26,7 @@
|
|||
'crop_hint' => 'Перетащите, чтобы переместить',
|
||||
'crop_save' => 'Сохранить',
|
||||
'crop_cancel' => 'Отмена',
|
||||
'crop_error' => 'Не удалось загрузить это изображение. Попробуйте другой файл.',
|
||||
],
|
||||
|
||||
'timezone' => [
|
||||
|
|
|
|||
|
|
@ -26,6 +26,7 @@
|
|||
'crop_hint' => 'Yeniden konumlandırmak için sürükleyin',
|
||||
'crop_save' => 'Kaydet',
|
||||
'crop_cancel' => 'İptal',
|
||||
'crop_error' => 'Bu görsel yüklenemedi. Başka bir dosya deneyin.',
|
||||
],
|
||||
|
||||
'timezone' => [
|
||||
|
|
|
|||
|
|
@ -26,6 +26,7 @@
|
|||
'crop_hint' => '拖动以重新定位',
|
||||
'crop_save' => '保存',
|
||||
'crop_cancel' => '取消',
|
||||
'crop_error' => '无法加载此图片。请尝试其他文件。',
|
||||
],
|
||||
|
||||
'timezone' => [
|
||||
|
|
|
|||
|
|
@ -47,15 +47,25 @@ const natural = ref({ width: 0, height: 0 });
|
|||
const transform = ref<CropTransform>({ scale: 1, x: 0, y: 0 });
|
||||
const processing = ref(false);
|
||||
const initialized = ref(false);
|
||||
const imageError = ref(false);
|
||||
|
||||
let dragPointerId: number | null = null;
|
||||
let dragStart = { pointerX: 0, pointerY: 0, x: 0, y: 0 };
|
||||
let resizeObserver: ResizeObserver | null = null;
|
||||
|
||||
const encodableMimes = ['image/jpeg', 'image/png', 'image/webp'];
|
||||
const extensions: Record<string, string> = { 'image/jpeg': 'jpg', 'image/png': 'png', 'image/webp': 'webp' };
|
||||
|
||||
const ready = computed(() => viewportSize.value > 0 && natural.value.width > 0);
|
||||
|
||||
const maskClass = computed(() => (props.shape === 'square' ? 'rounded-lg' : 'rounded-full'));
|
||||
|
||||
const outputMime = computed(() => (encodableMimes.includes(props.mimeType) ? props.mimeType : 'image/png'));
|
||||
|
||||
const outputFileName = computed(
|
||||
() => `${props.fileName.replace(/\.[^./]+$/, '') || 'image'}.${extensions[outputMime.value]}`,
|
||||
);
|
||||
|
||||
const imageStyle = computed(() => ({
|
||||
width: `${natural.value.width * transform.value.scale}px`,
|
||||
height: `${natural.value.height * transform.value.scale}px`,
|
||||
|
|
@ -94,10 +104,20 @@ const onImageLoad = () => {
|
|||
return;
|
||||
}
|
||||
|
||||
if (img.naturalWidth === 0 || img.naturalHeight === 0) {
|
||||
imageError.value = true;
|
||||
|
||||
return;
|
||||
}
|
||||
|
||||
natural.value = { width: img.naturalWidth, height: img.naturalHeight };
|
||||
maybeInitialize();
|
||||
};
|
||||
|
||||
const onImageError = () => {
|
||||
imageError.value = true;
|
||||
};
|
||||
|
||||
const onPointerDown = (event: PointerEvent) => {
|
||||
if (!ready.value) {
|
||||
return;
|
||||
|
|
@ -187,10 +207,10 @@ const save = () => {
|
|||
return;
|
||||
}
|
||||
|
||||
emit('cropped', new File([blob], props.fileName, { type: props.mimeType }));
|
||||
emit('cropped', new File([blob], outputFileName.value, { type: outputMime.value }));
|
||||
close();
|
||||
},
|
||||
props.mimeType,
|
||||
outputMime.value,
|
||||
0.92,
|
||||
);
|
||||
};
|
||||
|
|
@ -201,6 +221,7 @@ watch(
|
|||
if (isOpen) {
|
||||
initialized.value = false;
|
||||
processing.value = false;
|
||||
imageError.value = false;
|
||||
await nextTick();
|
||||
measure();
|
||||
|
||||
|
|
@ -219,6 +240,7 @@ watch(
|
|||
() => props.src,
|
||||
() => {
|
||||
initialized.value = false;
|
||||
imageError.value = false;
|
||||
natural.value = { width: 0, height: 0 };
|
||||
},
|
||||
);
|
||||
|
|
@ -244,7 +266,7 @@ onBeforeUnmount(() => resizeObserver?.disconnect());
|
|||
@wheel="onWheel"
|
||||
>
|
||||
<img
|
||||
v-if="src"
|
||||
v-if="src && !imageError"
|
||||
ref="imageEl"
|
||||
:src="src"
|
||||
alt=""
|
||||
|
|
@ -252,8 +274,16 @@ onBeforeUnmount(() => resizeObserver?.disconnect());
|
|||
class="absolute left-0 top-0 max-w-none"
|
||||
:style="imageStyle"
|
||||
@load="onImageLoad"
|
||||
@error="onImageError"
|
||||
/>
|
||||
<div
|
||||
v-if="imageError"
|
||||
class="absolute inset-0 flex items-center justify-center p-4 text-center text-sm text-muted-foreground"
|
||||
>
|
||||
{{ $t('common.photo_upload.crop_error') }}
|
||||
</div>
|
||||
<div
|
||||
v-else
|
||||
class="pointer-events-none absolute inset-0"
|
||||
:class="maskClass"
|
||||
style="box-shadow: 0 0 0 9999px rgba(0, 0, 0, 0.5), inset 0 0 0 2px rgba(255, 255, 255, 0.7)"
|
||||
|
|
|
|||
|
|
@ -11,6 +11,8 @@ export type SourceRect = {
|
|||
sh: number;
|
||||
};
|
||||
|
||||
const MAX_ZOOM = 8;
|
||||
|
||||
export const coverScale = (naturalWidth: number, naturalHeight: number, viewport: number): number => {
|
||||
if (naturalWidth <= 0 || naturalHeight <= 0) {
|
||||
return 1;
|
||||
|
|
@ -52,7 +54,8 @@ export const zoomTransform = (
|
|||
naturalHeight: number,
|
||||
viewport: number,
|
||||
): CropTransform => {
|
||||
const nextScale = Math.max(transform.scale * factor, coverScale(naturalWidth, naturalHeight, viewport));
|
||||
const minScale = coverScale(naturalWidth, naturalHeight, viewport);
|
||||
const nextScale = Math.min(minScale * MAX_ZOOM, Math.max(transform.scale * factor, minScale));
|
||||
const center = viewport / 2;
|
||||
const sourceX = (center - transform.x) / transform.scale;
|
||||
const sourceY = (center - transform.y) / transform.scale;
|
||||
|
|
|
|||
|
|
@ -32,10 +32,11 @@ function selectPhoto(mixed $page): void
|
|||
}
|
||||
|
||||
/**
|
||||
* Capture the next multipart request the page sends. The Pest browser server
|
||||
* does not parse multipart bodies (its file handling is an open TODO), so we
|
||||
* assert the crop dispatches the right upload rather than that it persists —
|
||||
* server-side persistence is covered by ProfileUpdateTest.
|
||||
* Capture the next multipart upload the page sends, keeping the uploaded File so
|
||||
* the test can decode it. The Pest browser server does not parse multipart
|
||||
* bodies (its file handling is an open TODO), so we assert the crop dispatches
|
||||
* a valid image rather than that it persists — persistence is covered by
|
||||
* ProfileUpdateTest.
|
||||
*/
|
||||
function recordUpload(mixed $page): void
|
||||
{
|
||||
|
|
@ -51,7 +52,14 @@ function recordUpload(mixed $page): void
|
|||
};
|
||||
XMLHttpRequest.prototype.send = function (body) {
|
||||
if (body instanceof FormData) {
|
||||
window.__uploadRequest = { method: this.__method, url: this.__url, keys: [...body.keys()] };
|
||||
const photo = body.get('photo');
|
||||
window.__uploadFile = photo;
|
||||
window.__uploadRequest = {
|
||||
method: this.__method,
|
||||
url: this.__url,
|
||||
keys: [...body.keys()],
|
||||
size: photo instanceof File ? photo.size : 0,
|
||||
};
|
||||
}
|
||||
return send.apply(this, arguments);
|
||||
};
|
||||
|
|
@ -59,7 +67,7 @@ function recordUpload(mixed $page): void
|
|||
JS);
|
||||
}
|
||||
|
||||
test('cropping a selected photo dispatches a cropped avatar upload', function () {
|
||||
test('cropping a selected photo dispatches a valid 512x512 avatar upload', function () {
|
||||
$this->actingAs(User::factory()->create());
|
||||
|
||||
$page = visit(route('app.profile.edit'));
|
||||
|
|
@ -67,9 +75,6 @@ function recordUpload(mixed $page): void
|
|||
selectPhoto($page);
|
||||
recordUpload($page);
|
||||
|
||||
// Auto-waits for the cropper dialog to open and its Save button to become
|
||||
// enabled — the button only enables once the image has loaded and been
|
||||
// measured inside the modal, which is exactly what a broken cropper fails.
|
||||
$page->click('@crop-save')
|
||||
->assertNoJavaScriptErrors();
|
||||
|
||||
|
|
@ -78,12 +83,19 @@ function recordUpload(mixed $page): void
|
|||
for (let attempt = 0; attempt < 80 && !window.__uploadRequest; attempt++) {
|
||||
await new Promise((resolve) => setTimeout(resolve, 100));
|
||||
}
|
||||
return JSON.stringify(window.__uploadRequest);
|
||||
if (!window.__uploadRequest) {
|
||||
return 'null';
|
||||
}
|
||||
const bitmap = await createImageBitmap(window.__uploadFile);
|
||||
return JSON.stringify({ ...window.__uploadRequest, width: bitmap.width, height: bitmap.height });
|
||||
})();
|
||||
JS), true);
|
||||
|
||||
expect($request)->not->toBeNull()
|
||||
->and($request['method'])->toBe('POST')
|
||||
->and($request['url'])->toContain('/settings/profile/photo')
|
||||
->and($request['keys'])->toContain('photo');
|
||||
->and($request['url'])->toContain(route('app.profile.upload-photo', absolute: false))
|
||||
->and($request['keys'])->toContain('photo')
|
||||
->and($request['size'])->toBeGreaterThan(0)
|
||||
->and($request['width'])->toBe(512)
|
||||
->and($request['height'])->toBe(512);
|
||||
});
|
||||
|
|
|
|||
|
|
@ -1,41 +0,0 @@
|
|||
<?php
|
||||
|
||||
declare(strict_types=1);
|
||||
|
||||
use App\Models\User;
|
||||
|
||||
test('probe upload response', function () {
|
||||
$user = User::factory()->create();
|
||||
$this->actingAs($user);
|
||||
$base64 = base64_encode((string) file_get_contents(base_path('tests/fixtures/blue-logo.png')));
|
||||
|
||||
$page = visit(route('app.profile.edit'));
|
||||
$page->script(<<<JS
|
||||
(async () => {
|
||||
const findInput = () => document.querySelector('input[type="file"]');
|
||||
for (let i = 0; i < 50 && !findInput(); i++) { await new Promise(r => setTimeout(r, 100)); }
|
||||
const input = findInput();
|
||||
const bytes = Uint8Array.from(atob('{$base64}'), (c) => c.charCodeAt(0));
|
||||
const file = new File([bytes], 'logo.png', { type: 'image/png' });
|
||||
const dt = new DataTransfer(); dt.items.add(file);
|
||||
input.files = dt.files;
|
||||
input.dispatchEvent(new Event('change', { bubbles: true }));
|
||||
})();
|
||||
JS);
|
||||
$page->script(<<<'JS'
|
||||
(() => {
|
||||
window.__resp = null;
|
||||
const oOpen = XMLHttpRequest.prototype.open;
|
||||
XMLHttpRequest.prototype.open = function (m, u) { this.__u = u; this.addEventListener('loadend', () => {
|
||||
if (String(this.__u).includes('/photo')) window.__resp = { status: this.status, body: (this.responseText || '').slice(0, 300) };
|
||||
}); return oOpen.apply(this, arguments); };
|
||||
})();
|
||||
JS);
|
||||
$page->click('@crop-save');
|
||||
$info = $page->script(<<<'JS'
|
||||
(async () => { for (let i = 0; i < 80 && !window.__resp; i++) { await new Promise(r => setTimeout(r, 100)); } return JSON.stringify(window.__resp || 'NO RESPONSE'); })();
|
||||
JS);
|
||||
fwrite(STDERR, "\nUPLOAD_RESP => {$info}\n");
|
||||
fwrite(STDERR, 'DB_HAS_PHOTO => '.json_encode($user->fresh()->has_photo)."\n");
|
||||
expect(true)->toBeTrue();
|
||||
});
|
||||
14
tests/BrowserTestCase.php
Normal file
14
tests/BrowserTestCase.php
Normal file
|
|
@ -0,0 +1,14 @@
|
|||
<?php
|
||||
|
||||
declare(strict_types=1);
|
||||
|
||||
namespace Tests;
|
||||
|
||||
abstract class BrowserTestCase extends TestCase
|
||||
{
|
||||
/**
|
||||
* Browser tests drive a real browser and load the built Vite assets, so the
|
||||
* manifest must not be faked away.
|
||||
*/
|
||||
protected bool $fakesVite = false;
|
||||
}
|
||||
|
|
@ -9,6 +9,7 @@
|
|||
use App\Models\User;
|
||||
use App\Models\Workspace;
|
||||
use Illuminate\Foundation\Testing\RefreshDatabase;
|
||||
use Tests\BrowserTestCase;
|
||||
use Tests\TestCase;
|
||||
|
||||
/*
|
||||
|
|
@ -24,7 +25,11 @@
|
|||
|
||||
pest()->extend(TestCase::class)
|
||||
->use(RefreshDatabase::class)
|
||||
->in('Feature', 'Unit', 'Browser');
|
||||
->in('Feature', 'Unit');
|
||||
|
||||
pest()->extend(BrowserTestCase::class)
|
||||
->use(RefreshDatabase::class)
|
||||
->in('Browser');
|
||||
|
||||
/*
|
||||
|--------------------------------------------------------------------------
|
||||
|
|
|
|||
|
|
@ -17,21 +17,18 @@ abstract class TestCase extends BaseTestCase
|
|||
*/
|
||||
protected $seed = true;
|
||||
|
||||
/**
|
||||
* Whether to fake the Vite manifest. Browser tests drive a real browser and
|
||||
* need the built assets, so they opt out via BrowserTestCase.
|
||||
*/
|
||||
protected bool $fakesVite = true;
|
||||
|
||||
protected function setUp(): void
|
||||
{
|
||||
parent::setUp();
|
||||
|
||||
if (! $this->isBrowserTest()) {
|
||||
if ($this->fakesVite) {
|
||||
$this->withoutVite();
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Browser tests drive a real browser and need the built Vite assets, so the
|
||||
* Vite manifest must not be faked away for them.
|
||||
*/
|
||||
private function isBrowserTest(): bool
|
||||
{
|
||||
return str_contains(static::class, '\\Browser\\');
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue