Bug 48539 – Support the text-emphasis CSS property

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Dave Hyatt]

This is a master bug for text-emphasis support (both back end and front end).

Created attachment 75408 [details]
Part 1: CSS support

[Dave Hyatt]

Comment on attachment 75408 [details]
Part 1: CSS support

r=me, but can you go ahead and add a RenderStyle::diff hint as well?  I think it's going to end up being a layout hint (because of possible changes to overflow propagation, e.g., since emphasis marks can go above Rubies).

[Dave Hyatt]

Actually only changing the color is just a repaint hint I suppose, so just do the right thing with each property. :)

[Darin Adler]

Comment on attachment 75408 [details]
Part 1: CSS support

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

> WebCore/css/CSSComputedStyleDeclaration.cpp:1237
> +            }
> +            }

I think it would be slightly better to explicitly list all the cases so we get a warning if we ever add a new case. If you want to put the default code outside the switch and use "break" we’d get everything except for the runtime check for a bad enum value, which I think is a good tradeoff.

> WebCore/css/CSSParser.cpp:5147
> +    if (fill && shape) {
> +        RefPtr<CSSValueList> parsedValues = CSSValueList::createSpaceSeparated();
> +        parsedValues->append(fill.release());
> +        parsedValues->append(shape.release());
> +        addProperty(CSSPropertyWebkitTextEmphasisStyle, parsedValues.release(), important);
> +    } else if (fill)
> +        addProperty(CSSPropertyWebkitTextEmphasisStyle, fill.release(), important);
> +    else if (shape)
> +        addProperty(CSSPropertyWebkitTextEmphasisStyle, shape.release(), important);
> +    else
> +        return false;
> +
> +    return true;

I think this would read better if the return true was inside each if and we thus did not need all those elses.

Comment on attachment 75408 [details]
Part 1: CSS support

<http://trac.webkit.org/projects/webkit/changeset/73219>

Created attachment 76333 [details]
Part 2: Platform, layout and rendering (WIP, do not review)

Uploading this primarily for the EWS to look at.

[Early Warning System Bot]

Attachment 76333 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6912088

[WebKit Review Bot]

Attachment 76333 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6898081

Created attachment 76334 [details]
Part 2: Platform, layout and rendering (WIP, do not review)

[Build Bot]

Attachment 76333 [details] did not build on win:
Build output: http://queues.webkit.org/results/6982088

[Early Warning System Bot]

Attachment 76334 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6934089

[WebKit Review Bot]

Attachment 76334 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6967083

[Build Bot]

Attachment 76334 [details] did not build on win:
Build output: http://queues.webkit.org/results/6990072

Created attachment 76336 [details]
Part 2: Platform, layout and rendering (WIP, do not review)

Created attachment 76337 [details]
Part 2: Platform, layout and rendering (WIP, do not review)

I can probably move the implementation of smallCapsFontData() and emphasisMarkFontData() to cross-platform code.

[WebKit Review Bot]

Attachment 76336 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6909083

[Early Warning System Bot]

Attachment 76336 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6958074

[WebKit Review Bot]

Attachment 76337 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7031034

[Build Bot]

Attachment 76336 [details] did not build on win:
Build output: http://queues.webkit.org/results/6996081

[Early Warning System Bot]

Attachment 76337 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7032036

[Build Bot]

Attachment 76337 [details] did not build on win:
Build output: http://queues.webkit.org/results/7019077

[Build Bot]

Attachment 76337 [details] did not build on win:
Build output: http://queues.webkit.org/results/6907085

Created attachment 76345 [details]
Part 2: Platform, layout and rendering (WIP, do not review)

[Early Warning System Bot]

Attachment 76345 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6969087

Created attachment 76439 [details]
Part 2: Platform, layout and rendering (WIP, do not review)

Trying to appease Qt, correcting emphasis mark positioning in vertical fonts.

[Early Warning System Bot]

