Bug 37900 - cloneNode() does not preserve z-index with more than six digits
Summary: cloneNode() does not preserve z-index with more than six digits
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-04-20 15:56 PDT by Steven Lai
Modified: 2010-05-15 00:15 PDT (History)
8 users (show)

See Also:


Attachments
Test case for the bug (894 bytes, text/html)
2010-04-20 15:56 PDT, Steven Lai
no flags Details
Test case for the bug (924 bytes, text/html)
2010-04-20 16:00 PDT, Steven Lai
no flags Details
Minimal workaround for number formatting problem of large integral values (7.97 KB, patch)
2010-04-21 11:39 PDT, Steven Lai
eric: review-
Details | Formatted Diff | Diff
Reverted the order change in Element::cloneElementWithoutChildren r54372 (2.92 KB, patch)
2010-04-21 22:14 PDT, Steven Lai
eric: review-
Details | Formatted Diff | Diff
Patch (8.13 KB, patch)
2010-05-03 14:54 PDT, Steven Lai
no flags Details | Formatted Diff | Diff
Patch (8.11 KB, patch)
2010-05-03 15:32 PDT, Steven Lai
bdakin: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch (tab stripped) (8.13 KB, patch)
2010-05-04 10:17 PDT, Steven Lai
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Steven Lai 2010-04-20 15:56:34 PDT
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.
Comment 1 Steven Lai 2010-04-20 16:00:26 PDT
Created attachment 53902 [details]
Test case for the bug
Comment 2 Steven Lai 2010-04-20 17:24:33 PDT
<rdar://problem/7806164>
Comment 3 Steven Lai 2010-04-21 10:57:29 PDT
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
Comment 4 Steven Lai 2010-04-21 11:39:48 PDT
Created attachment 53977 [details]
Minimal workaround for number formatting problem of large integral values
Comment 5 Alexey Proskuryakov 2010-04-21 13:05:58 PDT
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
Comment 6 Alexey Proskuryakov 2010-04-21 13:52:20 PDT
Sounds like this can make bug 32078 return. Please investigate that first.
Comment 7 Steven Lai 2010-04-21 14:29:44 PDT
it did make it return :(
Comment 8 Eric Seidel (no email) 2010-04-21 18:29:14 PDT
Comment on attachment 53977 [details]
Minimal workaround for number formatting problem of large integral values

r- based on above comments.
Comment 9 Steven Lai 2010-04-21 22:14:56 PDT
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
Comment 10 Alexey Proskuryakov 2010-04-27 12:36:41 PDT
+        No new tests. (OOPS!)

There should be!
Comment 11 Darin Adler 2010-04-27 12:54:38 PDT
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 12 Eric Seidel (no email) 2010-05-02 19:12:37 PDT
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.
Comment 13 Steven Lai 2010-05-03 14:54:37 PDT
Created attachment 54962 [details]
Patch
Comment 14 WebKit Review Bot 2010-05-03 14:59:33 PDT
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 15 Darin Adler 2010-05-03 15:01:28 PDT
Comment on attachment 54962 [details]
Patch

> +    if (attached() && checked()) {
> +        checkedRadioButtons(this).addButton(this);
> +    }

No braces around single line if statements.

r=me
Comment 16 Steven Lai 2010-05-03 15:10:08 PDT
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.
Comment 17 Steven Lai 2010-05-03 15:32:05 PDT
Created attachment 54970 [details]
Patch
Comment 18 WebKit Commit Bot 2010-05-04 06:40:55 PDT
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
Comment 19 Steven Lai 2010-05-04 10:17:03 PDT
Created attachment 55026 [details]
Patch (tab stripped)
Comment 20 WebKit Commit Bot 2010-05-04 10:42:49 PDT
Comment on attachment 55026 [details]
Patch (tab stripped)

Clearing flags on attachment: 55026

Committed r58766: <http://trac.webkit.org/changeset/58766>
Comment 21 Steven Lai 2010-05-05 19:41:28 PDT
The layout case checked in failed on Windows (and possibly other platforms)
see: https://bugs.webkit.org/show_bug.cgi?id=38617
Comment 22 Eric Seidel (no email) 2010-05-08 23:10:37 PDT
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.
Comment 23 Alexey Proskuryakov 2010-05-08 23:43:05 PDT
It's unclear why this bug is still open. Did the commit bot fail to close it?
Comment 24 Eric Seidel (no email) 2010-05-09 14:09:27 PDT
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
Comment 25 Adam Barth 2010-05-15 00:15:31 PDT
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.