Attachment 347862 Details for Bug 188823 – WIP patch

[patch] WIP patch

bug-188823-20180822154703.patch (text/plain), 12.62 KB, created by Wenson Hsieh on 2018-08-22 15:47:03 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Wenson Hsieh

Created: 2018-08-22 15:47:03 PDT

Size: 12.62 KB

patch

obsolete

>Subversion Revision: 235202
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 743ae7dde05eb2eec6c2eb85186b3a09b93f5b0b..b4ea773960726a5f9d2dc7bb278226aace4d2b5b 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,42 @@
>+2018-08-22  Wenson Hsieh  <wenson_hsieh@apple.com>
>+
>+        [Attachment Support] Attachment elements don't appear in drag images on macOS
>+        https://bugs.webkit.org/show_bug.cgi?id=188823
>+        <rdar://problem/43616378>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Currently, attachment elements don't show up in the drag image snapshot on macOS. This is because only the
>+        "Selection" phase is painted when generating a drag image on macOS, and many replaced renderers (with some
>+        exceptions, such as RenderImage) only paint visible content during the "Foreground" phase. To fix this, we
>+        override RenderAttachment::paintReplaced to paint the attachment in the case where the Selection phase is being
>+        painted.
>+
>+        Tests:  WKAttachmentTestsMac.DragAttachmentAsFilePromise
>+                WKAttachmentTests.MoveAttachmentElementAsIconByDragging
>+
>+        * rendering/RenderAttachment.cpp:
>+        (WebCore::RenderAttachment::paintReplaced):
>+        * rendering/RenderAttachment.h:
>+        * rendering/RenderThemeMac.mm:
>+        (WebCore::titleTextColorForAttachment):
>+        (WebCore::AttachmentLayout::layOutTitle):
>+
>+        Plumb an AttachmentLayoutStyle (i.e. NonSelected or Selected) to AttachmentLayout, and use this bit when
>+        determining the title text color, as well whether to paint backgrounds for the icon and title.
>+
>+        (WebCore::AttachmentLayout::AttachmentLayout):
>+        (WebCore::RenderThemeMac::attachmentIntrinsicSize const):
>+        (WebCore::RenderThemeMac::attachmentBaseline const):
>+        (WebCore::paintAttachmentIconBackground):
>+        (WebCore::paintAttachmentTitleBackground):
>+        (WebCore::RenderThemeMac::paintAttachment):
>+
>+        Rather than check the RenderAttachment's selection state When determining whether to paint with a non-selected
>+        or selected style, only use selected style if the RenderAttachment has a selection _and_ the painting phase is
>+        not "Selection". This sounds extremely counter-intuitive; but the "Selection" painting phase refers to painting
>+        the selected content _without_ including any part of the selection highlight itself.
>+
> 2018-08-22  Wenson Hsieh  <wenson_hsieh@apple.com>
> 
>         [Attachment Support] Support dragging attachment elements out as files on macOS
>diff --git a/Source/WebCore/rendering/RenderAttachment.cpp b/Source/WebCore/rendering/RenderAttachment.cpp
>index a69fc934d5b063759918cf6713640f387e10db19..ed3335e1374d8f7229252d54ce067cc3847681bd 100644
>--- a/Source/WebCore/rendering/RenderAttachment.cpp
>+++ b/Source/WebCore/rendering/RenderAttachment.cpp
>@@ -80,6 +80,18 @@ bool RenderAttachment::shouldDrawBorder() const
>     return m_shouldDrawBorder;
> }
> 
>+void RenderAttachment::paintReplaced(PaintInfo& paintInfo, const LayoutPoint& offset)
>+{
>+    if (paintInfo.phase != PaintPhase::Selection || !hasVisibleBoxDecorations() || !style().hasAppearance())
>+        return;
>+
>+    auto paintRect = borderBoxRect();
>+    paintRect.moveBy(offset);
>+
>+    ControlStates controlStates;
>+    theme().paint(*this, controlStates, paintInfo, paintRect);
>+}
>+
> } // namespace WebCore
> 
> #endif
>diff --git a/Source/WebCore/rendering/RenderAttachment.h b/Source/WebCore/rendering/RenderAttachment.h
>index 4caae03709d9ca4647e9806f0cc36f2779dd1a2a..d32922a60b25148f3ea1f2fc3011cf34f0a1bf20 100644
>--- a/Source/WebCore/rendering/RenderAttachment.h
>+++ b/Source/WebCore/rendering/RenderAttachment.h
>@@ -50,6 +50,7 @@ private:
>     const char* renderName() const override { return "RenderAttachment"; }
> 
>     bool shouldDrawSelectionTint() const override { return false; }
>+    void paintReplaced(PaintInfo&, const LayoutPoint& offset) final;
> 
>     void layout() override;
> 
>diff --git a/Source/WebCore/rendering/RenderThemeMac.mm b/Source/WebCore/rendering/RenderThemeMac.mm
>index b889dac4d4a1221cd57eeb0fd12bc55610c93b43..1009c21f8205976fd19d54785c1d18cdcb0862f9 100644
>--- a/Source/WebCore/rendering/RenderThemeMac.mm
>+++ b/Source/WebCore/rendering/RenderThemeMac.mm
>@@ -2544,8 +2544,10 @@ const CGFloat attachmentPlaceholderBorderDashLength = 6;
> 
> const CGFloat attachmentMargin = 3;
> 
>+enum class AttachmentLayoutStyle : uint8_t { NonSelected, Selected };
>+
> struct AttachmentLayout {
>-    explicit AttachmentLayout(const RenderAttachment&);
>+    explicit AttachmentLayout(const RenderAttachment&, AttachmentLayoutStyle);
> 
>     struct LabelLine {
>         FloatRect backgroundRect;
>@@ -2560,6 +2562,7 @@ struct AttachmentLayout {
>     FloatRect attachmentRect;
> 
>     int baseline;
>+    AttachmentLayoutStyle style;
> 
>     RetainPtr<CTLineRef> subtitleLine;
>     FloatRect subtitleTextRect;
>@@ -2571,11 +2574,11 @@ private:
>     void addTitleLine(CTLineRef, CGFloat& yOffset, Vector<CGPoint> origins, CFIndex lineIndex, const RenderAttachment&);
> };
> 
>-static Color titleTextColorForAttachment(const RenderAttachment& attachment)
>+static Color titleTextColorForAttachment(const RenderAttachment& attachment, AttachmentLayoutStyle style)
> {
>     Color result = Color::black;
>     
>-    if (attachment.selectionState() != RenderObject::SelectionNone) {
>+    if (style == AttachmentLayoutStyle::Selected) {
>         if (attachment.frame().selection().isFocusedAndActive())
>             result = colorFromNSColor([NSColor alternateSelectedControlTextColor]);
>         else
>@@ -2631,7 +2634,7 @@ void AttachmentLayout::layOutTitle(const RenderAttachment& attachment)
> 
>     NSDictionary *textAttributes = @{
>         (__bridge id)kCTFontAttributeName: (__bridge id)font.get(),
>-        (__bridge id)kCTForegroundColorAttributeName: (__bridge NSColor *)cachedCGColor(titleTextColorForAttachment(attachment))
>+        (__bridge id)kCTForegroundColorAttributeName: (__bridge NSColor *)cachedCGColor(titleTextColorForAttachment(attachment, style))
>     };
>     RetainPtr<NSAttributedString> attributedTitle = adoptNS([[NSAttributedString alloc] initWithString:title attributes:textAttributes]);
>     RetainPtr<CTFramesetterRef> titleFramesetter = adoptCF(CTFramesetterCreateWithAttributedString((CFAttributedStringRef)attributedTitle.get()));
>@@ -2709,7 +2712,8 @@ void AttachmentLayout::layOutSubtitle(const RenderAttachment& attachment)
>     subtitleTextRect = FloatRect(xOffset, yOffset, lineBounds.size.width, lineBounds.size.height);
> }
> 
>-AttachmentLayout::AttachmentLayout(const RenderAttachment& attachment)
>+AttachmentLayout::AttachmentLayout(const RenderAttachment& attachment, AttachmentLayoutStyle layoutStyle)
>+    : style(layoutStyle)
> {
>     layOutTitle(attachment);
>     layOutSubtitle(attachment);
>@@ -2734,18 +2738,21 @@ AttachmentLayout::AttachmentLayout(const RenderAttachment& attachment)
> 
> LayoutSize RenderThemeMac::attachmentIntrinsicSize(const RenderAttachment& attachment) const
> {
>-    AttachmentLayout layout(attachment);
>+    AttachmentLayout layout(attachment, AttachmentLayoutStyle::NonSelected);
>     return LayoutSize(layout.attachmentRect.size());
> }
> 
> int RenderThemeMac::attachmentBaseline(const RenderAttachment& attachment) const
> {
>-    AttachmentLayout layout(attachment);
>+    AttachmentLayout layout(attachment, AttachmentLayoutStyle::NonSelected);
>     return layout.baseline;
> }
> 
> static void paintAttachmentIconBackground(const RenderAttachment& attachment, GraphicsContext& context, AttachmentLayout& layout)
> {
>+    if (layout.style == AttachmentLayoutStyle::NonSelected)
>+        return;
>+
>     // FIXME: Finder has a discontinuous behavior here when you have a background color other than white,
>     // where it switches into 'bordered mode' and the border pops in on top of the background.
>     bool paintBorder = true;
>@@ -2834,6 +2841,9 @@ static void paintAttachmentIconPlaceholder(const RenderAttachment& attachment, G
> 
> static void paintAttachmentTitleBackground(const RenderAttachment& attachment, GraphicsContext& context, AttachmentLayout& layout)
> {
>+    if (layout.style == AttachmentLayoutStyle::NonSelected)
>+        return;
>+
>     if (layout.lines.isEmpty())
>         return;
> 
>@@ -2932,7 +2942,11 @@ bool RenderThemeMac::paintAttachment(const RenderObject& renderer, const PaintIn
> 
>     const RenderAttachment& attachment = downcast<RenderAttachment>(renderer);
> 
>-    AttachmentLayout layout(attachment);
>+    auto layoutStyle = AttachmentLayoutStyle::NonSelected;
>+    if (attachment.selectionState() != RenderObject::SelectionNone && paintInfo.phase != PaintPhase::Selection)
>+        layoutStyle = AttachmentLayoutStyle::Selected;
>+
>+    AttachmentLayout layout(attachment, layoutStyle);
> 
>     auto& progressString = attachment.attachmentElement().attributeWithoutSynchronization(progressAttr);
>     bool validProgress = false;
>@@ -2947,18 +2961,15 @@ bool RenderThemeMac::paintAttachment(const RenderObject& renderer, const PaintIn
>     context.translate(toFloatSize(paintRect.location()));
>     context.translate(floorSizeToDevicePixels(LayoutSize((paintRect.width() - attachmentIconBackgroundSize) / 2, 0), renderer.document().deviceScaleFactor()));
> 
>-    bool useSelectedStyle = attachment.selectionState() != RenderObject::SelectionNone;
>     bool usePlaceholder = validProgress && !progress;
> 
>-    if (useSelectedStyle)
>-        paintAttachmentIconBackground(attachment, context, layout);
>+    paintAttachmentIconBackground(attachment, context, layout);
>     if (usePlaceholder)
>         paintAttachmentIconPlaceholder(attachment, context, layout);
>     else
>         paintAttachmentIcon(attachment, context, layout);
> 
>-    if (useSelectedStyle)
>-        paintAttachmentTitleBackground(attachment, context, layout);
>+    paintAttachmentTitleBackground(attachment, context, layout);
>     paintAttachmentTitle(attachment, context, layout);
>     paintAttachmentSubtitle(attachment, context, layout);
> 
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index 9321499fe94ec25cc713796d6d62b68b11feb5ff..cdf4a1ce5e6d9078f4b33383186e826872c2f90f 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,18 @@
>+2018-08-22  Wenson Hsieh  <wenson_hsieh@apple.com>
>+
>+        [Attachment Support] Attachment elements don't appear in drag images on macOS
>+        https://bugs.webkit.org/show_bug.cgi?id=188823
>+        <rdar://problem/43616378>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Adjusts a couple of existing tests to additionally verify that the drag image generated when dragging an
>+        attachment element in macOS is not completely transparent.
>+
>+        * TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm:
>+        (isCompletelyTransparent):
>+        (TestWebKitAPI::TEST):
>+
> 2018-08-22  Wenson Hsieh  <wenson_hsieh@apple.com>
> 
>         [Attachment Support] Support dragging attachment elements out as files on macOS
>diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm
>index fb8d515f05883d7edc142dc60b47f7e006d2381f..8aaa1da6004fe0575e064bbdcf7180b63dd8afaa 100644
>--- a/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm
>+++ b/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm
>@@ -394,6 +394,22 @@ static NSData *testPDFData()
> 
> #pragma mark - Platform testing helper functions
> 
>+#if PLATFORM(MAC)
>+
>+BOOL isCompletelyTransparent(NSImage *image)
>+{
>+    auto representation = adoptNS([[NSBitmapImageRep alloc] initWithData:image.TIFFRepresentation]);
>+    for (int row = 0; row < image.size.height; ++row) {
>+        for (int column = 0; column < image.size.width; ++column) {
>+            if ([representation colorAtX:column y:row].alphaComponent)
>+                return false;
>+        }
>+    }
>+    return true;
>+}
>+
>+#endif
>+
> #if PLATFORM(IOS)
> 
> typedef void(^ItemProviderDataLoadHandler)(NSData *, NSError *);
>@@ -1112,6 +1128,9 @@ TEST(WKAttachmentTests, MoveAttachmentElementAsIconByDragging)
>     [attachment expectRequestedDataToBe:data.get()];
>     EXPECT_WK_STREQ("document.pdf", [webView valueOfAttribute:@"title" forQuerySelector:@"attachment"]);
>     EXPECT_WK_STREQ("application/pdf", [webView valueOfAttribute:@"type" forQuerySelector:@"attachment"]);
>+#if PLATFORM(MAC)
>+    EXPECT_FALSE(isCompletelyTransparent([simulator draggingInfo].draggedImage));
>+#endif
> 
>     [webView expectElementTag:@"STRONG" toComeBefore:@"ATTACHMENT"];
>     [simulator endDataTransfer];
>@@ -1210,6 +1229,7 @@ TEST(WKAttachmentTestsMac, DragAttachmentAsFilePromise)
>     NSArray<NSURL *> *urls = [simulator receivePromisedFiles];
>     EXPECT_EQ(1U, urls.count);
>     EXPECT_TRUE([[NSData dataWithContentsOfURL:urls.firstObject] isEqualToData:testPDFData()]);
>+    EXPECT_FALSE(isCompletelyTransparent([simulator draggingInfo].draggedImage));
> }
> 
> #endif // PLATFORM(MAC)

Actions: View | Formatted Diff | Diff

Attachments on bug 188823: 347862 | 347881