Closed Bug 1093684 Opened 10 years ago Closed 10 years ago

nsGfxScrollFrame doesn't handle elements with vertical writing mode properly

Categories

(Core :: Layout: Block and Inline, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jfkthame, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 1 obsolete file)

See attached testcase; in vertical writing mode, the text doesn't become horizontally scrollable as it should.
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: nobody → jfkthame
Status: NEW → ASSIGNED
And this gives us the proper scrollable extent.
Attachment #8516776 - Flags: review?(smontagu)
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)
Attachment #8516775 - Attachment is obsolete: true
Attachment #8516775 - Flags: review?(smontagu)
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
(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.
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+
(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+
You need to log in before you can comment on or make changes to this bug.