Closed
Bug 638540
Opened 13 years ago
Closed 13 years ago
Progress element should be shown vertically when -moz-orient value is 'vertical'
Categories
(Core :: Layout: Form Controls, defect)
Core
Layout: Form Controls
Tracking
()
RESOLVED
FIXED
mozilla6
People
(Reporter: mounir, Assigned: mounir)
References
Details
(Keywords: dev-doc-complete)
Attachments
(1 file, 2 obsolete files)
33.77 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
The progress bar should go from bottom to top. See W3C bug: http://www.w3.org/Bugs/Public/show_bug.cgi?id=12232
Assignee | ||
Updated•13 years ago
|
Assignee | ||
Comment 1•13 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?
Comment 2•13 years ago
|
||
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•13 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•13 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.
Comment 8•13 years ago
|
||
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•13 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?
Comment 10•13 years ago
|
||
Dunno. We have lots of them in HTML already..... ;)
Assignee | ||
Comment 11•13 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...
Comment 12•13 years ago
|
||
That's possible yes. But CSS just can't handle this use case...
Assignee | ||
Comment 13•13 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
Comment 14•13 years ago
|
||
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•13 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•13 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•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Whiteboard: [needs review]
Assignee | ||
Comment 17•13 years ago
|
||
Don't be afraid by the patch size, it's mostly tests. The code is ~90 lines.
Assignee | ||
Comment 18•13 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)
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•13 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•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 25•13 years ago
|
||
Pushed: http://hg.mozilla.org/mozilla-central/rev/0ebaf2c9ce74
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs review]
Target Milestone: --- → mozilla6
Comment 26•13 years ago
|
||
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•13 years ago
|
||
Updated documentation: https://developer.mozilla.org/en/HTML/Element/progress Added orient doc: https://developer.mozilla.org/en/CSS/orient
Keywords: dev-doc-needed → dev-doc-complete
Assignee | ||
Comment 28•13 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 | ||
Comment 29•13 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.
Description
•