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)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: sideshowbarker, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

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.
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
(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.
(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)
(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)
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)
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 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+
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 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+
(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.
Attachment #8534366 - Flags: review?(smontagu) → review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: