Closed Bug 567872 Opened 14 years ago Closed 13 years ago

Implement the basic layout of the progress element

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla6

People

(Reporter: mounir, Assigned: mounir)

References

Details

(Keywords: html5)

Attachments

(2 files, 11 obsolete files)

This bug is about the layout/rendering of the HTML5 progress element (bug 514437 for the content).
Attached patch Patch - WIP (obsolete) — Splinter Review
This patch is still WIP. Actually, I have a weird issue: when I am (un)zooming, the progress bar doesn't update correctly. If I call nsBlockFrame::Reflow() _after_ updating the bar, the bar takes 100% of the width. If I do that before (like in the patch), the bar size is really approximative. The next reflow re-set the bar to the correct size.
Robert, would you have any idea of what I'm doing wrong here ?
Attachment #447550 - Flags: feedback?(roc)
This is weird since you're reflowing the child div using normal block reflow and then overriding its position manually.

I suggest not inheriting from nsBlockFrame and just writing your own implementation of Reflow that sizes and positions the child. nsGfxButtonControlFrame would be a good example to follow (its reflow method is in nsHTMLButtonControlFrame::Reflow).
Are you using the OS's native progress bars for styling, or have you come up with your own?
(In reply to comment #3)
> Are you using the OS's native progress bars for styling, or have you come up
> with your own?

This is using the OS native progress bar like xul:progressmeter. IOW, it's using -moz-appearance: progressbar; and -moz-appearance: progresschunk;.
It's working great with GTK. It looks like there is a bug on Windows (at least 7) and I have to check on MacOS X.
Attached patch Patch - WIP (obsolete) — Splinter Review
This should use the native rendering for MacOS X, Windows XP, Windows 7 and GNU/Linux (GTK). And there is a default rendering in case of there is no native rendering for the platform.

Some issues specific to platform:
 - on windows, there is a bug in progresschunk (see bug 568825), I had to add "overflow: hidden;" to prevent that.
 - on MacOS X, the progress bar mechanism is quite ugly: when an element use "-moz-appearance: progressbar;", the widget look at @value, @max and @mode to know how to show the progressbar. So there is no need for an anonymous element, that's why there is a lot of #IFDEF MOZ_WIDGET_COCOA in the patch. This is also breaking RTL on MacOS X. In addition, the MacOS X widget has a fixed height... IOW, it's a pain.

Another issue is for the 'undeterminated state'. MacOS has a native look I could use but there is nothing on GTK and I'm wondering what should be done for Windows. I don't like the way it's done in xul:progressmeter.

So, roc, could you give me your feedback on this patch ? I do not ask for a review because I don't know how I should manage the issues mentioned before, there is no tests atm and there other stuff (see TODO's in the code).
Attachment #447550 - Attachment is obsolete: true
Attachment #448018 - Flags: feedback?(roc)
Attachment #447550 - Flags: feedback?(roc)
CCing mstange to have his opinion about the MacOS widget.
You're still inheriting from nsBlockFrame, and I still think you should be inheriting from nsHTMLContainerFrame instead, instead of calling nsBlockFrame::Reflow and then overriding the child frame's position.

But we also need to think about supporting author styling here ...

