search

w3c / svgwg

SVG Working Group specifications
Other
698 stars 132 forks source link

Unclear step in Text Layout Algorithm (section 11.5) #617

Open faceless2 opened 5 years ago

faceless2 commented 5 years ago

There's a minor typo in the text layout algorithm as described in https://www.w3.org/TR/SVG/text.html#TextLayoutAlgorithm

faceless2 commented 5 years ago

While it's still open...

faceless2 commented 5 years ago

Actually I think there's a little more to this than I'd hoped in my first point. Take this example:

<text x="10" dx="20 20">AB</text>

this positions the A at 30 and B at 50 in Inkscape, Webkit and Firefox, so I presume that's the intention. But I don't see how this arises from the spec.

Here is section 4 which deals with "dx,dy", modified as I suggested in my original post

Let shift be the cumulative x and y shifts due to ‘x’ and ‘y’ attributes, initialized to (0,0).
For each array element with index i in result:
  If resolve_dx[i] is unspecified, set it to 0. If resolve_dy[i] is unspecified, set it to 0.
  Let shift.x = shift.x + resolve_dx[i] and shift.y = shift.y + resolve_dy[i].
  Let result[i].x = CSS_positions[i].x + shift.x and result[i].y = CSS_positions[i].y + shift.y.

and here's section 6, which deals with "x" and "y".

Let shift be the current adjustment due to the ‘x’ and ‘y’ attributes, initialized to (0,0).
Set index = 1.
While index < count:
  If resolved_x[index] is set, then let shift.x = resolved_x[index] − result.x[index].
  If resolved_y[index] is set, then let shift.y = resolved_y[index] − result.y[index].
  Let result.x[index] = result.x[index] + shift.x and result.y[index] = result.y[index] + shift.y.

First, there is the initial line in section 4: I presume the description of shift (and it is only a description, not an instruction) is incorrect, it's the cumulative x and y shifts due to "dx" and "dy", not "x" and "y".

Second, I try to run this algorithm for the example above. The first and second glyphs are shifted by 20 and 40 from their natural positions, which looks correct. But then in section 6 the "x" value overwrites this adjustment for the first glyph:

If resolved_x[index] is set, then let shift.x = resolved_x[index] − result.x[index]. This means shift.x = 10 - 20. Let result.x[index] = result.x[index] + shift.x This means result.x = 20 + -10 = 10, whereas it should be 30.

To sum up: section 6 will ensure that any x/y values replace any previous modifications to x or y, but this doesn't match any implementation I have tested. So I suspect it is section 6 that is at fault, but I'm not familiar enough with the intention to be able to suggest a solution, sorry.

karip commented 5 years ago

This is related to #271

Tavmjong commented 5 years ago

I believe the purpose of 6.3.1 and 6.3.2 is to remove the previous shift due to 'dx' and 'dy' (shift get's zeroed).

The example y="20 40 60" dy="-20 -40" should position the first character at y=0, the second at y=0 and the third at y=60.

Inkscape, Firefox, and Chrome do this.

Tavmjong commented 5 years ago

I've had time to look at this more closely. The problem seems to be that the algorithm assumes that 'dx' is not applied when 'x' is set for a given code point. Chrome/Firefox/Inkscape do apply the corresponding 'dx' when 'x' is set (ignoring previous 'dx' values). Changing the calculation in 6.3.3 to include 'dx' seems to fix the problem.

I find the variable names to be confusing and inconsistent. I propose renaming all in the form of variable[i].x. I propose also using shift.dx and result[i].dx in section 4 and shift.x and result[i].x in section 6. With these changes in names:

Section 4.2.2 should be: Let shift.dx = shift.dx + resolve[i].dx Section 4.2.3 should be: Let result[i].dx = CSS_positions[i].x + shift.dx Section 6.3.1 should be: If resolve[i].x is set, let shift.x = resolve[i].x + resolve[i].dx - result[i].dx Section 6.3.3 should be: Let result[i].x = result[i].dx + shift.x

Similar changes apply to 'y'.

css-meeting-bot commented 5 years ago

The SVG Working Group just discussed Unclear step in Text Layout Algorithm (section 11.5), and agreed to the following:

The full IRC log of that discussion <AmeliaBR> Topic: Unclear step in Text Layout Algorithm (section 11.5)
<AmeliaBR> github: https://github.com/w3c/svgwg/issues/617
<AmeliaBR> Tav: This is a rather confusing algorithm, but I think I've figured out where the issue is. According to the prose, if a given character has both `x` and `dx` values, they get combined (set `x`, and then add `dx`), but according to the algorithm `dx` gets dropped in that case. And that's not what Inkscape or browsers do.
<AmeliaBR> Chris: Are implementations consistent? Can we just adjust the algorithm to match?
<AmeliaBR> Tav: I think everyone's consistent.
<AmeliaBR> Chris: Have you figured out the suggested changes, in your last comment Tav?
<AmeliaBR> Tav: I think so. Needs a review.
<AmeliaBR> Dirk: Can you make it a proper PR?
<AmeliaBR> Tav: Sure.
<chris> https://bfo.com/
<AmeliaBR> Chris: And tag the original poster for review, who seems to be working on an implementation.
<AmeliaBR> RESOLUTION: Fix algorithm to be consistent with SVG 1 and implementations (dx/dy add to x/y on same character).
faceless2 commented 5 years ago

Thank you everyone and @Tavmjong in particular for your time on this, the proposed change to the algorithm has fixed the issue. I'm aware there's a PR for this change to the spec outstanding, but I'm satisfied so closing. Thanks again.

faceless2 commented 3 years ago

Reopening only because the proposals in https://github.com/w3c/svgwg/issues/617#issuecomment-456187562 haven't been applied to the spec yet.