Closed
Bug 1155261
Opened 10 years ago
Closed 10 years ago
vertical textframes with upright-orientation glyphs have an excessive visual-overflow area
Categories
(Core :: Layout: Text and Fonts, defect)
Core
Layout: Text and Fonts
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: jfkthame, Assigned: jfkthame)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
(deleted),
patch
|
smontagu
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
Blocks: enable-writing-mode-release
Assignee | ||
Comment 1•10 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•10 years ago
|
||
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•10 years ago
|
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•10 years ago
|
||
Comment 4•10 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;
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•10 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 6•10 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]:
-----------------------------------------------------------------
Ah, now I get it.
Attachment #8594842 -
Flags: review?(smontagu) → review+
Assignee | ||
Comment 7•10 years ago
|
||
Comment 8•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•