Initial support for HTMLMeterElement

• Component: DOM
• Status: RESOLVED FIXED

[Yael]

Initial support for HTMLMeterElement, supporting Qt platform only.

As I discussed with MORITA Hajime during the WebKit contributors meeting, I was going to upload this patch once the work on HTMLProgressElement is done.

[Yael]

Created attachment 54332 [details]
Patch

This patch is initial support for HTMLMeterElement, supporting Qt platform only.
Rendering of the meter element is very basic and is likely to change once I get more clarification on how it should look like.

[WebKit Review Bot]

Attachment 54332 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/WebCore.vcproj/WebCore.vcproj:6504:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 120 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yael]

Created attachment 54335 [details]
Fix style issue

Uploaded the wrong patch :-(

[WebKit Review Bot]

Attachment 54335 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/WebCore.vcproj/WebCore.vcproj:6504:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 120 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yael]

Created attachment 54336 [details]
Third time is a charm

[WebKit Review Bot]

Attachment 54336 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/WebCore.vcproj/WebCore.vcproj:6504:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 120 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yael]

Now I am very confused. My local copy shows no errors when I run check-webkit-style.

[WebKit Review Bot]

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

[Yael]

Created attachment 54352 [details]
Attempt to fix the Chromium build.

[WebKit Review Bot]

Attachment 54352 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/WebCore.vcproj/WebCore.vcproj:6504:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 120 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Hajime Morrita]

Hi yeal, thank you for working on this!
There is one request:

> +#if ENABLE(METER_TAG)
> +void RenderThemeQt::adjustMeterStyle(CSSStyleSelector*, RenderStyle* style, Element*) const
> +
> +bool RenderThemeQt::paintMeter(RenderObject* o, const RenderObject::PaintInfo& pi, const IntRect& r)
How about to move these method up to RenderTheme class using GraphicsContext?
There looks no Qt specific feature to draw it (just fillRect() - right?).
Because only a few platform has its own "meter" control, reusing this on other platform (ex. Chromium linux) would be fine.
Platforms with custom "meter" can just override it and implement their own.

[Kent Tamura]

Comment on attachment 54352 [details]
Attempt to fix the Chromium build.

WebCore/html/HTMLMeterElement.cpp:89
 +          value = 0;

Need to check NaN and Infinitiy.
HTMLInputElement::parseToDoubleForNumber() implements the rules of HTML5.  Probably we should move parseToDoubelForNumber() to a common place and use it here too.
I found the same issue in HTMLProgressElement.cpp.


WebCore/html/HTMLMeterElement.cpp:95
 +      setAttribute(valueAttr, String::number(isnan(value) ? 0 : value));

* We need to check infinity too.  So we should use !isfinite(value).
* The HTML5 rule to convert a floating point number to a string is implemented in HTMLInputElement::serializeForNumberType().


WebCore/html/HTMLMeterElement.cpp:102
 +      double min = minString.toDouble(&ok);

Need to check NaN and Infinity.



LayoutTests/fast/dom/HTMLMeterElement/script-tests/set-meter-properties.js:73
 +  

We should have tests for NaN, Infinity.

[Yael]

I think the style errors are side effects of http://trac.webkit.org/changeset/58249 . When I reverted that change, the errors disappeared.

[Yael]

