Bug 40280 - <meter> should allow styling for each gauge-level and component
Summary: <meter> should allow styling for each gauge-level and component
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 37074
  Show dependency treegraph
 
Reported: 2010-06-07 21:24 PDT by Hajime Morrita
Modified: 2010-06-17 23:53 PDT (History)
8 users (show)

See Also:


Attachments
Mock HTML (2.05 KB, text/html)
2010-06-07 21:42 PDT, Kent Tamura
no flags Details
patch v0 (297.13 KB, patch)
2010-06-14 04:14 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
fix (274.10 KB, patch)
2010-06-15 18:35 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
patch v2 (270.41 KB, patch)
2010-06-15 22:12 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
patch v3 (307.76 KB, patch)
2010-06-16 22:00 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
patch v4 (307.87 KB, patch)
2010-06-17 18:07 PDT, Hajime Morrita
tkent: review+
tkent: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2010-06-07 21:24:40 PDT
<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
Comment 1 Kent Tamura 2010-06-07 21:42:11 PDT
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
Comment 2 Hajime Morrita 2010-06-14 04:14:58 PDT
Created attachment 58632 [details]
patch v0
Comment 3 Kent Tamura 2010-06-14 04:37:51 PDT
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 :-)
Comment 4 Kent Tamura 2010-06-14 19:59:07 PDT
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()?
Comment 5 Hajime Morrita 2010-06-15 18:35:09 PDT
Created attachment 58843 [details]
fix
Comment 6 Hajime Morrita 2010-06-15 18:37:20 PDT
Comment on attachment 58843 [details]
fix

I missed some point from the last review. will re-submit.
Comment 7 Kent Tamura 2010-06-15 19:19:10 PDT
(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.
Comment 8 Hajime Morrita 2010-06-15 22:12:13 PDT
Created attachment 58853 [details]
patch v2
Comment 9 Hajime Morrita 2010-06-15 22:17:08 PDT
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.
Comment 10 Kent Tamura 2010-06-16 00:13:11 PDT
(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?
Comment 11 Hajime Morrita 2010-06-16 00:47:39 PDT
(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.
Comment 12 Kent Tamura 2010-06-16 02:13:01 PDT
(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().
Comment 13 Kent Tamura 2010-06-16 03:26:08 PDT
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.
Comment 14 Kent Tamura 2010-06-16 03:29:20 PDT
(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."
Comment 15 Hajime Morrita 2010-06-16 22:00:31 PDT
Created attachment 58962 [details]
patch v3
Comment 16 Hajime Morrita 2010-06-16 23:28:30 PDT
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.
Comment 17 Kent Tamura 2010-06-17 00:06:48 PDT
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.
Comment 18 Hajime Morrita 2010-06-17 18:07:18 PDT
Created attachment 59055 [details]
patch v4
Comment 19 Hajime Morrita 2010-06-17 18:09:05 PDT
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.
Comment 20 Kent Tamura 2010-06-17 19:07:46 PDT
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
Comment 21 Hajime Morrita 2010-06-17 22:13:57 PDT
Committed r61376: <http://trac.webkit.org/changeset/61376>
Comment 22 WebKit Review Bot 2010-06-17 22:38:52 PDT
http://trac.webkit.org/changeset/61376 might have broken Qt Linux Release
Comment 23 Eric Seidel (no email) 2010-06-17 23:26:03 PDT
This broke a test on Gtk.
Comment 24 Hajime Morrita 2010-06-17 23:53:38 PDT
> 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.