[webkit-reviews] review denied: [Bug 5776] REGRESSION: YAHOO: Generated numbers of ordered lists (OL) are not calculated right : [Attachment 6206] Proposed patch 3

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Andrew Wellington
<proton at wiretapped.net>'s request for review:
Bug 5776: REGRESSION: YAHOO: Generated numbers of ordered lists (OL) are not
calculated right
http://bugzilla.opendarwin.org/show_bug.cgi?id=5776

Attachment 6206: Proposed patch 3
http://bugzilla.opendarwin.org/attachment.cgi?id=6206&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
This is looking really good. Almost ready to land.

I see one bug, so I'll have to mark this review-.

The bug is here:

+	     NodeImpl* list = enclosingList(node());
+	     if (list)
+		 m_marker->m_value =
static_cast<HTMLOListElementImpl*>(list)->start();

This casts the list to HTMLOListElementImpl* without checking if it's an <ol>
element or a <ul> element. Need to add a hasTagName(olTag).

--------

I also think we need some more test cases.

    1) Test with list items that use list styles that don't display the number.
You said, "List items with styles that won't display a number still get
assigned an invisible number." And you tested this in other browsers. Great!
But we don't have a test that demonstrates this and regression-tests it.

    2) Test with list items that are in the list but not displayed (using
style="display: none") to test whether they are skipped when assigning numbers,
get assigned invisible numbers, or have some other behavior. Such items will
not have renderers. I believe the proposed code will start over at 1 if there's
a list item that is not displayed, which may not be right. And depending on the
behavior we may have to find out some way to store values for list items
without renderers.

    3) Test with list items that use other "display" modes. There is "none",
which is test (2) above, but there are also modes like "display: block" that
make a list item no longer look like a list item. Specifically, "display:
list-item" will make anything behave like a list item, even if it's not an <li>
element and any display mode other than "display: list-item" can make an <li>
element behave as if it's not a list item. Do the numbers on these types of
items work properly?

Some of these test cases will probably uncover further problems that might be
beyond the scope of this patch. For example the assumption that If so, we can
land the test cases with the current results even if incorrect, and write
further bug reports about the outstanding problems.

To fix problems uncovered by (3) we might have to change
previousListItemElement to work based on the render tree instead of the DOM
tree. If so, then we might have to write the RenderObject version of the
NodeImpl::traversePreviousNode function, and use isListItem() rather than
hasTagName(liTag) to identify list items.

But depending on the results of (2) we might have to go in a conflicting
direction, because "display: none" elements don't get render objects. If they
have to participate in the numbering then we'll have a conundrum on our hands.
If we're lucky, the changing to use the render tree instead of the DOM tree
will give us correct results for both (2) and (3)!

What's important to me is having the test cases -- eventually it would be nice
to handle all the cases well too but that's less urgent.

--------

Here are five much-less-important optional improvements. Let me reiterate,
these are all optional:

A)

+    NodeImpl* n;
+    for (n = node; n; n = n->parentNode()) {

Declare NodeImpl* inside the for loop here.

B)

+	     // We found ourself inside another list; lets skip the rest of 
it.

Remove one of the two spaces between "of" and "it".

C)

+	 NodeImpl *n = 0;

Put the * next to the NodeImpl intead of next to the n.

D)

+	 RenderObject *o = 0;

...

+	 if (n) {
+	     o = n->renderer();

Declare o only inside the if statement right where it's initialized rather than
declaring it earlier and initializing it to 0.

E)

Since we need to know the list in the case where previousListItemElement return
0, perhaps we could change that function so that it takes the list as a
parameter. Then we could restructure the calling code to get the list and pass
it in, saving us from finding the list twice in the case where there's no
previous list item.