1290813 - Correctly number indirect descendants of <ol reversed>

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Manish Goregaokar [:manishearth]]

https://github.com/whatwg/html/issues/1617

This isn't specced, but currently if we have `<li>` elements that are not direct descendants of `<ol reversed>`, the numbering is weird.

For example, |<ol reversed><div><li>x</li><li>x</li></div></ol>| will get numbered as 0,-1 (http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4349), and |<ol reversed><li>y</li><div><li>x</li><li>x</li></div></ol>| renders as 1,0,-1 (http://software.hixie.ch/utilities/js/live-dom-viewer/saved/4352).


When numbering list elements, we recurse down the tree, however when deciding what ordinal to start at for `<ol reversed>`, we only count the immediate children (https://dxr.mozilla.org/mozilla-central/source/layout/generic/nsBlockFrame.cpp#7020)

Simply doing a reverse traversal for this case isn't enough, since `<li value=..>` sets the ordinal in the forward direction. We must traverse twice.

Comment 1

[Manish Goregaokar [:manishearth]]

(Shouldn't be merged until there's some consensus on the spec)

Comment 2

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1290813 - Correctly number indirect descendants of <ol reversed>;

Review commit: https://reviewboard.mozilla.org/r/68214/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68214/

Comment 3

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1290813 - Include <ol reversed> reftests in wpt manifest, add test for nested div;

Review commit: https://reviewboard.mozilla.org/r/68254/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68254/

Comment 4

[Manish Goregaokar [:manishearth]]

Comment on attachment 8776486 [details]
Bug 1290813 - Include <ol reversed> reftests in wpt manifest, add test for nested div;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68254/diff/1-2/

Comment 5

[Manish Goregaokar [:manishearth]]

Comment on attachment 8776486 [details]
Bug 1290813 - Include <ol reversed> reftests in wpt manifest, add test for nested div;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68254/diff/2-3/

Comment 6

[Manish Goregaokar [:manishearth]]

The wpt tests seem to have been imported from https://dxr.mozilla.org/mozilla-central/source/layout/reftests/list-item. I added a test to wpt, but not to layout/reftests, hope that's okay.

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8776465 [details]
Bug 1290813 - Correctly number indirect descendants of <ol reversed>;

https://reviewboard.mozilla.org/r/68214/#review65302

It looks fine otherwise. I'd like to have a look again with the comment below addressed.

::: layout/generic/nsBlockFrame.h:792
(Diff revision 1)
> +   * @param aOrdinal Ordinal number to start counting at.
> +   *        Modifies this number for each associated list
> +   *        item. Changes in the numbering due to setting
> +   *        the |value| attribute are included if |aActuallyRenumber|
> +   *        is true.

Probably worth indicating this acts as both input and output of this function.

::: layout/generic/nsBlockFrame.h:797
(Diff revision 1)
> +   * @param aOrdinal Ordinal number to start counting at.
> +   *        Modifies this number for each associated list
> +   *        item. Changes in the numbering due to setting
> +   *        the |value| attribute are included if |aActuallyRenumber|
> +   *        is true.
> +   * @param aIncrement Amount to increment by after visiting each associated

"increment" is not a verb. I suppose it should probably be "Amount to increase by".

::: layout/generic/nsBlockFrame.h:799
(Diff revision 1)
> +   * @param aActuallyRenumber Whether we should actually renumber the elements
> +   *        as we visit them. When false, this simply visits all children,
> +   *        ignoring |<li value="..">| changes, effectively counting them
> +   *        and storing the result in aOrdinal. This is useful for
> +   *        |<ol reversed>|, where we need to count the number of
> +   *        applicable child list elements before numbering.

Based on this comment, I'd suggest you pick some other name for this param, e.g. aForCounting. "aActuallyRenumber" sounds like the result would be identical whatever its value is, except that there is no side effect if "aActuallyRenumber" is false. However, that is not true given the behavior around value attribute.

::: layout/generic/nsBlockFrame.h:818
(Diff revision 1)
>  
> +  /**
> +   * Renumber the lists for a single frame.
> +   * May recurse into RenumberListsInBlock.
> +   * See RenumberListsInBlock for description of parameters.
> +   * 

Please remove the tailing whitespace. Actually I think you can remove this line.

::: layout/generic/nsBlockFrame.cpp:34
(Diff revision 1)
>  #include "nsPresContext.h"
>  #include "nsIPresShell.h"
>  #include "nsStyleContext.h"
>  #include "nsHTMLParts.h"
>  #include "nsGkAtoms.h"
> +#include "nsIContentIterator.h"

You don't need this now, right?

::: layout/generic/nsBlockFrame.cpp:7121
(Diff revision 1)
>      // something foreign has crept in.
>      nsBlockFrame* listItem = nsLayoutUtils::GetAsBlock(kid);
>      if (listItem) {
>        nsBulletFrame* bullet = listItem->GetBullet();
>        if (bullet) {
>          bool changed;

This should be moved into the if-block as well.

::: layout/generic/nsBlockFrame.cpp:7122
(Diff revision 1)
>      nsBlockFrame* listItem = nsLayoutUtils::GetAsBlock(kid);
>      if (listItem) {
>        nsBulletFrame* bullet = listItem->GetBullet();
>        if (bullet) {
>          bool changed;
> +        if (aActuallyRenumber) {

Please add some comment mentioning that one branch would take value attribute into consideration, and the other would not.

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8776486 [details]
Bug 1290813 - Include <ol reversed> reftests in wpt manifest, add test for nested div;

https://reviewboard.mozilla.org/r/68254/#review65328

I'd prefer this patch to be splitted into two, one for fixing the existing ones, the other for the new one. But I'm also okay with having them together as well.

::: testing/web-platform/tests/html/semantics/grouping-content/the-ol-element/reversed-1a.html:4
(Diff revision 3)
>  <!DOCTYPE html>
> +<meta charset=utf-8>
> +<title></title>
> +<link rel=match href=/html/semantics/grouping-content/the-ol-element/reversed-1-ref.html>

You don't need the full path here. It could be just "reversed-1-ref.html". And please wrap attribute values with quotes.

In addition, it would be great if you can add <link rel="help"> with link to the spec. It is also good to always have a title indicating what it is testing.

::: testing/web-platform/tests/html/semantics/grouping-content/the-ol-element/reversed-1b.html:4
(Diff revision 3)
>  <!DOCTYPE html>
> +<meta charset=utf-8>
> +<title></title>
> +<link rel=match href=/html/semantics/grouping-content/the-ol-element/reversed-1-ref.html>

Same here.

::: testing/web-platform/tests/html/semantics/grouping-content/the-ol-element/reversed-1c.html:4
(Diff revision 3)
>  <!DOCTYPE html>
> +<meta charset=utf-8>
> +<title></title>
> +<link rel=match href=/html/semantics/grouping-content/the-ol-element/reversed-1-ref.html>

And here.

::: testing/web-platform/tests/html/semantics/grouping-content/the-ol-element/reversed-1d.html:4
(Diff revision 3)
> +<!DOCTYPE html>
> +<meta charset=utf-8>
> +<title></title>
> +<link rel=match href=/html/semantics/grouping-content/the-ol-element/reversed-1-ref.html>

And here.

Comment 9

[Manish Goregaokar [:manishearth]]

Comment on attachment 8776465 [details]
Bug 1290813 - Correctly number indirect descendants of <ol reversed>;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68214/diff/1-2/

Comment 10

[Manish Goregaokar [:manishearth]]

Comment on attachment 8776486 [details]
Bug 1290813 - Include <ol reversed> reftests in wpt manifest, add test for nested div;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68254/diff/3-4/

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8776465 [details]
Bug 1290813 - Correctly number indirect descendants of <ol reversed>;

https://reviewboard.mozilla.org/r/68214/#review65336

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

https://reviewboard.mozilla.org/r/68254/#review65338

::: testing/web-platform/tests/html/semantics/grouping-content/the-ol-element/reversed-1a.html:4
(Diff revision 4)
>  <!DOCTYPE html>
> +<meta charset=utf-8>
> +<title>`reversed` should reverse the numbering correctly</title>
> +<link rel=match href=/html/semantics/grouping-content/the-ol-element/reversed-1-ref.html>

It's still a full path.

Comment 13

[Manish Goregaokar [:manishearth]]

Comment on attachment 8776486 [details]
Bug 1290813 - Include <ol reversed> reftests in wpt manifest, add test for nested div;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68254/diff/4-5/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2248a7530477
Correctly number indirect descendants of <ol reversed>; r=xidorn
https://hg.mozilla.org/integration/autoland/rev/738847eb3831
Include <ol reversed> reftests in wpt manifest, add test for nested div; r=xidorn

Comment 15

[Manish Goregaokar [:manishearth]]

(Landing because this lets us match the other browsers, who have more sensible behavior)

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2248a7530477
https://hg.mozilla.org/mozilla-central/rev/738847eb3831