1123133 - fieldset reflow (in particular, legend placement) needs to handle vertical writing modes

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: testcase for fieldset support

Here's a small testcase I've been using to experiment.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: pt 1 - Utility to map from logical to physical side values

Just a small helper to simplify the following patch.

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: pt 2 - Vertical writing-mode support in nsFieldSetFrame

With this, the legend works nicely in vertical mode. (Note that dir=rtl doesn't seem to work as desired in vertical mode; but that's a wider issue, I think, so I'm leaving that for a separate bug.)

Comment 4

[Simon Montagu :smontagu]

Comment on attachment 8550978 [details] [diff] [review]
pt 1 - Utility to map from logical to physical side values

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

r=me, but do we really need both |css::Side PhysicalSideFor(LogicalSide aLogicalSide)| and |mozilla::Side PhysicalSide(LogicalSide aSide)|? Can we coalesce them in a follow up?

Comment 5

[Jonathan Kew [:jfkthame]]

(In reply to Simon Montagu :smontagu from comment #4)
> Comment on attachment 8550978 [details] [diff] [review]
> pt 1 - Utility to map from logical to physical side values
> 
> Review of attachment 8550978 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me, but do we really need both |css::Side PhysicalSideFor(LogicalSide
> aLogicalSide)| and |mozilla::Side PhysicalSide(LogicalSide aSide)|? Can we
> coalesce them in a follow up?

Oh, right - I'm sure we can tidy that up. I was working with a tree where Cam's patches hadn't landed yet, and just didn't think about the connection with what was incoming.

Comment 6

[Simon Montagu :smontagu]

Comment on attachment 8550979 [details] [diff] [review]
pt 2 - Vertical writing-mode support in nsFieldSetFrame

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

This overlaps a lot with a patch in my queue, but where they differ I think yours is better. The only thing lacking is using the new logical methods from bug 1079154, but we can add that later.

Comment 7

[Jonathan Kew [:jfkthame]]

Comment on attachment 8550978 [details] [diff] [review]
pt 1 - Utility to map from logical to physical side values

Obsoleting this; we can simply use the PhysicalSide method that was recently landed as part of CSS logical-property support, rather than introducing a new method here.

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: Vertical writing-mode support in nsFieldSetFrame

Trivially updated to use the WritingMode::PhysicalSide method; carrying over r=smontagu.

Comment 9

[Jonathan Kew [:jfkthame]]

Pushed this to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/350013f91dd2

(Feel free to propose a followup to make better use of bug 1079154 methods (comment 6), if that seems appropriate.)

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/350013f91dd2