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

RESOLVED FIXED in mozilla6

Status

()

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

({dev-doc-complete})

Trunk
mozilla6
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

8 years ago
The progress bar should go from bottom to top.

See W3C bug:
http://www.w3.org/Bugs/Public/show_bug.cgi?id=12232
Assignee

Comment 1

8 years ago
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?
Assignee

Comment 4

8 years ago
(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?
Assignee

Comment 6

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

Comment 9

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

Comment 11

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

Comment 13

8 years ago
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.
Assignee

Comment 15

8 years ago
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.
Assignee

Updated

8 years ago
Depends on: 655065
Assignee

Updated

8 years ago
Summary: Progress element should be shown vertically when height>width → Progress element should be shown vertically when -moz-orient value is 'vertical'
Assignee

Comment 16

8 years ago
Posted patch Patch v1 (obsolete) — Splinter Review
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Attachment #530662 - Flags: review?(dbaron)
Assignee

Updated

8 years ago
Whiteboard: [needs review]
Assignee

Comment 17

8 years ago
Don't be afraid by the patch size, it's mostly tests. The code is ~90 lines.
Assignee

Updated

8 years ago
Blocks: 655313
Assignee

Comment 18

8 years ago
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)
Assignee

Updated

8 years ago
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.
Assignee

Comment 20

8 years ago
(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?
Assignee

Comment 21

8 years ago
Posted 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.
Assignee

Comment 23

8 years ago
Posted 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+
Assignee

Updated

8 years ago
Blocks: 656909
Assignee

Updated

8 years ago
Keywords: dev-doc-needed
Assignee

Comment 25

8 years ago
Pushed:
http://hg.mozilla.org/mozilla-central/rev/0ebaf2c9ce74
Status: ASSIGNED → RESOLVED
Closed: 8 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)?
Assignee

Comment 28

8 years ago
(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 :)
Assignee

Updated

8 years ago
Depends on: 661113
Assignee

Comment 29

8 years ago
(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.