Attachment 76439 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6963114

Created attachment 76477 [details]
Part 2: Platform, layout and rendering (WIP, do not review)

Created attachment 76481 [details]
Platform, layout and rendering (WIP, do not review)

Created attachment 76617 [details]
Part 2: Additional RenderStyle support

Another small piece, ready for review.

Created attachment 76619 [details]
Part 2: Additional RenderStyle support

<rdar://problem/7720300>

Comment on attachment 76619 [details]
Part 2: Additional RenderStyle support

<http://trac.webkit.org/changeset/74095>

Created attachment 76712 [details]
Part 3: Platform support

[Darin Adler]

Comment on attachment 76712 [details]
Part 3: Platform support

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

> WebCore/platform/graphics/FontFastPath.cpp:200
> +// FIXME: This method is not complex-text aware.

Anthropomorphizing the function and saying it’s not aware of complex text doesn’t make clear to me what the problem is here. Is it incorrect in some way? For what cases? I think you can probably write a slightly different comment that will be less ambiguous.

Also, I’d prefer you use the C++ term function rather than the term method.

> WebCore/platform/graphics/SimpleFontData.cpp:200
> +        m_derivedFontData = new DerivedFontData(isCustomFont());

Can I talk you into future-proofing this code by calling adoptPtr in all the call sites like this one? In a related question, should we use the create function idiom for this struct?

> WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:85
> +    return new SimpleFontData(
> +        FontPlatformData(cairo_scaled_font_get_font_face(m_platformData.scaledFont()),

I would have made that first argument be on the first line because of my distaste for breaking right after an open paren. I think the line length ends up being not too horrible.

> WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:96
> +        m_derivedFontData->smallCaps = scaledFontData(fontDescription, .7);

We should really name that 0.7 constant at some point. I know it wasn’t named before.

> WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:106
> +        m_derivedFontData->emphasisMark = scaledFontData(fontDescription, .5);

And this 0.5 constant.

Comment on attachment 76712 [details]
Part 3: Platform support

<http://trac.webkit.org/projects/webkit/changeset/74169>

Created attachment 76813 [details]
Part 4: Layout and rendering

I am going to file a follow-up bug for the interaction with ruby

Comment on attachment 76813 [details]
Part 4: Layout and rendering

Hyatt suggested that I try do this the right way instead :)

[WebKit Review Bot]

http://trac.webkit.org/changeset/74169 might have broken Leopard Intel Debug (Tests)

Created attachment 76852 [details]
Part 4: Layout and rendering

[Dave Hyatt]

Comment on attachment 76852 [details]
Part 4: Layout and rendering

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

r=me!

> WebCore/rendering/InlineFlowBox.cpp:754
> +    if (style->textEmphasisMark() != TextEmphasisMarkNone) {
> +        int emphasisMarkHeight = style->font().emphasisMarkHeight(style->textEmphasisMarkString());
> +        if (style->textEmphasisPosition() == TextEmphasisPositionOver)
> +            topGlyphOverflow = min(topGlyphOverflow, -emphasisMarkHeight);
> +        else
> +            bottomGlyphOverflow = max(bottomGlyphOverflow, emphasisMarkHeight);
> +    }

It would be great to have a repaint test that tests the overflow repainting.

Fixed in <http://trac.webkit.org/changeset/74281>

[Jessie Berlin]

Are platform-specific tests required on Windows?

http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r74283%20(7526)/results.html

(In reply to comment #43)
> Are platform-specific tests required on Windows?
> 
> http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r74283%20(7526)/results.html

Yes

[Jessie Berlin]

(In reply to comment #44)
> (In reply to comment #43)
> > Are platform-specific tests required on Windows?
> > 
> > http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r74283%20(7526)/results.html
> 
> Yes

Are you adding them or should I?

[Jessie Berlin]

Landed Windows-specific results in
http://trac.webkit.org/changeset/74290