Bug 35937 – Support for HTMLProgressElement

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Yael]

Add support for HTMLProgressElement. 
http://www.w3.org/TR/html5/forms.html#the-progress-element.

[Yael]

Created attachment 50339 [details]
Patch V1

[Yael]

Created attachment 50344 [details]
Patch v2

I hope the png applies ok this time.

[Yael]

Created attachment 50346 [details]
Patch v3

Resolve conflict in Layout/platform/gtk/Skipped.

[WebKit Review Bot]

Attachment 50346 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/495008

[Darin Adler]

Comment on attachment 50346 [details]
Patch v3

Great to see you tackling this form element.

> +#if PLATFORM(QT)
> +#if !defined(ENABLE_PROGRESS_TAG)
> +#define ENABLE_PROGRESS_TAG 1
> +#endif
> +#else
> +#if !defined(ENABLE_PROGRESS_TAG)
> +#define ENABLE_PROGRESS_TAG 0
> +#endif
> +#endif

We've handled different ENABLE defaults per platform earlier in the file in the past. See the blocks above with things like #if PLATFORM(WX) around them.

In other cases we handled those defaults outside this file entirely in the build system.

Lets keep to one of those if possible.

> +        was modified to actualy draw the progress element.

Typo: "actualy"

Seems like there's a lot of work here to make the progress element code conditional. Can we do this in some simpler way with less logic in all the build systems? It seems to me that it's even OK to compile the code as long as we don't instantiate HTMLProgressElement on other platforms. I presume the plan is to get this working on the other platforms as soon as possible, and taking a little extra code size in the meantime seems OK to me.

> +/* progress */
> +
> +progress {
> +    -webkit-appearance: progress-bar;
> +    padding: initial;
> +    border: initial;
> +    margin: 2px;
> +}
> +
> +progress::-webkit-progress-bar {
> +    -webkit-appearance: progress-bar;
> +}

What effect does this have on the platforms with no progress support compiled in?

> +HTMLProgressElement::HTMLProgressElement(const QualifiedName& tagName, Document* doc, HTMLFormElement* form)

Please use "document" instead of "doc".

> +    m_max = 1;
> +    m_value = 1;
> +    m_isDeteminate = false;

Please use member initialization instead of assignment.

> +HTMLProgressElement::~HTMLProgressElement()
> +{
> +}

I suggest not explicitly declaring or defining the destructor if it doesn't need any custom code in it.

