Bug 49769 – <progress> element is unsupported on Windows

• Component: Forms
• Status: NEW

[Kent Tamura]

We need to add some code to RenderThemeWin to enable ENABLE_PROGRESS_TAG.

[John Sullivan]

<rdar://problem/8686175>

[Adam Roben (:aroben)]

We'll need to implement support in RenderThemeSafari, too, for regression tests.

[Yael]

If nobody is working on this, I'd like to take it up.
thanks!

[Yael]

Created attachment 76141 [details]
Patch.

This patch is adopting the Chromium implementation to support RenderThemeWin::paintProgressBar().
I did not remove the tests that require images from the skip list because I am not sure my set-up is correct for creating the needed images. I will need some help in doing that.

[Yael]

Created attachment 76386 [details]
Patch.

Update patch after fixing svn:eol-style for AccessibilityAllInOne.cpp.

[Yael]

Created attachment 76762 [details]
Patch.

This patch also adds code to RenderThemeSafari to draw basic progress bar, and removes progress bar tests from the skipped list.

[Hajime Morrita]

Hi, thank you for doing this!
progress-element-expected.png doesn't look to have the native look.
Is it expected?

[Yael]

(In reply to comment #7)
> Hi, thank you for doing this!
> progress-element-expected.png doesn't look to have the native look.
> Is it expected?

On Windows 7, the progress bar is animating both in determinate and indeterminate modes. If we take a snapshot of a native looking progress bar, the test will become flaky. 
Please see https://bugs.webkit.org/show_bug.cgi?id=36961#c12 for the same issue on OS X.

[Hajime Morrita]

> On Windows 7, the progress bar is animating both in determinate and indeterminate modes. If we take a snapshot of a native looking progress bar, the test will become flaky. 
> Please see https://bugs.webkit.org/show_bug.cgi?id=36961#c12 for the same issue on OS X.
Yes. So I thought it is reasonable to just skip it. If it gets pass, it is OK.
I might miss something... 
Could you explain why does progress-element.html doesn't have a native look?
It doesn't look any style applied and should have a native look in my understanding.

[Yael]

(In reply to comment #9)
> > On Windows 7, the progress bar is animating both in determinate and indeterminate modes. If we take a snapshot of a native looking progress bar, the test will become flaky. 
> > Please see https://bugs.webkit.org/show_bug.cgi?id=36961#c12 for the same issue on OS X.
> Yes. So I thought it is reasonable to just skip it. If it gets pass, it is OK.
> I might miss something... 
> Could you explain why does progress-element.html doesn't have a native look?
> It doesn't look any style applied and should have a native look in my understanding.

If you load the test in Safari on Windows 7 (with my patch applied) you would see a native looking progress bar, including the animation that is expected from the progress bar on Windows 7. This is done in RenderThemeWin.cpp.
If you load it in DRT, for the reason I explained above, it does not look native. This is done in RenderThemeSafari.cpp, and I confirmed with Adam Roben on IRC that RenderThemeSafari.cpp is only used for the test, so I thought it was ok. 
BTW, I thought you did the same in test_shell_webthemecontrol.cc ?

[Yael]

(In reply to comment #9)
> > On Windows 7, the progress bar is animating both in determinate and indeterminate modes. If we take a snapshot of a native looking progress bar, the test will become flaky. 
> > Please see https://bugs.webkit.org/show_bug.cgi?id=36961#c12 for the same issue on OS X.
> Yes. So I thought it is reasonable to just skip it. If it gets pass, it is OK.
> I might miss something... 
> Could you explain why does progress-element.html doesn't have a native look?
> It doesn't look any style applied and should have a native look in my understanding.

Perhaps I misunderstood your question. If you are referring to the hard-coded colors, that was because when I tried to use used the background and foreground colors from RenderTheme, they came back as light blue and white. On a white background, it is not possible to see the progress bar properly.

[Adam Roben (:aroben)]

Comment on attachment 76762 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=76762&action=review

> WebCore/rendering/RenderThemeSafari.cpp:57
> +const Color progressBarBackgroundColor = Color(0x7f, 0x7f, 0x7f);
> +const Color progressBarForegroundColor = Color(0, 0, 0x7f);

These should be marked static so they'll have internal linkage. But they'll also create static initializers and exit-time destructors, which we normally try to avoid. It would be better to use DEFINE_STATIC_LOCAL to define these inside the paintProgressBar function.

> WebCore/rendering/RenderThemeSafari.cpp:1232
> +bool RenderThemeSafari::paintProgressBar(RenderObject* o, const PaintInfo& paintInfo, const IntRect& r)
> +{
> +    FloatRect widgetRect = r;
> +    RenderProgress* renderProgress = toRenderProgress(o);
> +    FloatRect valueRect;
> +    if (renderProgress->isDeterminate()) {
> +        int dx = r.width() * renderProgress->position();
> +        if (renderProgress->style()->isLeftToRightDirection())
> +            valueRect = IntRect(r.x(), r.y(), dx, r.height());
> +        else
> +            valueRect = IntRect(r.x() + r.width() - dx, r.y(), dx, r.height());
> +    }
> +
> +    paintInfo.context->fillRect(widgetRect, progressBarBackgroundColor, ColorSpaceDeviceRGB);
> +    paintInfo.context->fillRect(valueRect, progressBarForegroundColor, ColorSpaceDeviceRGB);

I think IntRect can be implicitly converted to FloatRect, so I don't think you need the widgetRect variable. I also think valueRect could be an IntRect for the same reason.

If you initialize valueRect to r then you could do things like valueRect.setWidth(dx).

We generally prefer unabbreviated names, so "filledWidth" (or similar) would be better than "dx".

It's good to have an implementation here so that something will show up in the regression tests. Unfortunately, this implementation won't help us in our goal of being able to share test results with Mac. But someone at Apple would need to do that work.

> WebCore/rendering/RenderThemeSafari.cpp:1236
> +}
> +
> +#endif

Extra blank line before the if.

> WebCore/rendering/RenderThemeWin.cpp:162
> +static const double progressAnimationFrameRate = 0.033;

Maybe 1.0 / 30 would be clearer?

> WebCore/rendering/RenderThemeWin.cpp:1159
> +double RenderThemeWin::animationDurationForProgressBar(RenderProgress*) const
> +{
> +    return progressAnimationFrameRate;
> +}

I think you're returning progressAnimationFrameRate here because you need to return some non-0 value so that RenderProgress will run its animation timer, but the actual duration doesn't matter because Windows uses a constant-speed animation rather than a constant-duration animation. A comment to this effect (or some other effect, if I'm wrong) would be helpful. And maybe choosing an obviously bogus duration value would be better.

Since there's no animation on XP for determinate progress bars, or in Classic mode, it seems like we should return 0 for both the repeat interval and the animation duration in those cases. Otherwise the animation timer will be firing needlessly.

> WebCore/rendering/RenderThemeWin.cpp:1165
> +static int computeAnimationProgress(int frameWidth, int objectWidth, int pixelsPerSecond, double animatedSeconds)

This function name is a bit confusing, given how overloaded "progress" is. RenderProgress::animationProgress returns a number between 0 and 1, while this returns a width. Maybe it could be made clearer if it returned the rect for the overlay. Something like this:

static RECT progressAnimationOverlayRect(const RECT& rectToTraverse, int overlayWidth, int pixelsTraversedPerSecond, double secondsSinceAnimationBegan)

Returning a rect would also reduce some code duplication you currently have in the calling function.

Adding a comment somewhere to explain that some progress bar styles have an overlay that animates over top of the regular progress bar would also help.

> WebCore/rendering/RenderThemeWin.cpp:1176
> +    // There is no known API to obtain these numbers,
> +    // so they were taken from the Chrome implementation.

Does the Chrome implementation have comments about how they were chosen or determined? If so it would be good to include those comments here.

> WebCore/rendering/RenderThemeWin.cpp:1183
> +    const int kDeteminateOverlayPixelsPerSecond = 300;
> +    const int kDeteminateOverlayWidth = 120;
> +    const int kIndeterminateOverlayPixelsPerSecond =  175;
> +    const int kVistaIndeterminateOverlayWidth = 120;
> +    const int kXPIndeterminateOverlayWidth = 55;
> +    // The thickness of the bar frame inside |valueRect|
> +    const int kXPBarPadding = 3;

We don't use "k" for constants like Chrome does. You're also missing a period at the end of the comment.

Extra space before "175".

> WebCore/rendering/RenderThemeWin.cpp:1187
> +    LocalWindowsContext windowsContext(i.context, r, false);
> +    RECT widgetRect = r;
> +    HDC hdc = windowsContext.hdc();

You should declare these a bit closer to where they're first used.

> WebCore/rendering/RenderThemeWin.cpp:1191
> +        int dx = r.width() * renderProgress->position();

Same comment as before about names like "dx".

> WebCore/rendering/RenderThemeWin.cpp:1200
> +    if (progressBarTheme()) {

Reversing this if and using an early-return in the Classic case would allow you to un-indent a bunch of code in this function.

> WebCore/rendering/RenderThemeWin.cpp:1205
> +            // We should care the direction here because PP_CNUNK painting

Typos: "We should care the direction" and "PP_CNUNK"

> WebCore/rendering/RenderThemeWin.cpp:1225
> +                // On XP, progress bar is chunk-style and has no glossy effect.

Missing a "the" before "progress bar".

> WebCore/rendering/RenderThemeWin.cpp:1226
> +                // We need to shrink destination rect to fit the part inside the bar

Missing a "the" after "shrink".

> WebCore/rendering/RenderThemeWin.cpp:1232
> +        } else {

You could use an early-return here instead of an else.

> WebCore/rendering/RenderThemeWin.cpp:1237
> +            int overlayWidth = !isRunningOnVistaOrLater() ? kXPIndeterminateOverlayWidth : kVistaIndeterminateOverlayWidth;

I think this would be easier to read if you reversed it (i.e., get rid of the !).

> WebCore/rendering/RenderThemeWin.h:129
> +    // Returns the repeat interval of the animation for the progress bar.
> +    virtual double animationRepeatIntervalForProgressBar(RenderProgress*) const;
> +    // Returns the duration of the animation for the progress bar.
> +    virtual double animationDurationForProgressBar(RenderProgress*) const;

These comments don't add any information. They're just repeating the function names.

[Adam Roben (:aroben)]

Sorry it took so long to review this!

[Jessie Berlin]

Added Windows platform specific results for some tests that use the progress element: http://trac.webkit.org/changeset/118849