Bug 40280 – <meter> should allow styling for each gauge-level and component

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Hajime Morrita]

<meter> should allow styling using CSS like <progress> does.
It should have pseudo classes to 
- specify a style for the gauge level like optimum/suboptimal/even-less-good, and
- specify a style for the value part and the background(bar) part

[Kent Tamura]

Created attachment 58109 [details]
Mock HTML

(In reply to comment #0)
> <meter> should allow styling using CSS like <progress> does.
> It should have pseudo classes to 
> - specify a style for the gauge level like optimum/suboptimal/even-less-good, and
> - specify a style for the value part and the background(bar) part

I think we should have 8 pseudo classes.

- horizontal background
- horizontal optimum value
- horizontal suboptimal value
- horizontal even-less-good value
- vertical background
- vertical optimum value
- vertical suboptimal value
- vertical even-less-good value

[Hajime Morrita]

Created attachment 58632 [details]
patch v0

[Kent Tamura]

Comment on attachment 58632 [details]
patch v0

Initial comments:
  - Please update the base WebKit revision. Style bot and EWS are really helpful.
  - The default style of <meter> is not attractive :-)

[Kent Tamura]

Comment on attachment 58632 [details]
patch v0

LayoutTests/ChangeLog:10
 +          - Disabled pixel tests for HTMLMeterElement is temporality disabled.
This sentence looks curious.  Maybe either of "disabled" is not needed?
typo: temporality -> temporarily


