Closed Bug 1290813 Opened 4 years ago Closed 4 years ago

Correctly number indirect descendants of <ol reversed>

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: manishearth, Assigned: manishearth)

Details

Attachments

(2 files)

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.
(Shouldn't be merged until there's some consensus on the spec)
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/
Attachment #8776486 - Attachment description: Bug 1290813 - Include <ol reversed> reftests in manifest → Bug 1290813 - Include <ol reversed> reftests in manifest;
Attachment #8776486 - Flags: review?(xidorn+moz)
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/
Attachment #8776486 - Attachment description: Bug 1290813 - Include <ol reversed> reftests in manifest; → Bug 1290813 - Include <ol reversed> reftests in wpt manifest, add test for nested div;
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 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.
Attachment #8776465 - Flags: review?(xidorn+moz)
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.
Attachment #8776486 - Flags: review?(xidorn+moz) → review+
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/
Attachment #8776465 - Flags: review?(xidorn+moz)
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 on attachment 8776465 [details]
Bug 1290813 - Correctly number indirect descendants of <ol reversed>;

https://reviewboard.mozilla.org/r/68214/#review65336
Attachment #8776465 - Flags: review?(xidorn+moz) → review+
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 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/
Pushed by xquan@mozilla.com:
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
(Landing because this lets us match the other browsers, who have more sensible behavior)
https://hg.mozilla.org/mozilla-central/rev/2248a7530477
https://hg.mozilla.org/mozilla-central/rev/738847eb3831
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.