835883 - Give the -moz-orient property an 'auto' value, and make that its initial value

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jonathan Watt [:jwatt]]

In order to fix bug 833742 while implementing bug 833742 comment 4, we need to give the -moz-orient property an 'auto' value, and make that its initial value.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Daniel Holbert [:dholbert]]

I think this needs an update to
 https://mxr.mozilla.org/mozilla-central/source/layout/style/test/property_database.js#2789
in order to pass tests, no?

At first glance, I'd assume:
 - "horizontal" needs to move out of "initial_values" and into "other_values"
 - "auto" needs to move out of "invalid_values" and into "initial_values"

Without that change, I'd expect this to fail one or more mochitests in layout/style/test.

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 707696 [details] [diff] [review]
patch

>diff --git a/layout/forms/nsMeterFrame.cpp b/layout/forms/nsMeterFrame.cpp
> nsMeterFrame::GetMinWidth(nsRenderingContext *aRenderingContext)
[...]
>-  if (GetStyleDisplay()->mOrient == NS_STYLE_ORIENT_HORIZONTAL) {
>+  if (GetStyleDisplay()->mOrient != NS_STYLE_ORIENT_VERTICAL) {
>     minWidth *= 5; // 5em
>   }

>diff --git a/layout/forms/nsProgressFrame.cpp b/layout/forms/nsProgressFrame.cpp
> nsProgressFrame::GetMinWidth(nsRenderingContext *aRenderingContext)
[...]
>-  if (GetStyleDisplay()->mOrient == NS_STYLE_ORIENT_HORIZONTAL) {
>+  if (GetStyleDisplay()->mOrient != NS_STYLE_ORIENT_VERTICAL) {
>     minWidth *= 10; // 10em
>   }

So "auto" is just being treated as "horizontal" unconditionally in these two places.  Will bug 833742 make us do something trickier, or are these places expected to remain like this?

Probably worth adding  a comment either way, explaining (or at least stating) that the "auto" behavior here is the same as horizontal, or (if you'll be improving it) saying something like "//XXXjwatt bug 833742 will make this smarter"

Comment 4

[Jonathan Watt [:jwatt]]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> I think this needs an update to
>  https://mxr.mozilla.org/mozilla-central/source/layout/style/test/
> property_database.js#2789
> in order to pass tests, no?

Yes - I do have that locally but apparently didn't qref.

(In reply to Daniel Holbert [:dholbert] from comment #3)
> So "auto" is just being treated as "horizontal" unconditionally in these two
> places.

Yes. For <progress> and <meter> the current behavior is always to use horizontal orientation unless -moz-orient:vertical is specified. I'm just preserving that behavior.

> Will bug 833742 make us do something trickier, or are these places
> expected to remain like this?

It will be "trickier" for <input type=range>, but <progress> and <meter> will continue to behave as they do now.

> Probably worth adding  a comment

Okay.

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 6

[Mounir Lamouri (:mounir)]

(In reply to Jonathan Watt [:jwatt] from comment #4)
> It will be "trickier" for <input type=range>, but <progress> and <meter>
> will continue to behave as they do now.

I think it would be better for the platform to have a consistent behaviour. Why would <meter> and <progress> behave differently from <input type='range'>?

Comment 7

[Mounir Lamouri (:mounir)]

Comment on attachment 709367 [details] [diff] [review]
patch

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

::: layout/forms/nsMeterFrame.cpp
@@ +248,5 @@
>  
>    nscoord minWidth = fontMet->Font().size; // 1em
>  
> +  if (GetStyleDisplay()->mOrient != NS_STYLE_ORIENT_VERTICAL) {
> +    // The orientation is horizontal (-moz-orient is 'auto' or 'horizontal').

nit: if we do that, I think being explicit is better. ie:
if (GetStyleDisplay()->mOrient == NS_STYLE_ORIENT_AUTO ||
    GetStyleDisplay()->mOrient == NS_STYLE_ORIENT_HORIZONTAL) {
... with a comment explaining why.

::: layout/forms/nsProgressFrame.cpp
@@ +264,5 @@
>  
>    nscoord minWidth = fontMet->Font().size; // 1em
>  
> +  if (GetStyleDisplay()->mOrient != NS_STYLE_ORIENT_VERTICAL) {
> +    // The orientation is horizontal (-moz-orient is 'auto' or 'horizontal').

ditto

Comment 8

[Jonathan Watt [:jwatt]]

(In reply to Mounir Lamouri (:mounir) from comment #6)
> I think it would be better for the platform to have a consistent behaviour.
> Why would <meter> and <progress> behave differently from <input
> type='range'>?

<input type=range> really needs the behavior defined in bug 833742 comment 4, I think (see the discussion over there). I think it would make sense for <meter> and <progress> to behave in the same way, but think that should be a separate bug.

As for you review comments I'm fine with making those changes, but I'll wait for dholbert's review before addressing them.

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 709367 [details] [diff] [review]
patch

># HG changeset patch
># Parent d9c36d39f7bcc5b33f227030486d90c26b1881ca
># User Jonathan Watt <[e-mail]>
>Bug 835883 - Give the -moz-orient property an 'auto' value, and make that its initial value. r=dbaron.

s/dbaron/dholbert/.  ;) (I imagine you were probably going to change that before landing.)

I agree w/ mounir's suggestion. I guess that makes the comment somewhat less necessary -- feel free to drop it if you like. (but I don't object to keeping it)

So, r=dholbert, with the VERTICAL checks split out.

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to Mounir Lamouri (:mounir) from comment #6)
> I think it would be better for the platform to have a consistent behaviour.
> Why would <meter> and <progress> behave differently from <input
> type='range'>?

(I'm curious about this to, but I don't think it needs to be resolved before this lands, since this bug's patch is just adding the value, and the fancy behavior comes later.)

Comment 11

[Daniel Holbert [:dholbert]]

(Oh, sorry, I skipped over the first part of comment 8; didn't see that jwatt had replied to that already.)

Comment 12

[Jonathan Watt [:jwatt]]

Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/13654177590a

(In reply to Daniel Holbert [:dholbert] from comment #9)
> s/dbaron/dholbert/.  ;) (I imagine you were probably going to change that
> before landing.)

Actually I so rarely switch reviewer that I might have missed that - thanks for the reminder. :)

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/13654177590a

Comment 14

[Mounir Lamouri (:mounir)]

FWIW, I wonder if implementing this is needed given that Webkit doesn't implement that behaviour. We could have delayed this until we find a common behaviour with Webkit.

Comment 15

[Jean-Yves Perrier [:teoli]]

Added a note in Firefox 21 for developers and updated https://developer.mozilla.org/en-US/docs/CSS/-moz-orient.