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

RESOLVED FIXED in mozilla21

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla21
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 707696 [details] [diff] [review]
patch
Attachment #707696 - Flags: review?(dbaron)
(Assignee)

Updated

4 years ago
Component: DOM: Core & HTML → Style System (CSS)
(Assignee)

Updated

4 years ago
Attachment #707696 - Flags: review?(dbaron) → review?(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 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"
(Assignee)

Comment 4

4 years ago
(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.
(Assignee)

Comment 5

4 years ago
Created attachment 709367 [details] [diff] [review]
patch
Attachment #709367 - Flags: review?(dholbert)
(Assignee)

Updated

4 years ago
Attachment #707696 - Attachment is obsolete: true
Attachment #707696 - Flags: review?(dholbert)
(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 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
(Assignee)

Comment 8

4 years ago
(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 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.
Attachment #709367 - Flags: review?(dholbert) → review+
(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.)
(Oh, sorry, I skipped over the first part of comment 8; didn't see that jwatt had replied to that already.)
(Assignee)

Comment 12

4 years ago
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. :)
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/13654177590a
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
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.
Added a note in Firefox 21 for developers and updated https://developer.mozilla.org/en-US/docs/CSS/-moz-orient.
Keywords: dev-doc-needed → dev-doc-complete
relnote-firefox: --- → ?
relnote-firefox: ? → ---
(Assignee)

Updated

4 years ago
Blocks: 854884
You need to log in before you can comment on or make changes to this bug.