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

Status: RESOLVED FIXED

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

Description

[Jonathan Kew [:jfkthame]]

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.

Comment 1

[Jonathan Kew [:jfkthame]]

(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.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: 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.

Comment 3

[Jonathan Kew [:jfkthame]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=96e9242b44cf

Comment 4

[Simon Montagu :smontagu]

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?

Comment 5

[Jonathan Kew [:jfkthame]]

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 6

[Simon Montagu :smontagu]

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.

Comment 7

[Jonathan Kew [:jfkthame]]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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