Closed Bug 638540 Opened 11 years ago Closed 11 years ago

Progress element should be shown vertically when -moz-orient value is 'vertical'

Categories

(Core :: Layout: Form Controls, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla6

People

(Reporter: mounir, Assigned: mounir)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 2 obsolete files)

The progress bar should go from bottom to top.

See W3C bug:
http://www.w3.org/Bugs/Public/show_bug.cgi?id=12232
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?
There's no really good infrastructure for that, no....
Is this something you want authors to use, or just for our UA sheets?
(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).
And what properties should be settable through a ::-moz-vertical pseudo?
(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.
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.
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....
(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?
Dunno.  We have lots of them in HTML already..... ;)
(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...
That's possible yes.  But CSS just can't handle this use case...
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
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.
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.
Depends on: 655065
Summary: Progress element should be shown vertically when height>width → Progress element should be shown vertically when -moz-orient value is 'vertical'
Attached patch Patch v1 (obsolete) — Splinter Review
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Attachment #530662 - Flags: review?(dbaron)
Whiteboard: [needs review]
Don't be afraid by the patch size, it's mostly tests. The code is ~90 lines.
Blocks: 655313
Comment on attachment 530662 [details] [diff] [review]
Patch v1

Adding roc in case of he can get to that review before David.
Attachment #530662 - Flags: review?(roc)
Depends on: 655960
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.
(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?
Attached patch Patch v1.1 (obsolete) — Splinter Review
Attachment #530662 - Attachment is obsolete: true
Attachment #530662 - Flags: review?(roc)
Attachment #530662 - Flags: review?(dbaron)
Attachment #531633 - Flags: review?(roc)
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.
Attached patch Patch v1.2Splinter Review
Attachment #531633 - Attachment is obsolete: true
Attachment #531633 - Flags: review?(roc)
Attachment #531795 - Flags: review?(roc)
Comment on attachment 531795 [details] [diff] [review]
Patch v1.2

Review of attachment 531795 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #531795 - Flags: review?(roc) → review+
Blocks: 656909
Keywords: dev-doc-needed
Pushed:
http://hg.mozilla.org/mozilla-central/rev/0ebaf2c9ce74
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs review]
Target Milestone: --- → mozilla6
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)?
(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 :)
Depends on: 661113
(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
You need to log in before you can comment on or make changes to this bug.