Maybe instead of the Cocoa #ifdefs we could just make moz-appearance:progresschunk draw nothing?
(In reply to comment #7)
> You're still inheriting from nsBlockFrame, and I still think you should be
> inheriting from nsHTMLContainerFrame instead, instead of calling
> nsBlockFrame::Reflow and then overriding the child frame's position.

Hmmm, ok.

> But we also need to think about supporting author styling here ...

I think a pseudo-element like ::-moz-progress-bar to be able to style the anonymous div would be great. However, I've not be able to do that for ::-moz-placeholder. IIRC, Boris told me the our style system isn't really ready for that kind of pseudo-element. (CCing Boris so he may say more)
Do you see any other issue related to author styling ?

> Maybe instead of the Cocoa #ifdefs we could just make
> moz-appearance:progresschunk draw nothing?

I may try but that would make us create an anonymous div for nothing. Do you think it worths it ? And I suppose the change would have to be done in the css with %if or in C++ in the frame code, right ?
(In reply to comment #8)
> I think a pseudo-element like ::-moz-progress-bar to be able to style the
> anonymous div would be great. However, I've not be able to do that for
> ::-moz-placeholder. IIRC, Boris told me the our style system isn't really ready
> for that kind of pseudo-element. (CCing Boris so he may say more)
> Do you see any other issue related to author styling ?

Not right now. We really need a spec for author styling though since it will constrain how we want to implement these controls.

> > Maybe instead of the Cocoa #ifdefs we could just make
> > moz-appearance:progresschunk draw nothing?
> 
> I may try but that would make us create an anonymous div for nothing. Do you
> think it worths it ?

Yes, I think that's fine. We'll need the anonymous div for author styling on Mac, anyway.

> And I suppose the change would have to be done in the css
> with %if or in C++ in the frame code, right ?

No, we should just do it in nsNativeThemeCocoa. ThemeSupportsWidget should return true for progresschunk, but then drawing the progresschunk should do nothing.
I'm trying to recall what the issue was with the pseudo-element in the moz-placeholder case, other than the fact that the frame constructor didn't know it needed to create the different style context....  Do you remember what problems you ran into?

It would be good to have a description of exactly what we want to do with pseudo-elements for form controls in general; then we can see what changes are needed to support it.
We want to map style rules for a pseudo-element to apply to a particular anonymous child of the base element. I guess that means the nsStyleContext for the pseudoelement gets assigned to the base element's frame. What particular issues do we need clarification on?
Is it always an anonymous child, or can it be a deeper anonymous descendant?
Good question. I don't know if we'll need non-child descendants.
Maybe we will for elements like <input type='number'> where we there will be a <div> containing the two buttons. The buttons are not direct descendants but we may want to style them.
Agreed with volkmar. One example of this is the ::value pseudo-element:

http://www.w3.org/TR/css3-ui/#pseudo-value

We'll likely have to make up some number of -moz- prefixed pseudo-elements to enable styling of the pieces of a form element like the two buttons that volkmar refers to.

Documenting some thoughts on this here: https://wiki.mozilla.org/Tantek-Mozilla-projects#Styling_HTML5_forms_elements
Depends on: 569216
(In reply to comment #10)
> I'm trying to recall what the issue was with the pseudo-element in the
> moz-placeholder case, other than the fact that the frame constructor didn't
> know it needed to create the different style context....  Do you remember what
> problems you ran into?

I remember now. The issue was we wanted to have -moz-placeholder limited to only some CSS properties (like first-letter) so we were not able to use -moz-placeholder to set the default style of the placeholder (like setting visibility: hidden). Maybe we could use -moz-progressbar to style the progress bar as we would not run into that kind of issue. I will try that.
If we're going to need random non-child descendants stylable as pseudo-elements, then the "parent to inherit from" code will need to be changed to support that.  We will also need to figure out a way to pass the relevant information out via the native anonymous content API so the frame ctor can create the right style contexts to start with.
Attached patch Patch - WIP (obsolete) — Splinter Review
nsProgressFrame is now inheriting from nsHTMLContainerFrame and redefines it own Reflow method. There is also a -moz-progress-bar pseudo element working.
I will do some cleaning and tweaks tomorrow. Until now, all feedbacks are welcome.
Attachment #448018 - Attachment is obsolete: true
Attachment #448650 - Flags: feedback?(roc)
Attachment #448018 - Flags: feedback?(roc)
Attachment #448650 - Attachment is patch: true
Attachment #448650 - Attachment mime type: application/octet-stream → text/plain
We probably need to do something to make ReResolveStyleContext work properly when we use these kinds of pseudo-elements. Boris or dbaron would tell you.

What's the goal of extrawidth?

You should size and position the child by setting up the right state and calling ReflowChild, instead of doing it last.
Attached patch Patch - WIP (obsolete) — Splinter Review
(In reply to comment #19)
> We probably need to do something to make ReResolveStyleContext work properly
> when we use these kinds of pseudo-elements. Boris or dbaron would tell you.
> 
> What's the goal of extrawidth?

None.

> You should size and position the child by setting up the right state and
> calling ReflowChild, instead of doing it last.

I did that in this patch. I've also moved make the reflow more clean by creating a new function for the child.

Maybe we could introduce a new pseudo-class to access the progress elements with position == -1. It could be named :indeterminated. It may help authors to style the progress bar when it's in the indeterminated state. What's your opinion Tantek ?
Attachment #448650 - Attachment is obsolete: true
Attachment #448903 - Flags: feedback?(roc)
Attachment #448650 - Flags: feedback?(roc)
> Maybe we could introduce a new pseudo-class to access the progress elements
with position == -1. It could be named :indeterminated. It may help authors to
style the progress bar when it's in the indeterminated state. What's your
opinion Tantek ?

There's an existing similarly-named pseudo-class that captures this semantic, albeit for other inputs, that we could reasonably re-use

:indeterminate

http://www.w3.org/TR/css3-ui/#pseudo-classes

http://www.w3.org/TR/2001/CR-css3-selectors-20011113/#UIstates

http://www.w3.org/TR/html5/interactive-elements.html#selector-indeterminate

We should implement that and the file bugs against CSS3-UI + HTML5 indicating that progress element with unknown progress (-1) is another valid use of :indeterminate.
> We should implement that

It's already implemented in general.  You'd just need IntrinsicState() for <progress> to set the right bit as needed (and you would need to send ContentStatesChanged as needed, of course).

I agree that reusing :indeterminate here makes sense.
Attached patch Patch - WIP (obsolete) — Splinter Review
Few questions with this patch:
- Boris and David, what should be needed for |ReResolveStyleContext| ?
- I don't think GetMinWidth() should be needed as the child frame size will depend on the container and the widgets should accept any size in every platform (even if that should be checked carefully) ;
- margin and padding on the child frame do not behave as expected (margin do not interact with the container but with the other element and padding behaves like I would expect margin to). I tried to work on that but it doesn't sound really trivial. Roc, do you have any pointer/code to look at ?

Except these list and two platform-specific issues (MacOS widget height is limited to 15 and Windows widget has a small bug), I think the patch is ready.
Attachment #448903 - Attachment is obsolete: true
Attachment #449286 - Flags: feedback?(roc)
Attachment #448903 - Flags: feedback?(roc)
dbaron may have a better idea about the margin/padding problem.
Attachment #449286 - Flags: feedback?(dbaron)
Do you have testcases I can look at?
(In reply to comment #25)
> Do you have testcases I can look at?

Unfortunately, I lost my test cases with all my data from my HD last week.
However, I think you can use this experimental build of Firefox (it supports the progress element):
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/mlamouri@mozilla.com-16a2c53aab0d/

Do you want me to write you some test cases considering you are using this version or you may have time to play with it ?
I just wanted to see what the markup looked like to understand what it was that you're implementing.

I noticed it might be better to implement nsIAnonymousContentCreator::CreateFrameFor rather than overriding SetInitialChildList.  But that's unlikely to be related to the margins problem.

It seems like nsProgressFrame might have more code than it really needs to have.  (There's a lot of reflow code there to go through looking for offset errors that would be causing margins to break, but I'm wondering if that code is necessary.)
Would it be sufficient to inherit from nsBlockFrame (which also corresponds better to the display:inline-block that you have), but then also override the frame type of the *child* frame, and in that child frame class, override nsIFrame::ComputeAutoSize (but nothing else) to deal with the width computation?  Then you could avoid overriding any Reflow methods at all, I think.
Comment on attachment 449286 [details] [diff] [review]
Patch - WIP

If you want additional feedback beyond the previous comment, please let me know.
Attachment #449286 - Flags: feedback?(dbaron) → feedback-
No longer blocks: 566348
Attached patch WIP Patch (obsolete) — Splinter Review
I removed the indeterminate state and the pseudo-element code. I will do that in other patches.
I had a look at |ComputeAutoSize| and I don't get how this could help me given that I want to set the size of the child's frame, not the frame itself.
Attachment #449286 - Attachment is obsolete: true
Blocks: 633207
No longer blocks: 344614
Summary: Implement progress element (layout part) → Implement the basic layout of the progress element
Attached patch Patch v1 (obsolete) — Splinter Review
I chose to split this in multiple bugs. This bug will only resolve the basic layout. I will open a bug for :indeterminate pseudo-class and ::progress-bar pseudo-element.

Not that the tests are disabled on MacOS because the widget is showing the progress depending on the element's value. I might need to open a bug for that if it's not following the specs.
Attachment #508807 - Attachment is obsolete: true
Attachment #511397 - Flags: review?(roc)
Attached patch Patch v1 (obsolete) — Splinter Review
It's better with the tests.
Attachment #511397 - Attachment is obsolete: true
Attachment #511398 - Flags: review?(roc)
Attachment #511397 - Flags: review?(roc)
Whiteboard: [needs review]
+  nsAutoString classValue;
+  classValue.AppendLiteral("progress-bar");
+  rv = mBarDiv->SetAttr(kNameSpaceID_None, nsGkAtoms::_class, classValue,
+                        PR_FALSE);
+  NS_ENSURE_SUCCESS(rv, rv);

Why do we need to set a class here?

+  // If the state in indeterminated, the progress bar will be hidden.

indeterminate

+    xoffset = aReflowState.ComputedWidth() - availSize.width +
+              aReflowState.mComputedBorderPadding.right;

Shouldn't this be .left? In which case you can just += here.

Shouldn't progress bars have some intrinsic size?

+    NS_MAX(yoffset, 0);

This does nothing.

What about dbaron's comment #27? Especially the last part. It would be nice to be able to avoid implementing Reflow.
(In reply to comment #32)
> +  nsAutoString classValue;
> +  classValue.AppendLiteral("progress-bar");
> +  rv = mBarDiv->SetAttr(kNameSpaceID_None, nsGkAtoms::_class, classValue,
> +                        PR_FALSE);
> +  NS_ENSURE_SUCCESS(rv, rv);
> 
> Why do we need to set a class here?

Indeed, progress > div in the css is enough. I don't need the class.

> +    xoffset = aReflowState.ComputedWidth() - availSize.width +
> +              aReflowState.mComputedBorderPadding.right;
> 
> Shouldn't this be .left? In which case you can just += here.

Indeed. I've added two tests with margin/padding right/left/top/bottom having different values.

> Shouldn't progress bars have some intrinsic size?

What the intrinsic size represents isn't really clear to me so I'm not sure.

> +    NS_MAX(yoffset, 0);
> 
> This does nothing.

Oups, removed.

> What about dbaron's comment #27? Especially the last part. It would be nice to
> be able to avoid implementing Reflow.

I thought creating a new frame class just to set the size wasn't ideal (and it was more interesting for me to implement Reflow). If you think we should prevent implementing Reflow at any cost, I can try to implement a nsProgressBarChildFrame class.

By the way, bug 633209 is cleaning a lot the Reflow implementation.
(In reply to comment #33)
> What the intrinsic size represents isn't really clear to me so I'm not sure.

If someone writes just

<p><progress value="0" max="100"></progress>

It should get its intrinsic width and height. What do you want them to be?
(In reply to comment #34)
> (In reply to comment #33)
> > What the intrinsic size represents isn't really clear to me so I'm not sure.
> 
> If someone writes just
> 
> <p><progress value="0" max="100"></progress>
> 
> It should get its intrinsic width and height. What do you want them to be?

It's set in forms.css: height is 1em and width is 10em.
What if the site explicitly styles as "height: auto; width: auto"?
I guess I should override ComputeAutoSize, right?
Probably, yes.
Attached patch Patch v1.1 (obsolete) — Splinter Review
With review comments applied including auto size.
Attachment #511398 - Attachment is obsolete: true
Attachment #515905 - Flags: review?(roc)
Attachment #511398 - Flags: review?(roc)
Attachment #515905 - Flags: review?(roc)
Attachment #515905 - Flags: review?(dbaron)
Attachment #515905 - Flags: review+
Comment on attachment 515905 [details] [diff] [review]
Patch v1.1

In both DestroyFrom and Reflow, you should assert !GetPrevContinuation()
before the calls to RegUnRegAccessKey.

I think it would be nice if we could make progress frames a reflow
root so that reflow of their bar frame would be fast, but I don't think
we can be sure that the native theme code won't cause overflow.

Up to Reflow.

In Reflow, what causes barFrame to be null?  Given your current code,
I think that needs an assertion and probably even a failure nsresult.
Otherwise, I think you need to properly fill in aStatus and
aDesiredSize.


In ReflowBarFrame:

You really shouldn't be using the same nsHTMLReflowMetrics for both
parent and child... though you fix that in bug 633209.  If they land
together I guess that's ok.  But I'd prefer if you roll the changes to
ReflowBarFrame from that patch into this one; they'd make things much
easier to understand.  (It's relatively easy to do by just copying bits
of patch files around in your patch queue... just copy the relevant
chunks from one patch to the *end* of the chunks for that file in the
other patch, and then apply and refresh both patches.  Of course...
commit to your patch queue first so you have a backup in case you mess
up.)

I think your use of the availSize parameter to nsHTMLReflowState is
somewhat bogus; I'd rather just do the "normal thing" here since it
doesn't really do anything useful for you anyway, as far as I can tell.
That is, just pass
  nsSize availSize(aReflowState.ComputedWidth(), NS_UNCONSTRAINEDSIZE);
for the aAvailableSize parameter to the reflow state constructor, and
keep the later modifications you make to it in other variables.

I'm also not confident your interaction of |position| with margins,
padding, and border seems correct, but I'm also not confident it's
wrong.  Could you explain the intent in the patch (preferably once
the bug 633209 ReflowBarFrame changes are rolled in)?

>+  // If the state in indeterminate, the progress bar will be hidden.

What causes this?


In nsProgressFrame::AttributeChanged, again, I don't like the !barFrame
case.  If it's really extreme, it should be asserting; if it's not, it
should be continuing on to nsHTMLContainerFrame::AttributeChanged.

nsProgressFrame.h:

IsFrameOfType should be public.

html.css:

If you have to rewrap this, make it one selector per line while you're
there.

With those changes, this looks good, but I'd like to look at the
revised patch.  I'm really sorry for taking this long to get to this.
Attachment #515905 - Flags: review?(dbaron) → review-
(In reply to comment #40)
> I'm also not confident your interaction of |position| with margins,
> padding, and border seems correct, but I'm also not confident it's
> wrong.  Could you explain the intent in the patch (preferably once
> the bug 633209 ReflowBarFrame changes are rolled in)?

It would be hard to explain every line given that I wrote this patch a few months ago. Though, everything should be tested and if you thing something should be changed, I'm fine to do it as long as I can find a test (and it doesn't break the current ones).

> >+  // If the state in indeterminate, the progress bar will be hidden.
> 
> What causes this?

The comment was completely wrong. I think I have another patch fixing it but let's do that here.
Attached patch Inter diff (v1.1 to v1.2) (obsolete) — Splinter Review
r=roc
Attachment #530036 - Flags: review?(dbaron)
Attached patch Patch v1.2 (obsolete) — Splinter Review
Attachment #515905 - Attachment is obsolete: true
Attachment #530038 - Flags: review?(dbaron)
Attachment #530036 - Flags: review?(dbaron)
Attached patch Patch v1.3Splinter Review
xOffset doesn't depend on RTL anymore. See bug 633209 review comments.
Attachment #530038 - Attachment is obsolete: true
Attachment #530184 - Flags: review?(dbaron)
Attachment #530038 - Flags: review?(dbaron)
I will write a test for that before landing the patch.
Comment on attachment 530184 [details] [diff] [review]
Patch v1.3

NS_ASSERTION(!GetPrevContinuation(), "Should be called for primary frame!");

The assertion text should probably say "nsProgressFrame should not have continuations; if it does we need to call RegUnregAccessKey only for the first".

(twice)

>++  // If the state in indeterminate, the progress bar will be shown with width=100%.

wrap at less than 80 chars

In Reflow, you don't need the |availSize| variable anymore; you don't
use availSize.height and you can merge availSize.width and width, so just
call it width and drop availSize.

>+  if (position >= 0.0f) {

This should be 0.0, not 0.0f (merge in that piece of one of the higher
patches).


r=dbaron with that
Attachment #530184 - Flags: review?(dbaron) → review+
Attached patch Inter diffSplinter Review
Applying review comments.

r=roc,dbaron
Attachment #530036 - Attachment is obsolete: true
Whiteboard: [needs review] → [ready to land][waits for dependencies]
Pushed:
http://hg.mozilla.org/mozilla-central/rev/6d42dd8b3d0d
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [ready to land][waits for dependencies]
Target Milestone: --- → mozilla6
Depends on: 655860
Backed out in http://hg.mozilla.org/mozilla-central/rev/dd9ba28d2bd9 to resolve bug 655860.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The regression wasn't caused by these patches. Re-landed:
http://hg.mozilla.org/mozilla-central/rev/97e11f940f25
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
No longer depends on: 655860
You need to log in before you can comment on or make changes to this bug.