Bug 48539 - Support the text-emphasis CSS property
Summary: Support the text-emphasis CSS property
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: mitz
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-10-28 11:47 PDT by Dave Hyatt
Modified: 2010-12-17 13:38 PST (History)
10 users (show)

See Also:


Attachments
Part 1: CSS support (36.89 KB, patch)
2010-12-02 12:58 PST, mitz
no flags Details | Formatted Diff | Diff
Part 2: Platform, layout and rendering (WIP, do not review) (49.51 KB, patch)
2010-12-12 13:30 PST, mitz
no flags Details | Formatted Diff | Diff
Part 2: Platform, layout and rendering (WIP, do not review) (52.06 KB, patch)
2010-12-12 14:04 PST, mitz
no flags Details | Formatted Diff | Diff
Part 2: Platform, layout and rendering (WIP, do not review) (68.64 KB, patch)
2010-12-12 15:06 PST, mitz
no flags Details | Formatted Diff | Diff
Part 2: Platform, layout and rendering (WIP, do not review) (68.97 KB, patch)
2010-12-12 15:08 PST, mitz
no flags Details | Formatted Diff | Diff
Part 2: Platform, layout and rendering (WIP, do not review) (81.55 KB, patch)
2010-12-12 18:56 PST, mitz
no flags Details | Formatted Diff | Diff
Part 2: Platform, layout and rendering (WIP, do not review) (82.66 KB, patch)
2010-12-13 14:25 PST, mitz
no flags Details | Formatted Diff | Diff
Part 2: Platform, layout and rendering (WIP, do not review) (83.50 KB, patch)
2010-12-13 18:09 PST, mitz
no flags Details | Formatted Diff | Diff
Platform, layout and rendering (WIP, do not review) (83.52 KB, patch)
2010-12-13 18:38 PST, mitz
no flags Details | Formatted Diff | Diff
Part 2: Additional RenderStyle support (3.93 KB, patch)
2010-12-14 19:21 PST, mitz
no flags Details | Formatted Diff | Diff
Part 2: Additional RenderStyle support (5.65 KB, patch)
2010-12-14 19:28 PST, mitz
no flags Details | Formatted Diff | Diff
Part 3: Platform support (82.43 KB, patch)
2010-12-15 16:57 PST, mitz
no flags Details | Formatted Diff | Diff
Part 4: Layout and rendering (294.00 KB, patch)
2010-12-16 13:50 PST, mitz
no flags Details | Formatted Diff | Diff
Part 4: Layout and rendering (306.56 KB, patch)
2010-12-17 00:11 PST, mitz
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Hyatt 2010-10-28 11:47:53 PDT
This is a master bug for text-emphasis support (both back end and front end).
Comment 1 mitz 2010-12-02 12:58:53 PST
Created attachment 75408 [details]
Part 1: CSS support
Comment 2 Dave Hyatt 2010-12-02 13:08:54 PST
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).
Comment 3 Dave Hyatt 2010-12-02 13:09:22 PST
Actually only changing the color is just a repaint hint I suppose, so just do the right thing with each property. :)
Comment 4 Darin Adler 2010-12-02 13:14:45 PST
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 5 mitz 2010-12-02 16:37:58 PST
Comment on attachment 75408 [details]
Part 1: CSS support

<http://trac.webkit.org/projects/webkit/changeset/73219>
Comment 6 mitz 2010-12-12 13:30:12 PST
Created attachment 76333 [details]
Part 2: Platform, layout and rendering (WIP, do not review)

