1094247 - Overhaul the way that we provide and propagate gfxTextContextPaint objects

Status: NEW

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

The way that we provide and propagate gfxTextContextPaint objects is currently pretty busted and buggy.

* Since we only use gfxTextContextPaint objects during paint time,
  we can fail to correctly calculate overflow areas for
  |stroke-width:context-value;|, for example.

* Contexts can be nested. For example we can have a <use> element
  (which is a context element) that uses content that contains
  text that uses a font with SVG-in-OpenType glyphs. We make a
  halfhearted effort to take account of this in SVGTextFrame, but
  it's incomplete and we don't do it anywhere else.

Context properties may need to be obtained during reflow, overflow updates or painting.

One complication to obtaining these properties during ReflowSVG and UpdateOverflow is that these methods do not take any arguments that a context object could be added to. I don't think for this uncommon use case we should be adding one either.

One place that a stack of context objects might be built up is on the document, where we have a function called something like SetTemporaryStackBasedSVGPropertyContext that pushes a context every time we recurse passed a context element during reflow/update-overflow/painting...except, as dholbert pointed out, UpdateOverflow is not a recursive function, so that doesn't work.

Due to performance concerns I have been trying to avoid solving these issues by making code that needs context information look up the tree for the nearest nsSVGUseFrame/nsSVGPathGeometryFrame/document that has a context. But I think that's maybe what we should do first. If the performance concerns turn out to be real then we can try to come up with something else. (In general the number of ancestors between an element referencing context properties and the context element is likely to be small. At least for SVG-in-OT and <marker> content. Maybe less so for <use> and SVG-as-an-image.)

Comment 1

[Jonathan Watt [:jwatt]]

Another thing that is wrong is calling these context objects "text" context "paint". First, the contexts can come from <use>, geometry using <marker> and in the future from SVG-as-an-image contexts, not just text. Second, the 'context-value' keyword can be used with the 'stroke-width' property, and that's more geometry than paint (and relevant to reflow/overflow areas, not just painting). Going forward other non-paint properties may also allow context references.

To resolve the naming issues I'd propose calling the class SVGContextProps, or something.

Comment 2

[Jonathan Watt [:jwatt]]

This probably obviates the need for bug 1074544.

Comment 3

[Jonathan Watt [:jwatt]]

One of the things that's required here is to get rid of SimpleTextContextPaint. That gfxTextContextPaint subclass depends on us having a gfxContext with a pattern set on it. (The other subclass, SVGTextContextPaint, is set up explicitly for a context (currently only SVG text elements), so presumably the bit of code that creates a SimpleTextContextPaint does so to pass the currentColor through to SVG-in-OpenType glyphs for HTML text). We should probably flatten these classes into a single class, and propagate the full context info properly.

Comment 4

[Jonathan Watt [:jwatt]]

Note the patch for bug 1290781 made us set the context paint on the document instead of on a DrawTarget.