Bug 37900 – cloneNode() does not preserve z-index with more than six digits

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Steven Lai]

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.

[Steven Lai]

Created attachment 53902 [details]
Test case for the bug

[Steven Lai]

<rdar://problem/7806164>

[Steven Lai]

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

[Steven Lai]

Created attachment 53977 [details]
Minimal workaround for number formatting problem of large integral values

[Alexey Proskuryakov]

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

[Alexey Proskuryakov]

Sounds like this can make bug 32078 return. Please investigate that first.

[Steven Lai]

it did make it return :(

[Eric Seidel (no email)]

Comment on attachment 53977 [details]
Minimal workaround for number formatting problem of large integral values

r- based on above comments.

[Steven Lai]

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

[Alexey Proskuryakov]

+        No new tests. (OOPS!)

There should be!

[Darin Adler]

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.

[Eric Seidel (no email)]

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.

[Steven Lai]

Created attachment 54962 [details]
Patch

[WebKit Review Bot]

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.

[Darin Adler]

Comment on attachment 54962 [details]
Patch

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

No braces around single line if statements.

r=me

[Steven Lai]

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.

[Steven Lai]

Created attachment 54970 [details]
Patch

[WebKit Commit Bot]

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

[Steven Lai]

Created attachment 55026 [details]
Patch (tab stripped)

[WebKit Commit Bot]

Comment on attachment 55026 [details]
Patch (tab stripped)

Clearing flags on attachment: 55026

Committed r58766: <http://trac.webkit.org/changeset/58766>

[Steven Lai]

The layout case checked in failed on Windows (and possibly other platforms)
see: https://bugs.webkit.org/show_bug.cgi?id=38617

[Eric Seidel (no email)]

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.

[Alexey Proskuryakov]

It's unclear why this bug is still open. Did the commit bot fail to close it?

[Eric Seidel (no email)]

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

[Adam Barth]

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.