Bug 36206 - Fix the rendering of HTMLProgressElement
Summary: Fix the rendering of HTMLProgressElement
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Yael
URL:
Keywords:
Depends on: 36113
Blocks: 36224
  Show dependency treegraph
 
Reported: 2010-03-16 19:34 PDT by Yael
Modified: 2010-03-26 08:43 PDT (History)
3 users (show)

See Also:


Attachments
Patch v1. (18.01 KB, patch)
2010-03-17 12:33 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch v2. (12.30 KB, patch)
2010-03-17 14:28 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch v3 (13.05 KB, patch)
2010-03-26 07:10 PDT, Yael
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yael 2010-03-16 19:34:28 PDT
I received today this link from Ian Hickson, which details the expected style for HTMLProgressElement.
http://www.whatwg.org/specs/web-apps/current-work/complete.html#the-progress-element-0 .
Comment 1 Yael 2010-03-17 12:33:46 PDT
Created attachment 50932 [details]
Patch v1.

Update the style information to conform to http://www.whatwg.org/specs/web-apps/current-work/complete.html#the-progress-element-0.
Originally, I did copied the style of <input type="range">, not knowing that the style was actually specified.

Since this patch is blocked by https://bugs.webkit.org/show_bug.cgi?id=36113, and I am not sure how to handle this situation, I included the code that is up for review in https://bugs.webkit.org/show_bug.cgi?id=36113 in this patch.
Comment 2 Yael 2010-03-17 14:28:59 PDT
Created attachment 50959 [details]
Patch v2.

Update the patch after https://bugs.webkit.org/show_bug.cgi?id=36113 was resolved.
Comment 3 Yael 2010-03-22 09:51:14 PDT
This style adjustment will be merged to https://bugs.webkit.org/show_bug.cgi?id=36224.
Comment 4 Yael 2010-03-26 05:34:58 PDT
Splitting up the patch from https://bugs.webkit.org/show_bug.cgi?id=36224 again.
Comment 5 Yael 2010-03-26 07:10:20 PDT
Created attachment 51738 [details]
Patch v3

Previously, I thought that <progress> element should be simillar to the slider of <input type="range">, so I used code from RenderSlider, (baselinePosition and calcPrefWidths).
http://www.whatwg.org/specs/web-apps/current-work/complete.html#the-progress-element-0 specifies the rendering rules for <progress> element, so this patch fixes that mistake.
This patch also removes code in RenderProgress::updateFromElement() that tries to optimize the size of the rect to be repainted. The optimization will be redone later.
Comment 6 Antti Koivisto 2010-03-26 07:44:53 PDT
Comment on attachment 51738 [details]
Patch v3

r=me, but consider changing these:

> Index: WebCore/platform/qt/RenderThemeQt.cpp
> ===================================================================
> --- WebCore/platform/qt/RenderThemeQt.cpp	(revision 56617)
> +++ WebCore/platform/qt/RenderThemeQt.cpp	(working copy)
> @@ -679,7 +679,7 @@ bool RenderThemeQt::paintProgressBar(Ren
>      option.rect = r;
>      option.maximum = 65536;
>      option.minimum = 0;
> -    option.progress = renderProgress->position();
> +    option.progress = (renderProgress->position() * 65536);

Where does the magic 65536 come from? Please use std::numeric_limits<int>::max() or similar.

> @@ -97,21 +57,12 @@ void RenderProgress::layout()
>  void RenderProgress::updateFromElement()
>  {
>      HTMLProgressElement* element = static_cast<HTMLProgressElement*>(node());
> -    double position = element->position();
> -    int oldPosition = m_position;
> -    bool canOptimize = theme()->getNumberOfPixelsForProgressPosition(position, m_position);
> +    double oldPosition = m_position;
> +    m_position = element->position();
>      if (oldPosition == m_position)
>          return;

Could you just do if (m_position == element->position()). oldPosition does not seem to be used anywhere else.
yea
Comment 7 Yael 2010-03-26 08:43:21 PDT
Landed in 56629