1079157 - Logical coordinate support for columns

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Simon Montagu :smontagu]

Part of support for vertical text

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: Make nsColumnSetFrame support vertical writing mode.

Here's a WIP patch. This seems to make columns basically work in vertical writing modes, though hasn't been tested much yet.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: Make nsColumnSetFrame support vertical writing mode

Updated and tidied up this patch a bit; it seems pretty functional in my simple testing. (We'll want a bunch of proper testcases...)

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: sample with columns in vertical writing mode

Here's a simple example to play with.

Note that it behaves quite erratically (chunks of the text disappear or are smashed together on the right) during horizontal resizing, and sometimes when changing the various options. I'm about to file a separate bug on this; it's not specific to columns.

Comment 4

[Jonathan Kew [:jfkthame]]

(In reply to Jonathan Kew (:jfkthame) from comment #3)
> Created attachment 8530287 [details]
> sample with columns in vertical writing mode
> 
> Here's a simple example to play with.
> 
> Note that it behaves quite erratically (chunks of the text disappear or are
> smashed together on the right) during horizontal resizing, and sometimes
> when changing the various options. I'm about to file a separate bug on this;
> it's not specific to columns.

smontagu got there first, and filed bug 1106083 on this.

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: more complex testcase for columns

The patch above doesn't return proper metrics for the columnset, with the result that things break badly in vertical-rl mode when the columnset frame is only one of several children of its container; it's liable to disappear, as in this example.

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: Make nsColumnSetFrame support vertical writing mode

And here's a slightly revised patch that works much better; the testcase above now looks good in all writing modes.

Comment 7

[Jonathan Kew [:jfkthame]]

Comment on attachment 8530513 [details] [diff] [review]
Make nsColumnSetFrame support vertical writing mode

To make this a bit easier to review (I hope), I'm splitting it into two patches; first, purely mechanical renaming of a bunch of local variables and members in nsColumnSetFrame, generated with sed; and then the manual changes that actually make things work in vertical mode. That way, the first patch should need little more than a rubber-stamp, and it'll be easier to follow the real changes in the second.

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: part 1 - Purely mechanical renaming of local variables and members in nsColumnSetFrame

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: part 2 - Make nsColumnSetFrame support vertical writing mode

Comment 10

[Jonathan Kew [:jfkthame]]

Please excuse the churn, but I'm going to refresh these patches once more: in addition to the mechanical renaming in part 1, I've split a bunch of comment-only hunks, as well as some whitespace cleanup, out into a new part 2 patch, to make it easier to see the actual code changes in the final part 3.

Comment 11

[Jonathan Kew [:jfkthame]]

Attachment: part 1 - Purely mechanical renaming of local variables and members in nsColumnSetFrame

Comment 12

[Jonathan Kew [:jfkthame]]

Attachment: part 2 - Comment updates and whitespace cleanup only, no actual code changes

Comment 13

[Jonathan Kew [:jfkthame]]

Attachment: part 3 - Make nsColumnSetFrame support vertical writing mode

Comment 14

[Simon Montagu :smontagu]

Comment on attachment 8530533 [details] [diff] [review]
part 1 - Purely mechanical renaming of local variables and members in nsColumnSetFrame

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

Thanks for doing the split!

Comment 15

[Simon Montagu :smontagu]

Comment on attachment 8530535 [details] [diff] [review]
part 3 - Make nsColumnSetFrame support vertical writing mode

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

::: layout/generic/nsColumnSetFrame.cpp
@@ +771,5 @@
>      }
>    }
>    
>    aColData.mMaxBSize = contentBEnd;
>    contentRect.height = std::max(contentRect.height, contentBEnd);

This seems like mixing logical and physical coordinates, as does the initialization of contentSize a few lines below.

Is there any reason why contentRect can't be a LogicalRect?

Comment 16

[Jonathan Kew [:jfkthame]]

Attachment: part 3 - Make nsColumnSetFrame support vertical writing mode

You're right, things were a little fishy there. Here's an updated patch that fixes the confusion, and appears to work better too. It seemed easier to keep contentRect as physical while it's being computed (unioning in the rects of the child frames), but then the final contentSize is converted properly to logical coordinates.

Comment 17

[Simon Montagu :smontagu]

Comment on attachment 8531014 [details] [diff] [review]
part 3 - Make nsColumnSetFrame support vertical writing mode

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

::: layout/generic/nsColumnSetFrame.cpp
@@ -721,3 @@
>  
> -  WritingMode wm = aReflowState.GetWritingMode();
> -  LogicalSize contentSize(wm, nsSize(contentRect.XMost(), contentRect.YMost()));

So instead of these two lines you just have 
 LogicalSize contentSize = LogicalSize(wm, contentRect.Size());

Do subtracting border padding and the difference between the size and the XMost/YMost necessarily cancel out?

Comment 18

[Simon Montagu :smontagu]

*sigh*. "review" doesn't show the same quoted content in the preview of a comment that appears when publishing. Do we have a bug on that?

"These two lines" in comment 17 mean 

-  contentRect -= nsPoint(physicalBP.left, physicalBP.top);

and

-   // contentRect included the borderPadding.left,borderPadding.top of the child rects
-  LogicalSize contentSize(wm, nsSize(contentRect.XMost(), contentRect.YMost()));

Comment 19

[Jonathan Kew [:jfkthame]]

(In reply to Simon Montagu :smontagu from comment #17)
> Comment on attachment 8531014 [details] [diff] [review]
> part 3 - Make nsColumnSetFrame support vertical writing mode
> 
> Review of attachment 8531014 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/nsColumnSetFrame.cpp
> @@ -721,3 @@
> >  
> > -  WritingMode wm = aReflowState.GetWritingMode();
> > -  LogicalSize contentSize(wm, nsSize(contentRect.XMost(), contentRect.YMost()));
> 
> So instead of these two lines you just have 
>  LogicalSize contentSize = LogicalSize(wm, contentRect.Size());
> 
> Do subtracting border padding and the difference between the size and the
> XMost/YMost necessarily cancel out?

AFAICT, yes. The accumulated contentRect will have been "offset" by the topLeft of the borderPadding; subtracting that leaves its topLeft at 0,0, and its XMost/YMost is then the same as Size. I tried putting a big fat border on the columnSet frame and confirmed that this still behaves as expected.

Comment 20

[Simon Montagu :smontagu]

Comment on attachment 8531014 [details] [diff] [review]
part 3 - Make nsColumnSetFrame support vertical writing mode

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

OK, I'll buy that

Comment 21

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d17bd17b3786
https://hg.mozilla.org/integration/mozilla-inbound/rev/d588126b6d92
https://hg.mozilla.org/integration/mozilla-inbound/rev/52a6bc20f60d

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d17bd17b3786
https://hg.mozilla.org/mozilla-central/rev/d588126b6d92
https://hg.mozilla.org/mozilla-central/rev/52a6bc20f60d