Last Comment Bug 835883 - Give the -moz-orient property an 'auto' value, and make that its initial value
: Give the -moz-orient property an 'auto' value, and make that its initial value
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla21
Assigned To: Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
:
Mentors:
Depends on:
Blocks: 854884 833742
  Show dependency treegraph
 
Reported: 2013-01-29 09:31 PST by Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
Modified: 2013-03-26 07:19 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (4.97 KB, patch)
2013-01-29 09:43 PST, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
no flags Details | Diff | Review
patch (5.96 KB, patch)
2013-02-02 02:44 PST, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
dholbert: review+
Details | Diff | Review

Description Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2013-01-29 09:31:32 PST
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] (Away Jun. 27 - Jul. 13) 2013-01-29 09:43:44 PST
Created attachment 707696 [details] [diff] [review]
patch
Comment 2 Daniel Holbert [:dholbert] 2013-02-01 13:54:24 PST
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] 2013-02-01 14:10:23 PST
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] (Away Jun. 27 - Jul. 13) 2013-02-02 02:43:29 PST
(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] (Away Jun. 27 - Jul. 13) 2013-02-02 02:44:31 PST
Created attachment 709367 [details] [diff] [review]
patch
Comment 6 Mounir Lamouri (:mounir) 2013-02-02 06:26:10 PST
(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) 2013-02-02 06:27:58 PST
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] (Away Jun. 27 - Jul. 13) 2013-02-02 06:43:47 PST
(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] 2013-02-02 09:18:45 PST
Comment on attachment 709367 [details] [diff] [review]
patch

># HG changeset patch
># Parent d9c36d39f7bcc5b33f227030486d90c26b1881ca
># User Jonathan Watt <jwatt@jwatt.org>
>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] 2013-02-02 09:19:45 PST
(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] 2013-02-02 09:21:53 PST
(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] (Away Jun. 27 - Jul. 13) 2013-02-02 10:16:58 PST
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) 2013-02-03 12:41:25 PST
https://hg.mozilla.org/mozilla-central/rev/13654177590a
Comment 14 Mounir Lamouri (:mounir) 2013-02-05 05:28:37 PST
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] 2013-02-06 02:12:13 PST
Added a note in Firefox 21 for developers and updated https://developer.mozilla.org/en-US/docs/CSS/-moz-orient.

Note You need to log in before you can comment on or make changes to this bug.