Closed Bug 634551 Opened 13 years ago Closed 13 years ago

Cocoa's progress bar widget shouldn't have a max size

Categories

(Core :: Widget: Cocoa, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla6

People

(Reporter: mounir, Assigned: mounir)

References

Details

Attachments

(3 files, 6 obsolete files)

If you try to draw a progress widget bigger than a given size, the Cocoa widget will not grow as requested. This is even more annoying given that the default size for HTML progress element is bigger than the maximum size we currently allow.
Markus, what I would like to do here is the following:
- Get the size of the 3 different Cocoa's progress elements ;
- Use the size equals or bigger than the requested size to draw the element.

But I wonder what to do when the requested size is too big (bigger than the largest size we have): should we fallback to a non-native rendering or scale the largest size?

In addition, as usual with Cocoa, the height can't be changed. Is there any simple method to force it? Or we do have to do something as complicated as what's done for dropdowns.
FWIW, the 'large' size seems to fit (more or less) with the default (per spec) size. I wonder if it's on purpose ;)
(In reply to comment #1)
> Markus, what I would like to do here is the following:
> - Get the size of the 3 different Cocoa's progress elements ;

I think there are only two sizes for progress bars.

> - Use the size equals or bigger than the requested size to draw the element.

I'd use the smaller size. We don't want to overflow the requested rect.

> But I wonder what to do when the requested size is too big (bigger than the
> largest size we have): should we fallback to a non-native rendering or scale
> the largest size?

I really don't know. It will probably look bad no matter what we do. What do we do for search input fields?

> In addition, as usual with Cocoa, the height can't be changed. Is there any
> simple method to force it? Or we do have to do something as complicated as
> what's done for dropdowns.

The latter, unfortunately. You could write an NSCell wrapper around progress bar drawing so that we can reuse DrawCellWithSnapping. I can help you with that.

More specifically, this is what I'd do:
 - Create a new subclass of NSCell, e.g. ProgressBarCell.
 - Create an initWithXXX initializer that takes things like active/inactive, min/max/value, whether the bar is horizontal or vertical, etc. and save all these things in member variables on the cell.
 - Implement setControlSize: by setting another member variable.
 - Implement drawWithFrame:inView: by drawing into the CGContext gotten from [[NSGraphicsContext currentContext] graphicsPort] and putting all the saved data from the member variables into the HIThemeTrackDrawInfo. You don't need to use RenderTransformedHIThemeControl here anymore because DrawCellWithScaling already sets up a buffer to draw into, and it will also handle horizontal RTL flipping.
Actually, you can probably reuse the inherited NSCell implementations of setControlSize: and setControlTint:. So you don't have to implement setControlSize:, and you don't have to pass the active/inactive state to the initializer (but instead to setControlTint:, like we do for the other NSCells we use).
(In reply to comment #3)
> (In reply to comment #1)
> > Markus, what I would like to do here is the following:
> > - Get the size of the 3 different Cocoa's progress elements ;
> 
> I think there are only two sizes for progress bars.

There are three sizes (medium, large and mini) but it seems like the last one isn't implemented yet.

> > But I wonder what to do when the requested size is too big (bigger than the
> > largest size we have): should we fallback to a non-native rendering or scale
> > the largest size?
> 
> I really don't know. It will probably look bad no matter what we do. What do we
> do for search input fields?

For the moment, we resize the native rendering to the requested size. I think we should fallback to the default text field for search field but that's easier because the text field might still look native. Here, we have to choose between an ugly native rendering and a default rendering that might not be shiny too. I would tend to fallback to the default rendering FWIW.

> > In addition, as usual with Cocoa, the height can't be changed. Is there any
> > simple method to force it? Or we do have to do something as complicated as
> > what's done for dropdowns.
> 
> The latter, unfortunately. You could write an NSCell wrapper around progress
> bar drawing so that we can reuse DrawCellWithSnapping. I can help you with
> that.
> 
> More specifically, this is what I'd do:
>  - Create a new subclass of NSCell, e.g. ProgressBarCell.
>  - Create an initWithXXX initializer that takes things like active/inactive,
> min/max/value, whether the bar is horizontal or vertical, etc. and save all
> these things in member variables on the cell.
>  - Implement setControlSize: by setting another member variable.
>  - Implement drawWithFrame:inView: by drawing into the CGContext gotten from
> [[NSGraphicsContext currentContext] graphicsPort] and putting all the saved
> data from the member variables into the HIThemeTrackDrawInfo. You don't need to
> use RenderTransformedHIThemeControl here anymore because DrawCellWithScaling
> already sets up a buffer to draw into, and it will also handle horizontal RTL
> flipping.

I'm going to try that. It seems to be a lot of fun.
(In reply to comment #4)
> Actually, you can probably reuse the inherited NSCell implementations of
> setControlSize: and setControlTint:. So you don't have to implement
> setControlSize:, and you don't have to pass the active/inactive state to the
> initializer (but instead to setControlTint:, like we do for the other NSCells
> we use).

Does that really prevent me from setting tdi.enableState? Is that a magic trick done somewhere by NSCell?

Markus, did you write the code setting |tdi.trackInfo.progress.phase|. The milliSecondsPerStep value seems to come from nowhere (and the % 16 too). Is that documented somewhere? Because it seems really wrong with large theme instead of medium.

Except these two things, I see two issues:
1. I think there are some margins that make the scaling go not really as expected. I suppose DrawCellWithSnapping uses CellRenderSettings to take this into account?
2. I believe we should define a max value from which we fallback to the default rendering. Ideally, it would be nice to have this set in CellRenderSettings. For the case of progress bar, we should have a max height. That could be a follow-up actually.
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Attached patch WIP Patch (obsolete) — Splinter Review
(In reply to comment #6)
> (In reply to comment #4)
> > Actually, you can probably reuse the inherited NSCell implementations of
> > setControlSize: and setControlTint:. So you don't have to implement
> > setControlSize:, and you don't have to pass the active/inactive state to the
> > initializer (but instead to setControlTint:, like we do for the other NSCells
> > we use).
> 
> Does that really prevent me from setting tdi.enableState? Is that a magic trick
> done somewhere by NSCell?

I don't really understand what you're asking here, but this is what I meant:

[mProgressBarCell setControlTint:(FrameIsInActiveWindow(aFrame) ? [NSColor currentControlTint] : NSClearControlTint)];
...
tdi.enableState = [self controlTint] != NSClearControlTint;

That way you don't need to implement something like setEnabledState, because setControlTint is already implemented by NSCell.

> Markus, did you write the code setting |tdi.trackInfo.progress.phase|. The
> milliSecondsPerStep value seems to come from nowhere (and the % 16 too).

Yes, I did, and yes, these numbers come from nowhere. I recorded videos of  progressbars in native Cocoa apps and then measured the number of pixels the animation moved during one second, or something like that.
(I use Snapz Pro X for those things. There are probably better apps for these use cases but this works ok and I picked it up in a cheap multi app bundle once.)

> Is that documented somewhere?

Not that I know of :(

> Because it seems really wrong with large theme
> instead of medium.

That's totally possible. I'll play with your patch and try to find the right numbers when I have time.

> Except these two things, I see two issues:
> 1. I think there are some margins that make the scaling go not really as
> expected. I suppose DrawCellWithSnapping uses CellRenderSettings to take this
> into account?

Yes, it does. I can't say more than that without testing it myself.

> 2. I believe we should define a max value from which we fallback to the default
> rendering. Ideally, it would be nice to have this set in CellRenderSettings.
> For the case of progress bar, we should have a max height. That could be a
> follow-up actually.

Sounds good.
Attached file testcase (obsolete) —
Attached patch tweaked margins (obsolete) — Splinter Review
I tweaked the margins until it looked ok. Unfortunately, the margins for the regular size differ between determined and undetermined mode, so we now have two cell render settings...

I fixed the animation for the regular size by turning the % 16 into a % 32.
Attachment #516683 - Attachment is obsolete: true
Attached patch WIP Patch (obsolete) — Splinter Review
I've applied your changes to some other changes I did and the margin issue didn't seem to have been fixed. I might have done something stupid while applying your changes though.

By the way, except that and a rendering issue when showing data:text/html,<progress></progress>, everything seems all right.
Attachment #516869 - Attachment is obsolete: true
Attached file testcase
Updated test case that alway show the outline. Easier and faster to use for debugging.
Attachment #516867 - Attachment is obsolete: true
This is a screenshot taken with only your patch applied. It's exactly what I got with my patch actually.
I'm afraid this isn't fixing the margin issue. Markus, do you have the same rendering?
Sick... Since bug 548097, we don't support Tiger so we only use the first array. I'm going to open a bug to remove the second array from CellRenderSettings...
Depends on: 641366
(In reply to comment #13)
> Created attachment 519037 [details]
> progress margin screenshot
> 
> This is a screenshot taken with only your patch applied. It's exactly what I
> got with my patch actually.
> I'm afraid this isn't fixing the margin issue. Markus, do you have the same
> rendering?

Yes, I have the same rendering. Are you referring to the fact that the upscaled widgets at the bottom don't hit their draw rect? That's a general problem for all widgets that use DrawCellWithSnapping/Scaling, and I'm not sure what to do about it. The problem is how to deal with overflow, e.g. focus rings or shadows: In GetWidgetOverflow, we return max 4px of overflow, regardless of how much the widget is scaled. If we now made upscaled widgets hit their draw rect exactly, the shadow / focus rings would extend the draw rect by more than 4px, which would lead to drawing bugs.
So we always try to keep the overflow inside the 4px margin, which leads to the actual control being made smaller than requested.
Let's not worry about that now.

I have a few more comments about the patch:
- You don't need to add a new file for the interface definition of NSProgressBarCell. Just add a "@class NSProgressBarCell;" somewhere at the beginning of nsNativeThemeCocoa.h and move the @interface block to nsNativeThemeCocoa.mm directly in front of the @implementation part.
- Don't implement setters and getters for max and value, just call the existing NSCell methods setDoubleValue and doubleValue (where you assume that max == 1.0). Move the PR_INT32_MAX business into the drawWithFrame implementation.
- Existing style is to prefix member variables with "m".
(In reply to comment #15)
> Yes, I have the same rendering. Are you referring to the fact that the upscaled
> widgets at the bottom don't hit their draw rect? That's a general problem for
> all widgets that use DrawCellWithSnapping/Scaling, and I'm not sure what to do
> about it. The problem is how to deal with overflow, e.g. focus rings or
> shadows: In GetWidgetOverflow, we return max 4px of overflow, regardless of how
> much the widget is scaled. If we now made upscaled widgets hit their draw rect
> exactly, the shadow / focus rings would extend the draw rect by more than 4px,
> which would lead to drawing bugs.
> So we always try to keep the overflow inside the 4px margin, which leads to the
> actual control being made smaller than requested.
> Let's not worry about that now.

Some widgets don't hit any edge but I guess we can indeed worry about it in a follow up.

> I have a few more comments about the patch:
> - You don't need to add a new file for the interface definition of
> NSProgressBarCell. Just add a "@class NSProgressBarCell;" somewhere at the
> beginning of nsNativeThemeCocoa.h and move the @interface block to
> nsNativeThemeCocoa.mm directly in front of the @implementation part.

I guess new files are cheap and adding a bunch of code in nsNativeThemeCocoa.mm is just going to make this file even bigger and less readable. If you don't mind, I prefer to keep the declaration in a separate header.

> - Don't implement setters and getters for max and value, just call the existing
> NSCell methods setDoubleValue and doubleValue (where you assume that max ==
> 1.0). Move the PR_INT32_MAX business into the drawWithFrame implementation.

If I use setDoubleValue instead of setValue, then calling [self doubleValue] always return 0. I guess I can't use that...
Though, I moved the PR_INT32_MAX business to drawWithFrame.

> - Existing style is to prefix member variables with "m".

Indeed. I was actually using the Apple style here. Will fix that.
Attached patch Patch v1 (obsolete) — Splinter Review
I see two follow-ups:
- improve the margin handling ;
- fallback to the default rendering when the height get higher than a given value.
Attachment #516885 - Attachment is obsolete: true
Attachment #520226 - Flags: review?
Whiteboard: [needs review]
Attachment #520226 - Flags: review? → review?(mstange)
Comment on attachment 520226 [details] [diff] [review]
Patch v1

I don't think you need the Makefile change. I think that's only necessary when a file outside of widget/src/cocoa wants to include NSProgressBarCell.h.

Also, forward declaration should always be preferred over #including header files, so please move the #include to nsNativeThemeCocoa.mm and add the forward declaration to nsNativeThemeCocoa.h ("@class NSProgressBarCell;"). It doesn't make much of a difference in this case, but it's better style. (The only advantage in this case would be that you don't have to recompile nsWidgetFactory.mm when changing NSProgressBarCell.h.)

Looks good otherwise.

Please request another review from Josh.
Attachment #520226 - Flags: review?(mstange) → review+
Attached patch Patch v1.1 (obsolete) — Splinter Review
r=mstange

With requested changes.
Attachment #520226 - Attachment is obsolete: true
Attachment #521786 - Flags: review?(joshmoz)
Comment on attachment 521786 [details] [diff] [review]
Patch v1.1

No need for the new file "NSProgressBarCell.h", just put that class definition near the top of "nsNativeThemeCocoa.mm". r+ with that change.
Attachment #521786 - Flags: review?(joshmoz) → review+
Depends on: 634549
Whiteboard: [needs review] → [ready to land][waits for dependencies]
Attached patch Patch v1.2Splinter Review
r=joshmoz

The compilation fails on some slaves on try. I don't have a Mac around so I can't really check deeper.
Attachment #521786 - Attachment is obsolete: true
Pushed:
http://hg.mozilla.org/mozilla-central/rev/a48aa0f63d90
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
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/9722e19af82c
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
No longer depends on: 655860
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0) Gecko/20100101 Firefox/6.0

I've run the testcase and everything seems in order although I am not sure what after should I be looking. Can you please clarify so I can verify whether this patch does the right thing?

Thanks!
(In reply to comment #25)
> I've run the testcase and everything seems in order although I am not sure
> what after should I be looking. Can you please clarify so I can verify
> whether this patch does the right thing?

data:text/html,<progress style="width:300px; height:100px;"></progress>

should show a native progress bar (even if ugly)
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:8.0a1) Gecko/20110725 Firefox/8.0a1

Considering last comment, setting status to Verified Fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.