Closed
Bug 1028716
Opened 10 years ago
Closed 9 years ago
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
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: jfkthame, Assigned: jfkthame)
References
(Blocks 2 open bugs)
Details
(Keywords: dev-doc-needed, site-compat)
Attachments
(4 files, 3 obsolete files)
3.75 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
7.69 KB,
patch
|
smontagu
:
review+
|
Details | Diff | Splinter Review |
6.44 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
13.69 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
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').
-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.)
Assignee | ||
Comment 3•10 years ago
|
||
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
Flags: needinfo?(smontagu)
Comment 4•10 years ago
|
||
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.
Flags: needinfo?(smontagu)
Assignee | ||
Comment 5•10 years ago
|
||
(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.
Assignee | ||
Updated•10 years ago
|
Summary: make the 'auto' value of -moz-orient respect vertical writing modes, and add an 'orthogonal' value → make the 'auto' value of -moz-orient for <progress> and <meter> respect vertical writing modes, and add 'inline-axis' and 'block-axis' values
Assignee | ||
Comment 6•10 years ago
|
||
This just adds the suggested new values for -moz-orient; code to actually handle them follows.
Attachment #8445354 -
Flags: review?(dbaron)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8445355 -
Flags: review?(smontagu)
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8445356 -
Flags: review?(roc)
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)
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 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.
Flags: needinfo?(jfkthame)
OS: Mac OS X → All
Hardware: x86 → All
Assignee | ||
Comment 12•10 years ago
|
||
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.... :(
Flags: needinfo?(jfkthame) → needinfo?(jwatt)
Attachment #8445356 -
Flags: review?(roc) → review+
Assignee | ||
Comment 13•10 years ago
|
||
(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.
Assignee | ||
Comment 14•10 years ago
|
||
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...
Attachment #8447091 -
Flags: review?(dbaron)
Assignee | ||
Updated•10 years ago
|
Attachment #8445354 -
Attachment is obsolete: true
Attachment #8445354 -
Flags: review?(dbaron)
Assignee | ||
Comment 15•10 years ago
|
||
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.
Attachment #8447092 -
Flags: review?(smontagu)
Assignee | ||
Updated•10 years ago
|
Attachment #8445355 -
Attachment is obsolete: true
Attachment #8445355 -
Flags: review?(smontagu)
Comment 16•10 years ago
|
||
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
Attachment #8447092 -
Flags: review?(smontagu) → review+
Assignee | ||
Comment 17•10 years ago
|
||
(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.
Assignee | ||
Comment 18•10 years ago
|
||
s/GetAutoSize/ComputeAutoSize/, of course.
Assignee | ||
Comment 19•10 years ago
|
||
: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•10 years ago
|
||
(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.
Flags: needinfo?(jwatt)
Assignee | ||
Comment 21•10 years ago
|
||
(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?
Flags: needinfo?(jwatt)
Comment 22•10 years ago
|
||
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. :(
Flags: needinfo?(jwatt)
Assignee | ||
Comment 23•10 years ago
|
||
(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•10 years ago
|
||
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•10 years ago
|
||
(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.
Assignee | ||
Comment 26•10 years ago
|
||
Regarding the input type=range aspect of this, see also http://code.google.com/p/chromium/issues/detail?id=341071.
Comment 27•10 years ago
|
||
And how we came to have an attribute for range:
http://lists.w3.org/Archives/Public/www-style/2013Mar/0370.html
Assignee | ||
Comment 28•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
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... :/
(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 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.
Attachment #8447091 -
Flags: review?(dbaron) → review-
Updated•10 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Updated•9 years ago
|
Blocks: enable-writing-mode-release
Assignee | ||
Comment 33•9 years ago
|
||
(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?
Assignee | ||
Comment 34•9 years ago
|
||
Attachment #8595377 -
Flags: review?(dbaron)
Assignee | ||
Comment 35•9 years ago
|
||
Also, a collection of tests to check that the new values are behaving as expected.
Attachment #8595379 -
Flags: review?(dbaron)
If attachment 8447091 [details] [diff] [review] is obsolete, could you mark it as such?
Flags: needinfo?(jfkthame)
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)
Attachment #8595377 -
Flags: review?(dbaron) → review+
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
Attachment #8595379 -
Flags: review?(dbaron) → review+
Assignee | ||
Updated•9 years ago
|
Attachment #8447091 -
Attachment is obsolete: true
Flags: needinfo?(jfkthame)
Assignee | ||
Comment 39•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Summary: make the 'auto' value of -moz-orient for <progress> and <meter> respect vertical writing modes, and add 'inline-axis' and 'block-axis' values → 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
Assignee | ||
Comment 40•9 years ago
|
||
Assignee | ||
Comment 41•9 years ago
|
||
Comment 42•9 years ago
|
||
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•9 years ago
|
||
Assignee | ||
Comment 44•9 years ago
|
||
(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.
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
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•9 years ago
|
Keywords: site-compat
Comment 46•9 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•