(In reply to comment #12)
> (From update of attachment 54352 [details])
> WebCore/html/HTMLMeterElement.cpp:89
>  +          value = 0;
> 
> Need to check NaN and Infinitiy.
> HTMLInputElement::parseToDoubleForNumber() implements the rules of HTML5. 
> Probably we should move parseToDoubelForNumber() to a common place and use it
> here too.
> I found the same issue in HTMLProgressElement.cpp.
> 
> 
> WebCore/html/HTMLMeterElement.cpp:95
>  +      setAttribute(valueAttr, String::number(isnan(value) ? 0 : value));
> 
> * We need to check infinity too.  So we should use !isfinite(value).
> * The HTML5 rule to convert a floating point number to a string is implemented
> in HTMLInputElement::serializeForNumberType().
> 
> 
> WebCore/html/HTMLMeterElement.cpp:102
>  +      double min = minString.toDouble(&ok);
> 
> Need to check NaN and Infinity.
> 
> 
> 
> LayoutTests/fast/dom/HTMLMeterElement/script-tests/set-meter-properties.js:73
>  +  
> 
> We should have tests for NaN, Infinity.

Thank you for your comments Kent, I will look into moving the parsing code into a common place.

[Kent Tamura]

(In reply to comment #12)
> * We need to check infinity too.  So we should use !isfinite(value).
> * The HTML5 rule to convert a floating point number to a string is implemented
> in HTMLInputElement::serializeForNumberType().

and we should throw an exception.
http://www.whatwg.org/specs/web-apps/current-work/multipage/infrastructure.html#float-nan

[Yael]

Created attachment 54893 [details]
Patch addressing comments #6, #12 and #15.

1) Moved the rendering code to RenderTheme.cpp so that other platforms can use it. (comment #6)
2) Take into use the more strict number parsing algorithm. (comment #12)
3) Throw an exception when an invalid number is used. (comment #15)

[Yael]

(In reply to comment #16)
> Created an attachment (id=54893) [details]
> Patch addressing comments #6, #12 and #15.
> 
> 1) Moved the rendering code to RenderTheme.cpp so that other platforms can use
> it. (comment #6)
> 2) Take into use the more strict number parsing algorithm. (comment #12)
> 3) Throw an exception when an invalid number is used. (comment #15)
I meant comment #11, of course :-)

[Yael]

Created attachment 54921 [details]
Patch adding more tests and removing redundant isnan calls

Comments from https://bugs.webkit.org/show_bug.cgi?id=38434#c2 apply here too.

[Darin Adler]

Comment on attachment 54921 [details]
Patch adding more tests and removing redundant isnan calls

The tests that expect exception should use shouldThrow instead of being hand-written.

Why do we need a derived class, RenderMeter, instead of just using a RenderBlock? For the custom layout function?

> +        case MeterPart:
> +#if ENABLE(METER_TAG)
> +            m_value.ident = CSSValueMeter;
> +#endif
> +            break;

This doesn't seem right. With the ifdef written like this won't m_value.ident be uninitialized in this case?

> +HTMLMeterElement::HTMLMeterElement(const QualifiedName& tagName, Document* document, HTMLFormElement* form)
> +    : HTMLFormControlElement(tagName, document, form, CreateElement)
> +{
> +    ASSERT(hasTagName(meterTag));
> +}

Does this form element really need the HTMLFormElement argument to the constructor? If so, I'd like to see tests showing that it does. Did you add any tests that would fail if we passed 0 for the form?

> +void HTMLMeterElement::parseMappedAttribute(MappedAttribute* attr)

In new code, please use words instead of abbreviations whenever possible, as they are typically easier to read even if longer. This should be "attribute", not "attr".

> +    if (attr->name() == valueAttr) {
> +        if (renderer())
> +            renderer()->updateFromElement();
> +    } else if (attr->name() == minAttr) {
> +        if (renderer())
> +            renderer()->updateFromElement();
> +    } else if (attr->name() == maxAttr) {
> +        if (renderer())
> +            renderer()->updateFromElement();
> +    } else if (attr->name() == lowAttr) {
> +        if (renderer())
> +            renderer()->updateFromElement();
> +    } else if (attr->name() == highAttr) {
> +        if (renderer())
> +            renderer()->updateFromElement();
> +    } else if (attr->name() == optimumAttr) {
> +        if (renderer())
> +            renderer()->updateFromElement();

Can we find a less repetitive way of writing this?

> +    double value;
> +    bool ok = parseToDoubleForNumberType(getAttribute(valueAttr), &value);
> +    if (!ok)
> +        value = 0;

Can't you write that like this?

    double value = 0;
    parseToDoubleForNumberType(getAttribute(valueAttr), &value);

I think that's simpler.

> +    return std::min(std::max(value, min()), max());

When I write this I like to order the values like this:

    std::max(min(), std::min(value, max()))

It seems to make it more obvious that we are putting the value between the min and max values, but also my ordering makes the min win if the min and max conflict, which is typically what we want. I guess that you instead made the max() function call the min() function which is OK if that's what the standard requires.

As long as we have enough test cases I think we're OK either way.

> +    // Some platforms do not have a native gauge widget, so we draw here a default implementation.

It's strange to have a comment here partway through the function saying this. Isn't this comment the entire reason for the whole function?

> +    FloatRect valueRect;
> +    FloatRect backgroundRect;
> +    if (min >= max) {
> +        paintInfo.context->fillRect(innerRect, Color::black, DeviceColorSpace);
> +        return false;
> +    }

The definitions of the two rectangles should go further down, after this early return.

> +    if (rect.width() < rect.height()) {
> +        // Vertical gauge
> +        double scale = innerRect.height() / (max - min);
> +        valueRect.setLocation(FloatPoint(innerRect.x(), innerRect.y() + (max - value) * scale));
> +        valueRect.setSize(FloatSize(innerRect.width(), (value - min) * scale));
> +        backgroundRect.setLocation(innerRect.location());
> +        backgroundRect.setSize(FloatSize(innerRect.width(), (max - value) * scale));
> +    } else if (renderMeter->style()->direction() == RTL) {
> +        // right to left horizontal gauge
> +        double scale = innerRect.width() / (max - min);
> +        valueRect.setLocation(FloatPoint(innerRect.x() + (max - value) * scale, innerRect.y()));
> +        valueRect.setSize(FloatSize((value - min) * scale, innerRect.height()));
> +        backgroundRect.setLocation(innerRect.location());
> +        backgroundRect.setSize(FloatSize((max - value) * scale, innerRect.height()));
> +    } else {
> +        // left to right horizontal gauge
> +        double scale = innerRect.width() / (max - min);
> +        valueRect.setLocation(innerRect.location());
> +        valueRect.setSize(FloatSize((value - min) * scale, innerRect.height()));
> +        backgroundRect.setLocation(FloatPoint(innerRect.x() + valueRect.width(), innerRect.y()));
> +        backgroundRect.setSize(FloatSize((max - value) * scale, innerRect.height()));
> +    }
> +    if (!valueRect.isEmpty())
> +        paintInfo.context->fillRect(valueRect, Color::black, DeviceColorSpace);
> +
> +    if (!backgroundRect.isEmpty())
> +        paintInfo.context->fillRect(backgroundRect, Color::lightGray, DeviceColorSpace);

I am not sure this basic approach is solid enough. I suggest painting the entire rect with the background and then have the value rect painted on top of it. Generally speaking I don't know how the rectangles computed here will end up stitching together when the edges are not on pixel boundaries.

Also, to compute the rectangles you should only multiple by the scale factor once. The rectangle on the right or bottom should be sized to fill the rest of the space rather than separately doing "max - value" computation.

It's not correct to pass DeviceColorSpace for all these colors. Instead you should be passing style()->colorSpace().

What does the return value of this function mean? Is it right for the theme to always paint? Shouldn't the non-theme-specific painting be in the renderer class, not the theme at all? I think the theme should be returning true like the other functions such as paintProgressBar.

[Yael]

Created attachment 55016 [details]
Patch addressing comment #19

(In reply to comment #19)
> (From update of attachment 54921 [details])
> Why do we need a derived class, RenderMeter, instead of just using a
> RenderBlock? For the custom layout function?
RenderMeter does a custom layout and also triggers a repaint when an attribute of the meter element changes. As the implementation of the meter element becomes more complete, we would need more support from this renderer. 

> > +    return std::min(std::max(value, min()), max());
> 
> When I write this I like to order the values like this:
> 
>     std::max(min(), std::min(value, max()))
> 
> It seems to make it more obvious that we are putting the value between the min
> and max values, but also my ordering makes the min win if the min and max
> conflict, which is typically what we want. I guess that you instead made the
> max() function call the min() function which is OK if that's what the standard
> requires.
> 
> As long as we have enough test cases I think we're OK either way.

From the user agent requirements for the meter element:

User agents must then use all these numbers to obtain values for six points on the gauge, as follows. (The order in which these are evaluated is important, as some of the values refer to earlier ones.)

I wanted to follow the order as defined by the spec. I added 4 tests to test that the calculation is correct. Please see the tests marked "Set attributes to improper values - 1 through 4".

> What does the return value of this function mean? Is it right for the theme to
> always paint? Shouldn't the non-theme-specific painting be in the renderer
> class, not the theme at all? I think the theme should be returning true like
> the other functions such as paintProgressBar.

The return value is used by RenderBox::paintBoxDecorationsWithSize to determine if the RenderTheme painted or not. In case of the progress element, RenderTheme does not paint, thus returns true. Each platform's RenderTheme would return false. In case of paintMeter, we have a default implementation that does paint, so it should return false.
From comment #11 I understand that some platforms will want to overwrite Rendertheme::paintMeter, and that is why this method is implemented in RenderTheme and not in RenderMeter.

[Kent Tamura]

Comment on attachment 55016 [details]
Patch addressing comment #19

WebCore/html/HTMLMeterElement.h:54
 +      virtual bool isOptionalFormControl() const { return true; }
This seems curious.  Why do we need to return false for isOptionalFormControl()?


Could you add a test confirming that <meter> tag behavior is not changed on platforms without ENABLE_METER_TAG?
For example, "<meter>56%</meter>" should work like "<span>56%</span>"

[Yael]

(In reply to comment #21)
> (From update of attachment 55016 [details])
> WebCore/html/HTMLMeterElement.h:54
>  +      virtual bool isOptionalFormControl() const { return true; }
> This seems curious.  Why do we need to return false for isOptionalFormControl()?
> 
From http://dev.w3.org/html5/spec/Overview.html#attr-input-required , the required attribute applies only to input (and textarea) elements. All other form control elements return true.

> Could you add a test confirming that <meter> tag behavior is not changed on platforms without ENABLE_METER_TAG?
> For example, "<meter>56%</meter>" should work like "<span>56%</span>"
This was discussed before, in https://bugs.webkit.org/show_bug.cgi?id=35937. There is no mechanism for adding conditions to html.css, so the new style definition will affect ports with this flag turned off. There is precedence with other new tags e.g. datagrid for doing the same.

[Yael]

Created attachment 55925 [details]
Update the patch with latest trunk changes.

[WebKit Review Bot]

Attachment 55925 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/html/HTMLMeterElement.cpp:66:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Total errors found: 1 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yael]

Created attachment 55926 [details]
Fix style issue

[Kent Tamura]

Comment on attachment 55926 [details]
Fix style issue

WebCore/rendering/RenderTheme.cpp:924
 +          valueRect.setLocation(FloatPoint(innerRect.x(), innerRect.y() + (max - value) * scale));
Need static_cast<float>() because max, value and scale are double.

WebCore/rendering/RenderTheme.cpp:925
 +          valueRect.setSize(FloatSize(innerRect.width(), (value - min) * scale));
ditto.

WebCore/rendering/RenderTheme.cpp:929
 +          valueRect.setLocation(FloatPoint(innerRect.x() + (max - value) * scale, innerRect.y()));
ditto.

WebCore/rendering/RenderTheme.cpp:930
 +          valueRect.setSize(FloatSize((value - min) * scale, innerRect.height()));
ditto.

WebCore/rendering/RenderTheme.cpp:935
 +          valueRect.setSize(FloatSize((value - min) * scale, innerRect.height()));
ditto.

(In reply to comment #22)
> (In reply to comment #21)
> > (From update of attachment 55016 [details] [details])
> > WebCore/html/HTMLMeterElement.h:54
> >  +      virtual bool isOptionalFormControl() const { return true; }
> > This seems curious.  Why do we need to return false for isOptionalFormControl()?
> > 
> From http://dev.w3.org/html5/spec/Overview.html#attr-input-required , the required attribute applies only to input (and textarea) elements. All other form control elements return true.

isOptionalFormControl() is used only for checking whether :optional CSS selector is applied or not.
So your code mean that :optional is always applied to <meter>.
I don't think we need to make it true for elements without form values.
If you'd like to avoid form validation for <meter>, override recalcWillValidate(). See HTMLFieldSetElement.h.


> > Could you add a test confirming that <meter> tag behavior is not changed on platforms without ENABLE_METER_TAG?
> > For example, "<meter>56%</meter>" should work like "<span>56%</span>"
> This was discussed before, in https://bugs.webkit.org/show_bug.cgi?id=35937. There is no mechanism for adding conditions to html.css, so the new style definition will affect ports with this flag turned off. There is precedence with other new tags e.g. datagrid for doing the same.

I see.  How about parsing test?
I tried your patch on Mac without ENABLE_METER_TAG, and found <meter> tags were completely disappeared.

[Yael]

(In reply to comment #26)
> I see.  How about parsing test?
> I tried your patch on Mac without ENABLE_METER_TAG, and found <meter> tags were completely disappeared.
Thank you for the review.
Could you please share the test page you were using? I tried <meter>%56</meter> and that showed in safari the same as <span>%56</span>.
The only problem I saw was the style, and that was due to the webkit-appearance.
thanks.

[Kent Tamura]

(In reply to comment #27)
> Could you please share the test page you were using? I tried <meter>%56</meter> and that showed in safari the same as <span>%56</span>.
> The only problem I saw was the style, and that was due to the webkit-appearance.
> thanks.

- run-safari --debug
- open an HTML page with <meter>56%</meter>
- Ctrl-Click on the 56%, and select "Inspect element"
- You'll see no <meter> around 56%

[Yael]

Created attachment 56099 [details]
Patch addressing comment #26.

Tested on Safari and with this patch I do see the meter element in the web inspector.

[Kent Tamura]

Comment on attachment 56099 [details]
Patch addressing comment #26.

LayoutTests/platform/chromium/test_expectations.txt:2631
 +  BUG37074 DEFER : fast/dom/HTMLMeterElement/set-meter-properties.html = TEXT
Please replace "BUG37074" with "BUGWK37074".

[Yael]

Committed r59541: <http://trac.webkit.org/changeset/59541>

[Adam Roben (:aroben)]

Comment on attachment 56099 [details]
Patch addressing comment #26.

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

> LayoutTests/platform/win/Skipped:840
> +# meter element is not supported
> +fast/dom/HTMLMeterElement/meter-element.html
> +fast/dom/HTMLMeterElement/set-meter-properties.html
> +

It's rude to add tests to a port's Skipped file without also filing a bug that describes why the test is skipped. Could someone please file a bug for implementing <meter> support on each platform that is currently skipping these tests and add those bugs to the Skipped files?

[Yael]

(In reply to comment #32)
> (From update of attachment 56099 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=56099&action=review
> 
> > LayoutTests/platform/win/Skipped:840
> > +# meter element is not supported
> > +fast/dom/HTMLMeterElement/meter-element.html
> > +fast/dom/HTMLMeterElement/set-meter-properties.html
> > +
> 
> It's rude to add tests to a port's Skipped file without also filing a bug that describes why the test is skipped. Could someone please file a bug for implementing <meter> support on each platform that is currently skipping these tests and add those bugs to the Skipped files?

I certainly did not mean to be rude, I simply did not know :-)
Filed https://bugs.webkit.org/show_bug.cgi?id=48713 for GTK port.

[Adam Roben (:aroben)]

(In reply to comment #33)
> (In reply to comment #32)
> > (From update of attachment 56099 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=56099&action=review
> > 
> > > LayoutTests/platform/win/Skipped:840
> > > +# meter element is not supported
> > > +fast/dom/HTMLMeterElement/meter-element.html
> > > +fast/dom/HTMLMeterElement/set-meter-properties.html
> > > +
> > 
> > It's rude to add tests to a port's Skipped file without also filing a bug that describes why the test is skipped. Could someone please file a bug for implementing <meter> support on each platform that is currently skipping these tests and add those bugs to the Skipped files?
> 
> I certainly did not mean to be rude, I simply did not know :-)
> Filed https://bugs.webkit.org/show_bug.cgi?id=48713 for GTK port.

Thank you!