62430 – <progress> should support :indeterminate pseudo-class

Bug 62430 - <progress> should support :indeterminate pseudo-class

Summary: <progress> should support :indeterminate pseudo-class

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	CSS (show other bugs)
Version:	528+ (Nightly build)
Hardware:	Unspecified Unspecified

Importance:	P2 Normal
Assignee:	Nobody

URL:
Keywords:

Depends on:
Blocks:	HTML5Forms
	Show dependency tree / graph

Reported:	2011-06-09 23:10 PDT by Hajime Morrita
Modified:	2011-06-14 06:22 PDT (History)
CC List:	7 users (show)

See Also:

Attachments
Patch (27.76 KB, patch) 2011-06-12 18:19 PDT, Yael	no flags	Details \| Formatted Diff \| Diff
Patch. (27.75 KB, patch) 2011-06-12 18:22 PDT, Yael	webkit.review.bot: commit-queue-	Details \| Formatted Diff \| Diff
Patch. (27.75 KB, patch) 2011-06-12 18:38 PDT, Yael	webkit.review.bot: commit-queue-	Details \| Formatted Diff \| Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.60 MB, application/zip) 2011-06-12 18:54 PDT, WebKit Review Bot	no flags	Details
Patch. (29.29 KB, patch) 2011-06-13 06:34 PDT, Yael	tkent: review+	Details \| Formatted Diff \| Diff
Patch. (29.24 KB, patch) 2011-06-14 05:26 PDT, Yael	webkit.review.bot: commit-queue-	Details \| Formatted Diff \| Diff
Patch. (29.35 KB, patch) 2011-06-14 05:41 PDT, Yael	no flags	Details \| Formatted Diff \| Diff
Show Obsolete (5) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Hajime Morrita 2011-06-09 23:10:14 PDT

As Mozilla does. https://developer.mozilla.org/en/HTML/element/progress

Comment 1 Yael 2011-06-12 18:19:20 PDT

Created attachment 96899 [details]
Patch

Comment 2 WebKit Review Bot 2011-06-12 18:21:52 PDT

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/html/HTMLProgressElement.cpp:152:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 9 files


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

Comment 3 Yael 2011-06-12 18:22:57 PDT

Created attachment 96901 [details]
Patch.

Fix style :)

Comment 4 WebKit Review Bot 2011-06-12 18:27:38 PDT

Comment on attachment 96901 [details]
Patch.

Attachment 96901 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8835058

Comment 5 Yael 2011-06-12 18:38:05 PDT

Created attachment 96903 [details]
Patch.

Fix Chromium build.

Comment 6 WebKit Review Bot 2011-06-12 18:54:28 PDT

Comment on attachment 96903 [details]
Patch.

Attachment 96903 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8831617

New failing tests:
fast/dom/HTMLProgressElement/indeterminate-progress-001.html

Comment 7 WebKit Review Bot 2011-06-12 18:54:34 PDT

Created attachment 96905 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Comment 8 Kent Tamura 2011-06-12 18:59:48 PDT

