Closed
Bug 1108923
Opened 9 years ago
Closed 9 years ago
setting vertical writing-mode on body element results in broken layout
Categories
(Core :: Layout: Block and Inline, defect)
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: sideshowbarker, Assigned: jfkthame)
References
(Blocks 1 open bug)
Details
Attachments
(4 files)
37.68 KB,
text/html
|
Details | |
18.62 KB,
patch
|
smontagu
:
review+
|
Details | Diff | Splinter Review |
3.15 KB,
patch
|
smontagu
:
review+
|
Details | Diff | Splinter Review |
4.31 KB,
patch
|
smontagu
:
review+
|
Details | Diff | Splinter Review |
Setting vertical writing-mode on the body element results in a document with layout broken in unexpected ways that are not seen when setting writing-mode on a descendant of the body. I've attached the same test case to a couple of other bugs but I'm not sure if the problem exposed in this test case is strictly a duplicate of any other bug. But if it is then of course please just mark it as a duplicate.
Assignee: nobody → jfkthame
Assignee | ||
Comment 1•9 years ago
|
||
One problem here (perhaps the main problem) is that margins on the <body> are not being handled correctly. Tinkering with the body margins in the inspector, it's clear that (a) the right margin is being applied in the wrong direction, so that instead of being inset from the right edge of the window, the content overflows out of sight; and (b) the top and bottom margins, specified as %age, are being calculated from the width rather than the height of the window.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 2•9 years ago
|
||
(In reply to Jonathan Kew (:jfkthame) from comment #1) > (b) the top and bottom > margins, specified as %age, are being calculated from the width rather than > the height of the window. Well, this is of course correct per spec (percentage values refer to the width of the containing block), though it seems somewhat counterintuitive for vertical-mode documents. The right margin is clearly wrong, though.
(In reply to Jonathan Kew (:jfkthame) from comment #2) > Well, this is of course correct per spec (percentage values refer to the > width of the containing block), though it seems somewhat counterintuitive > for vertical-mode documents. I suspect it should probably be the other way around (% values all based on height) when the containing block has a vertical writing-mode.
Assignee | ||
Comment 4•9 years ago
|
||
(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #3) > (In reply to Jonathan Kew (:jfkthame) from comment #2) > > Well, this is of course correct per spec (percentage values refer to the > > width of the containing block), though it seems somewhat counterintuitive > > for vertical-mode documents. > > I suspect it should probably be the other way around (% values all based on > height) when the containing block has a vertical writing-mode. Right; I've checked, and the Writing Modes does say this, AFAICT. Sorry, didn't post a followup comment to clarify that.
If there isn't a bug filed on doing that, could you file one?
Flags: needinfo?(jfkthame)
Assignee | ||
Comment 6•9 years ago
|
||
(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #5) > If there isn't a bug filed on doing that, could you file one? I'm expecting to address it as part of the margin issues in this bug (but will split out patches into separate bugs if there seem to be several independent pieces involved).
Flags: needinfo?(jfkthame)
Assignee | ||
Comment 7•9 years ago
|
||
I believe this fixes the margin issues illustrated by this testcase (i.e. percent-specified margins (or padding) are calculated incorrectly in vertical modes). Other issues also shown by the testcase are (a) failure to propagate the writing-mode from <body> to the root (bug 1102175); and (b) failure to handle overflow and provide the expected scrollbar (bug 1108067).
Attachment #8534361 -
Flags: review?(smontagu)
Assignee | ||
Comment 8•9 years ago
|
||
Here's a reftest for this; with current trunk code, it gets both vertical and horizontal margins wrong, and with the patch here it passes.
Attachment #8534366 -
Flags: review?(smontagu)
Comment 9•9 years ago
|
||
Comment on attachment 8534361 [details] [diff] [review] Use the correct containing box dimension as the percent basis for margin/padding in vertical writing modes Review of attachment 8534361 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsHTMLReflowState.cpp @@ +1904,5 @@ > // Flex items resolve percentage margin & padding against the flex > // container's height (which is the containing block height). > // For everything else: the CSS21 spec requires that margin and padding > // percentage values are calculated with respect to the *width* of the > // containing block, even for margin & padding in the vertical axis. This comment needs updating too @@ +1908,5 @@ > // containing block, even for margin & padding in the vertical axis. > static nscoord > +BlockDirOffsetPercentBasis(const nsIFrame* aFrame, > + nscoord aContainingBlockISize, > + nscoord aContainingBlockBSize) I think it's preferable to pass around LogicalSizes wherever possible instead of individual nscoords, but I suppose it's not critical in such a short method. ::: layout/generic/nsHTMLReflowState.h @@ +194,1 @@ > Please update the javadoc comments on ComputeMargin and ComputePadding to reflect the changes.
Attachment #8534361 -
Flags: review?(smontagu) → review+
Assignee | ||
Comment 10•9 years ago
|
||
Ah yes, I remembered after uploading the patch that I'd intended to fix the comments once I confirmed the code worked. Here's a followup to do that.
Attachment #8534515 -
Flags: review?(smontagu)
Comment 11•9 years ago
|
||
Comment on attachment 8534515 [details] [diff] [review] Update comments to reflect the newly logicalized parameters Review of attachment 8534515 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! ::: layout/generic/nsHTMLReflowState.cpp @@ +1901,5 @@ > return static_cast<nsFlexContainerFrame*>(parent); > } > > // Flex items resolve percentage margin & padding against the flex > +// container's block-size (which is the containing block block-size). Nit (applies to the unpatched version as well): this sounds at first reading as if flex items even resolve inline percentage margin & padding against the container's block size. Can you insert "block-size" into the first line to make it clearer?
Attachment #8534515 -
Flags: review?(smontagu) → review+
Assignee | ||
Comment 12•9 years ago
|
||
(In reply to Simon Montagu :smontagu from comment #11) > Nit (applies to the unpatched version as well): this sounds at first reading > as if flex items even resolve inline percentage margin & padding against the > container's block size. Can you insert "block-size" into the first line to > make it clearer? Fixed locally; I'll land it all once the reftest is reviewed as well.
Updated•9 years ago
|
Attachment #8534366 -
Flags: review?(smontagu) → review+
Assignee | ||
Comment 13•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5bc284ebe1c https://hg.mozilla.org/integration/mozilla-inbound/rev/d1f3262e8b98 https://hg.mozilla.org/integration/mozilla-inbound/rev/32ae9436ebbe
Blocks: 1079125, enable-writing-mode-dev
Target Milestone: --- → mozilla37
https://hg.mozilla.org/mozilla-central/rev/a5bc284ebe1c https://hg.mozilla.org/mozilla-central/rev/d1f3262e8b98 https://hg.mozilla.org/mozilla-central/rev/32ae9436ebbe
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•