Bug 142214 – <attachment> label can get very wide, doesn't wrap/truncate

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tim Horton]

<attachment> label can get very wide, doesn't wrap/truncate

[Tim Horton]

Created attachment 247756 [details]
WIP

[Tim Horton]

rdar://problem/19982499

[Tim Horton]

This isn't quite done, so no r? yet. Needs tests and some comment cleanup (and a sanity check).

[WebKit Commit Bot]

Attachment 247756 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 247756 [details]
WIP

Attachment 247756 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5027709713383424

New failing tests:
fast/attachment/attachment-rendering.html

[Build Bot]

Created attachment 247758 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 247756 [details]
WIP

Attachment 247756 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6052353350303744

New failing tests:
fast/attachment/attachment-rendering.html

[Build Bot]

Created attachment 247759 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Tim Horton]

Created attachment 247774 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 247774 [details]
Patch

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

> Source/WebCore/rendering/RenderThemeMac.mm:2163
> +    lines.append(labelLine);

This confused me, not being m_lines. Maybe it's time to call this thing a class.

> Source/WebCore/rendering/RenderThemeMac.mm:2175
> +    NSDictionary *textAttributes = @{
> +        (id)kCTFontAttributeName: (id)labelFont.get(),
> +        (id)kCTForegroundColorAttributeName: labelTextColorForAttachment(attachment)
> +    };

Does this work on all platforms we care about?

> Source/WebCore/rendering/RenderThemeMac.mm:2213
> +    CFRange fitRange;
> +    CGSize labelTextSize = CTFramesetterSuggestFrameSizeWithConstraints(labelFramesetter.get(), CFRangeMake(0, 0), nullptr, CGSizeMake(attachmentLabelMaximumWidth, CGFLOAT_MAX), &fitRange);
> +
> +    RetainPtr<CGPathRef> labelPath = adoptCF(CGPathCreateWithRect(CGRectMake(0, 0, labelTextSize.width, labelTextSize.height), nullptr));
> +    RetainPtr<CTFrameRef> labelFrame = adoptCF(CTFramesetterCreateFrame(labelFramesetter.get(), fitRange, labelPath.get(), nullptr));
> +
> +    CFArrayRef ctLines = CTFrameGetLines(labelFrame.get());
> +    CFIndex lineCount = CFArrayGetCount(ctLines);
> +    if (!lineCount)
> +        return;
> +
> +    Vector<CGPoint> origins(lineCount);
> +    CTFrameGetLineOrigins(labelFrame.get(), CFRangeMake(0, 0), origins.data());
> +
> +    // Lay out and record the first (attachmentLabelMaximumLineCount - 1) lines.
> +    CFIndex lineIndex = 0;
> +    CGFloat yOffset = attachmentIconBackgroundSize + attachmentIconToLabelMargin;
> +    for (; lineIndex < std::min(attachmentLabelMaximumLineCount - 1, lineCount); ++lineIndex) {
> +        CTLineRef line = (CTLineRef)CFArrayGetValueAtIndex(ctLines, lineIndex);
> +        addLine(line, yOffset, origins, lineIndex, attachment);
> +    }
> +
> +    if (lineIndex == lineCount)
> +        return;
> +
> +    // We had text that didn't fit in the first (attachmentLabelMaximumLineCount - 1) lines.
> +    // Combine it into one last line, and center-truncate it.
> +    CTLineRef firstRemainingLine = (CTLineRef)CFArrayGetValueAtIndex(ctLines, lineIndex);
> +    CFIndex remainingRangeStart = CTLineGetStringRange(firstRemainingLine).location;
> +    NSRange remainingRange = NSMakeRange(remainingRangeStart, [attributedFilename length] - remainingRangeStart);
> +    NSAttributedString *remainingString = [attributedFilename attributedSubstringFromRange:remainingRange];
> +    RetainPtr<CTLineRef> remainingLine = adoptCF(CTLineCreateWithAttributedString((CFAttributedStringRef)remainingString));
> +    RetainPtr<NSAttributedString> ellipsisString = adoptNS([[NSAttributedString alloc] initWithString:@"\u2026" attributes:textAttributes]);
> +    RetainPtr<CTLineRef> ellipsisLine = adoptCF(CTLineCreateWithAttributedString((CFAttributedStringRef)ellipsisString.get()));
> +    RetainPtr<CTLineRef> truncatedLine = adoptCF(CTLineCreateTruncatedLine(remainingLine.get(), attachmentLabelMaximumWidth, kCTLineTruncationMiddle, ellipsisLine.get()));

So sad that we're not using layout.

[Tim Horton]

(In reply to comment #10)
> Comment on attachment 247774 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247774&action=review
> 
> > Source/WebCore/rendering/RenderThemeMac.mm:2163
> > +    lines.append(labelLine);
> 
> This confused me, not being m_lines. Maybe it's time to call this thing a
> class.

Seems reasonable, I'll do that in a separate patch.

> > Source/WebCore/rendering/RenderThemeMac.mm:2175
> > +    NSDictionary *textAttributes = @{
> > +        (id)kCTFontAttributeName: (id)labelFont.get(),
> > +        (id)kCTForegroundColorAttributeName: labelTextColorForAttachment(attachment)
> > +    };
> 
> Does this work on all platforms we care about?

The dictionary literal, or CoreText?

> So sad that we're not using layout.

We should figure out how to make these things possible!

[Tim Horton]

http://trac.webkit.org/changeset/180950