1028716 - update values of -moz-orient for <progress> and <meter> to remove 'auto', and add 'inline' (new initial value) and 'block' values with writing-mode support

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jonathan Kew [:jfkthame]]

Currently, specifying -moz-orient:auto (or simply omitting it, as this is the initial value) on a progress bar or meter is synonymous with -moz-orient:horizontal.

For vertical writing modes, I think it's pretty obvious that the default for these elements should change to vertical. To achieve this, I suggest that the 'auto' value should behave as 'horizontal' in horizontal writing mode, but as 'vertical' in vertical modes.

In addition, it would seem logical to provide a value that gives the opposite result: a vertical control in horizontal modes, and vice versa. I suggest the name 'orthogonal' for this value.

(The explicit 'horizontal' and 'vertical' values would retain their physical meanings, regardless of writing mode.)

An alternative (which I think I might prefer, if -moz-orient:auto didn't already exist) would be to replace 'auto' with 'inline-dir', and add 'block-dir' as the alternative.

cc'ing some people who appear to have been involved in the initial -moz-orient:auto. Any opinions/comments welcome.

Comment 1

[Jonathan Kew [:jfkthame]]

Hmm, I see that for -moz-box-orient, we apparently have 'horizontal', 'vertical', 'inline-axis' and 'block-axis'. Perhaps we should use the same values for -moz-orient, then. And keep 'auto' for the behavior described in bug 835883, where the orientation depends on the height and width of the element; but where height and width are not explicitly set, 'auto' would behave like 'inline-axis' (rather than 'horizontal').

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

-moz-box-orient is an old XUL box property that should go away; we shouldn't try being consistent with it for consistency's sake.  If you think the values are good, that's fine, though.  (But it is worth being consistent with values in actual CSS specs.)

Comment 3

[Jonathan Kew [:jfkthame]]

I don't see any current CSS property with values that would be useful here; the nearest thing seems to be text-orientation, but its values are too text-specific and tied to the various CJK-related glyph orientation options. (I don't think it'd be at all clear what 'upright' and 'sideways' should mean with respect to orienting a progress bar.)

The terms "block axis" and "inline axis" are explicitly defined in Writing Modes, however,[1] which suggests these are reasonable values to use.

Simon, do you have an opinion about the appropriate options for -moz-orient? I've got patches to add 'inline-axis' and 'block-axis' values, with the corresponding updates in layout and widget, which I'll post for review if you agree this is the way to go.

(We may also want to propose the 'orient' property for standardization, if we're happy that we have a sensible model for what it should mean.)

[1] http://www.w3.org/TR/css3-writing-modes/#abstract-dimensions

Comment 4

[Simon Montagu :smontagu]

You mean add 'inline-axis' and 'block-axis' and keep all the existing values as they are (with 'auto' changing to default to 'inline-axis')?
That sounds good to me, better than 'orient: inline-dir' which feels tautologous.

Comment 5

[Jonathan Kew [:jfkthame]]

(In reply to Simon Montagu :smontagu from comment #4)
> You mean add 'inline-axis' and 'block-axis' and keep all the existing values
> as they are (with 'auto' changing to default to 'inline-axis')?

Right, for <progress> and <meter>, the 'auto' would simply be equivalent to 'inline-axis'.

I'm not entirely clear on the current status of <input type=range>, which was the original motivation for adding the 'auto' value in bug 835883; there's discussion in bug 833742, but that bug remains open. Maybe it's time to re-examine that as well, and finish whatever is currently lacking.

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: part 1 - add 'inline-axis' and 'block-axis' values for -moz-orient.

This just adds the suggested new values for -moz-orient; code to actually handle them follows.

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: part 2 - handle the new orient values in <progress> and <meter> layout.

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: part 3 - widget updates to support the extended set of -moz-orient values.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8445354 [details] [diff] [review]
part 1 - add 'inline-axis' and 'block-axis' values for -moz-orient.

If auto and inline-axis mean the same thing, should we file a followup on switching the initial value to inline-axis and then removing auto, or maybe even do that here?

Also, should you be making the same changes to the orient attribute, and proposing them to the whatwg list?

Also, can we get this proposed to www-style if it hasn't been already?

All I have found so far is:
http://lists.w3.org/Archives/Public/www-style/2011Apr/0386.html
http://lists.w3.org/Archives/Public/www-style/2013Mar/0365.html
http://lists.w3.org/Archives/Public/public-whatwg-archive/2013Mar/0268.html
(and threads following)

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It seems like you're also missing changes to forms.css -- although it also seems like forms.css is assuming that the 'orient' property matches the orient attribute, which probably seems like a bad assumption.

Also, it's not clear to me why we have both an attribute and a property - should we be picking one?

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8445355 [details] [diff] [review]
part 2 - handle the new orient values in <progress> and <meter> layout.

>+// XXX This should actually be GetMinISize
> nscoord
> nsProgressFrame::GetMinWidth(nsRenderingContext *aRenderingContext)

>+// XXX GetPrefISize
> nscoord
> nsProgressFrame::GetPrefWidth(nsRenderingContext *aRenderingContext)

These comments seem out of place given that the method is an override of a method on nsIFrame.

Comment 12

[Jonathan Kew [:jfkthame]]

Hmm... so I missed the prior question of whether this should be a property or an attribute. (I started from wanting to handle writing modes properly in nsProgressFrame, and things kinda grew from there...)

So AFAICT, we currently support the 'orient' attribute on <input type=range> (though I can't find any reference to it in the specs), and the '-moz-orient' property (also not in any spec) on <meter> and <progress> elements. That does seem unnecessarily confusing.

The discussion in bug 833742 (unresolved) was about -moz-orient (CSS property) on <input type=range>, and didn't mention the orient attribute. :jwatt, can you shed any light on where we might be going here? It looks like the discussions you started on www-style and whatwg trailed off without any real conclusions regarding the specs.... :(

Comment 13

[Jonathan Kew [:jfkthame]]

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #11)
> Comment on attachment 8445355 [details] [diff] [review]
> part 2 - handle the new orient values in <progress> and <meter> layout.
> 
> >+// XXX This should actually be GetMinISize
> > nscoord
> > nsProgressFrame::GetMinWidth(nsRenderingContext *aRenderingContext)
> 
> >+// XXX GetPrefISize
> > nscoord
> > nsProgressFrame::GetPrefWidth(nsRenderingContext *aRenderingContext)
> 
> These comments seem out of place given that the method is an override of a
> method on nsIFrame.

True. These were really a note to smontagu and myself that we probably want to change these method names (globally) at some point, so as to make reading the code that uses them less confusing.

No real need to include them here, but expect to see a bug filed for that renaming. Though it's going to be another of those cases where we have to decide whether the code should stick to CSS naming (which becomes "illogical" in vertical writing modes) or should reflect how it's actually working. In this case, I think I lean towards the latter.

Comment 14

[Jonathan Kew [:jfkthame]]

Attachment: part 1 - remove the 'auto' value of -moz-orient, and add 'inline' (new initial value) and 'block'.

Updated to track the current www-style discussion (although this is not necessarily finalized yet). This does not involve forms.css because it is addressing only the CSS property (used by <progress> and <meter>), and not the HTML attribute (used by <input type=range>). We probably want to do something there, too, but still need to decide exactly what...

Comment 15

[Jonathan Kew [:jfkthame]]

Attachment: part 2 - handle the new orient values in <progress> and <meter> layout.

Updated to track the current www-style discussion, and rebased on top of the proposed change in bug 1031241. If we decide against that rename, then the obvious adjustments will apply here.

Comment 16

[Simon Montagu :smontagu]

Comment on attachment 8447092 [details] [diff] [review]
part 2 - handle the new orient values in <progress> and <meter> layout.

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

::: layout/forms/nsMeterFrame.cpp
@@ +241,2 @@
>  
> +  if (ResolvedOrientationIsVertical() == GetWritingMode().IsVertical()) {

This condition comes up a couple of times. What about encapsulating it, or would that be overkill?

I'm not sure about an appropriate name. With the condition reversed, it could be IsOrthogonal, but I can't think of an antonym for that.

r=me however you decide on that

Comment 17

[Jonathan Kew [:jfkthame]]

(In reply to Simon Montagu :smontagu from comment #16)
> Comment on attachment 8447092 [details] [diff] [review]
> part 2 - handle the new orient values in <progress> and <meter> layout.
> 
> Review of attachment 8447092 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/forms/nsMeterFrame.cpp
> @@ +241,2 @@
> >  
> > +  if (ResolvedOrientationIsVertical() == GetWritingMode().IsVertical()) {
> 
> This condition comes up a couple of times. What about encapsulating it, or
> would that be overkill?
> 
> I'm not sure about an appropriate name. With the condition reversed, it
> could be IsOrthogonal, but I can't think of an antonym for that.

Perhaps simply IsInlineOriented()?

> 
> r=me however you decide on that

I'm inclined to leave it for now, in the expectation that it'll be revised again soon, when GetAutoSize goes to logical coordinates. Overall, I think we may end up with fewer places we need to test this, and may want to switch the test over to logical rather than physical terms anyway.

Comment 18

[Jonathan Kew [:jfkthame]]

s/GetAutoSize/ComputeAutoSize/, of course.

Comment 19

[Jonathan Kew [:jfkthame]]

:dbaron, given the generally favorable response on www-style[1], could we go forward with the changes in patch 1 here, as a step towards the expected eventual standard?

There will be several issues remaining to be handled in future bugs: there's interest in adding a 'circular' value, which we don't yet have any support for; the patches here do not address <input type=range> at all (comment 14); and our current property is -moz-prefixed, so at some point we'll want to drop the -moz- if the WG is going to accept this.

[1] Thread at http://lists.w3.org/Archives/Public/www-style/2014Jun/0396.html and responses.

Comment 20

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Kew (:jfkthame) from comment #5)
> I'm not entirely clear on the current status of <input type=range>, which
> was the original motivation for adding the 'auto' value in bug 835883;
> there's discussion in bug 833742, but that bug remains open. Maybe it's time
> to re-examine that as well, and finish whatever is currently lacking.

I just wontfix'ed that bug.

Maybe at some future point we'll want an auto value, but currently I don't think we have a need for it.

Regarding making this a property or attribute, it should probably be a property, and I don't recall a good reason for it not being so on range.

Comment 21

[Jonathan Kew [:jfkthame]]

(In reply to Jonathan Watt [:jwatt] from comment #20)
> Regarding making this a property or attribute, it should probably be a
> property, and I don't recall a good reason for it not being so on range.

I'm guessing you made it an attribute in order to be able to select on it in forms.css:

  http://mxr.mozilla.org/mozilla-central/source/layout/style/forms.css#849

If we remove the attribute, and use the (-moz-)orient property to control the orientation of range, how will we handle the styling that's currently done here?

Comment 22

[Jonathan Watt [:jwatt]]

Oh, right. It was the thumb styling that forced me to make it an attribute. I'm not sure there is a way to replace that styling. :(

Comment 23

[Jonathan Kew [:jfkthame]]

(s/thumb/track/, I think.)

How terrible would it be if we simplified this so that the track just has the same gray border all around (i.e. the ends as well as the sides)? Then it wouldn't need orientation-dependent border styling.

There'd still be the size (width/height) of the track to deal with, but maybe they could be made to do the appropriate thing by default via some magic in the code.

Authors who explicitly set a physical orientation for the range could of course re-style the track as desired; and authors who don't style it at all will get the system appearance anyway, so none of this matters then AFAICS. (Not sure if that's true on all platforms, though?)

Comment 24

[Jonathan Watt [:jwatt]]

We may be styling the track based on orient there, but the thumb is really more of an issue. When you want to do things like add a point to one of the sides depending on orient, for example like this:

http://www.java2s.com/Tutorial/PythonImages/Slidercontrol.PNG

you need to be able to select against the orient.

Comment 25

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Kew (:jfkthame) from comment #23)
> Authors who explicitly set a physical orientation for the range could of
> course re-style the track as desired;

Maybe. In the case of authors pulling in a library of code and styles though, it's going to be clunky to have to specify the orient property, plus some attribute or something that the libraries style sheets can select against to get the library styling right.

Comment 26

[Jonathan Kew [:jfkthame]]

Regarding the input type=range aspect of this, see also http://code.google.com/p/chromium/issues/detail?id=341071.

Comment 27

[Jonathan Watt [:jwatt]]

And how we came to have an attribute for range:

http://lists.w3.org/Archives/Public/www-style/2013Mar/0370.html

Comment 28

[Jonathan Kew [:jfkthame]]

If we replace range's orient attribute with the css -moz-orient property, perhaps we could solve the styling problem by splitting ::-moz-range-{track,thumb} into ::-moz-range-{track,thumb}-{vertical,horizontal} pseudo-elements. Then we'd suppress the display of whichever pair of them are not relevant for the range's actual direction.

Comment 29

[Jonathan Watt [:jwatt]]

We'd need to create the native anonymous content for both the vertical and horizontal pseudo-elements since we don't know the computed value of '-moz-orient' at frame construction time, but we could suppress the non-applicable pseudo-elements in nsRangeFrame::Reflow/BuildDisplayList.

This could be the best idea so far, although I would note that we're considering adding more pseudo-elements in bug 855149, and likely we'd need similar vertical/horizontal variants of those.

Comment 30

[Jonathan Watt [:jwatt]]

Hmm, this still doesn't solve the issue of having the thumb chamfer on the right vs. the left side for vertical range based on block-dir...unless we had a further split of the pseudo-elements... :/

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Jonathan Watt [:jwatt] from comment #29)
> We'd need to create the native anonymous content for both the vertical and
> horizontal pseudo-elements since we don't know the computed value of
> '-moz-orient' at frame construction time,

Why not?  We generally know computed values of properties in frame construction.  We'd just need to change the style change handling (nsStyle*::CalcDifference) so a change in that property would retrigger frame construction.

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8447091 [details] [diff] [review]
part 1 - remove the 'auto' value of -moz-orient, and add 'inline' (new initial value) and 'block'.

Sorry, I missed comment 19.  (I probably should have cancelled or minused the original review request.)

I guess I'm ok with this conceptually (although we still need to figure out what to do with the attribute).

However, if you're adding new BLOCK and INLINE values you need to make sure that all the users of nsStyleDisplay::mOrient handle those values appropriately.
http://mxr.mozilla.org/mozilla-central/search?string=-%3EmOrient
We shouldn't accept values at parse time if we don't do the right thing with them.

That requires additional changes to nsProgressFrame and nsMeterFrame, and also requires touching the nsNativeTheme* code that looks at them.

It's also fine if you use this patch to remove 'auto', and then add inline and block in a later patch.

Comment 33

[Jonathan Kew [:jfkthame]]

(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #32)
> That requires additional changes to nsProgressFrame and nsMeterFrame, and
> also requires touching the nsNativeTheme* code that looks at them.

That's what the (separately-reviewed) part 2 and part 3 patches do. I believe with those, the new values are handled appropriately throughout.

I'll attach an updated "part 1" patch for re-review in the light of this; or would you prefer to see a combined patch of parts 1-3 that includes all the code changes?

Comment 34

[Jonathan Kew [:jfkthame]]

Attachment: part 1 - Remove the 'auto' value of the -moz-orient property, and add 'inline' (new initial value) and 'block'

Comment 35

[Jonathan Kew [:jfkthame]]

Attachment: Reftests for <progress> and <meter> with various writing-mode and -moz-orient combinations

Also, a collection of tests to check that the new values are behaving as expected.

Comment 36

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

If attachment 8447091 [details] [diff] [review] is obsolete, could you mark it as such?

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8595377 [details] [diff] [review]
part 1 - Remove the 'auto' value of the -moz-orient property, and add 'inline' (new initial value) and 'block'

r=dbaron (when landed along with patch 2)

Comment 38

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8595379 [details] [diff] [review]
Reftests for <progress> and <meter> with various writing-mode and -moz-orient combinations

I think it would be good to drop the bug number from the filenames.

You should also stick in a != test between the various pairs of references.

I think it's also better to put the tests in layout/reftests/meter/ and layout/reftests/progress/.

r=dbaron with that

Comment 39

[Jonathan Kew [:jfkthame]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1bce18ffe999

Comment 40

[Jonathan Kew [:jfkthame]]

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

Comment 41

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2204929c7540
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a39e04e9ab1
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9810c65cd69
https://hg.mozilla.org/integration/mozilla-inbound/rev/03da3b6d21b9

Comment 42

[Ryan VanderMeulen [:RyanVM]]

progress-orient-vertical.html is getting unexpected fails on OSX 10.10.
https://treeherder.mozilla.org/logviewer.html#?job_id=9187561&repo=mozilla-inbound

Comment 43

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e20b97847244

Comment 44

[Jonathan Kew [:jfkthame]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #42)
> progress-orient-vertical.html is getting unexpected fails on OSX 10.10.
> https://treeherder.mozilla.org/logviewer.html#?job_id=9187561&repo=mozilla-
> inbound

That's odd, it works for me locally on 10.10. Oh well, that test is not a big deal; thanks for fixing up the annotation.

Comment 45

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2204929c7540
https://hg.mozilla.org/mozilla-central/rev/7a39e04e9ab1
https://hg.mozilla.org/mozilla-central/rev/f9810c65cd69
https://hg.mozilla.org/mozilla-central/rev/03da3b6d21b9
https://hg.mozilla.org/mozilla-central/rev/e20b97847244

Comment 46

[Jean-Yves Perrier [:teoli]]

Updated:
https://developer.mozilla.org/en-US/docs/Web/CSS/-moz-orient
and
https://developer.mozilla.org/en-US/Firefox/Releases/40#CSS

I keep the ddn on the bug for kohei and his Compat page.