Uploading this primarily for the EWS to look at.
Comment 7 Early Warning System Bot 2010-12-12 13:48:10 PST
Attachment 76333 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6912088
Comment 8 WebKit Review Bot 2010-12-12 13:49:35 PST
Attachment 76333 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6898081
Comment 9 mitz 2010-12-12 14:04:28 PST
Created attachment 76334 [details]
Part 2: Platform, layout and rendering (WIP, do not review)
Comment 10 Build Bot 2010-12-12 14:23:43 PST
Attachment 76333 [details] did not build on win:
Build output: http://queues.webkit.org/results/6982088
Comment 11 Early Warning System Bot 2010-12-12 14:29:41 PST
Attachment 76334 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6934089
Comment 12 WebKit Review Bot 2010-12-12 14:34:38 PST
Attachment 76334 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6967083
Comment 13 Build Bot 2010-12-12 14:42:45 PST
Attachment 76334 [details] did not build on win:
Build output: http://queues.webkit.org/results/6990072
Comment 14 mitz 2010-12-12 15:06:07 PST
Created attachment 76336 [details]
Part 2: Platform, layout and rendering (WIP, do not review)
Comment 15 mitz 2010-12-12 15:08:22 PST
Created attachment 76337 [details]
Part 2: Platform, layout and rendering (WIP, do not review)
Comment 16 mitz 2010-12-12 15:10:06 PST
I can probably move the implementation of smallCapsFontData() and emphasisMarkFontData() to cross-platform code.
Comment 17 WebKit Review Bot 2010-12-12 15:28:57 PST
Attachment 76336 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6909083
Comment 18 Early Warning System Bot 2010-12-12 15:29:19 PST
Attachment 76336 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6958074
Comment 19 WebKit Review Bot 2010-12-12 15:44:21 PST
Attachment 76337 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7031034
Comment 20 Build Bot 2010-12-12 15:48:56 PST
Attachment 76336 [details] did not build on win:
Build output: http://queues.webkit.org/results/6996081
Comment 21 Early Warning System Bot 2010-12-12 15:50:40 PST
Attachment 76337 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7032036
Comment 22 Build Bot 2010-12-12 16:17:32 PST
Attachment 76337 [details] did not build on win:
Build output: http://queues.webkit.org/results/7019077
Comment 23 Build Bot 2010-12-12 16:56:16 PST
Attachment 76337 [details] did not build on win:
Build output: http://queues.webkit.org/results/6907085
Comment 24 mitz 2010-12-12 18:56:45 PST
Created attachment 76345 [details]
Part 2: Platform, layout and rendering (WIP, do not review)
Comment 25 Early Warning System Bot 2010-12-12 19:23:23 PST
Attachment 76345 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6969087
Comment 26 mitz 2010-12-13 14:25:10 PST
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.
Comment 27 Early Warning System Bot 2010-12-13 17:53:10 PST
Attachment 76439 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6963114
Comment 28 mitz 2010-12-13 18:09:19 PST
Created attachment 76477 [details]
Part 2: Platform, layout and rendering (WIP, do not review)
Comment 29 mitz 2010-12-13 18:38:03 PST
Created attachment 76481 [details]
Platform, layout and rendering (WIP, do not review)
Comment 30 mitz 2010-12-14 19:21:14 PST
Created attachment 76617 [details]
Part 2: Additional RenderStyle support

Another small piece, ready for review.
Comment 31 mitz 2010-12-14 19:28:18 PST
Created attachment 76619 [details]
Part 2: Additional RenderStyle support
Comment 32 mitz 2010-12-14 19:33:52 PST
<rdar://problem/7720300>
Comment 33 mitz 2010-12-14 19:40:42 PST
Comment on attachment 76619 [details]
Part 2: Additional RenderStyle support

<http://trac.webkit.org/changeset/74095>
Comment 34 mitz 2010-12-15 16:57:24 PST
Created attachment 76712 [details]
Part 3: Platform support
Comment 35 Darin Adler 2010-12-15 17:59:24 PST
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 36 mitz 2010-12-15 18:24:29 PST
Comment on attachment 76712 [details]
Part 3: Platform support

<http://trac.webkit.org/projects/webkit/changeset/74169>
Comment 37 mitz 2010-12-16 13:50:51 PST
Created attachment 76813 [details]
Part 4: Layout and rendering

I am going to file a follow-up bug for the interaction with ruby
Comment 38 mitz 2010-12-16 14:25:53 PST
Comment on attachment 76813 [details]
Part 4: Layout and rendering

Hyatt suggested that I try do this the right way instead :)
Comment 39 WebKit Review Bot 2010-12-16 15:37:42 PST
http://trac.webkit.org/changeset/74169 might have broken Leopard Intel Debug (Tests)
Comment 40 mitz 2010-12-17 00:11:30 PST
Created attachment 76852 [details]
Part 4: Layout and rendering
Comment 41 Dave Hyatt 2010-12-17 11:29:05 PST
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.
Comment 42 mitz 2010-12-17 11:33:20 PST
Fixed in <http://trac.webkit.org/changeset/74281>
Comment 43 Jessie Berlin 2010-12-17 12:53:12 PST
Are platform-specific tests required on Windows?

http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r74283%20(7526)/results.html
Comment 44 mitz 2010-12-17 12:55:10 PST
(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
Comment 45 Jessie Berlin 2010-12-17 12:57:40 PST
(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?
Comment 46 Jessie Berlin 2010-12-17 13:38:29 PST
Landed Windows-specific results in
http://trac.webkit.org/changeset/74290