Skip to content

Attachment: Use image element to show icon/thumbnail#15228

Merged
webkit-commit-queue merged 1 commit intoWebKit:mainfrom
squelart:eng/attachment-inner-image-src-blob
Jun 29, 2023
Merged

Attachment: Use image element to show icon/thumbnail#15228
webkit-commit-queue merged 1 commit intoWebKit:mainfrom
squelart:eng/attachment-inner-image-src-blob

Conversation

@squelart
Copy link
Copy Markdown
Contributor

@squelart squelart commented Jun 23, 2023

b0e7ef7

Attachment: Use image element to show icon/thumbnail
https://bugs.webkit.org/show_bug.cgi?id=258437
rdar://105252742

Reviewed by Tim Nguyen.

Instead of using an inner attachment with legacy rendering, use a standard img element with a data URL
pointing at a blob that contains the image data.

Widespread changes:
- Removed m_innerLegacyAttachment and all related code.
- Removed image-only code.
- Consistently using "wide-layout" instead of "modern", and "narrow" instead of "legacy".

* LayoutTests/fast/attachment/mac/wide-attachment-image-controls-basic-expected.txt:
* LayoutTests/platform/ios-wk2/fast/attachment/cocoa/wide-attachment-rendering-expected.txt:
* LayoutTests/platform/mac-wk2/fast/attachment/cocoa/wide-attachment-rendering-expected.txt:
* Source/WebCore/editing/Editor.cpp:
(WebCore::Editor::didInsertAttachmentElement):
* Source/WebCore/html/HTMLAttachmentElement.cpp:
(WebCore::HTMLAttachmentElement::create):
(WebCore::HTMLAttachmentElement::didAddUserAgentShadowRoot):
(WebCore::attachmentPlaceholderIdentifier):
(WebCore::attachmentIconIdentifier):
(WebCore::HTMLAttachmentElement::ensureWideLayoutShadowTree):
(WebCore::HTMLAttachmentElement::updateProgress):
(WebCore::HTMLAttachmentElement::setFile):
(WebCore::HTMLAttachmentElement::attributeChanged):
(WebCore::HTMLAttachmentElement::attachmentActionForDisplay const):
(WebCore::HTMLAttachmentElement::attachmentTitleForDisplay const):
(WebCore::HTMLAttachmentElement::attachmentSubtitleForDisplay const):
(WebCore::HTMLAttachmentElement::updateAttributes):

(WebCore::HTMLAttachmentElement::updateImage):
This sets the img element with the correct "src" data contents (thumbnail or icon or nothing).

