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)
Core
Layout: Block and Inline
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: jfkthame, Assigned: jfkthame)
References
(Blocks 1 open bug)
Details
Attachments
(5 files, 1 obsolete file)
1.62 KB,
text/html
|
Details | |
6.31 KB,
patch
|
smontagu
:
review+
|
Details | Diff | Splinter Review |
9.03 KB,
patch
|
smontagu
:
review+
|
Details | Diff | Splinter Review |
393.62 KB,
image/png
|
Details | |
19.61 KB,
patch
|
smontagu
:
review+
|
Details | Diff | Splinter Review |
See attached testcase; in vertical writing mode, the text doesn't become horizontally scrollable as it should.
Assignee | ||
Comment 1•10 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•10 years ago
|
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•10 years ago
|
||
And this gives us the proper scrollable extent.
Attachment #8516776 -
Flags: review?(smontagu)
Assignee | ||
Comment 3•10 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•10 years ago
|
Attachment #8516775 -
Attachment is obsolete: true
Attachment #8516775 -
Flags: review?(smontagu)
Comment 4•10 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•10 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•10 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.
Updated•10 years ago
|
Attachment #8517488 -
Flags: review?(smontagu) → review+
Comment 7•10 years ago
|
||
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•10 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.
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8521658 -
Flags: review?(smontagu)
Assignee | ||
Updated•10 years ago
|
Blocks: enable-writing-mode-dev
Comment 10•10 years ago
|
||
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 | ||
Comment 11•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b19236090ee https://hg.mozilla.org/integration/mozilla-inbound/rev/1fc75ce98bf6 https://hg.mozilla.org/integration/mozilla-inbound/rev/7e356c01134e
Target Milestone: --- → mozilla36
Comment 12•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0b19236090ee https://hg.mozilla.org/mozilla-central/rev/1fc75ce98bf6 https://hg.mozilla.org/mozilla-central/rev/7e356c01134e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•