> +void HTMLProgressElement::parseMappedAttribute(MappedAttribute* attr)
> +{
> +    if (attr->name() == valueAttr)
> +        setValue(attr->value().toFloat());
> +    else if (attr->name() == maxAttr)
> +        setMax(attr->value().toFloat());

This is wrong. DOM bindings for attributes such as value and max need to change the attribute values. Those in turn change the behavior of the class. It's never right for parseMappedAttrbute to call the actual DOM binding functin.

> +void HTMLProgressElement::defaultEventHandler(Event* evt)
> +{
> +    HTMLFormControlElement::defaultEventHandler(evt);
> +}

This should not be overridden if we don't need to change what it does. Also please use "event" not "evt".

> +float HTMLProgressElement::value() const
> +{
> +    if (m_value > m_max)
> +        return m_max;
> +    return m_value;
> +}

This is probably wrong for the DOM value attribute. It should just return the result of valueAttr. The best way to do that is to use the Reflect keyword in the DOM binding.

> +void HTMLProgressElement::setValue(float value)
> +{
> +    if (isnan(value) || value < 0)
> +        m_value = 0;
> +    else
> +        m_value = value;
> +    m_isDeteminate = true;
> +}

Same issue here. This function should just set valueAttr.

Typo: m_isDeteminate should instead be m_isDeterminate.

The code to handle nan is unnecessarily complex. It could be:

    m_value = value >= 0 ? value : 0;

But I don't think this should exist in this form. I don't think we should store m_value.

> +void HTMLProgressElement::setMax(float max)
> +{
> +    if (!isnan(max) && max > 0)
> +        m_max = max;
> +    else
> +        m_max = 1;
> +}

Same thing here. The nan aspect of this function is handled correctly if you write it like this:

    m_max = max > 0 ? max : 1;

> +public:
> +    HTMLProgressElement(const QualifiedName&, Document*, HTMLFormElement* = 0);

For new classes it's best to make the constructor private and provide a public create function. Eventually all classes will work this way.

> +    virtual ~HTMLProgressElement();

You should just leave this out.

> +    virtual const AtomicString& formControlType() const;
> +
> +    virtual RenderObject* createRenderer(RenderArena*, RenderStyle*);
> +
> +    virtual void parseMappedAttribute(MappedAttribute*);

These should all be private.

> +    virtual void defaultEventHandler(Event*);

You should just leave this out.

> +    float value() const;
> +    void setValue(float);
> +
> +    float max() const;
> +    void setMax(float);

I don't think you need these, since [Reflect] in the IDL file should take care of it.

> +    float position() const;

We may want to use double here instead of float. All numbers in JavaScript are doubles, so choosing float incurs a performance penalty. Not sure if there's some reason float is better.

> +    // NodeList* labels;

Please don't include any commented-out code. It's almost certainly not correct to have NodeList.

> +progress constructorNeedsFormElement, conditional=PROGRESS_TAG, createWithNew

You should not use createWithNew. Also, we should make a test case to show that the constructor really does need the form element. I'm not sure that's needed for this.

If it is needed, we need a test to show the incorrect behavior you'd otherwise get. And to try in other browsers. And read HTML5 to be sure it's helpful.

> +RenderProgress::~RenderProgress()
> +{
> +}

Same comment about unneeded destructors.

> +float RenderProgress::value() const
> +{
> +    HTMLProgressElement* element = static_cast<HTMLProgressElement*>(node());
> +    if (element)
> +        return element->value();
> +    return 1;
> +}

Do we really need this function? I don't think it's legal to have a RenderProgress with a node() of 0. Is this really done often enough that we need this helper function?

> +float RenderProgress::maxProgress() const
> +{
> +    HTMLProgressElement* element = static_cast<HTMLProgressElement*>(node());
> +    if (element)
> +        return element->max();
> +    return 1;
> +}

Same comment.

review- because I think you'll want to make at least some of the changes I mentioned above.

[Yael]

Created attachment 50423 [details]
Patch v4

Address most of the comments from Darin.

1. There should be no impact from the addition to html.css on platforms that don't support the progress tag. That's because the element would not be created.

2. I don't think "value" and "max" qualify as [reflect] attributes. If they are set with invalid values, those values are not accepted and some defaults are used instead.

[WebKit Review Bot]

Attachment 50423 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/566031

[WebKit Review Bot]

Attachment 50423 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/584023

[Darin Adler]

(In reply to comment #6)
> 1. There should be no impact from the addition to html.css on platforms that
> don't support the progress tag. That's because the element would not be
> created.

That’s incorrect. Today I can create a <progress> element and style it with rules that specify the progress tag. It won’t be a special type of element and will be handled as an inline element just as <span> is, but those style rules will change the rendering of existing web pages.

> 2. I don't think "value" and "max" qualify as [reflect] attributes. If they are
> set with invalid values, those values are not accepted and some defaults are
> used instead.

If they are not [Reflect] they still do need to change the attribute you would get with getAttribute, not just a hidden internal value.

HTMLInputElement has a max attribute. Are you sure this should be different in this respect? To convince me, you need to demonstrate the expected behavior with test cases and then show me either another browser or HTML5 specification to make it clear the behavior it tests is correct.

[Yael]

(In reply to comment #9)
> That’s incorrect. Today I can create a <progress> element and style it with
> rules that specify the progress tag. It won’t be a special type of element and
> will be handled as an inline element just as <span> is, but those style rules
> will change the rendering of existing web pages.
You are right. If a web site is using <progress> instead of <span> then there will be an additional 2px margin. I am not sure how common it is to use <progress> as if it was <span>.
To be on the safe side, is there a way to conditionally add the style?

[Yael]

(In reply to comment #9)
> If they are not [Reflect] they still do need to change the attribute you would
> get with getAttribute, not just a hidden internal value.
> 
> HTMLInputElement has a max attribute. Are you sure this should be different in
> this respect? To convince me, you need to demonstrate the expected behavior
> with test cases and then show me either another browser or HTML5 specification
> to make it clear the behavior it tests is correct.
The reason I wanted to keep these 2 parameters as members is that otherwise, every time we paint the progress bar, we would need to convert them from strings and verify their validity. What do you think?

[Yael]

Created attachment 50464 [details]
Patch v5

Attempt to fix the various builds.

[Kent Tamura]

Comment on attachment 50464 [details]
Patch v5

I think an indeterminate <progress> should be animated, and progress bar controls of Windows and Mac needs animation even if their value are not changed. This patch seems to have no capability of such cases.


> Index: WebCore/platform/qt/RenderThemeQt.cpp
> ===================================================================
> --- WebCore/platform/qt/RenderThemeQt.cpp	(revision 55826)
> +++ WebCore/platform/qt/RenderThemeQt.cpp	(working copy)
> +bool RenderThemeQt::paintProgressBar(RenderObject* o, const RenderObject::PaintInfo& pi, const IntRect& r)
> +{
> +    StylePainter p(this, pi);
> +    if (!p.isValid())
> +       return true;
> +
> +    QStyleOptionProgressBarV2 option;
> +    if (p.widget)
> +       option.initFrom(p.widget);
> +    initializeCommonQStyleOptions(option, o);
> +
> +    RenderProgress* renderProgress = toRenderProgress(o);
> +    HTMLProgressElement* element = static_cast<HTMLProgressElement*>(renderProgress->node());
> +    option.rect = r;
> +    option.maximum = element->max();
> +    option.minimum = 0;
> +    option.progress = element->value();
> +
> +    const QPoint topLeft = r.topLeft();
> +    p.painter->translate(topLeft);
> +    option.rect.moveTo(QPoint(0, 0));
> +    option.rect.setSize(r.size());
> +
> +    p.drawControl(QStyle::CE_ProgressBar, option);
> +    p.painter->translate(-topLeft);
> +
> +    return false;
> +}

What is drawn if the progress element is indeterminate?

> Index: WebCore/rendering/RenderProgress.cpp
> ===================================================================
> --- WebCore/rendering/RenderProgress.cpp	(revision 0)
> +++ WebCore/rendering/RenderProgress.cpp	(revision 0)

> +RenderProgress::RenderProgress(Element* element)
> +    : RenderBlock(element)
> +{
> +    setSize(QSize(defaultProgressWidth, defaultProgressHeight));

Do not use QSize here.

[Yael]

Created attachment 50497 [details]
Patch V6

Another attempt at fixing the mac build, and remove the QSize that snack in.

Kent, at the moment indeterminate progress bar looks quite boring. I'd love to see if you have an idea of how to animate it.

[Kent Tamura]

> Kent, at the moment indeterminate progress bar looks quite boring. I'd love to
> see if you have an idea of how to animate it.

RenderImage uses a timer to show animation GIFs.  But it doesn't work well for progress bars because there are no way to paint a NSProcessIndicator with a specific animation state in OS X.
Probably a progress bar should be a Widget like a scroll bar.

[Antti Koivisto]

Making <progress> style rules conditional too seems bit overkill considering that:

- while technically it affects builds where the feature is not enabled, the effect is minimal and unlikely to break anything in practice
- there is plenty of precedence of not bothering (datagrid for example, and it has more invasive style too)
- we don't have good mechanism in place for conditional style rules, adding it for this feature seems overkill

[Darin Adler]

(In reply to comment #11)
> (In reply to comment #9)
> > If they are not [Reflect] they still do need to change the attribute you would
> > get with getAttribute, not just a hidden internal value.
> > 
> > HTMLInputElement has a max attribute. Are you sure this should be different in
> > this respect? To convince me, you need to demonstrate the expected behavior
> > with test cases and then show me either another browser or HTML5 specification
> > to make it clear the behavior it tests is correct.
> The reason I wanted to keep these 2 parameters as members is that otherwise,
> every time we paint the progress bar, we would need to convert them from
> strings and verify their validity. What do you think?

I think this is not a good design.

If you are caching values to make repaint efficient, you can do it differently and better:

    1) The cached value should be in the renderer, not the DOM. It can be recomputed as needed, cached, and the DOM can make calls to invalidate when the parameters change.

    2) The cached value should be a single integer, not a pair of floating point numbers, and this it can be used to avoid repainting when there are DOM changes that don't affect the display. Thus an insignificant value change could be optimized. Or a case where both value and max are changed, but the displayed progress level does not change.

Whether there is a cache or not, however, is not the primary question here. The question is observable behavior. If you set the max, it's important that getAttribute("max") reflect the new maximum. And since setAttribute lets you set the max attribute to any arbitrary string, the code needs to handle that. You should have test cases showing how that works.

[Darin Adler]

(In reply to comment #10)
> You are right. If a web site is using <progress> instead of <span> then there
> will be an additional 2px margin. I am not sure how common it is to use
> <progress> as if it was <span>.

Probably not a major concern; but people can and do use custom elements of any name. This will be incompatible with the real <progress> element too.

> To be on the safe side, is there a way to conditionally add the style?

I think a better way to do this is to shoot for adding something for all the platforms in as quick a sequence as possible instead of doing this for only one platform.

[Yael]

Created attachment 50543 [details]
Patch v7

Address comment #17. removed the caching of the attributes and added tests for setting and getting the attributes.

[Darin Adler]

Comment on attachment 50543 [details]
Patch v7

> +    if (attr->name() == valueAttr) {
> +        // do nothing

If there is no code here, what the progress bar to repaint?

> +    } else if (attr->name() == maxAttr) {
> +        // do nothing

Same question here.

> +    if (hasAttribute(valueAttr)) {
> +        bool ok;
> +        double value = getAttribute(valueAttr).toDouble(&ok);
> +        if (!ok || value < 0)
> +            return 0;
> +        return (value > max()) ? max() : value;
> +    }
> +    return 1;
> +}

It's unnecessarily inefficient to always check hasAttribute(valueAttr). There are two things you could do to tighten this up.

    1) Put the hasAttribute check inside the error handling code, just before the "return 0".

    2) Put the result of getAttribute(valueAttr) into a local variable, and use it so you don't do a second hash table lookup.

If you did both, the code would look like this:

    const AtomicString& valueString = getAttribute(valueAttr);
    bool ok;
    double value = valueString.toDouble(&ok);
    if (!ok || value < 0)
        return valueString.isNull() ? 1 : 0;
    return (value > max()) ? max() : value;

> +double HTMLProgressElement::max() const
> +{
> +    if (hasAttribute(maxAttr)) {
> +        bool ok;
> +        double max = getAttribute(maxAttr).toDouble(&ok);
> +        if (!ok || max <= 0)
> +            return 1;
> +        return max;
> +    }
> +    return 1;
> +}

No need for the hasAttribute check here. The error handling code will already kick in correctly and return 1.

> +double HTMLProgressElement::position() const
> +{
> +    if (hasAttribute(valueAttr))
> +        return value() / max();
> +    return -1;
> +}

We normally handle errors with early return, rather than having the success case be the early return.

> +    HTMLProgressElement(const QualifiedName&, Document*, HTMLFormElement* = 0);

No need for the default value here for HTMLFormElement*.

> +        // readonly attribute NodeList labels;

Normally we don't check in commented-out code.

> +RenderProgress::RenderProgress(Element* element)

The constructor should specifically take a HTMLProgressElement* rather than just an Element*.

> +    : RenderBlock(element)

Should this instead inherit from RenderReplaced? What other render class were you basing this one on?

I'm going to mark this review- because I don't see how this triggers a repaint when the progress element changes.

[Yael]

Created attachment 50575 [details]
Patch v8

This patch is addressing comment #20.

(In reply to comment #20)
> Should this instead inherit from RenderReplaced? What other render class were
> you basing this one on?

I was asking myself the same question. I chose RenderBlock after reading dHyatt's blog at 
<http://webkit.org/blog/115/webcore-rendering-ii-blocks-and-inlines/>

"Form controls are actually a strange special case. They are still replaced elements, but because they are implemented by the engine, controls actually ultimately subclass from RenderBlock."

[Yael]

(In reply to comment #20)
> Should this instead inherit from RenderReplaced? What other render class were
> you basing this one on?

Sorry Darin, for not answering the other half of your question. I based the rendering implementation on RenderSlider.

[Darin Adler]

(In reply to comment #22)
> Sorry Darin, for not answering the other half of your question. I based the
> rendering implementation on RenderSlider.

If RenderSlider and RenderProgress are quite similar, then I suggest we make a common base class for code sharing. It can literally be named RenderSliderOrProgress or RenderBar or something along those lines. That would be better than copied and pasted code.

[Darin Adler]

Comment on attachment 50575 [details]
Patch v8

We need to get the looks for progress bars implemented on all platforms as soon as possible. It's not good to have all these conditionals. Generally speaking, though we have many of them, conditional features are bad for the WebKit project and the web platform!

> +void HTMLProgressElement::parseMappedAttribute(MappedAttribute* attr)
> +{
> +    if (attr->name() == valueAttr) {
> +        setNeedsStyleRecalc();
> +    } else if (attr->name() == maxAttr) {
> +        setNeedsStyleRecalc();
> +    } else
> +        HTMLFormControlElement::parseMappedAttribute(attr);
> +}

Please use "attribute" rather than "attr".

There should not be braces around these single-line if statement bodies. Not sure why the style checking script doesn't know that.

I don't understand why setNeedsStyleRecalc is the right thing to do here. How did you decide that was the right call? Do these attributes really change style? Is there a comparable call site elsewhere showing this is correct?

It's important to do precisely the minimum amount of calculation redrawing when the value or max of a progress element changes, because this is the use case for progress elements and we want good efficiency!

> +void RenderProgress::updateFromElement()
> +{
> +    setNeedsLayoutAndPrefWidthsRecalc();
> +}

Is this correct? How would a change to the element affect the layout and preferred widths?

Some of the strangest rules in this class are the rules for maximums of less than 1. For example, a maximum of 0.5 sets maximum to 0.5 but a maximum of 0 sets maximum to 1. I'd like to see those test cases covered.

I'm going to say r=me, because I think this is OK in its current form.

I have doubts that the way the DOM is connected to the rendering tree and the way that repainting is triggered is optimal here. When the progress element gets progressively larger numbers we should be able to see the minimum repainting work -- this will affect things like performance battery life.

[Yael]

Thank you, Darin, for your review.
Follow-up bugs will address the leftover issues.

[Yael]

Landed in r55980.

[Yael]

(In reply to comment #13)
> (From update of attachment 50464 [details])
> I think an indeterminate <progress> should be animated, and progress bar
> controls of Windows and Mac needs animation even if their value are not
> changed. This patch seems to have no capability of such cases.

The animation will be added in a later patch. Perhaps it was a bad judgement call to split it like that :-(

Qt is implementing the QProgressBar widget without instantiating NSProgressIndicator, by using HITheme API. Perhaps that would work for WebKit too?

[Kent Tamura]

(In reply to comment #27)
> Qt is implementing the QProgressBar widget without instantiating
> NSProgressIndicator, by using HITheme API. Perhaps that would work for WebKit
> too?

I didn't know HITheme API.  Thanks.
As for Windows, DrawFrameControl() doesn't support progress bars, but DrawThemeBackground() does.

I'm not sure if we can support animation with these APIs.
Do you have a plan to implement progress bars for other ports?