(WebCore::HTMLAttachmentElement::updateThumbnailForNarrowLayout):
(WebCore::HTMLAttachmentElement::updateThumbnailForWideLayout):
(WebCore::HTMLAttachmentElement::updateIconForNarrowLayout):
(WebCore::HTMLAttachmentElement::updateIconForWideLayout):
(WebCore::HTMLAttachmentElement::setNeedsWideLayoutIconRequest):
(WebCore::HTMLAttachmentElement::requestWideLayoutIconIfNeeded):
(WebCore::HTMLAttachmentElement::requestIconWithSize const):
(WebCore::attachmentPreviewIdentifier): Deleted.
(WebCore::HTMLAttachmentElement::ensureModernShadowTree): Deleted.
(WebCore::HTMLAttachmentElement::updateThumbnail): Deleted.
(WebCore::HTMLAttachmentElement::updateIcon): Deleted.
* Source/WebCore/html/HTMLAttachmentElement.h:
* Source/WebCore/html/shadow/attachmentElementShadow.css:
(div#attachment-preview-area):
(img#attachment-icon):
(attachment#attachment-preview): Deleted.
* Source/WebCore/rendering/AttachmentLayout.mm:
(WebCore::AttachmentLayout::AttachmentLayout):
* Source/WebCore/rendering/RenderAttachment.cpp:

(WebCore::RenderAttachment::paintWideLayoutAttachmentOnly const):
Call requestWideLayoutIconIfNeeded when painting the shadow tree.

* Source/WebCore/rendering/RenderThemeIOS.mm:
(WebCore::RenderThemeIOS::attachmentIntrinsicSize const):
(WebCore::RenderThemeIOS::paintAttachment):
* Source/WebCore/rendering/RenderThemeMac.mm:
(WebCore::RenderThemeMac::paintAttachment):

* Source/WebKit/WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::updateAttachmentThumbnail):
(WebKit::WebPage::updateAttachmentIcon):
Provide data blobs to wide-layout attachments.

Canonical link: https://commits.webkit.org/265615@main

36eb712

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@squelart squelart requested review from cdumez and rniwa as code owners June 23, 2023 05:46
@squelart squelart self-assigned this Jun 23, 2023
@squelart squelart added the HTML Editing For bugs in HTML editing support (including designMode and contentEditable). label Jun 23, 2023
@webkit-early-warning-system
Copy link
Copy Markdown
Collaborator

webkit-early-warning-system commented Jun 23, 2023

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 23, 2023
@squelart squelart removed the merging-blocked Applied to prevent a change from being merged label Jun 23, 2023
@squelart squelart force-pushed the eng/attachment-inner-image-src-blob branch from 5c00db6 to 0b16943 Compare June 23, 2023 06:10
@webkit-early-warning-system
Copy link
Copy Markdown
Collaborator

webkit-early-warning-system commented Jun 23, 2023

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 23, 2023
@nt1m
Copy link
Copy Markdown
Member

nt1m commented Jun 23, 2023

Thanks for doing this!

@squelart squelart removed the merging-blocked Applied to prevent a change from being merged label Jun 26, 2023
@squelart squelart force-pushed the eng/attachment-inner-image-src-blob branch from 0b16943 to 004d33d Compare June 26, 2023 04:18
@webkit-early-warning-system
Copy link
Copy Markdown
Collaborator

webkit-early-warning-system commented Jun 26, 2023

@squelart
Copy link
Copy Markdown
Contributor Author

Note: In the latest push I've updated the expected rendering test results to make them pass. I'm looking into replacing them with html-comparison tests, but it's not trivial as I'm using non-public system colors. If I can't solve this soon enough, I'll do it in a follow-up...

@squelart squelart force-pushed the eng/attachment-inner-image-src-blob branch from 004d33d to f67147a Compare June 26, 2023 05:26
@webkit-early-warning-system
Copy link
Copy Markdown
Collaborator

webkit-early-warning-system commented Jun 26, 2023

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 26, 2023
@squelart squelart removed the merging-blocked Applied to prevent a change from being merged label Jun 27, 2023
@squelart squelart force-pushed the eng/attachment-inner-image-src-blob branch from f67147a to d2aaaf6 Compare June 27, 2023 07:42
@webkit-early-warning-system
Copy link
Copy Markdown
Collaborator

webkit-early-warning-system commented Jun 27, 2023

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 27, 2023
@squelart squelart removed the merging-blocked Applied to prevent a change from being merged label Jun 27, 2023
@squelart squelart force-pushed the eng/attachment-inner-image-src-blob branch from d2aaaf6 to 5648781 Compare June 27, 2023 23:46
@webkit-early-warning-system
Copy link
Copy Markdown
Collaborator

webkit-early-warning-system commented Jun 27, 2023

@squelart squelart force-pushed the eng/attachment-inner-image-src-blob branch from 5648781 to a15fd8a Compare June 28, 2023 03:02
@webkit-early-warning-system
Copy link
Copy Markdown
Collaborator

webkit-early-warning-system commented Jun 28, 2023

@squelart squelart requested review from nt1m and pxlcoder June 28, 2023 06:37
@squelart squelart force-pushed the eng/attachment-inner-image-src-blob branch from a15fd8a to d4f7046 Compare June 29, 2023 05:06
Copy link
Copy Markdown
Member

@nt1m nt1m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine, I have naming nits for most part

@squelart squelart force-pushed the eng/attachment-inner-image-src-blob branch from d4f7046 to 36eb712 Compare June 29, 2023 05:33
@squelart squelart added the merge-queue Applied to send a pull request to merge-queue label Jun 29, 2023
https://bugs.webkit.org/show_bug.cgi?id=258437
rdar://105252742

Reviewed by Tim Nguyen.

Instead of using an inner attachment with legacy rendering, use a standard img element with a data URL
pointing at a blob that contains the image data.

Widespread changes:
- Removed m_innerLegacyAttachment and all related code.
- Removed image-only code.
- Consistently using "wide-layout" instead of "modern", and "narrow" instead of "legacy".

* LayoutTests/fast/attachment/mac/wide-attachment-image-controls-basic-expected.txt:
* LayoutTests/platform/ios-wk2/fast/attachment/cocoa/wide-attachment-rendering-expected.txt:
* LayoutTests/platform/mac-wk2/fast/attachment/cocoa/wide-attachment-rendering-expected.txt:
* Source/WebCore/editing/Editor.cpp:
(WebCore::Editor::didInsertAttachmentElement):
* Source/WebCore/html/HTMLAttachmentElement.cpp:
(WebCore::HTMLAttachmentElement::create):
(WebCore::HTMLAttachmentElement::didAddUserAgentShadowRoot):
(WebCore::attachmentPlaceholderIdentifier):
(WebCore::attachmentIconIdentifier):
(WebCore::HTMLAttachmentElement::ensureWideLayoutShadowTree):
(WebCore::HTMLAttachmentElement::updateProgress):
(WebCore::HTMLAttachmentElement::setFile):
(WebCore::HTMLAttachmentElement::attributeChanged):
(WebCore::HTMLAttachmentElement::attachmentActionForDisplay const):
(WebCore::HTMLAttachmentElement::attachmentTitleForDisplay const):
(WebCore::HTMLAttachmentElement::attachmentSubtitleForDisplay const):
(WebCore::HTMLAttachmentElement::updateAttributes):

(WebCore::HTMLAttachmentElement::updateImage):
This sets the img element with the correct "src" data contents (thumbnail or icon or nothing).

(WebCore::HTMLAttachmentElement::updateThumbnailForNarrowLayout):
(WebCore::HTMLAttachmentElement::updateThumbnailForWideLayout):
(WebCore::HTMLAttachmentElement::updateIconForNarrowLayout):
(WebCore::HTMLAttachmentElement::updateIconForWideLayout):
(WebCore::HTMLAttachmentElement::setNeedsWideLayoutIconRequest):
(WebCore::HTMLAttachmentElement::requestWideLayoutIconIfNeeded):
(WebCore::HTMLAttachmentElement::requestIconWithSize const):
(WebCore::attachmentPreviewIdentifier): Deleted.
(WebCore::HTMLAttachmentElement::ensureModernShadowTree): Deleted.
(WebCore::HTMLAttachmentElement::updateThumbnail): Deleted.
(WebCore::HTMLAttachmentElement::updateIcon): Deleted.
* Source/WebCore/html/HTMLAttachmentElement.h:
* Source/WebCore/html/shadow/attachmentElementShadow.css:
(div#attachment-preview-area):
(img#attachment-icon):
(attachment#attachment-preview): Deleted.
* Source/WebCore/rendering/AttachmentLayout.mm:
(WebCore::AttachmentLayout::AttachmentLayout):
* Source/WebCore/rendering/RenderAttachment.cpp:

(WebCore::RenderAttachment::paintWideLayoutAttachmentOnly const):
Call requestWideLayoutIconIfNeeded when painting the shadow tree.

* Source/WebCore/rendering/RenderThemeIOS.mm:
(WebCore::RenderThemeIOS::attachmentIntrinsicSize const):
(WebCore::RenderThemeIOS::paintAttachment):
* Source/WebCore/rendering/RenderThemeMac.mm:
(WebCore::RenderThemeMac::paintAttachment):

* Source/WebKit/WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::updateAttachmentThumbnail):
(WebKit::WebPage::updateAttachmentIcon):
Provide data blobs to wide-layout attachments.

Canonical link: https://commits.webkit.org/265615@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/attachment-inner-image-src-blob branch from 36eb712 to b0e7ef7 Compare June 29, 2023 11:05
@webkit-commit-queue
Copy link
Copy Markdown
Collaborator

Committed 265615@main (b0e7ef7): https://commits.webkit.org/265615@main

Reviewed commits have been landed. Closing PR #15228 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit b0e7ef7 into WebKit:main Jun 29, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jun 29, 2023
webkit-commit-queue pushed a commit to squelart/WebKit that referenced this pull request Jun 29, 2023
https://bugs.webkit.org/show_bug.cgi?id=258437
rdar://105252742

Reviewed by Tim Nguyen.

Follow-up to WebKit#15228, where I forgot to remove a comment
related to a previous version of the patch.

* Source/WebCore/html/shadow/attachmentElementShadow.css:
(img#attachment-icon):

Canonical link: https://commits.webkit.org/265629@main
mnutt pushed a commit to movableink/webkit that referenced this pull request Jul 18, 2023
https://bugs.webkit.org/show_bug.cgi?id=258437
rdar://105252742

Reviewed by Tim Nguyen.

Follow-up to WebKit/WebKit#15228, where I forgot to remove a comment
related to a previous version of the patch.

* Source/WebCore/html/shadow/attachmentElementShadow.css:
(img#attachment-icon):

Canonical link: https://commits.webkit.org/265629@main
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

HTML Editing For bugs in HTML editing support (including designMode and contentEditable).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants