Last Comment Bug 634551 - Cocoa's progress bar widget shouldn't have a max size
: Cocoa's progress bar widget shouldn't have a max size
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Widget: Cocoa (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
Depends on: 634549 641366
Blocks: 633207
  Show dependency treegraph
 
Reported: 2011-02-16 02:28 PST by Mounir Lamouri (:mounir)
Modified: 2011-07-26 02:19 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP Patch (9.92 KB, patch)
2011-03-03 13:07 PST, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
testcase (1.16 KB, text/html)
2011-03-04 05:40 PST, Markus Stange [:mstange]
no flags Details
tweaked margins (10.43 KB, patch)
2011-03-04 05:51 PST, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
WIP Patch (10.68 KB, patch)
2011-03-04 07:46 PST, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
testcase (1.15 KB, text/html)
2011-03-13 11:43 PDT, Mounir Lamouri (:mounir)
no flags Details
progress margin screenshot (172.30 KB, image/tiff)
2011-03-13 11:47 PDT, Mounir Lamouri (:mounir)
no flags Details
Patch v1 (10.43 KB, patch)
2011-03-18 09:28 PDT, Mounir Lamouri (:mounir)
mstange: review+
Details | Diff | Splinter Review
Patch v1.1 (10.46 KB, patch)
2011-03-25 05:18 PDT, Mounir Lamouri (:mounir)
jaas: review+
Details | Diff | Splinter Review
Patch v1.2 (8.03 KB, patch)
2011-04-11 15:13 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2011-02-16 02:28:39 PST
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.
Comment 1 Mounir Lamouri (:mounir) 2011-02-16 07:39:22 PST
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.
Comment 2 Mounir Lamouri (:mounir) 2011-02-16 07:43:05 PST
FWIW, the 'large' size seems to fit (more or less) with the default (per spec) size. I wonder if it's on purpose ;)
Comment 3 Markus Stange [:mstange] 2011-02-18 05:30:28 PST
(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.
Comment 4 Markus Stange [:mstange] 2011-02-18 05:38:36 PST
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).
Comment 5 Mounir Lamouri (:mounir) 2011-03-01 07:15:56 PST
(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.
Comment 6 Mounir Lamouri (:mounir) 2011-03-03 13:05:41 PST
(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.
Comment 7 Mounir Lamouri (:mounir) 2011-03-03 13:07:10 PST
Created attachment 516683 [details] [diff] [review]
WIP Patch
Comment 8 Markus Stange [:mstange] 2011-03-03 13:30:58 PST
(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.
Comment 9 Markus Stange [:mstange] 2011-03-04 05:40:16 PST
Created attachment 516867 [details]
testcase
Comment 10 Markus Stange [:mstange] 2011-03-04 05:51:00 PST
Created attachment 516869 [details] [diff] [review]
tweaked margins

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.
Comment 11 Mounir Lamouri (:mounir) 2011-03-04 07:46:38 PST
Created attachment 516885 [details] [diff] [review]
WIP Patch

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.
Comment 12 Mounir Lamouri (:mounir) 2011-03-13 11:43:33 PDT
Created attachment 519034 [details]
testcase

Updated test case that alway show the outline. Easier and faster to use for debugging.
Comment 13 Mounir Lamouri (:mounir) 2011-03-13 11:47:59 PDT
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?
Comment 14 Mounir Lamouri (:mounir) 2011-03-13 12:39:58 PDT
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...
Comment 15 Markus Stange [:mstange] 2011-03-16 04:57:39 PDT
(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".
Comment 16 Mounir Lamouri (:mounir) 2011-03-18 09:23:57 PDT
(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.
Comment 17 Mounir Lamouri (:mounir) 2011-03-18 09:28:19 PDT
Created attachment 520226 [details] [diff] [review]
Patch v1

I see two follow-ups:
- improve the margin handling ;
- fallback to the default rendering when the height get higher than a given value.
Comment 18 Markus Stange [:mstange] 2011-03-24 08:08:33 PDT
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.
Comment 19 Mounir Lamouri (:mounir) 2011-03-25 05:18:33 PDT
Created attachment 521786 [details] [diff] [review]
Patch v1.1

r=mstange

With requested changes.
Comment 20 Josh Aas 2011-04-06 21:54:40 PDT
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.
Comment 21 Mounir Lamouri (:mounir) 2011-04-11 15:13:39 PDT
Created attachment 525187 [details] [diff] [review]
Patch v1.2

r=joshmoz

The compilation fails on some slaves on try. I don't have a Mac around so I can't really check deeper.
Comment 22 Mounir Lamouri (:mounir) 2011-05-09 05:41:20 PDT
Pushed:
http://hg.mozilla.org/mozilla-central/rev/a48aa0f63d90
Comment 23 Shawn Wilsher :sdwilsh 2011-05-09 16:11:57 PDT
Backed out in http://hg.mozilla.org/mozilla-central/rev/dd9ba28d2bd9 to resolve bug 655860.
Comment 24 Mounir Lamouri (:mounir) 2011-05-10 07:00:17 PDT
The regression wasn't caused by these patches. Re-landed:
http://hg.mozilla.org/mozilla-central/rev/9722e19af82c
Comment 25 George Carstoiu 2011-07-20 07:05:58 PDT
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!
Comment 26 Mounir Lamouri (:mounir) 2011-07-20 09:19:36 PDT
(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)
Comment 27 George Carstoiu 2011-07-26 02:19:18 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.