Last Comment Bug 638540 - Progress element should be shown vertically when -moz-orient value is 'vertical'
: Progress element should be shown vertically when -moz-orient value is 'vertical'
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Layout: Form Controls (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla6
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
Depends on: 655065 655960 661113
Blocks: 633207 655313 656909
  Show dependency treegraph
 
Reported: 2011-03-03 12:25 PST by Mounir Lamouri (:mounir)
Modified: 2011-06-01 03:33 PDT (History)
7 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (34.01 KB, patch)
2011-05-06 10:48 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Review
Patch v1.1 (34.02 KB, patch)
2011-05-11 08:04 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Review
Patch v1.2 (33.77 KB, patch)
2011-05-11 16:35 PDT, Mounir Lamouri (:mounir)
roc: review+
Details | Diff | Review

Description Mounir Lamouri (:mounir) 2011-03-03 12:25:32 PST
The progress bar should go from bottom to top.

See W3C bug:
http://www.w3.org/Bugs/Public/show_bug.cgi?id=12232
Comment 1 Mounir Lamouri (:mounir) 2011-03-25 10:25:25 PDT
I was thinking of creating a :-moz-vertical pseudo-class to be able to style vertical progress elements but I realize that changing width/height in a rule including :-moz-vertical might create a real mess.
By any chance, do you guys know how to prevent that? IOW, is there a way to ignore width/heigh properties inside a rule including :-moz-vertical or can that kind of situations managed someway?
Comment 2 Boris Zbarsky [:bz] 2011-03-25 11:41:18 PDT
There's no really good infrastructure for that, no....
Comment 3 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-03-25 13:44:09 PDT
Is this something you want authors to use, or just for our UA sheets?
Comment 4 Mounir Lamouri (:mounir) 2011-03-28 03:26:58 PDT
(In reply to comment #3)
> Is this something you want authors to use, or just for our UA sheets?

For both actually. For UA sheets only would be a good first step if it's easier but ideally, I would like authors to be able to do:
<style>
  progress { /* style for horizontal progress bar */ }
  progress::-moz-vertical { /* style for vertical progress bar */ }
</style>

Though, the usefulness of this is debatable. I guess it's more convenient but workarounds might be easy (for example, the author can use a 'vertical' class assuming progress bars regular use case doesn't imply changing the direction).
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-03-28 03:58:20 PDT
And what properties should be settable through a ::-moz-vertical pseudo?
Comment 6 Mounir Lamouri (:mounir) 2011-03-28 04:03:07 PDT
(In reply to comment #5)
> And what properties should be settable through a ::-moz-vertical pseudo?

Given that it's a pseudo-class, I do not see a reson to filter properties but I guess for sanity, we should filter width and height.
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-03-28 05:01:38 PDT
It's not just width and height, but anything that could affect width and height, including min/max-width/height, and anything that could affect intrinsic width and height.
Comment 8 Boris Zbarsky [:bz] 2011-03-28 05:43:21 PDT
I think the sane (read: easy) way to do this is to just have an @vertical on <progress> that the page can set as desired....
Comment 9 Mounir Lamouri (:mounir) 2011-03-28 08:59:47 PDT
(In reply to comment #8)
> I think the sane (read: easy) way to do this is to just have an @vertical on
> <progress> that the page can set as desired....

A vertical attribute would be easy, indeed, but do we really want to have an HTML attribute defining presentation details?
Comment 10 Boris Zbarsky [:bz] 2011-03-28 09:14:53 PDT
Dunno.  We have lots of them in HTML already..... ;)
Comment 11 Mounir Lamouri (:mounir) 2011-03-28 09:18:10 PDT
(In reply to comment #10)
> Dunno.  We have lots of them in HTML already..... ;)

Sure, but they are legacy stuff, right? I thought Hixie was trying to prevent adding new ones with the new HTML specs...
Comment 12 Boris Zbarsky [:bz] 2011-03-28 09:55:04 PDT
That's possible yes.  But CSS just can't handle this use case...
Comment 13 Mounir Lamouri (:mounir) 2011-03-31 04:32:14 PDT
I'm trying to find a way to show a vertical progress bar without a :-moz-vertical pseudo-class. Though, I don't see any sane way (setting a class on the progress element seems doable but might be used by the authors too). Do you guys have an idea?

FWIW, Webkit has the same issue even if they seem to have found a workaround:
https://lists.webkit.org/pipermail/webkit-dev/2011-March/016140.html
https://bugs.webkit.org/show_bug.cgi?id=54440
Comment 14 Boris Zbarsky [:bz] 2011-03-31 06:33:27 PDT
Their workaround is to explicitly change the style from C++ at the end of layout as needed.  We could do that if we had to... but yes, this is a pain to do with CSS-like stuff.
Comment 15 Mounir Lamouri (:mounir) 2011-04-18 10:26:01 PDT
So, according to the thread in the CSS WG, the plan could be to introduce a CSS property like "-moz-orient" that could have these values: [ "horizontal", "vertical" ] with the possibility to add other values to specify the direction like "horizontal-left-right", "horizontal-right-left", "vertical-bottom-up", "vertical-top-bottom". I guess we don't need to handle those values for the moment.

I'm not a CSS guy so I would love any feedback on that before trying to implement anything.
Comment 16 Mounir Lamouri (:mounir) 2011-05-06 10:48:10 PDT
Created attachment 530662 [details] [diff] [review]
Patch v1
Comment 17 Mounir Lamouri (:mounir) 2011-05-06 10:49:53 PDT
Don't be afraid by the patch size, it's mostly tests. The code is ~90 lines.
Comment 18 Mounir Lamouri (:mounir) 2011-05-10 01:52:42 PDT
Comment on attachment 530662 [details] [diff] [review]
Patch v1

Adding roc in case of he can get to that review before David.
Comment 19 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-10 15:31:16 PDT
Comment on attachment 530662 [details] [diff] [review]
Patch v1

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

I'd like your tests more if they used meaningful class names instead of a lot of nth-child.

::: layout/forms/nsProgressFrame.cpp
@@ +193,5 @@
>    nsHTMLReflowState reflowState(aPresContext, aReflowState, aBarFrame,
> +                                vertical ? nsSize(aReflowState.ComputedWidth(),
> +                                                  NS_UNCONSTRAINEDSIZE)
> +                                         : nsSize(NS_UNCONSTRAINEDSIZE,
> +                                                  aReflowState.ComputedHeight()));

You need to set the available width here even for the !vertical case.
Comment 20 Mounir Lamouri (:mounir) 2011-05-11 08:03:42 PDT
(In reply to comment #19)
> I'd like your tests more if they used meaningful class names instead of a
> lot of nth-child.

The thing is all my tests in layout/reftests/forms/progress/ use that "style". IMO, it's readable if you open both files (test and ref) at the same time and I prefer that than long and painful class names. Though, I can fix that later in a follow-up if you think it should be fixed.

> ::: layout/forms/nsProgressFrame.cpp
> @@ +193,5 @@
> >    nsHTMLReflowState reflowState(aPresContext, aReflowState, aBarFrame,
> > +                                vertical ? nsSize(aReflowState.ComputedWidth(),
> > +                                                  NS_UNCONSTRAINEDSIZE)
> > +                                         : nsSize(NS_UNCONSTRAINEDSIZE,
> > +                                                  aReflowState.ComputedHeight()));
> 
> You need to set the available width here even for the !vertical case.

I'm going to attach a new patch with that fixed but my test suite is still passing so do you have any idea of a patch that could fail with the current code?
Comment 21 Mounir Lamouri (:mounir) 2011-05-11 08:04:44 PDT
Created attachment 531633 [details] [diff] [review]
Patch v1.1
Comment 22 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-11 16:11:17 PDT
It probably depends on what kind of frame the child frame is. If it was, say, a block frame I'd expect you to get assertions when you pass in an unconstrained available width.

I actually think you should always pass in nsSize(aReflowState.ComputedWidth(), NS_UNCONSTRAINEDSIZE) for your available width/height, regardless of orientation. Setting the available height is only useful for triggering vertical breaking (e.g., columns), which you'll never want here.
Comment 23 Mounir Lamouri (:mounir) 2011-05-11 16:35:21 PDT
Created attachment 531795 [details] [diff] [review]
Patch v1.2
Comment 24 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-11 17:22:27 PDT
Comment on attachment 531795 [details] [diff] [review]
Patch v1.2

Review of attachment 531795 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 25 Mounir Lamouri (:mounir) 2011-05-17 06:26:47 PDT
Pushed:
http://hg.mozilla.org/mozilla-central/rev/0ebaf2c9ce74
Comment 26 Eric Shepherd [:sheppy] 2011-05-31 11:38:10 PDT
Is it expected that specifying -moz-orient:vertical without explicitly changing the size of the progress bar results in a ridiculously short, horizontally stretched bar (that is, the same shape as if it were horizontal)?
Comment 27 Eric Shepherd [:sheppy] 2011-05-31 11:48:29 PDT
Updated documentation:

https://developer.mozilla.org/en/HTML/Element/progress

Added orient doc:

https://developer.mozilla.org/en/CSS/orient
Comment 28 Mounir Lamouri (:mounir) 2011-06-01 03:05:24 PDT
(In reply to comment #26)
> Is it expected that specifying -moz-orient:vertical without explicitly
> changing the size of the progress bar results in a ridiculously short,
> horizontally stretched bar (that is, the same shape as if it were
> horizontal)?

Yes. And thanks for pointing that: I knew this issue but reading this comment make this of a very simple fix :)
Comment 29 Mounir Lamouri (:mounir) 2011-06-01 03:33:23 PDT
(In reply to comment #26)
> Is it expected that specifying -moz-orient:vertical without explicitly
> changing the size of the progress bar results in a ridiculously short,
> horizontally stretched bar (that is, the same shape as if it were
> horizontal)?

Bug 661113

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