vertical textframes with upright-orientation glyphs have an excessive visual-overflow area

RESOLVED FIXED in Firefox 40

Status

()

Core
Layout: Text
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

(Blocks: 1 bug)

unspecified
mozilla40
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Testcase:

data:text/html;charset=utf-8,
  <div style="writing-mode:vertical-rl; padding:1em;" contenteditable>你好吗

Enable nglayout.debug.paint_flashing in about:config, then modify the text in the contenteditable <div>, e.g. by inserting a space. The paint flashing shows that we're invalidating and repainting a bunch of extra space to the left of the actual glyphs.

Compare the same example with "text-orientation:sideways-right" added to the style; in this case, there's no excess invalidation/painting.
(Assignee)

Updated

3 years ago
Blocks: 1138384
(Assignee)

Comment 1

3 years ago
(In reply to Jonathan Kew (:jfkthame) from comment #0)
> Testcase:
> 
> data:text/html;charset=utf-8,
>   <div style="writing-mode:vertical-rl; padding:1em;" contenteditable>你好吗
> 

Better testcase (now that orthogonal block sizing has been improved):

data:text/html;charset=utf-8,
   <div style="writing-mode:vertical-rl; padding:1em; height: 20em;" contenteditable>你好吗

Then try editing the text with paint flashing enabled.

This bug is also demonstrated by attachment 8592839 [details] in bug 1153510: drag-selecting various parts of the text, especially with paint flashing enabled, will show the bad overflow areas.
Blocks: 1153510
(Assignee)

Comment 2

3 years ago
Created attachment 8594842 [details] [diff] [review]
Fix computation of glyph extents and text-frame visual overflow for vertical text frames

This seems to fix the problems I'm seeing here, as well as in the SVG-text testcase in bug 1153510.
Attachment #8594842 - Flags: review?(smontagu)
(Assignee)

Updated

3 years ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
(Assignee)

Comment 3

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=96e9242b44cf
Comment on attachment 8594842 [details] [diff] [review]
Fix computation of glyph extents and text-frame visual overflow for vertical text frames

Review of attachment 8594842 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFont.cpp
@@ +2276,4 @@
>                      if (isRTL) {
>                          glyphRect -= gfxPoint(advance, 0);
>                      }
>                      glyphRect += glyphPt;

I don't understand what is happening here. Why are these adjustments to glyphRect after the Swap()s for the vertical case and not before?
(Assignee)

Comment 5

3 years ago
Comment on attachment 8594842 [details] [diff] [review]
Fix computation of glyph extents and text-frame visual overflow for vertical text frames

Review of attachment 8594842 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFont.cpp
@@ +2276,4 @@
>                      if (isRTL) {
>                          glyphRect -= gfxPoint(advance, 0);
>                      }
>                      glyphRect += glyphPt;

The orientation == eVertical case means we're in "upright" text-orientation for a vertical textrun (noting that vertical text with "sideways" orientation still uses purely horizontal glyph metrics to lay out the runs). In other words, the glyphs are going to be rotated 90° relative to the line. That's at the individual glyph-extents-box level, prior to adjusting the rect to the right position based on how successive glyphs are laid out along the line.
Comment on attachment 8594842 [details] [diff] [review]
Fix computation of glyph extents and text-frame visual overflow for vertical text frames

Review of attachment 8594842 [details] [diff] [review]:
-----------------------------------------------------------------

Ah, now I get it.
Attachment #8594842 - Flags: review?(smontagu) → review+
(Assignee)

Comment 7

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe832ef6cc60
https://hg.mozilla.org/mozilla-central/rev/fe832ef6cc60
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Depends on: 1161700
You need to log in before you can comment on or make changes to this bug.