Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
6 years, 8 months ago by fs Modified:
6 years, 8 months ago Reviewers:
pdr., Erik Dahlström (inactive), f(malita), Stephen Chennney, rwlbuis CC:
blink-reviews, krit, bemjb+rendering_chromium.org, dsinclair, zoltan1, eae+blinkwatch, leviw+renderwatch, kouhei+svg_chromium.org, ed+blinkwatch_opera.com, f(malita), gyuyoung.kim_webkit.org, jchaffraix+rendering, pdr., rwlbuis, Stephen Chennney, rune+blink Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionApply SVG paint-order to text-decorations
BUG=360571
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=171070
Patch Set 1 #
Total comments: 5
Created: 6 years, 8 months ago
Messages
Total messages: 15 (0 generated)
Seems the F2F is just too much fun...
LGTM. I presume the spec says that text decoration is still painted after text, or something to that effect. Maybe add a comment in the CL indicating any related spec. The alternative, that the decoration is considered an integral part of the text, would seem to make things a lot more complex for us.
LGTM with a nit and a suggestion https://codereview.chromium.org/225023029/diff/1/LayoutTests/svg/paintorder/p... File LayoutTests/svg/paintorder/paintorder-text-decorations.svg (right): https://codereview.chromium.org/225023029/diff/1/LayoutTests/svg/paintorder/p... LayoutTests/svg/paintorder/paintorder-text-decorations.svg:3: <marker id="m" refX="5" refY="5" viewBox="0 0 10 10" overflow="visible"> Nit: these markers just make the test more confusing since they aren't supported. https://codereview.chromium.org/225023029/diff/1/Source/core/rendering/svg/SV... File Source/core/rendering/svg/SVGInlineTextBox.cpp (right): https://codereview.chromium.org/225023029/diff/1/Source/core/rendering/svg/SV... Source/core/rendering/svg/SVGInlineTextBox.cpp:546: for (int i = 0; i < 3; i++) { Question: can we share this with the loop above (in SVGInlineTextBox::paint)?
https://codereview.chromium.org/225023029/diff/1/LayoutTests/svg/paintorder/p... File LayoutTests/svg/paintorder/paintorder-text-decorations.svg (right): https://codereview.chromium.org/225023029/diff/1/LayoutTests/svg/paintorder/p... LayoutTests/svg/paintorder/paintorder-text-decorations.svg:3: <marker id="m" refX="5" refY="5" viewBox="0 0 10 10" overflow="visible"> On 2014/04/08 14:20:57, pdr wrote: > Nit: these markers just make the test more confusing since they aren't > supported. I considered this but thought they should stay to test that we do not have problems when they are defined. i.e. if someone modifies the switch or adds new paint types to the switch.
On 2014/04/08 14:14:31, Stephen Chenney wrote: > LGTM. I presume the spec says that text decoration is still painted after text, > or something to that effect. Maybe add a comment in the CL indicating any > related spec. > > The alternative, that the decoration is considered an integral part of the text, > would seem to make things a lot more complex for us. AFAICR the spec says that overline and underline is painted before the glyphs, while strike-through is painted after. (Painting the decorations as an integral part would indeed make it more difficult - and if you want that, then using a clip/mask instead is not too bad a workaround.) I'll look up the spec. section and reference it.
On 2014/04/08 15:39:00, fs wrote: > On 2014/04/08 14:14:31, Stephen Chenney wrote: > > LGTM. I presume the spec says that text decoration is still painted after > text, > > or something to that effect. Maybe add a comment in the CL indicating any > > related spec. > > > > The alternative, that the decoration is considered an integral part of the > text, > > would seem to make things a lot more complex for us. > > AFAICR the spec says that overline and underline is painted before the glyphs, > while strike-through is painted after. (Painting the decorations as an integral > part would indeed make it more difficult - and if you want that, then using a > clip/mask instead is not too bad a workaround.) I'll look up the spec. section > and reference it. I see that SVGInlineTextBox::paint() already has a "Spec: ..." not to that effect. IMHO opinion it feels more suitable there (where the order of painting the decorations/text is). Maybe the paintDecoration() method could/should move closer to paint() though.
https://codereview.chromium.org/225023029/diff/1/LayoutTests/svg/paintorder/p... File LayoutTests/svg/paintorder/paintorder-text-decorations.svg (right): https://codereview.chromium.org/225023029/diff/1/LayoutTests/svg/paintorder/p... LayoutTests/svg/paintorder/paintorder-text-decorations.svg:3: <marker id="m" refX="5" refY="5" viewBox="0 0 10 10" overflow="visible"> On 2014/04/08 15:06:10, Stephen Chenney wrote: > On 2014/04/08 14:20:57, pdr wrote: > > Nit: these markers just make the test more confusing since they aren't > > supported. > > I considered this but thought they should stay to test that we do not have > problems when they are defined. i.e. if someone modifies the switch or adds new > paint types to the switch. Yes, that was my thinking - although I didn't think too much, but rather mostly just copied the paintorder-text.svg TC - so I blame ed for doing any thinking. It did seem reasonable to have a marker just in case (the unlikely) disaster strikes though (i.e, "what schenney said"). https://codereview.chromium.org/225023029/diff/1/Source/core/rendering/svg/SV... File Source/core/rendering/svg/SVGInlineTextBox.cpp (right): https://codereview.chromium.org/225023029/diff/1/Source/core/rendering/svg/SV... Source/core/rendering/svg/SVGInlineTextBox.cpp:546: for (int i = 0; i < 3; i++) { On 2014/04/08 14:20:57, pdr wrote: > Question: can we share this with the loop above (in SVGInlineTextBox::paint)? No, not as long as the stroke+fill for the decorations (under/overline) should be painted separately. (I.e. "fill deco.", "stroke deco.", "fill glyphs", "stroke glyphs" vs. "fill deco.", "fill glyphs", "stroke deco.", "stroke glyphs")
The CQ bit was checked by fs@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fs@opera.com/225023029/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on win_blink_compile_dbg
The CQ bit was checked by fs@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fs@opera.com/225023029/1
Message was sent while issue was closed.
Change committed as 171070 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
