Closed Bug 1082017 Opened 6 years ago Closed 6 years ago
writing-mode:vertical-* affects computed value of line-height, leading to mochitest failures
1.47 KB, text/html
Add writing-mode to prerequisites for line-height and font (shorthand) tests, because vertical vs horizontal mode may result in different line metrics
1.48 KB, patch
|Details | Diff | Splinter Review|
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.
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.)
(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?
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.
Attachment #8562251 - Flags: review?(dbaron)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Attachment #8562251 - Flags: review?(dbaron) → review+
Target Milestone: --- → mozilla38
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
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.
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.
Attachment #8565435 - Flags: review?(dbaron)
Attachment #8562251 - Attachment is obsolete: true
Attachment #8565435 - Flags: review?(dbaron) → review+
You need to log in before you can comment on or make changes to this bug.