Bug 202016 – Web Inspector: Local Resource Overrides: UI for overriding image and font resource content

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

Web Inspector: Local Resource Overrides: UI for overriding image resource content

The backend allows overriding image content, but the UI doesn't allow creating overrides for Image resources. Provide a UI.

[Radar WebKit Bug Importer]

<rdar://problem/55541475>

[Radar WebKit Bug Importer]

<rdar://problem/55541482>

[Joseph Pecoraro]

Created attachment 379180 [details]
[PATCH] Proposed Fix

Blocked so bots will not build.

[Joseph Pecoraro]

Created attachment 379181 [details]
[IMAGE] Light Mode (Old)

[Joseph Pecoraro]

Created attachment 379182 [details]
[IMAGE] Dark Mode (Old)

[Joseph Pecoraro]

Node there is actually a green (+) on the cursor but I was unable to capture that in simple screenshots, without going to a video.

[Joseph Pecoraro]

Comment on attachment 379180 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=379180&action=review

> Source/WebInspectorUI/UserInterface/Views/DropZoneView.css:49
> +.drop-zone > .box {
> +    width: 70%;
> +    height: 70%;
> +    border: 3px dashed hsl(0, 0%, 70%, 0.8);
> +    border-radius: 3px;
> +    pointer-events: none;
> +}

This UI is totally up for debate. I just made a simple drop box, but it doesn't look great in many cases depending on the image underneath.

[Joseph Pecoraro]

Created attachment 379267 [details]
[PATCH] Proposed Fix

[Devin Rousso]

Created attachment 379508 [details]
[Image] suggested drop UI

How about something like this for the drop zone UI?

I think we should completely cover (edge-to-edge) the entire drop zone with an indicator so that developers know where they can drop the new image/file.

```
    display: flex;
    justify-content: center;
    align-items: center;
    position: absolute;
    top: 0;
    bottom: 0;
    right: 0;
    left: 0;
    font-size: 80px;
    color: white;
    text-shadow: 0 1px black;
    background-color: hsla(0, 0%, 50%, 0.7);
    -webkit-backdrop-filter: blur(10px);
    border: 10px dashed white;
```

[Nikita Vasilyev]

