1173662 - Make nsNumberControlFrame::Reflow aware of vertical writing modes

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Daniel Holbert [:dholbert]]

Spinning this off of bug 1123299 to handle refactoring nsNumberControlFrame::Reflow to use logical coordinates (providing better support for vertical writing modes).

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: part 1: rename local variable 'wm' for clarity

This patch renames a local variable "wm" to "wrapperWM" (for outerWrapperFrame), to distinguish it from the number control frame's own WM, which we'll have in a new local variable alongside this in the next part here.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: part 2: use logical sizes in nsNumberControlFrame::Reflow

Here's the main patch.

This is mostly s/Width/ISize/, s/Height/BSize/.  There are only two sections that are (slightly) more complex than this:

 (a) getting the wrapper frame's margin-box height (now bsize): For this, we need to convert the frame's LogicalMargin into our own writing-mode, before we can grab BStartEnd off of it. (It's unlikely that its writing-mode will differ from ours, but it's possible that it could via tweaks to forms.css or other UA stylesheets, so might as well handle that case.)

 (b) populating our aDesiredSize at the end of reflow: For this, we have to make a LogicalSize that we can pass to the logical SetSize API. (instead of setting Width() & Height())

(If it's not clear, the "wrapper frame" described above is our sole child-frame, IIRC; it's a flex container that contains all our other children.)

Comment 3

[Jonathan Kew [:jfkthame]]

Comment on attachment 8620824 [details] [diff] [review]
part 2: use logical sizes in nsNumberControlFrame::Reflow

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

Thanks for looking into this!  r=me with the extraSpace issue below addressed.

Extra points if you can switch xoffset/yoffset to a LogicalPoint, too, as mentioned below; but that needn't block here if it's problematic.

::: layout/forms/nsNumberControlFrame.cpp
@@ +158,5 @@
>      nsReflowStatus childStatus;
>      ReflowChild(outerWrapperFrame, aPresContext, wrappersDesiredSize,
>                  wrapperReflowState, xoffset, yoffset, 0, childStatus);
>      MOZ_ASSERT(NS_FRAME_IS_FULLY_COMPLETE(childStatus),
>                 "We gave our child unconstrained height, so it should be complete");

s/height/block-size/ while you're here, please

@@ +170,2 @@
>        // We are intrinsically sized -- we should shrinkwrap the outer wrapper's
>        // height:

s/height/block-size/

@@ +187,1 @@
>      yoffset += std::max(0, extraSpace / 2);

Shouldn't this become

  // Center child in the block direction

and apply the extraSpace to xoffset instead of yoffset if the writing mode is vertical?

Ideally, I'd like to replace the use of xoffset/yoffset in this method with a LogicalPoint for childOrigin, and call the logical-coordinate versions of ReflowChild/FinishReflowChild (which take WritingMode, LogicalPoint and containerWidth instead of individual x and y coordinates). We'd eventually like to remove the old physical-coordinate versions. But if that proves difficult for some reason, we could leave it for a followup.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: part 2 v2: use logical sizes in nsNumberControlFrame::Reflow

All good points, thanks! (I'd actually noticed the comments & meant to fix them, but forgot to do so.)

Here's an updated patch with review comments addressed except for the LogicalPoint conversion. (I'll post a "part 3" that does that)

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: part 3: Combine xOffset/yOffset into a LogicalPoint

This addresses the end of comment 3 (including the extraSpace bit).

Note that passing a container width of 0 to ReflowChild & FinishReflowChild. To pass the correct value, we'd need a bit of extra logic, and it'd be untestable (since content can't restyle the child's writing-mode, and it'd need to for this container-width to matter, I think). And if it does end up mattering, the worst that can happen is we'd misposition some content slightly; no crashes or fatal assertions. So, I think it's reasonable to punt on that (with a code-comment) for now.

Comment 6

[Daniel Holbert [:dholbert]]

I tested 'part 3' by running existing reftests in layout/reftests/forms/input/number/ and by loading the data URI from bug 1123299 comment 4, btw. It doesn't seem to break anything.

Comment 7

[Jonathan Kew [:jfkthame]]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> Created attachment 8621162 [details] [diff] [review]
> part 3: Combine xOffset/yOffset into a LogicalPoint
> 
> This addresses the end of comment 3 (including the extraSpace bit).
> 
> Note that passing a container width of 0 to ReflowChild & FinishReflowChild.
> To pass the correct value, we'd need a bit of extra logic, and it'd be
> untestable (since content can't restyle the child's writing-mode, and it'd
> need to for this container-width to matter, I think). And if it does end up
> mattering, the worst that can happen is we'd misposition some content
> slightly; no crashes or fatal assertions. So, I think it's reasonable to
> punt on that (with a code-comment) for now.

Container-width is generally needed to convert logical coordinates to physical correctly when either the inline or block direction is progressing from right to left. So I'd be concerned this will misposition the child in horizontal mode with dir=rtl, or in vertical-rl writing mode.

How does

  data:text/html,<div dir=rtl><input type=number value=123>

appear with part 3?

Comment 8

[Daniel Holbert [:dholbert]]

Ah, OK - I thought the container-width only mattered when the child was reversed with respect to the parent.  But indeed, you're right -- that data URI renders with its contents (the number and the spinner) entirely to the left of the textbox, with the current "part 3".

I'll add the logic to pass the correct container-width, and I'll add a regression test to prevent others from breaking this like I almost did. :)

Comment 9

[Jonathan Kew [:jfkthame]]

My guess is that something like

    nscoord containerWidth = wrappersDesiredSize.Width() +
      wrapperMargin.LeftRight(myWM) +
      aReflowState.ComputedPhysicalBorderPadding().LeftRight();

before FinishReflowChild() may be what you want here. (It should be OK to call ReflowChild() with a bogus containerWidth, if the frame is then going to be placed correctly by FinishReflowChild anyway.)

But that'll want some testing...

Comment 10

[Daniel Holbert [:dholbert]]

I don't think that quite does the right thing if we've got extra space in the horizontal dimension (which would only happen if we're in a vertical writing-mode), and we're centering the 'wrapper' frame in that extra space.

I'm planning on just calculating the nsNumberControlFrame's own desired size a bit earlier, and using that here -- I think that should work.

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: part 3: Combine xOffset/yOffset into a LogicalPoint (now passing borderBoxWidth to FinishReflowChild)

Comment 12

[Daniel Holbert [:dholbert]]

(This still passes existing reftests, and it handles your data URI from comment 7 correctly. New reftest still coming.)

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: part 3: Combine xOffset/yOffset into a LogicalPoint (now passing borderBoxWidth to FinishReflowChild, w/ reftest)

Reftest now included with an RTL number frame, as a "hg cp" of another reftest. It fails with my original "part 3" patch, and passes with the current one.

(The spinbox is covered up in the reftest, so that we can compare it against a normal <input> textfield. It's still testing the bug brought up in comment 7 / comment 8 by virtue of the number in the textbox being visible, though.)

Comment 14

[Jonathan Kew [:jfkthame]]

Comment on attachment 8621276 [details] [diff] [review]
part 3: Combine xOffset/yOffset into a LogicalPoint (now passing borderBoxWidth to FinishReflowChild, w/ reftest)

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

LGTM, thanks!

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f9cdb5ebfa8
https://hg.mozilla.org/integration/mozilla-inbound/rev/b275d17433af
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bab6071fcd3

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0f9cdb5ebfa8
https://hg.mozilla.org/mozilla-central/rev/b275d17433af
https://hg.mozilla.org/mozilla-central/rev/0bab6071fcd3