1082017 - writing-mode:vertical-* affects computed value of line-height, leading to mochitest failures

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Jonathan Kew [:jfkthame]]

With current trunk code, enabling support for vertical writing modes (the compile-time option in WritingModes.h, and the runtime vertical-text pref) will lead to a number of failures in layout/style/test/test_inherit_computation.html; see https://tbpl.mozilla.org/?tree=Try&rev=cbf75f7e0bc8.

The issue here is that changing the writing mode to vertical will affect the computed value of 'normal' line-height, because we'll be using a different set of font metrics (see bug 1065002) as the source of ascent/descent/linegap values. This comes as a surprise to test_inherit_computation.html, which doesn't expect changes to "unrelated" properties to affect line-height (or the font shorthand, which includes the line-height value).

We need to consider whether it's OK for writing-mode to affect normal line-height in this way, or if it should remain constant across writing-mode changes. (The "best" answer here is not entirely clear to me; it seems to depend on the exact vertical-text use case one has in mind, and could also depend on the value of text-orientation. But then, do we want text-orientation to affect line-height...?)

If we conclude such variation *is* legitimate, we'll need to adjust test_inherit_computation.html so that it allows for it; or if not, we need to fix the line-spacing code to provide writing-mode-independent results.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: Simple testcase for playing with writing-mode/line-height effects

Here's a simple testcase that displays the issue here, whereby line-height reports different values when writing-mode is changed. This exhibits the issue on OS X; behavior on other platforms may vary due to font availability, and because standard (horizontal) font metrics are provided by platform-specific code that has some inconsistencies at present.

(Note that this also illustrates a couple of other issues we currently have: changing text-orientation does not affect the reported value of line-height, but does in fact change the rendered line spacing; and enabling vertical writing mode for the test <div> causes a vertical scrollbar to appear on the window, which shouldn't be needed. But these are separate issues from the question of how writing-mode and line-height *ought* to interact.)

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Jonathan Kew (:jfkthame) from comment #0)
> If we conclude such variation *is* legitimate, we'll need to adjust
> test_inherit_computation.html so that it allows for it; or if not, we need
> to fix the line-spacing code to provide writing-mode-independent results.

Or maybe just add to the prerequisites line for line-height in property_database.js?

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: Add writing-mode to prerequisites for line-height and font (shorthand) tests, because vertical vs horizontal mode may result in different line metrics

This prevents a bunch of errors from test_inherit_computation.html when writing-mode:vertical-* is enabled, due to font metrics being different depending on the orientation.

Comment 4

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/48b2d69db54d

Comment 5

[Carsten Book [:Tomcat]]

sorry had to back this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e064c1c87d02 since one of this changes caused https://treeherder.mozilla.org/logviewer.html#?job_id=6509981&repo=mozilla-inbound

Comment 6

[Jonathan Kew [:jfkthame]]

Drat, this can't land until we enable writing-mode (in bug 1099032), as it'll cause mochitest errors when the writing-mode property is not available.

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: Add writing-mode to prerequisites for line-height and font (shorthand) tests, because vertical vs horizontal mode may result in different line metrics

Here's an alternative version of the patch, moving the addition of the prerequisite inside of the test for whether writing-mode is enabled; this should allow us to land this patch independently of when we actually toggle the pref.

Comment 8

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f20cf9991555

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f20cf9991555