LayoutTests/fast/dom/HTMLMeterElement/meter-styles-changing-pseudo.html:13
 +          window.setTimeout(function(evt) {
What's the intention of this setTimeout()?


LayoutTests/platform/qt/Skipped:3289
 +  fast/dom/HTMLMeterElement/meter-element.html
Because Qt has ENABLE_METER_TAG, we don't need to skip the test.  we can:
 - Commit this change
 - Qt buildbot runs
 - Pick the Qt test result
 - Commit it


WebCore/WebCore.xcodeproj/project.pbxproj:21781
 +  				A7AA66D611C5ED6A001D8C8C /* RenderIndicator.cpp in Sources */,
Should be sorted.


WebCore/rendering/RenderMeter.cpp:66
 +      return IntRect(borderLeft() + paddingLeft(), borderTop() + paddingTop(), lround(width() - borderLeft() - paddingLeft() - borderRight() - paddingRight()), height()  - borderTop() - paddingTop() - borderBottom() - paddingBottom());
We should have a test for this.  e.g.
meter {
  border-left-width: 100px;
  border-right-width: 1px;
}
and the meter value is just a half.



WebCore/rendering/RenderTheme.cpp:236
 +  bool RenderTheme::canPaintControlsFor(RenderObject* renderObject) const
Can we merge this into RenderTheme::isControlStyled()?

[Hajime Morrita]

Created attachment 58843 [details]
fix

[Hajime Morrita]

Comment on attachment 58843 [details]
fix

I missed some point from the last review. will re-submit.

[Kent Tamura]

(In reply to comment #3)
> (From update of attachment 58632 [details])
> Initial comments:
>   - Please update the base WebKit revision. Style bot and EWS are really helpful.

Ah, I remember svn-apply doesn't support git binary diff.  So the style bot and EWS don't work for your patch.
Please make sure your patch doesn't have style errors before uploading.

[Hajime Morrita]

Created attachment 58853 [details]
patch v2

[Hajime Morrita]

Hi Kent-san, thank you for reviewing!
I updated the patch.

(In reply to comment #4)
> (From update of attachment 58632 [details])
> LayoutTests/ChangeLog:10
>  +          - Disabled pixel tests for HTMLMeterElement is temporality disabled.
> This sentence looks curious.  Maybe either of "disabled" is not needed?
> typo: temporality -> temporarily
Done (removed).

> 
> 
> LayoutTests/fast/dom/HTMLMeterElement/meter-styles-changing-pseudo.html:13
>  +          window.setTimeout(function(evt) {
> What's the intention of this setTimeout()?
Removed.
On writing it, I'd like to make sure that rendering is done, though I realized that it's useless.

> 
> 
> LayoutTests/platform/qt/Skipped:3289
>  +  fast/dom/HTMLMeterElement/meter-element.html
> Because Qt has ENABLE_METER_TAG, we don't need to skip the test.  we can:
>  - Commit this change
>  - Qt buildbot runs
>  - Pick the Qt test result
>  - Commit it
OK, remove the line from Skipped file.

> 
> 
> WebCore/WebCore.xcodeproj/project.pbxproj:21781
>  +                  A7AA66D611C5ED6A001D8C8C /* RenderIndicator.cpp in Sources */,
> Should be sorted.
Oops. I'll sort it before landing because sorting tend to cause a conflict.

> WebCore/rendering/RenderMeter.cpp:66
>  +      return IntRect(borderLeft() + paddingLeft(), borderTop() + paddingTop(), lround(width() - borderLeft() - paddingLeft() - borderRight() - paddingRight()), height()  - borderTop() - paddingTop() - borderBottom() - paddingBottom());
> We should have a test for this.  e.g.
> meter {
>   border-left-width: 100px;
>   border-right-width: 1px;
> }
> and the meter value is just a half.
Added a test to meter-style.html.

> WebCore/rendering/RenderTheme.cpp:236
>  +  bool RenderTheme::canPaintControlsFor(RenderObject* renderObject) const
> Can we merge this into RenderTheme::isControlStyled()?
No, It cannot.
But I agree that canPaintControlsFor() is over-generalized.
So I removed it and added supportsHorizontalMeter() instead.

[Kent Tamura]

(In reply to comment #9)
> > WebCore/rendering/RenderTheme.cpp:236
> >  +  bool RenderTheme::canPaintControlsFor(RenderObject* renderObject) const
> > Can we merge this into RenderTheme::isControlStyled()?

> No, It cannot.
> But I agree that canPaintControlsFor() is over-generalized.
> So I removed it and added supportsHorizontalMeter() instead.

Could you explain the reason why it can't?

[Hajime Morrita]

(In reply to comment #10)
> (In reply to comment #9)
> > > WebCore/rendering/RenderTheme.cpp:236
> > >  +  bool RenderTheme::canPaintControlsFor(RenderObject* renderObject) const
> > > Can we merge this into RenderTheme::isControlStyled()?
> 
> > No, It cannot.
> > But I agree that canPaintControlsFor() is over-generalized.
> > So I removed it and added supportsHorizontalMeter() instead.
> 
> Could you explain the reason why it can't?

- We need a RenderObject to refer the layout result, but isControlStyled() doesn't have it. 
   Because it is called before RenderObjects creation. 
- isControlStyle() requires extra arguments like BorderData which are not available for us.
- RenderTheme::adjustStyle() rewrites RenderStyle::m_appearance based on the result of isControlStyled().
   But we want to preserve the original appearance value always for <meter>.

After all, isControlStyled() is based on the specified style, 
canPaintControlsFor() is based on the layout result.

[Kent Tamura]

(In reply to comment #11)
> > Could you explain the reason why it can't?
> 
> - We need a RenderObject to refer the layout result, but isControlStyled() doesn't have it. 
>    Because it is called before RenderObjects creation. 
> - isControlStyle() requires extra arguments like BorderData which are not available for us.
> - RenderTheme::adjustStyle() rewrites RenderStyle::m_appearance based on the result of isControlStyled().
>    But we want to preserve the original appearance value always for <meter>.
> 
> After all, isControlStyled() is based on the specified style, 
> canPaintControlsFor() is based on the layout result.

I see.  <meter> always has width/height information in its style, but they may be relative.  e.g. height:50%. So we can't detect horizontal/vertical in adjustStyle()-isContrlStyled().

[Kent Tamura]

Comment on attachment 58853 [details]
patch v2


LayoutTests/fast/dom/HTMLMeterElement/meter-styles-changing-pseudo.html:8
 +      window.setTimeout(function(evt) {
Need a comment why setTimeou() is used.

Do you want to confirm the value change updates the element style?
if so, .offsetWidth may help to force repainting.
  document.getElementById("target1").offsetWidth;
  document.getElementById("target1").value = 90;
  ...
  document.getElementById("target1").offsetWidth;



LayoutTests/fast/dom/HTMLMeterElement/meter-styles-changing-pseudo.html:19
 +    meter.styled { -webkit-appearance: none; }
WebKit should reset -webkit-appearance automatically if an element has user-specified style, like <progress>.
Ideally, if
  #id1::-webkit-meter-horizonta-bar { background-color: white; }
is specified, -webkit-appearance for <meter id=id1> should be reset automatically.  However, I don't have no idea how to implement it.

WebCore/rendering/RenderIndicator.cpp:22
 +  #include "RenderIndicator.h"
We had better enclose the code with #if ENABLE(PROGRESS_TAG) || ENABLE(METER_TAG)


WebCore/rendering/RenderIndicator.h:24
 +  #if ENABLE(PROGRESS_TAG)
ditto.

WebCore/rendering/RenderIndicator.h:29
 +  //class ShadowBlockElement;
This line should be removed.


WebCore/rendering/RenderTheme.h:198
 +      virtual bool supportsHorizontalMeter() const { return false; }
supportsHorizontalMeter() should have a ControlPart parameter because a platform might support just one or two of the three indicator appearances.

[Kent Tamura]

(In reply to comment #13)
> However, I don't have no idea how to implement it.

I meant "I don't have good idea how to implement it."

[Hajime Morrita]

Created attachment 58962 [details]
patch v3

[Hajime Morrita]

Hi Kent-san, thank you for reviewing again!
I updated the patch.

> LayoutTests/fast/dom/HTMLMeterElement/meter-styles-changing-pseudo.html:8
>  +      window.setTimeout(function(evt) {
> Need a comment why setTimeou() is used.
> 
> Do you want to confirm the value change updates the element style?
> if so, .offsetWidth may help to force repainting.
Fixed to remove setTimeout() and force repaint by touching style object.

> LayoutTests/fast/dom/HTMLMeterElement/meter-styles-changing-pseudo.html:19
>  +    meter.styled { -webkit-appearance: none; }
> WebKit should reset -webkit-appearance automatically if an element has user-specified style, like <progress>.
> Ideally, if
>   #id1::-webkit-meter-horizonta-bar { background-color: white; }
> is specified, -webkit-appearance for <meter id=id1> should be reset automatically.  
Done. tests are also updated.

> WebCore/rendering/RenderIndicator.cpp:22
>  +  #include "RenderIndicator.h"
> We had better enclose the code with #if ENABLE(PROGRESS_TAG) || ENABLE(METER_TAG)
Done.

> WebCore/rendering/RenderIndicator.h:24
>  +  #if ENABLE(PROGRESS_TAG)
> ditto.
Doe.

> WebCore/rendering/RenderIndicator.h:29
>  +  //class ShadowBlockElement;
> This line should be removed.
Done.

> WebCore/rendering/RenderTheme.h:198
>  +      virtual bool supportsHorizontalMeter() const { return false; }
> supportsHorizontalMeter() should have a ControlPart parameter because a platform might support just one or two of the three indicator appearances.
Done.

[Kent Tamura]

Comment on attachment 58962 [details]
patch v3

Almost OK.

WebCore/rendering/RenderTheme.h:198
 +      virtual bool supportsHorizontalMeter(ControlPart) const { return false; }
Other platforms might support themes for vertical meters.
So supportsHorizontalMeter(ControlPart) should be supportsMeter(ControlPart, bool isHorizontal)


WebCore/rendering/ShadowElement.cpp:96
 +      //
No need this empty comment line


WebCore/rendering/ShadowElement.cpp:101
 +      //
ditto.

[Hajime Morrita]

Created attachment 59055 [details]
patch v4

[Hajime Morrita]

Hi, thank you for reviewing!
I updated the patch:
> WebCore/rendering/RenderTheme.h:198
>  +      virtual bool supportsHorizontalMeter(ControlPart) const { return false; }
> Other platforms might support themes for vertical meters.
> So supportsHorizontalMeter(ControlPart) should be supportsMeter(ControlPart, bool isHorizontal)
Done.

> WebCore/rendering/ShadowElement.cpp:96
>  +      //
> No need this empty comment line
Done.

> WebCore/rendering/ShadowElement.cpp:101
>  +      //
> ditto.
Done.

[Kent Tamura]

Comment on attachment 59055 [details]
patch v4

Looks OK.

Please commit this when the bots status are good.
Watch test bots results and correct test expectations if problems occur. See [1] for Chromium tests.
Sort project.pbxproj later.


[1] http://build.chromium.org/buildbot/waterfall.fyi/waterfall?branch=&builder=Webkit+(webkit.org)&builder=Webkit+Linux+(webkit.org)&builder=Webkit+Mac+(webkit.org)&builder=Webkit.org+Builder

[Hajime Morrita]

Committed r61376: <http://trac.webkit.org/changeset/61376>

[WebKit Review Bot]

http://trac.webkit.org/changeset/61376 might have broken Qt Linux Release

[Eric Seidel (no email)]

This broke a test on Gtk.

[Hajime Morrita]

> This broke a test on Gtk.
Which one?  I couldn't figure out that.
It seems a successive checkin overrides the waterfall status.
I'm watching the tree a while, anyway.