nsGfxScrollFrame doesn't handle elements with vertical writing mode properly

RESOLVED FIXED in mozilla36

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

(Blocks 1 bug)

unspecified
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 1 obsolete attachment)

Assignee

Description

5 years ago
See attached testcase; in vertical writing mode, the text doesn't become horizontally scrollable as it should.
Assignee

Comment 1

5 years ago
Here's a first effort at making the scrollFrame work with vertical modes. With this, the testcase mostly behaves as expected: scrolling works properly in all modes/directions. The remaining problem is that in vertical writing mode, it's scrollable too far in the vertical direction; this is due to nsBlockFrame computing its overflow area incorrectly, which will be fixed in the next patch.
Attachment #8516775 - Flags: review?(smontagu)
Assignee

Updated

5 years ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Assignee

Comment 2

5 years ago
And this gives us the proper scrollable extent.
Attachment #8516776 - Flags: review?(smontagu)
Assignee

Comment 3

5 years ago
Minor update to part 1, to make scrollbars correctly adjust for a resizer (when present) in the vertical-rl case.
Attachment #8517488 - Flags: review?(smontagu)
Assignee

Updated

5 years ago
Attachment #8516775 - Attachment is obsolete: true
Attachment #8516775 - Flags: review?(smontagu)

Comment 4

5 years ago
If possible,
It is better to put vertical scroll bar on left side of the container in vertical-rl mode.
like ie did.

there is a testcase with chinese,japanese,mongolian.
http://www.mongolfont.com/test/firefox/div.html
Assignee

Comment 5

5 years ago
(In reply to siqinbilige from comment #4)
> If possible,
> It is better to put vertical scroll bar on left side of the container in
> vertical-rl mode.

Agreed; and that should be the result we get with the patch here.
Assignee

Comment 6

5 years ago
Here's a screen grab of your example (thanks!) as displayed with a local build that includes this and other pending-review patches for vertical support.
Attachment #8517488 - Flags: review?(smontagu) → review+
Comment on attachment 8516776 [details] [diff] [review]
part 2 - Handle vertical writing mode when computing overflow areas in nsBlockFrame.

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

::: layout/generic/nsBlockFrame.cpp
@@ +1579,4 @@
>  {
>    // Factor in the bottom edge of the children.  Child frames will be added
>    // to the overflow area as we iterate through the lines, but their margins
>    // won't, so we need to account for bottom margins here.

s/bottom/block-end/ throughout this comment
Attachment #8516776 - Flags: review?(smontagu) → review+
Assignee

Comment 8

5 years ago
(In reply to Simon Montagu :smontagu from comment #7)
> Comment on attachment 8516776 [details] [diff] [review]
> part 2 - Handle vertical writing mode when computing overflow areas in
> nsBlockFrame.
> 
> Review of attachment 8516776 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/nsBlockFrame.cpp
> @@ +1579,4 @@
> >  {
> >    // Factor in the bottom edge of the children.  Child frames will be added
> >    // to the overflow area as we iterate through the lines, but their margins
> >    // won't, so we need to account for bottom margins here.
> 
> s/bottom/block-end/ throughout this comment

For more consistency, I've made that change in a whole bunch of places in nsBlockFrame.cpp that are all talking about this margin. And renamed the mCarriedOutBottomMargin field in nsHTMLReflowMetrics to mCarriedOutBEndMargin. I'll attach this as an additional patch.
Comment on attachment 8521658 [details] [diff] [review]
part 3 - Replace |bottom| with |block-end| in a bunch of comments, and rename mCarriedOutBottomMargin to mCarriedOutBEndMargin.

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

::: layout/generic/nsBlockFrame.cpp
@@ +2182,2 @@
>        // reflow the previous line and we do need to reflow (or repair
>        // the top position of) the next line.

If you're doing this, might as well fix "top" too
Attachment #8521658 - Flags: review?(smontagu) → review+
Assignee

Updated

5 years ago
Duplicate of this bug: 1079159
You need to log in before you can comment on or make changes to this bug.