Bug 31723 – CSS Counter Nesting still does not work according to the spec.

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

[Carol Szabo]

The following attached test case does not render correctly.
Patch to come soon. This is patch is a partial for the big fix for 11031.

[Carol Szabo]

Created attachment 43583 [details]
Test Case

I tried to attach this with the bug creation, but the attachment was dropped.

[Carol Szabo]

Created attachment 43589 [details]
Proposed Patch

This patch is an intermediary step towards fixing 11031. So it addresses some of the issues Darin raised there and fixes the findPlaceForCounter algorithm.

[Carol Szabo]

Created attachment 43740 [details]
Proposed Patch

This is the first part of a comprehensive patch to 11031

[Darin Adler]

Comment on attachment 43740 [details]
Proposed Patch

> +                        // We found a reset counter that is on a renderer that is a sibling of ours or a parent.
> +                        if (isReset && (currentRenderer->parent() == counterOwner->parent())) {

Formatting nit: We normally don't use parentheses in this common type of expression, and later in this same function this patch does not.

> +                    // CurrentCounter, the counter at the EndSearchRenderer is not reset.

Missing comma after the word "EndSearchRenderer" in this comment.

> +                    if (!isReset || (currentRenderer->parent() != counterOwner->parent())) {

Formatting nit: We normally don't use parentheses in this common type of expression, and later in this same function this patch does not.

> +                        // We are no longer interested in previous siblings of the currentRenderer or their children
> +                        // as counters they may have attached cannot be the previous sibling of the counter we are placing

Missing period here.

> +        // This function is designed so that the same test is not done twice in an iteration, except for this one
> +        // which may be done twice in some cases.

I think this design choice has made the function a little harder to read than most with more repeated code than I would like to see.

[Carol Szabo]

Created attachment 44101 [details]
Proposed Patch

I have addressed all style related comments that Darin made.
I did not address the readability issue as my understanding is that Darin's concern not withstanding the current form is acceptable and I wasn't able to find a solution that would make the code significantly smaller and more readable.

One thought that I had was to codify in all 7 decision points as bits in an offset and build a decision vector of chars, where the 8 bits would represent the about 8 different actions in the algorithm. This way the code size would be probably the same, maybe somewhat smaller, the speed might be slightly lower, but probably not significantly, 90% of the code would be easy to read, the entire complexity of the algorithm being packaged in the decision vector which would be as hard to figure out as the current code is to read if not harder hence in the end I decided against this solution.

If I have not made my self clear above here is what I had in mind:

const char ActionSetParentToCurrent=1;
const char ActionSetParentToCurrentsParent=2;
const char ActionSetPreviousSiblingToCurrent=4;
const char ActionReturnParent=8;
.....

const char DecisionIsReset=1
const char DecisionIsCurrentReset=2
const char DecisionCurrentAndOwnerSiblings=4;
const char DecisionCurrentIsEndOfSearch=8;
.....

const char decisionVector[128] = {
    0,
    0,
    ActionSetPreviousSiblingToCurrent,
    ActionSetPreviousSiblingToZero,
    ActionSetPreviousSiblingToZero | ActionSetParentToCurrent,
    ActionSetPreviousSiblingToZero | ActionSetParentToCurrent | ReturnParent,
.....
};
 while(currentRenderer) {
  current = makeCounterNode(...);
  action = decisionVector [ DecisionIsReset * isReset + DecisionIsCurrentReset * current->actsAsReset() + ... ];
  if (action & ActionSetParentToCurrent)
      parent=current;
  if (action & ActionSetPreviousSiblingToCurrent)
      previousSibling=current;
  if (action & ActionReturnParent)
      return parent;
.....
  if (previousSibling)
      currentRenderer = previousSiblingOrParent(currentRenderer);
  else
      currentRenderer = currentRenderer->previousInPreOrder();
}
return false;

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44101 [details] without any errors.

[WebKit Commit Bot]

Comment on attachment 44101 [details]
Proposed Patch

Rejecting patch 44101 from commit-queue.

Failed to run "['git', 'svn', 'rebase']" exit_code: 1
Last 500 characters of output:
geLog
r51773 = e09427d72cfc7ad43d286711fd170387100dad22 (refs/remotes/trunk)
	M	LayoutTests/platform/win/Skipped
	M	LayoutTests/ChangeLog
r51775 = c68367ef88c3fdc4a45069529814374838fcba35 (refs/remotes/trunk)
First, rewinding head to replay your work on top of it...
error: Untracked working tree file 'LayoutTests/http/tests/security/cross-frame-access-getOwnPropertyDescriptor-expected.txt' would be overwritten by merge.
could not detach HEAD
rebase refs/remotes/trunk: command returned error: 1

[Eric Seidel (no email)]

Sorry, I"ll fix that on the bot.  That's caused by bug 28603.

[WebKit Commit Bot]

Comment on attachment 44101 [details]
Proposed Patch

Rejecting patch 44101 from commit-queue.

Failed to run "['git', 'svn', 'dcommit']" exit_code: 1
Last 500 characters of output:
g.html
	M	WebCore/ChangeLog
	M	WebCore/rendering/RenderCounter.cpp
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/css/counters/nesting.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 558

[Carol Szabo]

Created attachment 44464 [details]
Proposed Patch

Fixed tab in html test case, otherwise the patch is identical with the previously reviewed one.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44464 [details] without any errors.

[WebKit Commit Bot]

Comment on attachment 44464 [details]
Proposed Patch

Clearing flags on attachment: 44464

Committed r51851: <http://trac.webkit.org/changeset/51851>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.