(In reply to Joseph Pecoraro from comment #4)
> Created attachment 379181 [details]
> [IMAGE] Light Mode

What is "#" doing at the bottom right side of the content view?

[Joseph Pecoraro]

(In reply to Nikita Vasilyev from comment #10)
> (In reply to Joseph Pecoraro from comment #4)
> > Created attachment 379181 [details]
> > [IMAGE] Light Mode
> 
> What is "#" doing at the bottom right side of the content view?

That is the image being dragged over the current image.

[Devin Rousso]

Comment on attachment 379267 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=379267&action=review

The logic of this is awesome.  I love the changes to `WI.SourceCodeRevision`.  I have a lot of style comments, as well as some more general design feedback.  I'd suggest iterating the UI once more before landing.

> Source/WebInspectorUI/ChangeLog:54
> +        * UserInterface/Views/DropZoneView.css: Copied from Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.css.

lol

> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:658
>                  let revision = styleSheet.currentRevision;

I'd inline this.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:825
> +        let {content, base64Encoded, mimeType} = revision;
> +        let {statusCode, statusText, responseHeaders} = localResource;

I personally don't like destructuring non-POD objects, as it's potentially harder to do global searches of getters (e.g. `.content` or `.base64Encoded`).

> Source/WebInspectorUI/UserInterface/Models/LocalResource.js:86
> +        let {content, base64Encoded} = response;

Ditto

> Source/WebInspectorUI/UserInterface/Models/Resource.js:428
>              return this._url;

NIT: I'd invert this and have the early-return be the `URL.createObjectURL(blobContent)`, so then the variable declaration is closer to us usage.

> Source/WebInspectorUI/UserInterface/Models/SourceCode.js:32
> +        this._originalRevision = new WI.SourceCodeRevision(this, null, false, null);

Can we make these into `const` variables, so it's clear at the callsite what they are?

> Source/WebInspectorUI/UserInterface/Models/SourceCodeRevision.js:32
>          console.assert(sourceCode instanceof WI.SourceCode);

I'd like us to move towards putting all optional parameters into an object-bag-like final parameter in the constructor, so they can be omitted at callsites if not needed.

If you do want to make these required, I'd suggest adding assertions that they match the right type/shape.

> Source/WebInspectorUI/UserInterface/Models/SourceCodeRevision.js:53
> +                this._blobContent = decodeBase64ToBlob(this._content, this.mimeType);

Aside: it'd be awesome to have something `WI.BlobUtilities.decodeBase64`, as well as similar ones for the other functions :P

> Source/WebInspectorUI/UserInterface/Models/SourceCodeRevision.js:62
> +    updateRevisionContent(content, base64Encoded, mimeType, blobContent)

It seems like everything other than `content` is optional, so can we move them into an optional POD parameter?
```
    updateRevisionContent(content, {base64Encoded, mimeType, blobContent} = {})
```

> Source/WebInspectorUI/UserInterface/Models/SourceCodeRevision.js:74
> +        this._blobContent = blobContent !== undefined ? blobContent : null;

We assert that `this._blobContent instanceof Blob` if it's truthy.

> Source/WebInspectorUI/UserInterface/Views/DropZoneView.css:33
> +    z-index: var(--z-index-glass-pane-for-drag);

NIT: I'd rephrase this to be less awkward `--z-index-for-drag-glass-pane`.

> Source/WebInspectorUI/UserInterface/Views/DropZoneView.js:28
> +// drag progress such as entering and leaving, and the drop itself. Clients should

NIT: you could move the "drag" to the previous line, as it looks like there's room for it.

> Source/WebInspectorUI/UserInterface/Views/DropZoneView.js:81
> +        console.assert(this._targetElement);

How does the `this._targetElement` relate to `this.element`?  Should it be a parent/child/sibling?

It seems like this class really should be more of a controller, not a view, as it really just listens for events on a target and adds/removes styles depending on them.

> Source/WebInspectorUI/UserInterface/Views/DropZoneView.js:87
> +        this.element.addEventListener("dragleave", this._handleDragLeave.bind(this));
> +        this.element.addEventListener("dragover", this._handleDragOver.bind(this));

NIT: I'd switch the order of these, as "dragover" should be fired before "dragleave".

> Source/WebInspectorUI/UserInterface/Views/DropZoneView.js:95
> +        this._activelyHandlingDrag = true;

Should we assert that `!this._activelyHandlingDrag`?

> Source/WebInspectorUI/UserInterface/Views/DropZoneView.js:101
> +        this._activelyHandlingDrag = false;

Ditto

> Source/WebInspectorUI/UserInterface/Views/DropZoneView.js:107
> +        if (this._activelyHandlingDrag)

Is this possible?  Should we assert against it?

> Source/WebInspectorUI/UserInterface/Views/DropZoneView.js:121
> +        if (!this._activelyHandlingDrag)

Ditto

> Source/WebInspectorUI/UserInterface/Views/DropZoneView.js:132
> +        if (!this._activelyHandlingDrag)

Ditto

> Source/WebInspectorUI/UserInterface/Views/DropZoneView.js:142
> +        if (!this._activelyHandlingDrag)

Ditto

> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:43
> +        if (WI.NetworkManager.supportsLocalResourceOverrides()) {

At this point, should we move this to `WI.ResourceContentView`?  It looks like the only other subclasses are `WI.FontResourceContentView` and `WI.GeneralResourceContentView` (which I think never actually gets created/used).

> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:52
> +                this._dropZoneView = new WI.DropZoneView(this);

Should we also allow drag/drop for text resource content?

> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:113
> +        imageContainer.appendChild(this._imageElement);

Can we merge this into the line that creates `this._imageElement`?

> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:116
> +

Style: unnecessary newline

> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:179
> +        fileReader.addEventListener("loadend", (event) => {

Should we make a `WI.FileUtilities.readDataURL`.

> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:189
> +            this._imageElement.src = dataURL;

NIT: I'm not actually sure if this matters, but since image decoding is async, perhaps we should move this above the other stuff.

> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:191
> +

Style: unnecessary newline

> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:126
> +        this.removeAllSubviews();

I think this is probably fine, given that this is always called only immediately after we get content (regardless of error).  Should we have any assertion about what the subviews are at this point, like that the only subview (if any) is the drop zone?

[Joseph Pecoraro]

Comment on attachment 379267 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=379267&action=review

>> Source/WebInspectorUI/UserInterface/Models/Resource.js:428
>>              return this._url;
> 
> NIT: I'd invert this and have the early-return be the `URL.createObjectURL(blobContent)`, so then the variable declaration is closer to us usage.

I don't buy the "variable declaration is closer" since this is effectively a bail. But I'll make the change and move the comment.

>> Source/WebInspectorUI/UserInterface/Models/SourceCode.js:32
>> +        this._originalRevision = new WI.SourceCodeRevision(this, null, false, null);
> 
> Can we make these into `const` variables, so it's clear at the callsite what they are?

I can just remove them, they were optional.

>> Source/WebInspectorUI/UserInterface/Models/SourceCodeRevision.js:62
>> +    updateRevisionContent(content, base64Encoded, mimeType, blobContent)
> 
> It seems like everything other than `content` is optional, so can we move them into an optional POD parameter?
> ```
>     updateRevisionContent(content, {base64Encoded, mimeType, blobContent} = {})
> ```

Done.

Aside: What do you mean by an "optional POD parameter"? Do you mean an "optional object parameter"?

>> Source/WebInspectorUI/UserInterface/Views/DropZoneView.js:81
>> +        console.assert(this._targetElement);
> 
> How does the `this._targetElement` relate to `this.element`?  Should it be a parent/child/sibling?
> 
> It seems like this class really should be more of a controller, not a view, as it really just listens for events on a target and adds/removes styles depending on them.

Indeed, DropZone is kind of both, hence the delegate. The DropZone view is displayed based on external conditions, but once appearing it has its own event handlers, over the content.

>> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:43
>> +        if (WI.NetworkManager.supportsLocalResourceOverrides()) {
> 
> At this point, should we move this to `WI.ResourceContentView`?  It looks like the only other subclasses are `WI.FontResourceContentView` and `WI.GeneralResourceContentView` (which I think never actually gets created/used).

Yeah, I'll test for Fonts and if they work I'll try moving it up. This was in my future plans but I can consider it now.

>> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:52
>> +                this._dropZoneView = new WI.DropZoneView(this);
> 
> Should we also allow drag/drop for text resource content?

My initial reaction was no. But that is not a strong opinion. Most text editors inline text content or a file path when a file is dropped, not replace contents. So I thought it was a bit weird for text content.

>> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:189
>> +            this._imageElement.src = dataURL;
> 
> NIT: I'm not actually sure if this matters, but since image decoding is async, perhaps we should move this above the other stuff.

Do you mean to encourage it to happen faster?

>> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:126
>> +        this.removeAllSubviews();
> 
> I think this is probably fine, given that this is always called only immediately after we get content (regardless of error).  Should we have any assertion about what the subviews are at this point, like that the only subview (if any) is the drop zone?

I did this just in case. If there were any subviews they wouldn't have been getting removed. I think I ran into that in earlier revisions, but kept this because it just makes sense.

[Devin Rousso]

Comment on attachment 379267 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=379267&action=review

>>> Source/WebInspectorUI/UserInterface/Models/SourceCodeRevision.js:62
>>> +    updateRevisionContent(content, base64Encoded, mimeType, blobContent)
>> 
>> It seems like everything other than `content` is optional, so can we move them into an optional POD parameter?
>> ```
>>     updateRevisionContent(content, {base64Encoded, mimeType, blobContent} = {})
>> ```
> 
> Done.
> 
> Aside: What do you mean by an "optional POD parameter"? Do you mean an "optional object parameter"?

POD as in "plain old data" like a JavaScript object, not like some special model object.  So yes, "optional object parameter".

>>> Source/WebInspectorUI/UserInterface/Views/DropZoneView.js:81
>>> +        console.assert(this._targetElement);
>> 
>> How does the `this._targetElement` relate to `this.element`?  Should it be a parent/child/sibling?
>> 
>> It seems like this class really should be more of a controller, not a view, as it really just listens for events on a target and adds/removes styles depending on them.
> 
> Indeed, DropZone is kind of both, hence the delegate. The DropZone view is displayed based on external conditions, but once appearing it has its own event handlers, over the content.

I suppose this is a bit like `WI.Popover`, so yeah I'm fine with it being a `WI.View` subclass.  My question was more about how `this._targetElement` relates to `this.element`, in that should we have some sort of assertion that they have the same parent or something like that.

>>> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:52
>>> +                this._dropZoneView = new WI.DropZoneView(this);
>> 
>> Should we also allow drag/drop for text resource content?
> 
> My initial reaction was no. But that is not a strong opinion. Most text editors inline text content or a file path when a file is dropped, not replace contents. So I thought it was a bit weird for text content.

Aaahhh good point!  I'm on the fence about that, especially since paths are pretty uncommon on the web, especially for local paths.  I'm leaning more towards allowing text drag/drop.

>>> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:189
>>> +            this._imageElement.src = dataURL;
>> 
>> NIT: I'm not actually sure if this matters, but since image decoding is async, perhaps we should move this above the other stuff.
> 
> Do you mean to encourage it to happen faster?

Yes, that's my thought process.  As I said, I'm not sure if it matters, but it couldn't hurt.

[Joseph Pecoraro]

Created attachment 380679 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 380681 [details]
[IMAGE] Light Mode

[Joseph Pecoraro]

Created attachment 380682 [details]
[IMAGE] Dark Mode

[Devin Rousso]

Comment on attachment 380679 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=380679&action=review

r=me, awesome work!

> LayoutTests/inspector/unit-tests/mimetype-utilities.html:13
> +            InspectorTest.expectEqual(WI.fileExtensionForFilename(null), null, `File extension for null filename should be null.`);

NIT: `InspectorTest.expectNull`

> Source/WebInspectorUI/UserInterface/Base/BlobUtilities.js:26
> +WI.BlobUtilities = class BlobUtilities {

NICE =D

> Source/WebInspectorUI/UserInterface/Base/BlobUtilities.js:39
> +        var byteCharacters = atob(base64Data);

Optional: while we're moving this code, why not update the `var` too =D

> Source/WebInspectorUI/UserInterface/Base/BlobUtilities.js:50
> +                bytes[i] = byteCharacters[offset].charCodeAt(0);

Optional: you could also use `offset - begin` instead of `i`

> Source/WebInspectorUI/UserInterface/Base/FileUtilities.js:174
> +    static async readDataURL(fileOrList, callback)

This doesn't appear to be used.

> Source/WebInspectorUI/UserInterface/Base/MIMETypeUtilities.js:39
> +}

Style: missing semicolon

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:-1702
> -function textToBlob(text, mimeType)

It looks like you missed:
 - http/tests/inspector/network/fetch-response-body.html
 - http/tests/inspector/network/xhr-response-body.html

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:-1707
> -function blobAsText(blob, callback)

Ditto

> Source/WebInspectorUI/UserInterface/Models/SourceCodeRevision.js:61
> +    updateRevisionContent(content, {base64Encoded, mimeType, blobContent} = {})

NIT: I'd probably call this `update` or `updateContent` (given that this class already has "revision" in the name), but I understand if you'd rather keep it this way for better code searching

> Source/WebInspectorUI/UserInterface/Views/DropZoneView.css:40
> +    text-align: center;

NIT: here's the order I'd use so the styles order from content inside/foreground to outside/background:
```
    font-size: 60px;
    text-align: center;
    color: white;
    text-shadow: 0 1px black;
    background-color: hsla(0, 0%, 50%, 0.5);
    border: 3px dashed hsla(0, 0%, 100%, 0.9);
    -webkit-backdrop-filter: blur(10px);
```

> Source/WebInspectorUI/UserInterface/Views/DropZoneView.css:51
> +        background-color: hsla(0, 0%, 30%, 0.5);

Ditto:
```
    color: hsl(0, 0%, 80%);
    text-shadow: 0 1px hsl(0, 0%, 20%);
    background-color: hsla(0, 0%, 30%, 0.5);
    border: 3px dashed hsla(0, 0%, 65%, 0.9);
```

> Source/WebInspectorUI/UserInterface/Views/DropZoneView.js:109
> +        if (this._activelyHandlingDrag)

We should assert that this view is attached to the DOM tree, so that this UI is shown.
```
    console.assert(this.isAttached);
```

> Source/WebInspectorUI/UserInterface/Views/FontResourceContentView.css:31
>  

might as well remove this too =D

> Source/WebInspectorUI/UserInterface/Views/FontResourceContentView.js:37
> +        if (this.showingLocalResourceOverride)
> +            this._dropZoneView = new WI.DropZoneView(this, {text: WI.UIString("Drop Font")});

NIT: should we do this inside an `initialLayout`?

> Source/WebInspectorUI/UserInterface/Views/FontResourceContentView.js:78
> +    {

I know this was just moved ,but please add `super.shown();`

> Source/WebInspectorUI/UserInterface/Views/FontResourceContentView.js:89
> +    }

I know this was just moved ,but please add `super.hidden();`

> Source/WebInspectorUI/UserInterface/Views/FontResourceContentView.js:99
> +    }

I know this was just moved ,but please add `super.closed();`

> Source/WebInspectorUI/UserInterface/Views/FontResourceContentView.js:130
> +            let dataURL = fileReader.result;

Should we inline this?

> Source/WebInspectorUI/UserInterface/Views/FontResourceContentView.js:181
> +        let lines = WI.FontResourceContentView.PreviewLines;

Ditto (this could also be `const`)

> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.css:31
> +

Can we remove these extra newlines?

> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:44
> +        if (this.showingLocalResourceOverride)
> +            this._dropZoneView = new WI.DropZoneView(this, {text: WI.UIString("Drop Image")});

Ditto (FontResourceContentView.js:36)

> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:52
> +

I'd personally remove these newlines, but I'm fine either way

> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:82
> +        this._draggingInternalImageElement = false;

Should we define this in the constructor instead?

> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:129
> +        // Do not appear if the drag is the current image inside this view.
> +        if (this._draggingInternalImageElement)

Good thinking!

> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:154
> +            let {base64, data, mimeType} = parseDataURL(dataURL);

Should we also attempt to derive the `mimeType` if `FileReader` doesn't give a MIME type for some reason, or do you think that that's unlikely?

> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:95
> +

Ditto (ImageResourceContentView.js:52)

> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:120
> +    {

We should add a `// Implemented by subclasses if needed.`

> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:121
> +        return undefined;

Can we use `null` instead?

> Source/WebInspectorUI/UserInterface/Views/TextResourceContentView.js:78
>  

Ditto (ImageResourceContentView.js:52)

[Joseph Pecoraro]

Comment on attachment 380679 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=380679&action=review

>> Source/WebInspectorUI/UserInterface/Base/FileUtilities.js:174
>> +    static async readDataURL(fileOrList, callback)
> 
> This doesn't appear to be used.

Oops, I meant to remove this. Yeah I decided against it.

>> Source/WebInspectorUI/UserInterface/Base/Utilities.js:-1702
>> -function textToBlob(text, mimeType)
> 
> It looks like you missed:
>  - http/tests/inspector/network/fetch-response-body.html
>  - http/tests/inspector/network/xhr-response-body.html

Thanks!

>> Source/WebInspectorUI/UserInterface/Models/SourceCodeRevision.js:61
>> +    updateRevisionContent(content, {base64Encoded, mimeType, blobContent} = {})
> 
> NIT: I'd probably call this `update` or `updateContent` (given that this class already has "revision" in the name), but I understand if you'd rather keep it this way for better code searching

Precisely, I like the unique name for code searching.

>> Source/WebInspectorUI/UserInterface/Views/DropZoneView.js:109
>> +        if (this._activelyHandlingDrag)
> 
> We should assert that this view is attached to the DOM tree, so that this UI is shown.
> ```
>     console.assert(this.isAttached);
> ```

Good idea.

>> Source/WebInspectorUI/UserInterface/Views/FontResourceContentView.js:37
>> +            this._dropZoneView = new WI.DropZoneView(this, {text: WI.UIString("Drop Font")});
> 
> NIT: should we do this inside an `initialLayout`?

I had the same thought. Done.

>> Source/WebInspectorUI/UserInterface/Views/FontResourceContentView.js:181
>> +        let lines = WI.FontResourceContentView.PreviewLines;
> 
> Ditto (this could also be `const`)

I just inlined it.

>> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:82
>> +        this._draggingInternalImageElement = false;
> 
> Should we define this in the constructor instead?

Done!

>> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:154
>> +            let {base64, data, mimeType} = parseDataURL(dataURL);
> 
> Should we also attempt to derive the `mimeType` if `FileReader` doesn't give a MIME type for some reason, or do you think that that's unlikely?

I like that idea.

>> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:121
>> +        return undefined;
> 
> Can we use `null` instead?

Unfortunately no, because createLocalResourceOverride looked for `undefined`. I will move this all to an empty object instead.

[Joseph Pecoraro]

Created attachment 380720 [details]
[PATCH] For Landing

[Joseph Pecoraro]

https://trac.webkit.org/r251024