(In reply to comment #6)
> (From update of attachment 96903 [details])
> Attachment 96903 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/8831617
> 
> New failing tests:
> fast/dom/HTMLProgressElement/indeterminate-progress-001.html

Would you add the following line to LayoutTests/chromium/test_expectations.txt please?

BUGWK62430 : fast/dom/HTMLProgressElement/indeterminate-progress-001.html = FAIL

Comment 9 Kent Tamura 2011-06-12 19:06:14 PDT

Comment on attachment 96903 [details]
Patch.

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

> Source/WebCore/css/CSSStyleSelector.cpp:1078
>      if (isControl) {

nit: This block for isControl is large.  Can we make a separated function?

> Source/WebCore/html/HTMLProgressElement.cpp:140
> +    return (HTMLProgressElement::IndeterminatePosition != position() && HTMLProgressElement::InvalidPosition != position());

Calling position() twice is not efficient.

> LayoutTests/fast/dom/HTMLProgressElement/indeterminate-progress-001.html:12
> +  document.getElementById("dymanicindeterminate").removeAttribute("value");
> +  document.getElementById("dymanicdeterminate").setAttribute("value", 0.5);
> +}

We usually use 4-space indentation in JavaScript code.

> LayoutTests/fast/dom/HTMLProgressElement/indeterminate-progress-002.html:10
> +window.onload = function() {
> +  if (window.layoutTestController)

ditto.

Comment 10 Dominic Cooney 2011-06-12 19:08:54 PDT

Comment on attachment 96903 [details]
Patch.

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

> Source/WebCore/css/CSSStyleSelector.cpp:1085
> +            if ((thisProgressElement->isDeterminate() && !otherProgressElement->isDeterminate()) || (!thisProgressElement->isDeterminate() && otherProgressElement->isDeterminate()))

Maybe

thisProgressElement->isDeterminate() != otherProgressElement->isDeterminate()

is simpler?

Then since there is only one expression, you could inline the casts to shorten this a bit more.

> LayoutTests/fast/dom/HTMLProgressElement/indeterminate-progress-001.html:1
> +<!DOCTYPE html>

Does the pixel result for this test really add anything that the textual result doesn't have?

Comment 11 Yael 2011-06-13 06:33:16 PDT

(In reply to comment #10)
> (From update of attachment 96903 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96903&action=review
Thank you for reviewing :)
> 
> > Source/WebCore/css/CSSStyleSelector.cpp:1085
> > +            if ((thisProgressElement->isDeterminate() && !otherProgressElement->isDeterminate()) || (!thisProgressElement->isDeterminate() && otherProgressElement->isDeterminate()))
> 
> Maybe
> 
> thisProgressElement->isDeterminate() != otherProgressElement->isDeterminate()
> 
> is simpler?
> 
ok

> Then since there is only one expression, you could inline the casts to shorten this a bit more.
> 
> > LayoutTests/fast/dom/HTMLProgressElement/indeterminate-progress-001.html:1
> > +<!DOCTYPE html>
> 
> Does the pixel result for this test really add anything that the textual result doesn't have?
I think so; Just like progress-bar-value-pseudo-element.html, the pixel result shows that we can see the changes to the style of the progress element.

Comment 12 Yael 2011-06-13 06:34:42 PDT

Created attachment 96951 [details]
Patch.

Address comments #8, #9 and #10.

Comment 13 Yael 2011-06-13 06:36:27 PDT

(In reply to comment #8)
> (In reply to comment #6)
> > (From update of attachment 96903 [details] [details])
> > Attachment 96903 [details] [details] did not pass chromium-ews (chromium-xvfb):
> > Output: http://queues.webkit.org/results/8831617
> > 
> > New failing tests:
> > fast/dom/HTMLProgressElement/indeterminate-progress-001.html
> 
> Would you add the following line to LayoutTests/chromium/test_expectations.txt please?
> 
> BUGWK62430 : fast/dom/HTMLProgressElement/indeterminate-progress-001.html = FAIL

Thank you for your review.
I added the line, but I think the failure is because of the missing expected result.

Comment 14 Eric Seidel (no email) 2011-06-13 15:04:06 PDT

I'm not really sure who has worked in this code.

Comment 15 Kent Tamura 2011-06-13 16:11:59 PDT

Comment on attachment 96951 [details]
Patch.

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

> Source/WebCore/html/HTMLProgressElement.cpp:151
> +        if ((wasIndeterminate && isDeterminate()) || (!wasIndeterminate && !isDeterminate()))

We can write this as
  if (wasIndeterminate == isDeterminate())
Also, I think wasIndeterminate should be changed to wasDeterminate.
  bool wasDeterminate = render->isDeterminate();
  render->updateFromEelemnt();
  if (wasDetermiante != isDeterminate)
      setNeedsStyleRecalc();

"if (status is change) setNeedsStyleRecalc()" is a common idiom and easy to read for us.

Comment 16 Yael 2011-06-14 05:26:15 PDT

Created attachment 97099 [details]
Patch.

Additional changes from comment #15.

Comment 17 WebKit Review Bot 2011-06-14 05:28:07 PDT

Comment on attachment 97099 [details]
Patch.

Rejecting attachment 97099 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-02', '--port..." exit_code: 2

Last 500 characters of output:
ected.txt
patching file LayoutTests/fast/dom/HTMLProgressElement/indeterminate-progress-002.html
patching file LayoutTests/platform/chromium/test_expectations.txt
Hunk #1 FAILED at 3994.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/chromium/test_expectations.txt.rej
patching file LayoutTests/platform/mac/fast/dom/HTMLProgressElement/indeterminate-progress-001-expected.txt

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/8844049

Comment 18 Yael 2011-06-14 05:41:07 PDT

Created attachment 97103 [details]
Patch.

Update test_expectations.txt

Comment 19 WebKit Review Bot 2011-06-14 06:22:31 PDT

Comment on attachment 97103 [details]
Patch.

Clearing flags on attachment: 97103

Committed r88793: <http://trac.webkit.org/changeset/88793>

Comment 20 WebKit Review Bot 2011-06-14 06:22:40 PDT

All reviewed patches have been landed.  Closing bug.