1085311 - vertical writing-mode support for single-line <input> fields

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Jonathan Kew [:jfkthame]]

Simple testcases:

  data:text/html,<input style="writing-mode:vertical-lr;" value="foo">

  data:text/html,<div style="writing-mode:vertical-lr;">
                   hello <input value="foo"> world

Currently, on trunk build with vertical mode enabled, the input field remains horizontal, although the text within it is vertical (and so only a couple of characters are visible).

Just resizing the field with CSS to make it tall and narrow does *not* help, as the text ends up offset horizontally (depending on the height) and hence clipped from view.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: vertical text support for text control

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

I guess we need new border/padding/margin CSS properties to describe styles for the controls? e.g -moz-padding-block-start, -moz-padding-inline-end?

Comment 3

[Jonathan Kew [:jfkthame]]

For part of this, see bug 1083134 re -moz-*-{start,end}, and its blocker bug 649142. We'll need additional properties for the block direction, too.

Comment 4

[Jonathan Kew [:jfkthame]]

Comment on attachment 8515755 [details] [diff] [review]
vertical text support for text control

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

LGTM, thanks. One writing-mode fix as noted below, please.

(Just FYI, I also have some baseline- and fontmetrics-related patches coming in other bugs that will improve the results this gives, especially for vertical-rl mode.)

::: layout/forms/nsTextControlFrame.cpp
@@ +425,5 @@
>      DISPLAY_PREF_WIDTH(this, result);
>  
>      float inflation = nsLayoutUtils::FontSizeInflationFor(this);
> +    // We don't care about the writing mode here
> +    WritingMode wm;

This seems wrong in principle; CalcIntrinsicSize could in theory return a different result for vertical vs horizontal modes.

I suspect in practice it doesn't matter (AFAICS, it would only become significant if scrollbars are present and the vertical and horizontal bars have different widths), but let's pass the right value anyway.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Jonathan Kew (:jfkthame) from comment #4)
> Comment on attachment 8515755 [details] [diff] [review]
> vertical text support for text control
> 
> Review of attachment 8515755 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM, thanks. One writing-mode fix as noted below, please.
> 
> (Just FYI, I also have some baseline- and fontmetrics-related patches coming
> in other bugs that will improve the results this gives, especially for
> vertical-rl mode.)
> 
> ::: layout/forms/nsTextControlFrame.cpp
> @@ +425,5 @@
> >      DISPLAY_PREF_WIDTH(this, result);
> >  
> >      float inflation = nsLayoutUtils::FontSizeInflationFor(this);
> > +    // We don't care about the writing mode here
> > +    WritingMode wm;
> 
> This seems wrong in principle; CalcIntrinsicSize could in theory return a
> different result for vertical vs horizontal modes.

I forgot that it is always possible to get the writing mode from frame...

BTW, is this bug only for the single line text <input>? Button ones and others depend on the new CSS properties I think.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Attachment: vertical text support for text control, r=jfkthame

Comment 7

[Jonathan Kew [:jfkthame]]

(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #5)
> BTW, is this bug only for the single line text <input>? 

Yes, I was intending this bug to be specifically about single-line text <input>. 

> Button ones and
> others depend on the new CSS properties I think.

Right; and they'll involve different frame classes. So we can file separate bugs as needed.

Comment 8

[Jonathan Kew [:jfkthame]]

Comment on attachment 8516277 [details] [diff] [review]
vertical text support for text control, r=jfkthame

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

::: layout/forms/nsTextControlFrame.cpp
@@ +142,5 @@
>  }
>  
>  nsresult
>  nsTextControlFrame::CalcIntrinsicSize(nsRenderingContext* aRenderingContext,
> +                                      const WritingMode& aWM,

Oh, one other thing - we usually pass WritingMode parameters by value rather than const reference, as they're only an 8-bit value. Did you have a specific reason to use the reference here? If not, let's remove it.

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch, r=jfkthame

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ca9c10e6001a

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4845f1221278

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4845f1221278