Summary: | cloneNode() does not preserve z-index with more than six digits | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Steven Lai <s3lance> | ||||||||||||||||
Component: | CSS | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | abarth, adele, ap, bdakin, commit-queue, eric, steven_lai, webkit.review.bot | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||
Attachments: |
|
Created attachment 53902 [details]
Test case for the bug
There used to be a fix for the formatting problem, but it was rollbacked because it broke Win and Chromium builds, please see: http://trac.webkit.org/changeset/49554 http://trac.webkit.org/changeset/49555 Created attachment 53977 [details]
Minimal workaround for number formatting problem of large integral values
Comment on attachment 53977 [details]
Minimal workaround for number formatting problem of large integral values
This test needn't use waitUntilDone/notifyDone.
+LOG(newEl.style.zIndex + " " + (newEl.style.zIndex == 20002000 ? "PASS" : "FAIL"));
It's often helpful to dump the failing value (e.g. "FAIL: 2.0002e+7") - that way, if the test starts failing on some buildbots, there will be more data for investigation. Not too important - I don't think platform specific failures are likely on this test.
r=me
Sounds like this can make bug 32078 return. Please investigate that first. it did make it return :( Comment on attachment 53977 [details]
Minimal workaround for number formatting problem of large integral values
r- based on above comments.
Created attachment 54023 [details] Reverted the order change in Element::cloneElementWithoutChildren r54372 Reverted the order change in Element::cloneElementWithoutChildren r54372 Avoided cloned radio buttons from getting a wrong checked state by postponing the uncheck of the old radio button till the new radio button is attached + No new tests. (OOPS!) There should be! Comment on attachment 54023 [details] Reverted the order change in Element::cloneElementWithoutChildren r54372 > + if (inputType() == RADIO) { > + // The radio button could be checked but not yet added to the checkedRadioButtons > + // map (when checked is set before appending to the DOM tree) > + setNeedsStyleRecalc(); > + if (checked()) > + checkedRadioButtons(this).addButton(this); > + > + if (renderer() && renderer()->style()->hasAppearance()) > + renderer()->theme()->stateChanged(renderer(), CheckedState); > + } I have trouble understanding why this is exactly the right code to run. Can we factor things so that HTMLInputElement::setChecked shares code with this? It seems to be just copied and pasted. Comment on attachment 54023 [details] Reverted the order change in Element::cloneElementWithoutChildren r54372 9 No new tests. (OOPS!) will prevent this from being Cq'd. Why no tests? r- for no tests and copy/paste code per darin's comment. Created attachment 54962 [details]
Patch
Attachment 54962 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/html/HTMLInputElement.cpp:172: 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 on attachment 54962 [details] Patch > + if (attached() && checked()) { > + checkedRadioButtons(this).addButton(this); > + } No braces around single line if statements. r=me Attached test cases for the bug as suggested. Re Darin: The change tries to force view update when the radio group state of the radio button changed state, without firing the accessibility state change again and without firing the form change events twice. Created attachment 54970 [details]
Patch
Comment on attachment 54970 [details] Patch Rejecting patch 54970 from commit-queue. Failed to run "[u'git', u'svn', u'dcommit']" exit_code: 1 Last 500 characters of output: ore/html/HTMLInputElement.cpp M WebCore/html/HTMLInputElement.h A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output: The following files contain tab characters: trunk/LayoutTests/fast/dom/clone-node-z-index.html Please use spaces instead to indent. If you must commit a file with tabs, use svn propset to set the "allow-tabs" property. at /usr/local/git/libexec/git-core/git-svn line 570 Full output: http://webkit-commit-queue.appspot.com/results/2129015 Created attachment 55026 [details]
Patch (tab stripped)
Comment on attachment 55026 [details] Patch (tab stripped) Clearing flags on attachment: 55026 Committed r58766: <http://trac.webkit.org/changeset/58766> The layout case checked in failed on Windows (and possibly other platforms) see: https://bugs.webkit.org/show_bug.cgi?id=38617 Comment on attachment 54962 [details] Patch Cleared Darin Adler's review+ from obsolete attachment 54962 [details] so that this bug does not appear in http://webkit.org/pending-commit. It's unclear why this bug is still open. Did the commit bot fail to close it? The commit bot did not close it because there was still a patch up for review when it was landed. The commit-bot should add a comment when it does that to explain its behavior (That behavior was specifically requested by members of webkit to allow use of the commit-queue with multi-patch bugs.) :) You can see the history here: https://bugs.webkit.org/show_activity.cgi?id=37900 Sounds like this bug ought to have been closed. If that's not right, please file a new bug about whatever's left to be done. |
Created attachment 53900 [details] Test case for the bug Test case attached. The test case shows that when assigning a large integer value to z-index to an element. It appears to be converted to a float from the inspector. When the node is cloned, the new node won't have this z-index copied.