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

RESOLVED FIXED 62430

<progress> should support :indeterminate pseudo-class

https://bugs.webkit.org/show_bug.cgi?id=62430

Summary <progress> should support :indeterminate pseudo-class

Hajime Morrita

Reported 2011-06-09 23:10:14 PDT

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

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 attachment proposed patch, testcase, etc.

Yael

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

Created attachment 96899 [details] Patch

WebKit Review Bot

Comment 2 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.

Yael

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

Created attachment 96901 [details] Patch. Fix style :)

WebKit Review Bot

Comment 4 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

Yael

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

Created attachment 96903 [details] Patch. Fix Chromium build.

WebKit Review Bot

Comment 6 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

WebKit Review Bot

Comment 7 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

Kent Tamura

Comment 8 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

Kent Tamura

Comment 9 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.

Dominic Cooney

Comment 10 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?

Yael

Comment 11 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.

Yael

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

Created attachment 96951 [details] Patch. Address comments #8, #9 and #10.

Yael

Comment 13 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.

Eric Seidel (no email)

Comment 14 2011-06-13 15:04:06 PDT

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

Kent Tamura

Comment 15 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.

Yael

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

Created attachment 97099 [details] Patch. Additional changes from comment #15.

WebKit Review Bot

Comment 17 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

Yael

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

Created attachment 97103 [details] Patch. Update test_expectations.txt

WebKit Review Bot

Comment 19 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>

WebKit Review Bot

Comment 20 2011-06-14 06:22:40 PDT

All reviewed patches have been landed. Closing bug.

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

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component CSS

Assignee

Nobody

Reported

2011-06-09 23:10 PDT

Modified

2011-06-14 06:22 PDT History

CC List

7 users Show

URL

Keywords

Depends on

Blocks

HTML5Forms

Dependencies

tree graph